Hotfix/external user delete sync error#551
Merged
Merged
Conversation
When the IDP deletes a user and reassigns its email to a different external id, registration failed with a Member.Email unique-constraint violation. Free the former member's email (the IDP is the source of truth) before assigning it. - MemberService::registerExternalUserByPayload: invalidate the former member's email when the email moved to a new external id - ResourceServerContext::getCurrentUser: same collision guard before setEmail - ScheduleEntity: wrap lifecycle event/cache hooks so a queue/cache failure cannot roll back the Doctrine delete/update - EventServiceProvider: dispatch ScheduleEntityLifeCycleEvent via JobDispatcher::withDbFallback - add reproducing functional test (tests/MemberServiceTest.php)
…inst email collision registerExternalUser created a Member with the IDP email without checking whether a former member (e.g. one whose delete failed) still held it, hitting the Member.Email unique constraint. Apply the same invalidate-former-email guard already used in registerExternalUserByPayload. Add a reproducing test for the create-branch collision (tests/MemberServiceTest.php).
📝 WalkthroughWalkthroughEmail collision handling is added to member registration and OAuth2 contexts to prevent duplicate email ownership. ScheduleEntity lifecycle hooks gain error resilience via try/catch wrapping. Event dispatching for schedule entity lifecycle changes is refactored to use JobDispatcher. Comprehensive tests validate email reassignment behavior. ChangesMember and schedule entity resilience improvements
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-551/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hotfixes external-user email reassignment flows so stale local members do not block IDP-driven registration/update on the unique Member.Email constraint, and hardens schedule lifecycle notifications against queue/cache failures.
Changes:
- Invalidates conflicting member emails before external user registration/update paths reuse an IDP-owned email.
- Dispatches schedule lifecycle processing through
JobDispatcher::withDbFallback. - Adds tests for member-service email reassignment collision scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
app/Services/Model/Imp/MemberService.php |
Frees conflicting emails during external user registration paths. |
app/Models/OAuth2/ResourceServerContext.php |
Handles email collisions while syncing current user fields from auth context. |
app/Models/Foundation/Summit/ScheduleEntity.php |
Makes schedule lifecycle dispatch/cache failures non-fatal. |
app/Providers/EventServiceProvider.php |
Uses DB fallback dispatch for schedule lifecycle processing jobs. |
tests/MemberServiceTest.php |
Adds regression coverage for external user email reassignment collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+260
to
262
| $member_by_email->setEmail(sprintf("%s-invalid@invalid", $member_by_email->getId())); | ||
| } | ||
| $member->setEmail($user_email); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Providers/EventServiceProvider.php (1)
351-360: 💤 Low valueConsider using named parameters for consistency.
The existing
JobDispatcher::withDbFallbackcalls in this file (lines 174-182) use named parameters (job:,logContext:), while this call uses positional arguments. Using named parameters here would improve consistency and readability.♻️ Suggested refactor for consistency
- JobDispatcher::withDbFallback( - new ProcessScheduleEntityLifeCycleEvent( - $event->entity_operator, - $event->summit_id, - $event->entity_id, - $event->entity_type, - $event->params - ), - ['entity_type' => $event->entity_type, 'entity_id' => $event->entity_id] + JobDispatcher::withDbFallback( + job: new ProcessScheduleEntityLifeCycleEvent( + $event->entity_operator, + $event->summit_id, + $event->entity_id, + $event->entity_type, + $event->params + ), + logContext: ['entity_type' => $event->entity_type, 'entity_id' => $event->entity_id] );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Providers/EventServiceProvider.php` around lines 351 - 360, The call to JobDispatcher::withDbFallback uses positional args; change it to use named parameters for consistency with other calls—pass the ProcessScheduleEntityLifeCycleEvent instance as job: and the context array as logContext: (i.e., call JobDispatcher::withDbFallback(job: new ProcessScheduleEntityLifeCycleEvent(...), logContext: ['entity_type' => ..., 'entity_id' => ...])) so it matches the earlier calls and improves readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/Providers/EventServiceProvider.php`:
- Around line 351-360: The call to JobDispatcher::withDbFallback uses positional
args; change it to use named parameters for consistency with other calls—pass
the ProcessScheduleEntityLifeCycleEvent instance as job: and the context array
as logContext: (i.e., call JobDispatcher::withDbFallback(job: new
ProcessScheduleEntityLifeCycleEvent(...), logContext: ['entity_type' => ...,
'entity_id' => ...])) so it matches the earlier calls and improves readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 015e6345-c90e-4f86-9295-b6673214c612
📒 Files selected for processing (5)
app/Models/Foundation/Summit/ScheduleEntity.phpapp/Models/OAuth2/ResourceServerContext.phpapp/Providers/EventServiceProvider.phpapp/Services/Model/Imp/MemberService.phptests/MemberServiceTest.php
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
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.
ref: https://app.clickup.com/t/86ba4rpw0
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests