Repository navigation
fix(attendees): stop member/attendee email conflicts from failing sync jobs and ticket assignment - #663
Open
romanetar wants to merge 5 commits into
Open
fix(attendees): stop member/attendee email conflicts from failing sync jobs and ticket assignment#663romanetar wants to merge 5 commits into
romanetar wants to merge 5 commits into
Conversation
…c jobs and ticket assignment When a member's email changes to the email of a member-less attendee of the same summit, UpdateAttendeeInfo, MemberAssocSummitOrders and ticket assignment collided on the SummitAttendee unique keys. - updateAttendeesByMemberId: flush the rename of the member-less attendee before the member's attendee takes the email, so the merge commits. When the merge does not apply (email not verified, or the email belongs to another member) keep the attendee email, log a warning and still update the other fields. - Share getLinkableByMember between the job and assocSummitOrders so a member-less attendee is not linked to a member that already has an attendee in that summit. - ownerAssignTicket: throw ValidationException (412) instead of failing with a 500 in that state. - Add summit:repair-attendee-member-email-pairs to list the existing pairs and dispatch UpdateAttendeeInfo for the ones with a verified email. - Add regression tests and run them in the integration-tests matrix.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 39 minutes. View limit details
 |
| Layer / File(s) | Summary |
|---|---|
Select linkable attendees app/Models/Foundation/Summit/Repositories/ISummitAttendeeRepository.php, app/Repositories/Summit/DoctrineSummitAttendeeRepository.php, app/Services/Model/Imp/MemberService.php, tests/AttendeeMemberEmailConflictTest.php |
The repository selects matching memberless attendees only when the member has no attendee in that summit. assocSummitOrders uses this selection. Tests cover association behavior. |
Reconcile attendee email conflicts app/Services/Model/AttendeeService.php, tests/AttendeeMemberEmailConflictTest.php |
For a verified member email, the service merges an unlinked attendee with a matching email, transfers tickets, and removes the duplicate attendee and its note. Tests cover verified and unverified emails and attendees owned by another member. |
Reject conflicting ticket assignments app/Services/Model/Imp/SummitOrderService.php, tests/oauth2/OAuth2SummitTicketsApiTest.php |
Ticket assignment now rejects a different attendee already associated with the member in that summit. Tests check the response and confirm attendee and ticket data remain unchanged for verified and unverified emails. |
Report and dispatch attendee-pair repairs app/Console/Commands/RepairAttendeeMemberEmailPairsCommand.php, app/Console/Kernel.php, .github/workflows/push.yml, tests/AttendeeMemberEmailConflictTest.php |
The registered command reports matching pairs. With --dispatch, it dispatches updates for distinct verified members and can apply a nonnegative --limit. Tests cover dry-run, dispatch, and unverified members; the workflow adds the test suite. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Suggested reviewers: smarcet
Merge Risk: 
0a4b3
-
Limited repair runs can leave later conflicts untouched, while long-email conflicts may remain unresolved. Address those failures and the repair report and logging concerns before merging.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main change: preventing attendee/member email conflicts from breaking sync jobs and ticket assignment. Docstring Coverage Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 9 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-663/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A limited run selected the same first members again if their job kept failing, so later pairs were never dispatched. Order the members by id and add --after-member-id, and print the last dispatched member id.
The dry run only showed counts. List the summit, member, attendee and member-less attendee ids of each pair, without emails.
email + "-deleted" can exceed SummitAttendee.Email (varchar 100). Use a value that is unique per attendee and always fits.
The ids already identify the conflict.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-663/ This page is automatically updated on each push to this PR. |
This branch has not been deployed
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/9014802374/86bch3bp3
Summary
When a member's email changes, a summit can end up with the member's attendee on the old email and a second attendee with no member that already has the member's current email. Three paths collided on the
SummitAttendeeunique keys:UpdateAttendeeInfofailed onSummitAttendee_Email_SummitIDand rolled back the update for all of the member's attendees.MemberAssocSummitOrdersfailed onSummitAttendee_Member_Summitand never linked the attendee.PUT /api/v1/summits/all/orders/{order_id}/tickets/{ticket_id}/attendeereturned 500.The merge code in
updateAttendeesByMemberIdalready existed but could never commit: Doctrine runs the UPDATEs in the order it loaded the entities, so the member's attendee took the email while the member-less row still had it.Changes
AttendeeService::updateAttendeesByMemberId: flush the rename of the member-less attendee before the member's attendee takes the email, so the merge commits. When the merge does not apply (email not verified, or the email belongs to another member) keep the attendee email, log a warning and still update first name, last name and company.ISummitAttendeeRepository::getLinkableByMember: shared by the job's last step andMemberService::assocSummitOrders. It skips (and logs) summits where the member already has an attendee.SummitOrderService::ownerAssignTicket: throwsValidationException(412) instead of failing with a 500 when the member that owns the email already has a different attendee in the summit.summit:repair-attendee-member-email-pairs(see Deployment Notes).<id>-deletedbefore the merge, so the temporary email always fitsSummitAttendee.Email(varchar 100).tests/AttendeeMemberEmailConflictTest.php(added to theintegration-testsmatrix inpush.yml) and two cases inOAuth2SummitTicketsApiTest.Only
UpdateAttendeeInforemoves a pair. The merge moves the tickets and deletes the member-less attendee together with its extra question answers, presentation votes, notes, RSVP invitations and announcement email records.Testing
main(SummitAttendee_Email_SummitIDx3,SummitAttendee_Member_Summitx1) and pass with the fix.mainand 412 with the fix.OAuth2SummitTicketsApiTesterrors intearDown(Can not delete ticket type ... has sold tickets) for almost every test, including ones unrelated to this change, and it does the same on a cleanmain. The assertions of the new HTTP tests pass; theOAuth2Testsjob needs to be checked in CI.Deployment Notes
After deploy, repair the existing attendee/member email pairs with the new
summit:repair-attendee-member-email-pairscommand. OnlyUpdateAttendeeInfomerges a pair, and only when the member's email is verified.What counts as a pair: an attendee linked to a member whose current email differs from the attendee's email, plus another attendee of the same summit with no member that already has the member's current email.
Command
--dispatchUpdateAttendeeInfojob per distinct member that has a pair with a verified email. Pairs with an unverified email are listed but skipped.--limit=N--dispatch, enqueues at most N members, in ascending member id order.--after-member-id=ID--dispatch, skips the members with an id lower than or equal to ID. The command prints the last member id it dispatched, so a run can resume after a member whose job keeps failing instead of selecting it again.Steps
--dispatch --limit=5and check that those merges succeeded. If a member's job keeps failing, check the worker logs and continue with--after-member-id=<last id printed>.--dispatchfor the rest.Verification over 7 days
{service_name="summit-api", k8s_cluster_name="prod-fn", container="summit-api-worker-php"} |~ "SummitAttendee_(Email_SummitID|Member_Summit)".PUT /api/v1/summits/all/orders/{order_id}/tickets/{ticket_id}/attendee.