feat: add PENDING_VERIFICATION status to identity domain#1979
Merged
feat: add PENDING_VERIFICATION status to identity domain#1979
Conversation
- Add IdentityStatusPendingVerification constant to identity domain - Add NewSelfRegisteredIdentity constructor (returns PENDING_VERIFICATION or ACTIVE depending on verificationRequired flag) - Add Verify() method transitioning PENDING_VERIFICATION -> ACTIVE - Add ErrNotPendingVerification and ErrEmailNotVerified domain errors - Update connector.Login to return ErrEmailNotVerified for PENDING_VERIFICATION accounts (rather than silent false) - Add migration to extend chk_identity_status CHECK constraint - Add IDENTITY_STATUS_PENDING_VERIFICATION = 5 to identity proto enum - Update domainStatusToProto mapper for new status - Tests for all new paths (constructor, Verify, connector rejection)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughIntroduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Claude Code ReviewCommit: SummaryClean, well-scoped addition of Risk Assessment
Findings
Previously Flagged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/identity/domain/identity.go (1)
72-98: Consider extracting shared constructor bootstrap logic to prevent drift.
NewSelfRegisteredIdentityandNewIdentitycurrently duplicate validation and initialization fields. A small internal helper would reduce future divergence risk.♻️ Proposed refactor
+func newIdentityWithStatus(tenantID tenant.TenantID, email string, status IdentityStatus) (*Identity, error) { + if tenantID.IsEmpty() { + return nil, ErrTenantIDRequired + } + if !emailRegex.MatchString(email) { + return nil, ErrInvalidEmail + } + + now := time.Now() + return &Identity{ + id: uuid.New(), + tenantID: tenantID, + email: email, + status: status, + createdAt: now, + updatedAt: now, + version: 1, + }, nil +} + // NewIdentity creates a new identity in PENDING_INVITE status. func NewIdentity(tenantID tenant.TenantID, email string) (*Identity, error) { - if tenantID.IsEmpty() { - return nil, ErrTenantIDRequired - } - if !emailRegex.MatchString(email) { - return nil, ErrInvalidEmail - } - - now := time.Now() - return &Identity{ - id: uuid.New(), - tenantID: tenantID, - email: email, - status: IdentityStatusPendingInvite, - createdAt: now, - updatedAt: now, - version: 1, - }, nil + return newIdentityWithStatus(tenantID, email, IdentityStatusPendingInvite) } @@ func NewSelfRegisteredIdentity(tenantID tenant.TenantID, email string, verificationRequired bool) (*Identity, error) { - if tenantID.IsEmpty() { - return nil, ErrTenantIDRequired - } - if !emailRegex.MatchString(email) { - return nil, ErrInvalidEmail - } - status := IdentityStatusActive if verificationRequired { status = IdentityStatusPendingVerification } - - now := time.Now() - return &Identity{ - id: uuid.New(), - tenantID: tenantID, - email: email, - status: status, - createdAt: now, - updatedAt: now, - version: 1, - }, nil + return newIdentityWithStatus(tenantID, email, status) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/identity/domain/identity.go` around lines 72 - 98, Both NewSelfRegisteredIdentity and NewIdentity duplicate validation and bootstrap fields; extract a small unexported helper (e.g., newIdentityBase or buildIdentity) that accepts tenantID, email and status (or a flag like verificationRequired) and performs tenantID.IsEmpty check, emailRegex validation, sets id (uuid.New()), tenantID, email, status, createdAt/updatedAt (time.Now()) and version=1, then returns *Identity, error; update NewSelfRegisteredIdentity and NewIdentity to call this helper (preserving the current logic that sets IdentityStatusPendingVerification when verificationRequired is true and IdentityStatusActive otherwise).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/identity/migrations/20260327000001_add_pending_verification_status.sql`:
- Around line 4-7: The migration is targeting the wrong table name; update both
ALTER TABLE statements to reference the existing quoted singular table name
"identity" (not identities) so the CHECK constraint chk_identity_status is
dropped and re-added on the correct table; ensure the ALTER TABLE lines use
"identity" and keep the constraint name chk_identity_status and the same CHECK
values.
---
Nitpick comments:
In `@services/identity/domain/identity.go`:
- Around line 72-98: Both NewSelfRegisteredIdentity and NewIdentity duplicate
validation and bootstrap fields; extract a small unexported helper (e.g.,
newIdentityBase or buildIdentity) that accepts tenantID, email and status (or a
flag like verificationRequired) and performs tenantID.IsEmpty check, emailRegex
validation, sets id (uuid.New()), tenantID, email, status, createdAt/updatedAt
(time.Now()) and version=1, then returns *Identity, error; update
NewSelfRegisteredIdentity and NewIdentity to call this helper (preserving the
current logic that sets IdentityStatusPendingVerification when
verificationRequired is true and IdentityStatusActive otherwise).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4875f50a-931c-4474-b2e8-e60f2333907e
⛔ Files ignored due to path filters (2)
api/proto/meridian/identity/v1/identity.pb.gois excluded by!**/*.pb.go,!**/*.pb.goservices/identity/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
api/proto/meridian/identity/v1/identity.protoservices/identity/connector/connector.goservices/identity/connector/connector_test.goservices/identity/domain/errors.goservices/identity/domain/identity.goservices/identity/domain/identity_test.goservices/identity/migrations/20260327000001_add_pending_verification_status.sqlservices/identity/service/mappers.goservices/identity/service/mappers_test.go
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/identity/domain/identity.go (1)
207-222: Consider updating the doc comment to mention PENDING_VERIFICATION.The code correctly rejects
PENDING_VERIFICATION(line 218), but the doc comment on lines 207-208 only mentionsLOCKEDas an invalid source state. For completeness, consider updating it to reflect this restriction.📝 Suggested doc update
// Activate transitions the identity to ACTIVE status. -// Valid from PENDING_INVITE or SUSPENDED; invalid from LOCKED. +// Valid from PENDING_INVITE or SUSPENDED; invalid from LOCKED or PENDING_VERIFICATION. func (i *Identity) Activate() error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/identity/domain/identity.go` around lines 207 - 222, Update the Activate method doc comment to accurately list invalid source states by including PENDING_VERIFICATION in addition to LOCKED; locate the comment above the Identity.Activate method and revise the sentence that currently reads "Valid from PENDING_INVITE or SUSPENDED; invalid from LOCKED." so it states that activation is invalid from LOCKED and PENDING_VERIFICATION (or equivalent wording referencing IdentityStatusLocked and IdentityStatusPendingVerification), keeping it concise and consistent with the behavior that returns ErrInvalidStatusTransition for those states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/identity/domain/identity.go`:
- Around line 207-222: Update the Activate method doc comment to accurately list
invalid source states by including PENDING_VERIFICATION in addition to LOCKED;
locate the comment above the Identity.Activate method and revise the sentence
that currently reads "Valid from PENDING_INVITE or SUSPENDED; invalid from
LOCKED." so it states that activation is invalid from LOCKED and
PENDING_VERIFICATION (or equivalent wording referencing IdentityStatusLocked and
IdentityStatusPendingVerification), keeping it concise and consistent with the
behavior that returns ErrInvalidStatusTransition for those states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14e1a363-6c3d-4224-93ef-94adfd382256
⛔ Files ignored due to path filters (1)
frontend/src/api/gen/meridian/identity/v1/identity_pb.tsis excluded by!**/gen/**
📒 Files selected for processing (1)
services/identity/domain/identity.go
bjcoombs
dismissed stale reviews from coderabbitai[bot], claude[bot], and claude[bot]
Stale bot review
…DD constraint limitation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PENDING_VERIFICATIONidentity status for self-registration flows where email verification is required before loginNewSelfRegisteredIdentity(tenantID, email, verificationRequired)constructor returnsPENDING_VERIFICATIONorACTIVEdepending on the flagVerify()method transitionsPENDING_VERIFICATION->ACTIVEconnector.Loginnow returnsErrEmailNotVerified(not silent false) forPENDING_VERIFICATIONaccounts, allowing callers to distinguish unverified from wrong-passwordIdentityStatusenum extended withIDENTITY_STATUS_PENDING_VERIFICATION = 5PENDING_VERIFICATIONto thechk_identity_statusCHECK constraintChanges Made
services/identity/domain/identity.go- new status constant, constructor, andVerify()methodservices/identity/domain/errors.go-ErrNotPendingVerification,ErrEmailNotVerifiedservices/identity/connector/connector.go- rejectPENDING_VERIFICATIONwithErrEmailNotVerifiedservices/identity/service/mappers.go- proto status mapper caseapi/proto/meridian/identity/v1/identity.proto+ regenerated.pb.goservices/identity/migrations/20260327000001_add_pending_verification_status.sql- CHECK constraint updateTesting
NewSelfRegisteredIdentity(both paths, invalid inputs)Verify()from correct and all incorrect statesRecordLoginAttemptrejectingPENDING_VERIFICATIONconnector.LoginreturningErrEmailNotVerifiedfor unverified accountIDENTITY_STATUS_PENDING_VERIFICATION