diff --git a/app/models/concerns/course/unique_external_id_concern.rb b/app/models/concerns/course/unique_external_id_concern.rb index c06993a7649..33093a52c55 100644 --- a/app/models/concerns/course/unique_external_id_concern.rb +++ b/app/models/concerns/course/unique_external_id_concern.rb @@ -7,6 +7,12 @@ module Course::UniqueExternalIdConcern extend ActiveSupport::Concern included do + # The invitation this record is being created from, if any. A record inheriting its external ID + # from the invitation that created it is not a conflict, so that invitation is excluded from the + # uniqueness check. Callers that set this must confirm or destroy the invitation in the same + # transaction, otherwise the two records are left sharing an external ID. + attr_accessor :source_invitation + before_validation :normalize_external_id validate :validate_unique_external_id_within_course, if: -> { new_record? || external_id_changed? } @@ -35,6 +41,7 @@ def validate_unique_external_id_within_course 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 diff --git a/app/models/user.rb b/app/models/user.rb index 41000928d5f..5872faf91e4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -166,6 +166,7 @@ def build_course_user_from_invitation(invitation) timeline_algorithm: invitation.timeline_algorithm || invitation.course&.default_timeline_algorithm, external_id: invitation.external_id.presence, + source_invitation: invitation, creator: self, updater: self) end diff --git a/spec/controllers/user/registration_controller_spec.rb b/spec/controllers/user/registration_controller_spec.rb index 26c268da868..ddccdf38885 100644 --- a/spec/controllers/user/registration_controller_spec.rb +++ b/spec/controllers/user/registration_controller_spec.rb @@ -154,6 +154,51 @@ end end end + + context 'when signing up through a course invitation' do + requires_login + + let(:course) { create(:course) } + let(:email) { generate(:email) } + let!(:invitation) do + create(:course_user_invitation, course: course, email: email, external_id: external_id) + end + + before { allow(controller).to receive(:verify_recaptcha).and_return(true) } + + subject do + post :create, params: { + invitation: invitation.invitation_key, + user: { name: 'New Student', email: email, + password: 'lolololol', password_confirmation: 'lolololol' } + } + end + + shared_examples 'a successful enrolment' do + it 'creates the user, enrols them once and confirms the invitation' do + expect { subject }.to change(User, :count).by(1).and change(CourseUser, :count).by(1) + + new_user = User.order(:created_at).last + course_user = CourseUser.find_by(course: course, user: new_user) + expect(course_user).to be_present + expect(course_user.external_id).to eq(external_id) + expect(invitation.reload).to be_confirmed + expect(invitation.confirmer).to eq(new_user) + end + end + + context 'when the invitation has no external_id' do + let(:external_id) { nil } + + it_behaves_like 'a successful enrolment' + end + + context 'when the invitation has an external_id' do + let(:external_id) { 'A0123456X' } + + it_behaves_like 'a successful enrolment' + end + end end end end diff --git a/spec/models/course_user_spec.rb b/spec/models/course_user_spec.rb index 366583d87a2..28b6a8abb8c 100644 --- a/spec/models/course_user_spec.rb +++ b/spec/models/course_user_spec.rb @@ -137,6 +137,36 @@ expect(student).to be_valid end end + + context 'when the course user is created from an invitation' do + let!(:invitation) { create(:course_user_invitation, course: course, external_id: 'invited-id') } + + # The invitation is confirmed in the same transaction that saves the course user, so a course + # user inheriting its external_id is not a conflict with the invitation it came from. + it 'is valid despite the still-unconfirmed invitation holding the same external_id' do + student = build(:course_student, course: course, external_id: 'invited-id', + source_invitation: invitation) + expect(student).to be_valid + end + + it 'is still invalid when a different pending invitation holds the external_id' do + other_invitation = create(:course_user_invitation, course: course, external_id: 'other-id') + student = build(:course_student, course: course, external_id: other_invitation.external_id, + source_invitation: invitation) + expect(student).not_to be_valid + expect(student.errors[:external_id]). + to include(I18n.t('activerecord.errors.models.course_user.attributes.external_id.taken')) + end + + it 'is still invalid when an enrolled course user holds the external_id' do + existing = create(:course_student, course: course, external_id: 'enrolled-id') + student = build(:course_student, course: course, external_id: existing.external_id, + source_invitation: invitation) + expect(student).not_to be_valid + expect(student.errors[:external_id]). + to include(I18n.t('activerecord.errors.models.course_user.attributes.external_id.taken')) + end + end end describe '.staff' do