fix(users): unblock sign up when the invitation carries an external ID - #8537
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The fix is narrowly scoped, matches the described root cause, and is backed by targeted controller and model specs for both the regression and non-regression cases.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes a sign-up failure when users register through a course invitation that carries an external_id, by preventing the cross-table uniqueness validation from treating the user’s own pending invitation as a conflict during CourseUser validation.
Changes:
- Add a non-persisted
source_invitationpointer inCourse::UniqueExternalIdConcernto exclude that invitation from the “unconfirmed invitations” external ID collision query. - Pass
source_invitationwhen building aCourseUserfrom an invitation inUser#build_course_user_from_invitation. - Add model + controller specs covering sign-up-through-invitation for both
external_id: niland present, plus model-level narrowness guarantees.
File summaries
| File | Description |
|---|---|
| app/models/concerns/course/unique_external_id_concern.rb | Adds source_invitation and excludes it from the pending-invitation collision check. |
| app/models/user.rb | Ensures CourseUser built from an invitation carries source_invitation. |
| spec/models/course_user_spec.rb | Adds model-level examples verifying the exclusion is narrow and doesn’t weaken other collision cases. |
| spec/controllers/user/registration_controller_spec.rb | Adds regression coverage for the Devise sign-up path using course invitations (with/without external IDs). |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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.
Bug
Students invited to a course with an external ID could not create an account. Submitting the sign-up form returned a generic error toast and no account was created. Nothing appeared in error reporting, because no exception is ever raised — the save simply returns
false.Course::UniqueExternalIdConcernrejects an external ID already claimed by an unconfirmed invitation in the same course, and excludes the record under validation from that query only when the record is itself an invitation:A
CourseUsercreated from an invitation inherits that invitation'sexternal_id, so it collides with its own invitation — unless the invitation has already been confirmed by the time theCourseUseris validated. Three paths create aCourseUserfrom an invitation, and they order those two steps differently:Course::UserRegistrationService#accept_invitationCourseUseris builtUser::Email#accept_all_pending_invitationsCourseUseris builtUser::RegistrationsController#createresource.savereturnsThe first two dodge the defect deliberately —
User::Emaileven carries a comment naming this exact hazard:The sign-up path cannot dodge it by reordering.
build_resourcecallsUser#build_from_invitation, which builds theCourseUseronto the unsavedUser;has_many :course_usersautosave-validates it duringuser.valid?, which precedes everyafter_save. The controller's@invitation.confirm!runs aftersuperreturns, andUser::Email's hook fires during the save — both too late. Confirming earlier is not available either:confirm!setsconfirmerto aUserthat does not yet have an id.What the client sees.
resource.savereturnsfalse, Devise responds200with an empty body, andSignUpPagefalls through its!result.idbranch to a genericerrorSigningUptoast. The422branch that renders per-field errors is never reached, so the underlying message —activerecord.attributes.user.course_users is invalid, an unresolved i18n key on an association the form does not render — would not have helped even if it had been surfaced.Trigger conditions. Both must hold, which is why this presented as affecting a minority rather than everyone:
external_id. Blank is normalised tonilbefore validation andnilreturns early, so invitations predating the feature are unaffected.UserRegistrationServiceor theUser::Emailhook, both safe.Within those conditions the failure is total, not intermittent.
Regression window. Introduced with the concern itself in
4b4647885("feat(users): add optional external_id field to course members and invitations", 14 May 2026). The partial unique indexes added in the same commit never caught it — they are per-table, and this is a cross-table check that exists only in application code.Remediation
Let a record declare the invitation it is being created from, and exclude that one invitation from the uniqueness query:
User#build_course_user_from_invitation— the shared builder behind both the sign-up path and theUser::Emailhook — passessource_invitation: invitation.The exclusion is deliberately narrow. It removes exactly one row from one of the two queries; a different pending invitation or an already-enrolled course user holding the same external ID still fails validation, and
external_id_taken_by_course_user?is untouched.source_invitationis a plainattr_accessor, not an ActiveRecord attribute: it has no column, is never persisted or serialised, and appears in nopermitlist, so it cannot be set from a request.Scope check
Audited every site that writes
external_idor creates aCourseUserfrom an invitation.Course::UserRegistrationService#find_or_create_course_user!— confirms before building, so it never needed the exclusion. Left unchanged; settingsource_invitationhere too would guard against a future reordering of those two lines, at the cost of threading the invitation throughcreate_course_user_record!.User::Email#accept_all_pending_invitations— confirms before building. Now also receivessource_invitationvia the shared builder, which is redundant but harmless: by then the invitation is confirmed and already outside theunconfirmedscope.Course::UserInvitationService(parse/process concerns) — tracks collisions itself through an in-memory@taken_external_idsset and never creates aCourseUserfrom an invitation record. Untouched.Course::UsersControllerManagementConcern— permits:external_idon update. Unaffected; those records have nosource_invitation, so the full check applies.No DB migration. The partial unique indexes on
course_usersandcourse_user_invitationsare unchanged and continue to backstop the per-table cases.Tests
Five examples, all of which fail with the fix reverted and pass with it.
registration_controller_spec.rbgains sign-up-through-a-course-invitation coverage, parameterised overexternal_idnil and present, asserting the user is created, theCourseUsercarries the external ID, and the invitation is confirmed by the new user. It assertsCourseUser.countchanges by exactly 1, so a future change that lets both the eager build and theUser::Emailhook enrol the same user would be caught rather than silently double-enrolling.course_user_spec.rbpins the narrowness of the exclusion at the model level — valid against its own invitation, still invalid against a different pending invitation, still invalid against an enrolled course user. These live at model level by necessity: the conflicting state cannot be constructed through the controller, because the invitation's own validation prevents any other record from taking its external ID in the first place.Unrelated:
user_registration_service_spec.rb:42fails onmasteras well, a background-job deserialisation race inBackgroundThreadAdapter. Not introduced here.