Skip to content

Conversation

@jrmccannon
Copy link
Contributor

@jrmccannon jrmccannon commented Nov 24, 2025

🎟️ Tracking

PM-27468
PM-28737

📔 Objective

The reigstration of the AutomaticUserConfirmationPolicyEventHandler as an IPolicyValidator was missed in initial implementation. Since, FeatureFlagKeys.CreateDefaultLocation and FeatureFlagKeys.PolicyValidatorsRefactor was not enabled, the new flow of using the vNext save policy was not called. This adds the event handler to the DI container to support the previous flow.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@jrmccannon jrmccannon requested a review from a team as a code owner November 24, 2025 21:24
@jrmccannon jrmccannon requested a review from BTreston November 24, 2025 21:24
@jrmccannon jrmccannon changed the title Forgot to add AutomaticUserConfirmationPolicyEventHandler to the IPolicyValidator implementation registration. [PM-27468] [PM-28737] - Adding Auto Confirm Policy Event Handler as PolicyValidator Nov 24, 2025
@claude
Copy link
Contributor

claude bot commented Nov 24, 2025

Claude finished @jrmccannon's task —— View job


Code Review Complete

This is a clean, focused PR that addresses a missing DI registration discovered during implementation.


Summary

The PR adds the missing IPolicyValidator registration for AutomaticUserConfirmationPolicyEventHandler in the obsolete AddPolicyValidators method. This registration was omitted in PR #6552 when the policy was initially added - only the IPolicyUpdateEvent registration in AddPolicyUpdateEvents was included.

Why this matters: Until feature flags FeatureFlagKeys.CreateDefaultLocation and FeatureFlagKeys.PolicyValidatorsRefactor are enabled, the legacy SavePolicyCommand flow uses validators from the obsolete AddPolicyValidators method, making this registration necessary for the policy to function correctly in production.


Findings

None - This PR correctly addresses the missing registration with no issues identified.


Analysis

Pattern Consistency

The change follows the established pattern. When BlockClaimedDomainAccountCreationPolicyValidator was added in PR #6537, it was intentionally registered only in AddPolicyUpdateEvents (the new flow) and not in the obsolete AddPolicyValidators method. However, AutomaticUserConfirmationPolicyEventHandler needs both registrations because:

  1. It implements both IPolicyValidator (legacy interface) and IPolicyUpdateEvent (new interface) - see PolicyServiceCollectionExtensions.cs:32
  2. The legacy SavePolicyCommand uses IPolicyValidator implementations until feature flags are enabled
  3. The new VNextSavePolicyCommand uses IPolicyUpdateEvent implementations (already registered on line 58)

Test Coverage

Comprehensive unit tests exist (AutomaticUserConfirmationPolicyEventHandlerTests.cs) covering:

  • All validation scenarios (Single Org compliance, provider users, user statuses)
  • Side effects (organization feature flag updates)
  • Both validation interfaces (IPolicyValidator and IPolicyValidationEvent)

PR Metadata

  • Clear title explaining what was added
  • Well-structured description with Jira references
  • Accurate objective explaining why this was needed and the feature flag context
  • All CI checks passing (Checkmarx, Codecov)

Good Practices Observed

  • Single-line surgical fix addressing only the missing registration
  • Clear commit message explaining the omission
  • Feature flag awareness documented in PR description

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailsce98ccda-6da4-4e40-98a7-fb7c2b2a85d5

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.06%. Comparing base (9573cab) to head (02a1b7a).

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6637       +/-   ##
===========================================
+ Coverage   13.38%   53.06%   +39.68%     
===========================================
  Files        1125     1899      +774     
  Lines       49593    84643    +35050     
  Branches     3914     7610     +3696     
===========================================
+ Hits         6640    44920    +38280     
+ Misses      42831    37975     -4856     
- Partials      122     1748     +1626     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants