Skip to content

fix(users): unblock sign up when the invitation carries an external ID - #8537

Merged
adi-herwana-nus merged 1 commit into
masterfrom
adi/external-id-registration-hotfix
Aug 5, 2026
Merged

fix(users): unblock sign up when the invitation carries an external ID#8537
adi-herwana-nus merged 1 commit into
masterfrom
adi/external-id-registration-hotfix

Conversation

@adi-herwana-nus

Copy link
Copy Markdown
Contributor

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::UniqueExternalIdConcern rejects 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:

def external_id_taken_by_invitation?
  query = Course::UserInvitation.unconfirmed.where(course_id: course_id, external_id: external_id)
  query = query.where.not(id: id) if is_a?(Course::UserInvitation)
  query.exists?
end

A CourseUser created from an invitation inherits that invitation's external_id, so it collides with its own invitation — unless the invitation has already been confirmed by the time the CourseUser is validated. Three paths create a CourseUser from an invitation, and they order those two steps differently:

creation path invitation confirmed outcome
Course::UserRegistrationService#accept_invitation before the CourseUser is built ok
User::Email#accept_all_pending_invitations before the CourseUser is built ok
User::RegistrationsController#create after resource.save returns rejected

The first two dodge the defect deliberately — User::Email even carries a comment naming this exact hazard:

# Confirm the invitation before saving the CourseUser so that the
# UniqueExternalIdConcern validation doesn't reject the new CourseUser
# for sharing an external_id with what is now a confirmed invitation.

The sign-up path cannot dodge it by reordering. build_resource calls User#build_from_invitation, which builds the CourseUser onto the unsaved User; has_many :course_users autosave-validates it during user.valid?, which precedes every after_save. The controller's @invitation.confirm! runs after super returns, and User::Email's hook fires during the save — both too late. Confirming earlier is not available either: confirm! sets confirmer to a User that does not yet have an id.

What the client sees. resource.save returns false, Devise responds 200 with an empty body, and SignUpPage falls through its !result.id branch to a generic errorSigningUp toast. The 422 branch 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:

  • The invitation carries a non-blank external_id. Blank is normalised to nil before validation and nil returns early, so invitations predating the feature are unaffected.
  • The invitee has no Coursemology account yet. Invited users who already have one enrol through UserRegistrationService or the User::Email hook, 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:

# Course::UniqueExternalIdConcern
attr_accessor :source_invitation

def external_id_taken_by_invitation?
  query = Course::UserInvitation.unconfirmed.where(course_id: course_id, external_id: external_id)
  query = query.where.not(id: id) if is_a?(Course::UserInvitation)
  query = query.where.not(id: source_invitation.id) if source_invitation&.persisted?
  query.exists?
end

User#build_course_user_from_invitation — the shared builder behind both the sign-up path and the User::Email hook — passes source_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_invitation is a plain attr_accessor, not an ActiveRecord attribute: it has no column, is never persisted or serialised, and appears in no permit list, so it cannot be set from a request.

Scope check

Audited every site that writes external_id or creates a CourseUser from an invitation.

  1. Course::UserRegistrationService#find_or_create_course_user! — confirms before building, so it never needed the exclusion. Left unchanged; setting source_invitation here too would guard against a future reordering of those two lines, at the cost of threading the invitation through create_course_user_record!.

  2. User::Email#accept_all_pending_invitations — confirms before building. Now also receives source_invitation via the shared builder, which is redundant but harmless: by then the invitation is confirmed and already outside the unconfirmed scope.

  3. Course::UserInvitationService (parse/process concerns) — tracks collisions itself through an in-memory @taken_external_ids set and never creates a CourseUser from an invitation record. Untouched.

  4. Course::UsersControllerManagementConcern — permits :external_id on update. Unaffected; those records have no source_invitation, so the full check applies.

No DB migration. The partial unique indexes on course_users and course_user_invitations are 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.rb gains sign-up-through-a-course-invitation coverage, parameterised over external_id nil and present, asserting the user is created, the CourseUser carries the external ID, and the invitation is confirmed by the new user. It asserts CourseUser.count changes by exactly 1, so a future change that lets both the eager build and the User::Email hook enrol the same user would be caught rather than silently double-enrolling.

course_user_spec.rb pins 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:42 fails on master as well, a background-job deserialisation race in BackgroundThreadAdapter. Not introduced here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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_invitation pointer in Course::UniqueExternalIdConcern to exclude that invitation from the “unconfirmed invitations” external ID collision query.
  • Pass source_invitation when building a CourseUser from an invitation in User#build_course_user_from_invitation.
  • Add model + controller specs covering sign-up-through-invitation for both external_id: nil and 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.

@adi-herwana-nus
adi-herwana-nus merged commit 23b31c4 into master Aug 5, 2026
9 of 10 checks passed
@adi-herwana-nus
adi-herwana-nus deleted the adi/external-id-registration-hotfix branch August 5, 2026 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants