Repository navigation
Add EmailVerification.DisableNotifyUnlockState as a connector property - #1161
Conversation
The account unlock notification suppression for the email verification flow was only configurable through the server level identity.xml property, which cannot be set per tenant. Expose it on the email verification connector so it can be configured per tenant. The identity.xml value seeds the connector default, so deployments that only set the server level property keep their current behaviour.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds the ChangesEmail verification configuration
Priority: ⬇️ Low — Defer this narrow connector-configuration change because it adds one optional email-verification property while preserving existing behavior. Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This adds a tenant-configurable email-verification setting to suppress the account-unlock email, while retaining the existing false default. The configuration wiring and expected defaults are covered with no concrete current-head merge risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the issue, fix, compatibility behavior, and unit-test results. However, it omits most required template sections, including purpose, goals, approach, user stories, release note, documentation, security checks, and test environment.
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
UserEmailVerificationConfigImpl.getMetaData() is inconsistent with the exposed property set (missing metadata for an existing exposed property), which should be corrected alongside this update.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR registers EmailVerification.DisableNotifyUnlockState as an email verification connector property so it can be resolved per-tenant via IdentityGovernanceService, while seeding the connector default from the existing server-level identity.xml value for backward compatibility.
Changes:
- Added
EmailVerification.DisableNotifyUnlockStatetoIdentityRecoveryConstants.ConnectorConfig. - Registered the new property in
UserEmailVerificationConfigImpl(names, descriptions, property list, default seeding, and metadata). - Updated
UserEmailVerificationConfigImplTestexpectations to include the new property.
File summaries
| File | Description |
|---|---|
| components/org.wso2.carbon.identity.recovery/src/test/java/org/wso2/carbon/identity/recovery/connector/UserEmailVerificationConfigImplTest.java | Updates connector config tests to include the new property in mappings, names, and defaults. |
| components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/IdentityRecoveryConstants.java | Adds the new connector config key constant for per-tenant governance resolution. |
| components/org.wso2.carbon.identity.recovery/src/main/java/org/wso2/carbon/identity/recovery/connector/UserEmailVerificationConfigImpl.java | Registers the property across mappings/defaults and exposes it for tenant configuration. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1161 +/- ##
=========================================
Coverage 55.34% 55.34%
Complexity 3346 3346
=========================================
Files 317 317
Lines 22166 22176 +10
Branches 4585 4586 +1
=========================================
+ Hits 12267 12273 +6
- Misses 8323 8326 +3
- Partials 1576 1577 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
PR builder started |
|
PR builder completed |
|
Integration test note: the Adding a property at index 12 shifts the later entries. Companion PR with the expected-response update: wso2/product-is#28428. The two cannot both be green at the same time — a (The assertion is order-fragile independently of this change — it passed on 2 of its 4 invocations in the original run. Making it order-independent looks worthwhile as a separate change.) |
|
PR builder started |
|
PR builder completed |
Issue
EmailVerification.DisableNotifyUnlockStatesuppresses the account unlock email sent when a user confirms email verification, but it is only readable from the server levelidentity.xml. It cannot be configured per tenant.Fix
Register the property on the email verification connector so it resolves through
IdentityGovernanceServiceper tenant. Theidentity.xmlvalue seeds the connector default, so deployments that only set the server level property keep their current behaviour.The consumer of this property is in the account lock handler; that change is sent separately.
Testing
org.wso2.carbon.identity.recoverysuite: 526 tests, 0 failures.UserEmailVerificationConfigImplTestupdated for the new property.Summary by CodeRabbit