diff --git a/app/controllers/concerns/course/users_controller_management_concern.rb b/app/controllers/concerns/course/users_controller_management_concern.rb index f7a3c47ac4f..0210c642a00 100644 --- a/app/controllers/concerns/course/users_controller_management_concern.rb +++ b/app/controllers/concerns/course/users_controller_management_concern.rb @@ -123,7 +123,7 @@ def should_update_personalized_timeline def course_user_params @course_user_params ||= params.require(:course_user).permit( - :user_id, :name, :timeline_algorithm, :role, :phantom, :reference_timeline_id + :user_id, :name, :timeline_algorithm, :role, :phantom, :reference_timeline_id, :external_id ) end diff --git a/app/controllers/course/user_invitations_controller.rb b/app/controllers/course/user_invitations_controller.rb index 440addfb908..4e1e99bf2ff 100644 --- a/app/controllers/course/user_invitations_controller.rb +++ b/app/controllers/course/user_invitations_controller.rb @@ -19,7 +19,8 @@ def create create_invitation_success(result) else propagate_errors - render json: { errors: current_course.errors.full_messages.to_sentence }, status: :bad_request + errors = current_course.errors[:base] + render json: { errors: errors }, status: :bad_request end end @@ -59,7 +60,8 @@ def course_user_invitation_params @course_user_invitation_params ||= begin params[:course] = { invitations_attributes: {} } unless params.key?(:course) params.require(:course).permit(:invitations_file, :registration_key, - invitations_attributes: [:name, :email, :role, :phantom, :timeline_algorithm]) + invitations_attributes: [:name, :email, :role, :phantom, + :timeline_algorithm, :external_id]) end end @@ -120,7 +122,7 @@ def invite_by_file? def invite invitation_service.invite(invitation_params) rescue CSV::MalformedCSVError => e - current_course.errors.add(:invitations_file, e.message) + current_course.errors.add(:base, e.message) false end @@ -135,15 +137,7 @@ def invitation_service # # @return [void] def propagate_errors - propagate_errors_to_file if invite_by_file? - end - - # Propagates errors from the generated records to the file. - # - # @return [void] - def propagate_errors_to_file - errors = aggregate_errors - current_course.errors.add(:invitations_file, errors.to_sentence) unless errors.empty? + aggregate_errors.each { |msg| current_course.errors.add(:base, msg) } end # Aggregates errors from all the known sources of failure. @@ -157,9 +151,24 @@ def aggregate_errors # # @return [Array] def invalid_course_user_errors - invalid_course_users.map do |course_user| - user = self.class.helpers.display_course_user(course_user) - t('errors.course.user_invitations.duplicate_user', user: user) + invalid_course_users.flat_map do |course_user| + email = course_user.user&.email || course_user.name + errors = [] + if course_user.errors.of_kind?(:external_id, :taken) + errors << t('errors.course.user_invitations.duplicate_external_id', + email: email, external_id: course_user.external_id) + end + if course_user.errors.of_kind?(:user, :taken) || course_user.errors.of_kind?(:course, :taken) + errors << t('errors.course.user_invitations.already_enrolled', email: email) + end + other_errors = course_user.errors.reject { |e| %w[external_id user course].include?(e.attribute.to_s) } + # .any? is intentional: this is a catch-all for unrecognised errors; all messages are forwarded verbatim + if other_errors.any? + message = other_errors.map { |e| course_user.errors.full_message(e.attribute, e.message) }. + to_sentence + errors << t('errors.course.user_invitations.other_error', email: email, message: message) + end + errors end end @@ -174,9 +183,29 @@ def invalid_course_users # # @return [Array] def invalid_invitation_email_errors - invalid_invitations.map do |invitation| - message = invitation.errors.full_messages.to_sentence - t('errors.course.user_invitations.invalid_email', email: invitation.email, message: message) + invalid_invitations.flat_map do |invitation| + email = invitation.email + errors = [] + if invitation.errors.of_kind?(:external_id, :taken) + errors << t('errors.course.user_invitations.duplicate_external_id', + email: email, external_id: invitation.external_id) + end + # .any? is intentional: invalid_email is a broad wrapper; the actual messages are forwarded in `message` + if invitation.errors[:email].any? + message = invitation.errors[:email].to_sentence + errors << t('errors.course.user_invitations.invalid_email', email: email, message: message) + end + if invitation.errors.of_kind?(:base, :existing_invitation) + errors << t('errors.course.user_invitations.duplicate_invitation', email: email) + end + other_errors = invitation.errors.reject { |e| %w[external_id email base].include?(e.attribute.to_s) } + # .any? is intentional: this is a catch-all for unrecognised errors; all messages are forwarded verbatim + if other_errors.any? + message = other_errors.map { |e| invitation.errors.full_message(e.attribute, e.message) }. + to_sentence + errors << t('errors.course.user_invitations.other_error', email: email, message: message) + end + errors end end @@ -254,7 +283,7 @@ def destroy_invitation_success def destroy_invitation_failure respond_to do |format| - format.json { render json: { errors: @invitation.errors.full_messages.to_sentence }, status: :bad_request } + format.json { render json: { errors: @invitation.errors.full_messages }, status: :bad_request } end end diff --git a/app/models/concerns/course/unique_external_id_concern.rb b/app/models/concerns/course/unique_external_id_concern.rb new file mode 100644 index 00000000000..03956a2b9a0 --- /dev/null +++ b/app/models/concerns/course/unique_external_id_concern.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true +# This concern validates that external IDs are unique within a course, +# across both course users and pending invitations. +# +# Nil and blank external IDs are allowed. +module Course::UniqueExternalIdConcern + extend ActiveSupport::Concern + + included do + before_validation :normalize_external_id + + validate :validate_unique_external_id_within_course, if: -> { new_record? || external_id_changed? } + end + + private + + # Normalizes blank external IDs to nil. + # + # @return [void] + def normalize_external_id + self.external_id = nil if external_id.blank? + end + + # Validates that the external ID is unique within the course, + # across both course users and invitations. + # + # @return [void] + def validate_unique_external_id_within_course + return if external_id.blank? + + invitation_exists = Course::UserInvitation. + unconfirmed. + where(course_id: course_id, external_id: external_id). + where.not(id: id). + exists? + + course_user_exists = CourseUser. + where(course_id: course_id, external_id: external_id). + where.not(id: id). + exists? + + return unless invitation_exists || course_user_exists + + errors.add(:external_id, :taken) + end +end diff --git a/app/models/course/user_invitation.rb b/app/models/course/user_invitation.rb index bfbede2d9d8..e7dab5a9caa 100644 --- a/app/models/course/user_invitation.rb +++ b/app/models/course/user_invitation.rb @@ -4,6 +4,8 @@ class Course::UserInvitation < ApplicationRecord after_initialize :set_defaults, if: :new_record? before_validation :set_defaults, if: :new_record? + include Course::UniqueExternalIdConcern + validates :email, format: { with: Devise.email_regexp }, if: :email_changed? validates :name, presence: true validates :role, presence: true diff --git a/app/models/course_user.rb b/app/models/course_user.rb index d4c9bd9bfc4..944fe25e09c 100644 --- a/app/models/course_user.rb +++ b/app/models/course_user.rb @@ -3,6 +3,7 @@ class CourseUser < ApplicationRecord include CourseUser::StaffConcern include CourseUser::LevelProgressConcern include CourseUser::TodoConcern + include Course::UniqueExternalIdConcern after_initialize :set_defaults, if: :new_record? before_validation :set_defaults, if: :new_record? diff --git a/app/models/user.rb b/app/models/user.rb index 9415e285035..85923635741 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -129,6 +129,7 @@ def build_course_user_from_invitation(invitation) phantom: invitation.phantom, timeline_algorithm: invitation.timeline_algorithm || invitation.course&.default_timeline_algorithm, + external_id: invitation.external_id, creator: self, updater: self) end diff --git a/app/models/user/email.rb b/app/models/user/email.rb index bcb71d25b0a..6f39fef2a57 100644 --- a/app/models/user/email.rb +++ b/app/models/user/email.rb @@ -35,8 +35,14 @@ def accept_all_pending_invitations unconfirmed_invitation.confirm!(confirmer: user) next end - user.build_course_user_from_invitation(unconfirmed_invitation) - unconfirmed_invitation.confirm!(confirmer: user) if user.save && user.persisted? + # 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. + CourseUser.transaction(requires_new: true) do + unconfirmed_invitation.confirm!(confirmer: user) + user.build_course_user_from_invitation(unconfirmed_invitation) + raise ActiveRecord::Rollback unless user.save && user.persisted? + end end end end diff --git a/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb b/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb index 0db98b9643e..d65bf61ec92 100644 --- a/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb +++ b/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb @@ -47,16 +47,23 @@ def parse_invitations(users) # ] def partition_unique_users(users) users.each { |user| user[:email] = user[:email].downcase } - unique_users = {} + seen_emails = Set.new + seen_external_ids = Set.new + unique_users = [] duplicate_users = [] users.each do |user| - if unique_users.key?(user[:email]) - duplicate_users.push(user) + ext_id = user[:external_id].presence + if seen_emails.include?(user[:email]) + duplicate_users.push(user.merge(reason: :duplicate_email_in_file)) + elsif ext_id && seen_external_ids.include?(ext_id) + duplicate_users.push(user.merge(reason: :duplicate_external_id_in_file)) else - unique_users[user[:email]] = user + seen_emails.add(user[:email]) + seen_external_ids.add(ext_id) if ext_id + unique_users << user end end - [unique_users.values, duplicate_users] + [unique_users, duplicate_users] end # Change all invitees' roles to :student if inviter is a teaching_assistant. @@ -86,7 +93,8 @@ def parse_from_form(users) email: value[:email], role: value[:role], phantom: phantom, - timeline_algorithm: value[:timeline_algorithm] } + timeline_algorithm: value[:timeline_algorithm], + external_id: value[:external_id] } end end @@ -144,11 +152,17 @@ def parse_file_row(row) return nil if row[1].blank? row[0] = row[1] if row[0].blank? - role = parse_file_role(row[2]) phantom = parse_file_phantom(row[3]) - timeline_algorithm = parse_file_timeline_algorithm(row[4]) - { name: row[0], email: row[1], role: role, phantom: phantom, timeline_algorithm: timeline_algorithm } + if @current_course.show_personalized_timeline_features? + timeline_algorithm = parse_file_timeline_algorithm(row[4]) + external_id = parse_file_external_id(row[5]) + else + external_id = parse_file_external_id(row[4]) + timeline_algorithm = parse_file_timeline_algorithm(nil) + end + { name: row[0], email: row[1], role: role, phantom: phantom, + timeline_algorithm: timeline_algorithm, external_id: external_id } end # Parses the role column from the CSV file. @@ -188,6 +202,17 @@ def parse_file_timeline_algorithm(timeline_algorithm) symbol || @current_course.default_timeline_algorithm end + # Parses file value for an invitation's external ID. + # Returns nil if value is not specified. + # + # @param [String|nil] external_id External ID as specified in the CSV file. + # @return [String|nil] The external ID string, or nil if blank. + def parse_file_external_id(external_id) + return nil if external_id.blank? + + external_id + end + # Removes the UTF-8 byte order mark (BOM) from the string. # The BOM exists at the start of in CSVs (optionally) to indicate the # encoding of the file. diff --git a/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb b/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb index 03c433f79d2..a93b6831135 100644 --- a/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb +++ b/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb @@ -20,11 +20,13 @@ module Course::UserInvitationService::ProcessInvitationConcern # @return # [Array<(Array, Array, Array, Array)>] # A tuple containing the users newly invited, already invited, newly registered and already registered respectively. + # Conflicts are accumulated into +@duplicate_users+ as a side effect. def process_invitations(users) + @taken_external_ids = load_existing_external_ids augment_user_objects(users) existing_users, new_users = users.partition { |user| user[:user].present? } - - [*invite_new_users(new_users), *add_existing_users(existing_users)] + [*invite_new_users(new_users), + *add_existing_users(existing_users)] end # Given an array of hashes containing the email address and name of a user to invite, finds the @@ -60,27 +62,37 @@ def find_existing_users(email_addresses) # and already enrolled. def add_existing_users(users) ensure_instance_users(users.map { |u| u[:user] }) - all_course_users = @current_course.course_users.to_h { |cu| [cu.user_id, cu] } existing_course_users = [] new_course_users = [] users.each do |user| - course_user = all_course_users[user[:user].id] - if course_user + if (course_user = all_course_users[user[:user].id]) existing_course_users << course_user else - new_course_users << - @current_course.course_users.build(user: user[:user], name: user[:name], - role: user[:role], phantom: user[:phantom], - timeline_algorithm: @current_course.default_timeline_algorithm, - creator: @current_user, updater: @current_user) - @current_course.enrol_requests.pending.find_by(user: user[:user].id)&.destroy! + enroll_new_user(user, user[:external_id].presence, new_course_users) end end - [new_course_users, existing_course_users] end + def enroll_new_user(user, ext_id, new_course_users) + if ext_id && @taken_external_ids.include?(ext_id) + @duplicate_users.push(user.merge(reason: :external_id_taken)) + else + @taken_external_ids.add(ext_id) if ext_id + new_course_users << build_course_user(user) + @current_course.enrol_requests.pending.find_by(user: user[:user].id)&.destroy! + end + end + + def build_course_user(user) + @current_course.course_users.build(user: user[:user], name: user[:name], + role: user[:role], phantom: user[:phantom], + timeline_algorithm: @current_course.default_timeline_algorithm, + external_id: user[:external_id], + creator: @current_user, updater: @current_user) + end + # Ensures that all users have instance user records for the current instance. # # @param [Array] users The users to ensure have instance users. @@ -96,6 +108,15 @@ def ensure_instance_users(users) InstanceUser.insert_all(missing_instance_users) end + def load_existing_external_ids + course_ext_ids = CourseUser.where(course: @current_course). + where.not(external_id: nil).pluck(:external_id) + invitation_ext_ids = Course::UserInvitation.unconfirmed. + where(course: @current_course). + where.not(external_id: nil).pluck(:external_id) + Set.new(course_ext_ids + invitation_ext_ids) + end + # Generates invitations for users to the course. # # @param [Array] users The user descriptions to invite. @@ -110,13 +131,25 @@ def invite_new_users(users) if invitation existing_invitations << invitation else - new_invitations << - @current_course.invitations.build(name: user[:name], email: user[:email], - role: user[:role], phantom: user[:phantom], - timeline_algorithm: user[:timeline_algorithm]) + add_to_new_invitations(user, user[:external_id].presence, new_invitations) end end - [new_invitations, existing_invitations] end + + def add_to_new_invitations(user, ext_id, new_invitations) + if ext_id && @taken_external_ids.include?(ext_id) + @duplicate_users.push(user.merge(reason: :external_id_taken)) + else + @taken_external_ids.add(ext_id) if ext_id + new_invitations << build_invitation(user) + end + end + + def build_invitation(user) + @current_course.invitations.build(name: user[:name], email: user[:email], + role: user[:role], phantom: user[:phantom], + timeline_algorithm: user[:timeline_algorithm], + external_id: user[:external_id]) + end end diff --git a/app/services/course/statistics/assessments_score_summary_download_service.rb b/app/services/course/statistics/assessments_score_summary_download_service.rb index c42f4812517..f3375bbf6d0 100644 --- a/app/services/course/statistics/assessments_score_summary_download_service.rb +++ b/app/services/course/statistics/assessments_score_summary_download_service.rb @@ -46,6 +46,7 @@ def load_total_grades @submission_grade_hash = submission_grade_hash @all_students = @course.course_users.students.order_alphabetically.preload(user: :emails) + @include_external_id = @course.course_users.students.where.not(external_id: [nil, '']).exists? end def submission_grade_hash @@ -67,15 +68,15 @@ def download_score_summary(csv) I18n.t('csv.score_summary.headers.name'), I18n.t('csv.score_summary.headers.email'), I18n.t('csv.score_summary.headers.type'), + *(@include_external_id ? [I18n.t('csv.score_summary.headers.external_id')] : []), *@assessments.map(&:title) ] # content @all_students.each do |student| csv << [student.name, student.user.email, student.phantom? ? 'phantom' : 'normal', - *@assessments.flat_map do |assessment| - @submission_grade_hash[[student.id, assessment.id]] || '' - end] + *(@include_external_id ? [student.external_id || ''] : []), + *@assessments.flat_map { |a| @submission_grade_hash[[student.id, a.id]] || '' }] end end end diff --git a/app/services/course/user_invitation_service.rb b/app/services/course/user_invitation_service.rb index 12153432659..eb77da78879 100644 --- a/app/services/course/user_invitation_service.rb +++ b/app/services/course/user_invitation_service.rb @@ -75,7 +75,8 @@ def resend_invitation(invitations) # newly registered and already registered, and duplicate users respectively. # @raise [CSV::MalformedCSVError] When the file provided is invalid. def invite_users(users) - users, duplicate_users = parse_invitations(users) - process_invitations(users) + [duplicate_users] + unique_users, parse_duplicates = parse_invitations(users) + @duplicate_users = parse_duplicates + process_invitations(unique_users) + [@duplicate_users] end end diff --git a/app/services/course/user_registration_service.rb b/app/services/course/user_registration_service.rb index c397c19f025..8f5d9c7378c 100644 --- a/app/services/course/user_registration_service.rb +++ b/app/services/course/user_registration_service.rb @@ -48,14 +48,25 @@ def register_without_registration_code(registration) # @param [Course::UserInvitation] invitation The invitation from which we are creating a course user from. # @return [CourseUser] The Course User object which was found or created. def find_or_create_course_user!(registration, invitation = nil) - name = invitation.try(:name) || registration.user.name - role = invitation.try(:role) || :student - phantom = invitation.try(:phantom) || false - timeline_algorithm = invitation.try(:timeline_algorithm) || registration.course.default_timeline_algorithm + attrs = { + name: invitation.try(:name) || registration.user.name, + role: invitation.try(:role) || :student, + phantom: invitation.try(:phantom) || false, + timeline_algorithm: invitation.try(:timeline_algorithm) || registration.course.default_timeline_algorithm, + external_id: invitation.try(:external_id).presence + } + registration.course_user = create_course_user_record!(registration, attrs) + registration.course_user + end - registration.course_user = - CourseUser.find_or_create_by!(course: registration.course, user: registration.user, - name: name, role: role, phantom: phantom, timeline_algorithm: timeline_algorithm) + def create_course_user_record!(registration, attrs) + CourseUser.find_or_create_by!(course: registration.course, user: registration.user) do |cu| + cu.name = attrs[:name] + cu.role = attrs[:role] + cu.phantom = attrs[:phantom] + cu.timeline_algorithm = attrs[:timeline_algorithm] + cu.external_id = attrs[:external_id] + end end # Claims a given registration code. The correct type of code is deduced from the code itself and diff --git a/app/views/course/statistics/aggregate/all_students.json.jbuilder b/app/views/course/statistics/aggregate/all_students.json.jbuilder index d37f5b374c2..256694a7396 100644 --- a/app/views/course/statistics/aggregate/all_students.json.jbuilder +++ b/app/views/course/statistics/aggregate/all_students.json.jbuilder @@ -6,6 +6,7 @@ can_analyze_videos = can?(:analyze_videos, current_course) is_course_gamified = current_course.gamified? no_group_managers = @service.no_group_managers? has_my_students = false +has_external_ids = current_course.course_users.students.where.not(external_id: [nil, '']).exists? json.students @all_students do |student| is_my_student = false @@ -13,6 +14,7 @@ json.students @all_students do |student| json.name student.name json.nameLink course_user_path(current_course, student) json.email student.user.email + json.externalId student.external_id json.studentType student.phantom? ? 'Phantom' : 'Normal' unless no_group_managers @@ -48,6 +50,7 @@ json.metadata do json.courseVideoCount course_video_count json.hasGroupManagers !no_group_managers json.hasMyStudents has_my_students + json.hasExternalIds has_external_ids json.showRedirectToMissionControl current_course.component_enabled?(Course::StoriesComponent) && can?(:access_mission_control, current_course) end diff --git a/app/views/course/user_invitations/_course_user_invitation_list_data.json.jbuilder b/app/views/course/user_invitations/_course_user_invitation_list_data.json.jbuilder index fa5593e906f..886056a10d8 100644 --- a/app/views/course/user_invitations/_course_user_invitation_list_data.json.jbuilder +++ b/app/views/course/user_invitations/_course_user_invitation_list_data.json.jbuilder @@ -3,6 +3,7 @@ json.id invitation.id json.name invitation.name json.email invitation.email +json.externalId invitation.external_id json.role invitation.role json.phantom invitation.phantom json.timelineAlgorithm invitation.timeline_algorithm diff --git a/app/views/course/user_invitations/_invitation_result_data.json.jbuilder b/app/views/course/user_invitations/_invitation_result_data.json.jbuilder index 683da5be9da..421b05dc99d 100644 --- a/app/views/course/user_invitations/_invitation_result_data.json.jbuilder +++ b/app/views/course/user_invitations/_invitation_result_data.json.jbuilder @@ -4,6 +4,7 @@ json.newInvitations new_invitations.each do |invitation| json.id invitation.id json.name invitation.name json.email invitation.email + json.externalId invitation.external_id json.role invitation.role json.phantom invitation.phantom json.sentAt invitation.sent_at @@ -13,6 +14,7 @@ json.existingInvitations existing_invitations.each do |invitation| json.id invitation.id json.name invitation.name json.email invitation.email + json.externalId invitation.external_id json.role invitation.role json.phantom invitation.phantom json.sentAt invitation.sent_at @@ -22,6 +24,7 @@ json.newCourseUsers new_course_users.each do |course_user| json.id course_user.id if course_user.id json.name course_user.name.strip json.email course_user.user.email + json.externalId course_user.external_id json.role course_user.role json.phantom course_user.phantom? end @@ -30,6 +33,7 @@ json.existingCourseUsers existing_course_users.each do |course_user| json.id course_user.id if course_user.id json.name course_user.name.strip json.email course_user.user.email + json.externalId course_user.external_id json.role course_user.role json.phantom course_user.phantom? end @@ -38,6 +42,8 @@ json.duplicateUsers duplicate_users.each do |duplicate_user, index| json.id index json.name duplicate_user[:name] json.email duplicate_user[:email] + json.externalId duplicate_user[:external_id] json.role duplicate_user[:role] json.phantom duplicate_user[:phantom] + json.reason duplicate_user[:reason] end diff --git a/app/views/course/users/_user_list_data.json.jbuilder b/app/views/course/users/_user_list_data.json.jbuilder index 558dcb055e7..bd5400d113a 100644 --- a/app/views/course/users/_user_list_data.json.jbuilder +++ b/app/views/course/users/_user_list_data.json.jbuilder @@ -16,3 +16,4 @@ json.timelineAlgorithm course_user.timeline_algorithm if should_show_timeline json.role course_user.role json.phantom course_user.phantom? if should_show_phantom +json.externalId course_user.external_id diff --git a/client/app/assets/templates/course-user-invitation-template-no-timeline.csv b/client/app/assets/templates/course-user-invitation-template-no-timeline.csv new file mode 100644 index 00000000000..6874f099d4c --- /dev/null +++ b/client/app/assets/templates/course-user-invitation-template-no-timeline.csv @@ -0,0 +1,3 @@ +Name,Email,Role,Phantom,ExternalId +John,test1@example.com,student,y,a01234567 +Mary,test2@example.com,teaching_assistant,n,a01234568 diff --git a/client/app/assets/templates/course-user-invitation-template.csv b/client/app/assets/templates/course-user-invitation-template.csv index 05923277b94..4e0ee4106ed 100644 --- a/client/app/assets/templates/course-user-invitation-template.csv +++ b/client/app/assets/templates/course-user-invitation-template.csv @@ -1,3 +1,3 @@ -Name,Email,Role,Phantom,Timeline -John,test1@example.com,student,y,otot -Mary,test2@example.com,teaching_assistant,n,fixed \ No newline at end of file +Name,Email,Role,Phantom,Timeline,ExternalId +John,test1@example.com,student,y,otot,a01234567 +Mary,test2@example.com,teaching_assistant,n,fixed,a01234568 \ No newline at end of file diff --git a/client/app/bundles/course/helper/index.ts b/client/app/bundles/course/helper/index.ts index 84b9b02d036..01eb52ba487 100644 --- a/client/app/bundles/course/helper/index.ts +++ b/client/app/bundles/course/helper/index.ts @@ -1,5 +1,6 @@ import courseDefaultLogoUrl from 'assets/images/course-default-logo.svg?url'; import courseUserInvitationTemplateUrl from 'assets/templates/course-user-invitation-template.csv?url'; +import courseUserInvitationTemplateNoTimelineUrl from 'assets/templates/course-user-invitation-template-no-timeline.csv?url'; export const getCourseLogoUrl = (url?: string | null): string => { if (!url) { @@ -8,6 +9,10 @@ export const getCourseLogoUrl = (url?: string | null): string => { return url; }; -export const getCourseUserInviteTemplatePath = (): string => { - return courseUserInvitationTemplateUrl; +export const getCourseUserInviteTemplatePath = ( + hasPersonalTimelines: boolean, +): string => { + return hasPersonalTimelines + ? courseUserInvitationTemplateUrl + : courseUserInvitationTemplateNoTimelineUrl; }; diff --git a/client/app/bundles/course/statistics/pages/StatisticsIndex/students/StudentStatisticsTable.tsx b/client/app/bundles/course/statistics/pages/StatisticsIndex/students/StudentStatisticsTable.tsx index ce3a3f11b37..5f22c48ce17 100644 --- a/client/app/bundles/course/statistics/pages/StatisticsIndex/students/StudentStatisticsTable.tsx +++ b/client/app/bundles/course/statistics/pages/StatisticsIndex/students/StudentStatisticsTable.tsx @@ -15,6 +15,7 @@ import { NUM_CELL_CLASS_NAME, } from 'lib/constants/sharedConstants'; import useTranslation from 'lib/hooks/useTranslation'; +import tableTranslations from 'lib/translations/table'; const translations = defineMessages({ name: { @@ -76,6 +77,7 @@ const StudentsStatisticsTable: FC = (props) => { showVideo, courseVideoCount, hasGroupManagers, + hasExternalIds, showRedirectToMissionControl, }, students, @@ -135,6 +137,17 @@ const StudentsStatisticsTable: FC = (props) => { }, ]; + if (hasExternalIds) { + columns.push({ + of: 'externalId', + title: t(tableTranslations.externalId), + sortable: true, + searchable: true, + cell: (student) => student.externalId ?? '', + csvDownloadable: true, + }); + } + if (hasGroupManagers) { columns.push({ of: 'groupManagers', diff --git a/client/app/bundles/course/statistics/types.ts b/client/app/bundles/course/statistics/types.ts index 4e8118daac6..8dea0dfc4bd 100644 --- a/client/app/bundles/course/statistics/types.ts +++ b/client/app/bundles/course/statistics/types.ts @@ -13,6 +13,7 @@ export interface Student { name: string; nameLink: string; email: string; + externalId?: string | null; studentType: 'Phantom' | 'Normal'; isMyStudent: boolean; groupManagers?: GroupManager[]; @@ -50,6 +51,7 @@ export interface Metadata { courseVideoCount: number; hasGroupManagers: boolean; hasMyStudents: boolean; + hasExternalIds: boolean; showRedirectToMissionControl: boolean; } diff --git a/client/app/bundles/course/user-invitations/components/buttons/InvitationActionButtons.tsx b/client/app/bundles/course/user-invitations/components/buttons/InvitationActionButtons.tsx index 303bb904153..87d6ecd1f81 100644 --- a/client/app/bundles/course/user-invitations/components/buttons/InvitationActionButtons.tsx +++ b/client/app/bundles/course/user-invitations/components/buttons/InvitationActionButtons.tsx @@ -27,7 +27,7 @@ const translations = defineMessages({ }, resendFailure: { id: 'course.userInvitations.InvitationActionButtons.resendFailure', - defaultMessage: 'Failed to resend invitation - {error}', + defaultMessage: 'Failed to resend invitation.', }, deletionTooltip: { id: 'course.userInvitations.InvitationActionButtons.deletionTooltip', @@ -46,6 +46,10 @@ const translations = defineMessages({ id: 'course.userInvitations.InvitationActionButtons.deletionFailure', defaultMessage: 'Failed to delete user - {error}', }, + deletionFailureGeneric: { + id: 'course.userInvitations.InvitationActionButtons.deletionFailureGeneric', + defaultMessage: 'Failed to delete user.', + }, }); const InvitationActionButtons: FC = (props) => { @@ -65,15 +69,9 @@ const InvitationActionButtons: FC = (props) => { }), ); }) - .catch((error) => { - const errorMessage = error.response?.data?.errors - ? error.response.data.errors - : ''; - toast.error( - t(translations.resendFailure, { - error: errorMessage, - }), - ); + .catch(() => { + // resend failure endpoints return head :bad_request with no body + toast.error(t(translations.resendFailure)); }) .finally(() => setIsResending(false)); }; @@ -90,15 +88,16 @@ const InvitationActionButtons: FC = (props) => { }) .catch((error) => { setIsDeleting(false); - const errorMessage = error.response?.data?.errors - ? error.response.data.errors - : ''; - toast.error( - t(translations.deletionFailure, { - error: errorMessage, - }), - ); - throw error; + const rawErrors = error.response?.data?.errors; + let errorList: string[]; + if (Array.isArray(rawErrors)) errorList = rawErrors; + else if (typeof rawErrors === 'string') errorList = [rawErrors]; + else errorList = []; + if (errorList[0]) { + toast.error(t(translations.deletionFailure, { error: errorList[0] })); + } else { + toast.error(t(translations.deletionFailureGeneric)); + } }); }; diff --git a/client/app/bundles/course/user-invitations/components/forms/IndividualInvitation.tsx b/client/app/bundles/course/user-invitations/components/forms/IndividualInvitation.tsx index 7e7e88c168d..25b7bfb2cdc 100644 --- a/client/app/bundles/course/user-invitations/components/forms/IndividualInvitation.tsx +++ b/client/app/bundles/course/user-invitations/components/forms/IndividualInvitation.tsx @@ -134,6 +134,20 @@ const IndividualInvitation: FC = (props) => { )} /> )} + ( + + )} + /> void; } @@ -46,7 +57,8 @@ const validationSchema = yup.object({ }); const IndividualInviteForm: FC = (props) => { - const { openResultDialog, intl } = props; + const { openResultDialog } = props; + const { t } = useTranslation(); const [isLoading, setIsLoading] = useState(false); const dispatch = useAppDispatch(); const sharedData = useAppSelector(getManageCourseUsersSharedData); @@ -109,8 +121,22 @@ const IndividualInviteForm: FC = (props) => { reset(initialValues); openResultDialog(response); }) - .catch(() => { - toast.error(intl.formatMessage(messagesTranslations.formUpdateError)); + .catch((error) => { + const rawErrors = error.response?.data?.errors; + let errorList: string[]; + if (Array.isArray(rawErrors)) errorList = rawErrors; + else if (typeof rawErrors === 'string') errorList = [rawErrors]; + else errorList = []; + const first = errorList[0]; + const overflow = + errorList.length > 1 ? ` (and ${errorList.length - 1} more)` : ''; + if (first) { + toast.error(t(translations.failure, { error: first + overflow }), { + autoClose: false, + }); + } else { + toast.error(t(translations.failureGeneric), { autoClose: false }); + } }) .finally(() => { setIsLoading(false); @@ -139,4 +165,4 @@ const IndividualInviteForm: FC = (props) => { ); }; -export default injectIntl(IndividualInviteForm); +export default IndividualInviteForm; diff --git a/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx b/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx index e1958786c5f..8c6260c4434 100644 --- a/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx +++ b/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx @@ -52,11 +52,11 @@ const translations = defineMessages({ duplicateInfo: { id: 'course.userInvitations.InvitationResultDialog.duplicateInfo', defaultMessage: - 'Duplicate users were found in the invitation. Only the first instance of this user will be invited.', + 'Duplicate users were found in the invitation. Only the first instance of each user will be invited.', }, duplicateUsers: { id: 'course.userInvitations.InvitationResultDialog.duplicateUsers', - defaultMessage: 'Users with Duplicate Emails ({count})', + defaultMessage: 'Duplicate Users ({count})', }, existingCourseUsersInfo: { id: 'course.userInvitations.InvitationResultDialog.existingCourseUsersInfo', diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx index 2ee9ce1cd37..fb70f544196 100644 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx @@ -21,6 +21,8 @@ const InvitationResultInvitationsTable: FC = (props) => { if (invitations && invitations.length === 0) return null; + const showExternalId = invitations.some((i) => i.externalId != null); + const options: TableOptions = { download: true, filter: false, @@ -69,6 +71,18 @@ const InvitationResultInvitationsTable: FC = (props) => { sort: false, }, }, + ...(showExternalId + ? [ + { + name: 'externalId', + label: t(tableTranslations.externalId), + options: { + alignCenter: true, + sort: false, + }, + }, + ] + : []), { name: 'phantom', label: t(tableTranslations.phantom), diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx index 5164af7ec63..c8b0728fd6c 100644 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx @@ -1,17 +1,34 @@ import { FC, memo } from 'react'; +import { defineMessages } from 'react-intl'; import { Typography } from '@mui/material'; import equal from 'fast-deep-equal'; import { TableColumns, TableOptions } from 'types/components/DataTable'; -import { CourseUserData } from 'types/course/courseUsers'; +import { CourseUserListData } from 'types/course/courseUsers'; +import { DuplicateReason } from 'types/course/userInvitations'; import DataTable from 'lib/components/core/layouts/DataTable'; import useTranslation from 'lib/hooks/useTranslation'; import roleTranslations from 'lib/translations/course/users/roles'; import tableTranslations from 'lib/translations/table'; +const translations = defineMessages({ + duplicateEmailInFile: { + id: 'course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile', + defaultMessage: 'Duplicate email in upload', + }, + duplicateExternalIdInFile: { + id: 'course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile', + defaultMessage: 'Duplicate external ID in upload', + }, + externalIdTaken: { + id: 'course.userInvitations.InvitationResultUsersTable.externalIdTaken', + defaultMessage: 'External ID is already assigned to another course member', + }, +}); + interface Props { title: JSX.Element; - users: CourseUserData[]; + users: Array; } const InvitationResultUsersTable: FC = (props) => { @@ -20,6 +37,9 @@ const InvitationResultUsersTable: FC = (props) => { if (users && users.length === 0) return null; + const showExternalId = users.some((u) => u.externalId != null); + const showReason = users.some((u) => u.reason != null); + const options: TableOptions = { download: true, filter: false, @@ -66,6 +86,52 @@ const InvitationResultUsersTable: FC = (props) => { sort: false, }, }, + ...(showExternalId + ? [ + { + name: 'externalId', + label: t(tableTranslations.externalId), + options: { + alignCenter: true, + sort: false, + }, + }, + ] + : []), + ...(showReason + ? [ + { + name: 'reason', + label: t(tableTranslations.reason), + options: { + alignCenter: false, + sort: false, + customBodyRenderLite: (dataIndex: number): JSX.Element => { + const user = users[dataIndex]; + const reasonText = + { + duplicate_email_in_file: t( + translations.duplicateEmailInFile, + ), + duplicate_external_id_in_file: t( + translations.duplicateExternalIdInFile, + ), + external_id_taken: t(translations.externalIdTaken), + }[user.reason ?? ''] ?? ''; + return ( + + {reasonText} + + ); + }, + }, + }, + ] + : []), { name: 'phantom', label: t(tableTranslations.phantom), diff --git a/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx b/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx index a033d128c9a..8750fd0caf2 100644 --- a/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx @@ -135,6 +135,10 @@ const FailedChip: FC<{ sentAt: string }> = ({ sentAt }) => { const UserInvitationsTable: FC = (props) => { const { invitations } = props; + const showExternalId = invitations.some( + (invitation) => invitation.externalId != null, + ); + const { t } = useTranslation(); const permissions = useAppSelector(getManageCourseUserPermissions); @@ -158,6 +162,17 @@ const UserInvitationsTable: FC = (props) => { searchable: true, cell: (datum) => datum.email, }, + ...(showExternalId + ? [ + { + of: 'externalId', + title: t(tableTranslations.externalId ?? null), + sortable: false, + searchable: true, + cell: (datum) => datum.externalId ?? null, + } satisfies ColumnTemplate, + ] + : []), { of: 'role', title: t(tableTranslations.role), @@ -263,6 +278,9 @@ const UserInvitationsTable: FC = (props) => { data={processedInvitations} getRowId={(datum) => datum.id.toString()} indexing={{ indices: true }} + search={{ + searchPlaceholder: t(translations.searchText), + }} sort={{ initially: { by: 'status', order: 'asc' }, }} diff --git a/client/app/bundles/course/user-invitations/components/tables/__test__/UserInvitationsTable.test.tsx b/client/app/bundles/course/user-invitations/components/tables/__test__/UserInvitationsTable.test.tsx new file mode 100644 index 00000000000..62bde5be532 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/__test__/UserInvitationsTable.test.tsx @@ -0,0 +1,115 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { InvitationMiniEntity } from 'types/course/userInvitations'; + +import UserInvitationsTable from '../UserInvitationsTable'; + +const baseInvitation: InvitationMiniEntity = { + id: 1, + name: 'Alice Lim', + email: 'alice@example.com', + externalId: null, + role: 'student', + phantom: false, + confirmed: false, + isRetryable: true, + invitationKey: 'KEY001', + sentAt: '2024-01-01T00:00:00Z', + confirmedAt: null, +}; + +const SEARCH_PLACEHOLDER = 'Search by name, email or external ID'; + +describe('', () => { + describe('search', () => { + it('filters invitations by external ID', async () => { + const user = userEvent.setup(); + const invitations: InvitationMiniEntity[] = [ + { + ...baseInvitation, + id: 1, + name: 'Alice Lim', + externalId: 'EXT-ALICE', + }, + { + ...baseInvitation, + id: 2, + name: 'Bob Tan', + email: 'bob@example.com', + externalId: 'EXT-BOB', + }, + ]; + + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + await user.type( + screen.getByPlaceholderText(SEARCH_PLACEHOLDER), + 'EXT-ALICE', + ); + + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + }); + + it('searches external ID case-insensitively', async () => { + const user = userEvent.setup(); + const invitations: InvitationMiniEntity[] = [ + { + ...baseInvitation, + id: 1, + name: 'Alice Lim', + externalId: 'ext-alice', + }, + { + ...baseInvitation, + id: 2, + name: 'Bob Tan', + email: 'bob@example.com', + externalId: 'EXT-BOB', + }, + ]; + + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + await user.type( + screen.getByPlaceholderText(SEARCH_PLACEHOLDER), + 'EXT-ALICE', + ); + + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + }); + + it('shows all invitations when search is cleared', async () => { + const user = userEvent.setup(); + const invitations: InvitationMiniEntity[] = [ + { + ...baseInvitation, + id: 1, + name: 'Alice Lim', + externalId: 'EXT-ALICE', + }, + { + ...baseInvitation, + id: 2, + name: 'Bob Tan', + email: 'bob@example.com', + externalId: 'EXT-BOB', + }, + ]; + + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + const searchInput = screen.getByPlaceholderText(SEARCH_PLACEHOLDER); + await user.type(searchInput, 'EXT-ALICE'); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + + await user.clear(searchInput); + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.getByText('Bob Tan')).toBeInTheDocument(); + }); + }); +}); diff --git a/client/app/bundles/course/user-invitations/operations.ts b/client/app/bundles/course/user-invitations/operations.ts index 49ad29fb24a..d9a0c409114 100644 --- a/client/app/bundles/course/user-invitations/operations.ts +++ b/client/app/bundles/course/user-invitations/operations.ts @@ -21,24 +21,32 @@ const formatInvitations = (invitations: InvitationPostData[]): FormData => { const payload = new FormData(); invitations.forEach((invite, index) => { - ['name', 'email', 'role', 'phantom', 'timelineAlgorithm'].forEach( - (field) => { - if (invite[field] !== undefined && invite[field] !== null) { - let fieldName = field; - let value = invite[field]; - if (field === 'timelineAlgorithm') { - fieldName = 'timeline_algorithm'; - } - if (field === 'phantom') { - value = value ? 1 : 0; - } - payload.append( - `course[invitations_attributes][${index}][${fieldName}]`, - value, - ); + [ + 'name', + 'email', + 'role', + 'phantom', + 'timelineAlgorithm', + 'externalId', + ].forEach((field) => { + if (invite[field] !== undefined && invite[field] !== null) { + let fieldName = field; + let value = invite[field]; + if (field === 'timelineAlgorithm') { + fieldName = 'timeline_algorithm'; } - }, - ); + if (field === 'externalId') { + fieldName = 'external_id'; + } + if (field === 'phantom') { + value = value ? 1 : 0; + } + payload.append( + `course[invitations_attributes][${index}][${fieldName}]`, + value, + ); + } + }); }); return payload; }; diff --git a/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx index 656ae9099a8..f6dc1980458 100644 --- a/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx +++ b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx @@ -52,6 +52,16 @@ const translations = defineMessages({ 'Personal Timelines can be [fixed, otot, stragglers, fomo],\ with course default: {defaultTimelineAlgorithm} if omitted.', }, + fileUploadInfoEmail: { + id: 'course.userInvitations.InviteUsersFileUpload.fileUploadInfoEmail', + defaultMessage: + 'Each invitation must use a unique email address within the course. Duplicate emails will be skipped.', + }, + fileUploadInfoExternalId: { + id: 'course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId', + defaultMessage: + 'External ID is an optional field for linking a user to an external system. If provided, it must be unique within the course - duplicate external IDs will be skipped.', + }, exampleHeader: { id: 'course.userInvitations.InviteUsersFileUpload.exampleHeader', defaultMessage: 'Example ', @@ -63,22 +73,27 @@ const translations = defineMessages({ fileUploadExample: { id: 'course.userInvitations.InviteUsersFileUpload.fileUploadExample', defaultMessage: - 'Name,Email[,Role,Phantom]' + - '{br}John,test1@example.org[,student,y]' + - '{br}Mary,test2@example.org[,teaching_assistant,n]', + 'Name,Email,Role,Phantom,ExternalId' + + '{br}John,test1@example.org[,student,y,A0123456' + + '{br}Mary,test2@example.org[,teaching_assistant,n,A0123457', }, fileUploadExamplePersonalTimeline: { id: 'course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline', defaultMessage: - 'Name,Email[,Role,Phantom,PersonalTimeline]' + - '{br}John,test1@example.org[,student,y,otot]' + - '{br}Mary,test2@example.org[,teaching_assistant,n,fixed]', + 'Name,Email,Role,Phantom,PersonalTimeline,ExternalId' + + '{br}John,test1@example.org,student,y,otot,A0123456' + + '{br}Mary,test2@example.org,teaching_assistant,n,fixed,A0123457', }, failure: { id: 'course.userInvitations.InviteUsersFileUpload.failure', defaultMessage: 'Failed to invite users. Please ensure your data is formatted correctly - {error}', }, + failureGeneric: { + id: 'course.userInvitations.InviteUsersFileUpload.failureGeneric', + defaultMessage: + 'Failed to invite users. Please ensure your data is formatted correctly.', + }, }); const InviteUsersFileUpload: FC = (props) => { @@ -102,15 +117,21 @@ const InviteUsersFileUpload: FC = (props) => { openResultDialog(response); }) .catch((error) => { - const errorMessage = error.response?.data?.errors - ? error.response.data.errors - : ''; - toast.error( - t(translations.failure, { - error: errorMessage, - }), - { autoClose: false }, - ); + const rawErrors = error.response?.data?.errors; + let errorList: string[]; + if (Array.isArray(rawErrors)) errorList = rawErrors; + else if (typeof rawErrors === 'string') errorList = [rawErrors]; + else errorList = []; + const first = errorList[0]; + const overflow = + errorList.length > 1 ? ` (and ${errorList.length - 1} more)` : ''; + if (first) { + toast.error(t(translations.failure, { error: first + overflow }), { + autoClose: false, + }); + } else { + toast.error(t(translations.failureGeneric), { autoClose: false }); + } }); }; @@ -118,6 +139,11 @@ const InviteUsersFileUpload: FC = (props) => { <> {t(translations.fileUploadInfo)}
    +
  • + + {t(translations.fileUploadInfoEmail)} + +
  • @@ -141,12 +167,19 @@ const InviteUsersFileUpload: FC = (props) => {
    • )} +
  • + + + +
{t(translations.exampleHeader)} diff --git a/client/app/bundles/course/user-invitations/translations.ts b/client/app/bundles/course/user-invitations/translations.ts index c3c249f6b4c..0569076b635 100644 --- a/client/app/bundles/course/user-invitations/translations.ts +++ b/client/app/bundles/course/user-invitations/translations.ts @@ -22,6 +22,10 @@ const translations = defineMessages({ id: 'course.userInvitations.UserInvitationsTable.noInvitations', defaultMessage: 'There are no invitations.', }, + searchText: { + id: 'course.userInvitations.UserInvitationsTable.searchText', + defaultMessage: 'Search by name, email or external ID', + }, pending: { id: 'course.userInvitations.UserInvitationsTable.pending', defaultMessage: 'Pending', diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/ExternalIdField.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/ExternalIdField.tsx new file mode 100644 index 00000000000..b049d26efbe --- /dev/null +++ b/client/app/bundles/course/users/components/tables/ManageUsersTable/ExternalIdField.tsx @@ -0,0 +1,72 @@ +import { memo } from 'react'; +import equal from 'fast-deep-equal'; +import { CourseUserMiniEntity } from 'types/course/courseUsers'; + +import { updateUser } from 'bundles/course/users/operations'; +import InlineEditTextField from 'lib/components/form/fields/DataTableInlineEditable/TextField'; +import { useAppDispatch } from 'lib/hooks/store'; +import toast from 'lib/hooks/toast'; +import useTranslation from 'lib/hooks/useTranslation'; + +import translations from '../../../translations'; + +interface ExternalIdFieldProps { + for: CourseUserMiniEntity; +} + +const ExternalIdField = (props: ExternalIdFieldProps): JSX.Element => { + const { for: user } = props; + + const dispatch = useAppDispatch(); + + const { t } = useTranslation(); + + const handleIdUpdate = (id: string): Promise => { + return dispatch(updateUser(user.id, { externalId: id || null })) + .then(() => { + if (!id && user.externalId) { + toast.success(t(translations.deleteIdSuccess)); + } else if (id && !user.externalId) { + toast.success(t(translations.addIdSuccess, { newId: id })); + } else { + toast.success( + t(translations.changeIdSuccess, { + oldId: user.externalId ?? '', + newId: id, + }), + ); + } + }) + .catch((error) => { + if (!id && user.externalId) { + toast.error(t(translations.deleteIdFailure)); + } else if (id && !user.externalId) { + toast.error(t(translations.addIdFailure, { newId: id })); + } else { + toast.error( + t(translations.changeIdFailure, { + oldId: user.externalId ?? '', + newId: id, + }), + ); + } + throw error; + }); + }; + + return ( + => handleIdUpdate(newId)} + updateValue={(): void => {}} + value={user.externalId ?? ''} + variant="standard" + /> + ); +}; + +export default memo(ExternalIdField, (prevProps, nextProps) => + equal(prevProps.for.externalId, nextProps.for.externalId), +); diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/UserNameField.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/UserNameField.tsx index 1eee7291d5d..9787df3e0b8 100644 --- a/client/app/bundles/course/users/components/tables/ManageUsersTable/UserNameField.tsx +++ b/client/app/bundles/course/users/components/tables/ManageUsersTable/UserNameField.tsx @@ -46,6 +46,7 @@ const UserNameField = (props: UserNameFieldProps): JSX.Element => { return ( => handleNameUpdate(newName)} updateValue={(): void => {}} diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/__test__/index.test.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/__test__/index.test.tsx new file mode 100644 index 00000000000..fdbaedfc26c --- /dev/null +++ b/client/app/bundles/course/users/components/tables/ManageUsersTable/__test__/index.test.tsx @@ -0,0 +1,99 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { CourseUserMiniEntity } from 'types/course/courseUsers'; + +import ManageUsersTable from '../index'; + +const baseUser: CourseUserMiniEntity = { + id: 1, + name: 'Alice Lim', + email: 'alice@example.com', + role: 'student', +}; + +const SEARCH_PLACEHOLDER = 'Search by name, email or external ID'; + +describe('', () => { + describe('search', () => { + it('filters users by external ID', async () => { + const user = userEvent.setup(); + const users: CourseUserMiniEntity[] = [ + { ...baseUser, id: 1, name: 'Alice Lim', externalId: 'EXT-ALICE' }, + { + ...baseUser, + id: 2, + name: 'Bob Tan', + email: 'bob@example.com', + externalId: 'EXT-BOB', + }, + ]; + + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + await user.type( + screen.getByPlaceholderText(SEARCH_PLACEHOLDER), + 'EXT-ALICE', + ); + + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + }); + + it('searches external ID case-insensitively', async () => { + const user = userEvent.setup(); + const users: CourseUserMiniEntity[] = [ + { ...baseUser, id: 1, name: 'Alice Lim', externalId: 'ext-alice' }, + { + ...baseUser, + id: 2, + name: 'Bob Tan', + email: 'bob@example.com', + externalId: 'EXT-BOB', + }, + ]; + + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + await user.type( + screen.getByPlaceholderText(SEARCH_PLACEHOLDER), + 'EXT-ALICE', + ); + + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + }); + + it('shows all users when search is cleared', async () => { + const user = userEvent.setup(); + const users: CourseUserMiniEntity[] = [ + { ...baseUser, id: 1, name: 'Alice Lim', externalId: 'EXT-ALICE' }, + { + ...baseUser, + id: 2, + name: 'Bob Tan', + email: 'bob@example.com', + externalId: 'EXT-BOB', + }, + ]; + + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + const searchInput = screen.getByPlaceholderText(SEARCH_PLACEHOLDER); + await user.type(searchInput, 'EXT-ALICE'); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + + await user.clear(searchInput); + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.getByText('Bob Tan')).toBeInTheDocument(); + }); + }); +}); diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx index 2343c8f10f2..72bf3413222 100644 --- a/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx +++ b/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx @@ -15,6 +15,7 @@ import translations from '../../../translations'; import ActiveTableToolbar from './ActiveTableToolbar'; import AlgorithmMenu from './AlgorithmMenu'; +import ExternalIdField from './ExternalIdField'; import PhantomSwitch from './PhantomSwitch'; import RoleMenu from './RoleMenu'; import TimelineMenu from './TimelineMenu'; @@ -58,6 +59,14 @@ const ManageUsersTable = (props: ManageUsersTableProps): JSX.Element => { cell: (user) => , csvDownloadable: true, }, + { + of: 'externalId', + title: t(tableTranslations.externalId), + sortable: false, + searchable: true, + cell: (user) => , + csvDownloadable: true, + }, { of: 'email', sortable: true, @@ -186,15 +195,11 @@ const ManageUsersTable = (props: ManageUsersTableProps): JSX.Element => { if (!user.name && !user.email) return false; if (!filterValue?.length) return true; + const query = filterValue.toLowerCase().trim(); return ( - user.name - .toLowerCase() - .trim() - .includes(filterValue.toLowerCase().trim()) || - user.email - .toLowerCase() - .trim() - .includes(filterValue.toLowerCase().trim()) + user.name.toLowerCase().trim().includes(query) || + user.email.toLowerCase().trim().includes(query) || + (user.externalId?.toLowerCase().trim().includes(query) ?? false) ); }, }, diff --git a/client/app/bundles/course/users/operations.ts b/client/app/bundles/course/users/operations.ts index 81b7c216d50..3d9990666da 100644 --- a/client/app/bundles/course/users/operations.ts +++ b/client/app/bundles/course/users/operations.ts @@ -40,6 +40,7 @@ const formatUpdateUser = ( role: data.role, reference_timeline_id: data.referenceTimelineId, timeline_algorithm: data.timelineAlgorithm, + external_id: data.externalId, }, }; }; diff --git a/client/app/bundles/course/users/translations.ts b/client/app/bundles/course/users/translations.ts index 0272f787c96..c4e66a140b5 100644 --- a/client/app/bundles/course/users/translations.ts +++ b/client/app/bundles/course/users/translations.ts @@ -7,7 +7,7 @@ const translations = defineMessages({ }, searchText: { id: 'course.users.ManageUsersTable.ManageUsersTable.searchText', - defaultMessage: 'Search by name or email', + defaultMessage: 'Search by name, email or external ID', }, renameSuccess: { id: 'course.users.ManageUsersTable.renameSuccess', @@ -17,6 +17,30 @@ const translations = defineMessages({ id: 'course.users.ManageUsersTable.renameFailure', defaultMessage: 'Failed to rename {oldName} to {newName}', }, + changeIdSuccess: { + id: 'course.users.ManageUsersTable.changeIdSuccess', + defaultMessage: 'ID was changed from {oldId} to {newId}', + }, + addIdSuccess: { + id: 'course.users.ManageUsersTable.addIdSuccess', + defaultMessage: 'External ID set to {newId}', + }, + deleteIdSuccess: { + id: 'course.users.ManageUsersTable.deleteIdSuccess', + defaultMessage: 'External ID deleted', + }, + addIdFailure: { + id: 'course.users.ManageUsersTable.addIdFailure', + defaultMessage: 'Failed to set External ID to {newId}', + }, + deleteIdFailure: { + id: 'course.users.ManageUsersTable.deleteIdFailure', + defaultMessage: 'Failed to delete External ID', + }, + changeIdFailure: { + id: 'course.users.ManageUsersTable.changeIdFailure', + defaultMessage: 'Failed to change ID from {oldId} to {newId}', + }, phantomSuccess: { id: 'course.users.ManageUsersTable.phantomSuccess', defaultMessage: diff --git a/client/app/lib/components/form/fields/DataTableInlineEditable/TextField.tsx b/client/app/lib/components/form/fields/DataTableInlineEditable/TextField.tsx index 7d416acfb7e..0820e28e755 100644 --- a/client/app/lib/components/form/fields/DataTableInlineEditable/TextField.tsx +++ b/client/app/lib/components/form/fields/DataTableInlineEditable/TextField.tsx @@ -19,6 +19,7 @@ interface Props { link?: string; onUpdate?: (newValue: string) => Promise; alwaysEditable?: boolean; + allowEmpty?: boolean; } const styles = { @@ -52,6 +53,7 @@ const InlineEditTextField: FC = (props): JSX.Element | null => { link, onUpdate, alwaysEditable = false, + allowEmpty = false, ...custom } = props; const [controlledVal, setControlledVal] = useState(value); @@ -68,7 +70,7 @@ const InlineEditTextField: FC = (props): JSX.Element | null => { const handleSave = (): void => { setIsSaving(true); - if (controlledVal.trim() === '') { + if (!allowEmpty && controlledVal.trim() === '') { setHelperText('Cannot be empty.'); setIsSaving(false); return; diff --git a/client/app/lib/translations/table.ts b/client/app/lib/translations/table.ts index c15b5316d8a..7136e9c9d73 100644 --- a/client/app/lib/translations/table.ts +++ b/client/app/lib/translations/table.ts @@ -37,6 +37,10 @@ const translations = defineMessages({ id: 'lib.translations.table.column.timelineAlgorithm', defaultMessage: 'Algorithm', }, + externalId: { + id: 'lib.translations.table.column.externalId', + defaultMessage: 'External ID', + }, invitationSentAt: { id: 'lib.translations.table.column.invitationSentAt', defaultMessage: 'Invitation Sent At', @@ -213,6 +217,10 @@ const translations = defineMessages({ id: 'lib.translations.table.column.groups', defaultMessage: 'Group(s)', }, + optional: { + id: 'lib.translations.table.column.optional', + defaultMessage: 'Optional', + }, }); export default translations; diff --git a/client/app/types/course/courseUsers.ts b/client/app/types/course/courseUsers.ts index 2b7adfeac76..9affb2d7bf7 100644 --- a/client/app/types/course/courseUsers.ts +++ b/client/app/types/course/courseUsers.ts @@ -52,6 +52,7 @@ export interface CourseUserListData extends CourseUserBasicListData { phantom?: boolean; isSuspended?: boolean; timelineAlgorithm?: TimelineAlgorithm; + externalId?: string | null; } export interface CourseUserBasicMiniEntity { @@ -69,6 +70,7 @@ export interface CourseUserMiniEntity extends CourseUserBasicMiniEntity { email: CourseUserListData['email']; role: CourseUserListData['role']; timelineAlgorithm?: CourseUserListData['timelineAlgorithm']; + externalId?: CourseUserListData['externalId']; referenceTimelineId?: number | null; groups?: string[]; } @@ -118,6 +120,7 @@ export interface UpdateCourseUserPatchData { timeline_algorithm?: TimelineAlgorithm; reference_timeline_id?: number | null; role?: CourseUserRole; + external_id?: string | null; }; } diff --git a/client/app/types/course/userInvitations.ts b/client/app/types/course/userInvitations.ts index c75fee13e8e..e67607eada9 100644 --- a/client/app/types/course/userInvitations.ts +++ b/client/app/types/course/userInvitations.ts @@ -1,4 +1,8 @@ -import { CourseUserData, CourseUserRole } from './courseUsers'; +import { + CourseUserData, + CourseUserListData, + CourseUserRole, +} from './courseUsers'; import { TimelineAlgorithm } from './personalTimes'; export interface InvitationFileEntity { @@ -7,8 +11,17 @@ export interface InvitationFileEntity { file?: Blob; } +export type DuplicateReason = + | 'duplicate_email_in_file' + | 'duplicate_external_id_in_file' + | 'external_id_taken'; + +export interface DuplicateUserData extends CourseUserListData { + reason?: DuplicateReason; +} + export interface InvitationResult { - duplicateUsers?: CourseUserData[]; + duplicateUsers?: DuplicateUserData[]; existingCourseUsers?: CourseUserData[]; existingInvitations?: InvitationListData[]; newCourseUsers?: CourseUserData[]; @@ -22,6 +35,7 @@ export interface IndividualInvite { id?: string; name: string; email: string; + externalId?: string; role: string; phantom: boolean; timelineAlgorithm?: string; @@ -34,6 +48,7 @@ export interface InvitationPostData { id?: string | undefined; name: string; email: string; + externalId?: string | null; role: string; phantom: boolean; timelineAlgorithm?: string | undefined; @@ -44,6 +59,7 @@ export interface InvitationsPostData { id?: string | undefined; name: string; email: string; + externalId?: string | null; role: string; phantom: boolean; timelineAlgorithm?: string | undefined; @@ -54,6 +70,7 @@ export interface InvitationMiniEntity { id: number; name: string; email: string; + externalId: string | null; role: CourseUserRole; phantom: boolean; timelineAlgorithm?: TimelineAlgorithm; @@ -68,6 +85,7 @@ export interface InvitationListData { id: number; name: string; email: string; + externalId: string | null; role: CourseUserRole; phantom: boolean; timelineAlgorithm?: TimelineAlgorithm; diff --git a/client/locales/en.json b/client/locales/en.json index 66413747666..05459ddc589 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -6898,10 +6898,10 @@ "defaultMessage": "Close" }, "course.userInvitations.InvitationResultDialog.duplicateInfo": { - "defaultMessage": "Duplicate users were found in the invitation. Only the first instance of this user will be invited." + "defaultMessage": "Duplicate users were found in the invitation. Only the first instance of each user will be invited." }, "course.userInvitations.InvitationResultDialog.duplicateUsers": { - "defaultMessage": "Users with Duplicate Emails ({count})" + "defaultMessage": "Duplicate Users ({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsers": { "defaultMessage": "Existing Course Users ({count})" @@ -6924,6 +6924,18 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "New Invitations ({count})" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { + "defaultMessage": "Duplicate email in upload" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { + "defaultMessage": "Duplicate external ID in upload" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { + "defaultMessage": "External ID is already assigned to another course member" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { + "defaultMessage": "Already a course member — external ID could not be applied (already assigned to another member)" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "Accepted Invitations" }, @@ -6954,11 +6966,20 @@ "course.userInvitations.InviteUsersFileUpload.failure": { "defaultMessage": "Failed to invite users. Please ensure your data is formatted correctly - {error}" }, + "course.userInvitations.InviteUsersFileUpload.failureGeneric": { + "defaultMessage": "Failed to invite users. Please ensure your data is formatted correctly." + }, "course.userInvitations.InviteUsersFileUpload.fileUploadExample": { - "defaultMessage": "Name,Email[,Role,Phantom]{br}John,test1@example.org[,student,y]{br}Mary,test2@example.org[,teaching_assistant,n]" + "defaultMessage": "Name,Email,Role,Phantom,ExternalId{br}John,test1@example.org,student,y,A0123456{br}Mary,test2@example.org,teaching_assistant,n,A0123457" }, "course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline": { - "defaultMessage": "Name,Email[,Role,Phantom,PersonalTimeline]{br}John,test1@example.org[,student,y,otot]{br}Mary,test2@example.org[,teaching_assistant,n,fixed]" + "defaultMessage": "Name,Email,Role,Phantom,PersonalTimeline,ExternalId{br}John,test1@example.org,student,y,otot,A0123456{br}Mary,test2@example.org,teaching_assistant,n,fixed,A0123457" + }, + "course.userInvitations.InviteUsersFileUpload.fileUploadInfoEmail": { + "defaultMessage": "Each invitation must use a unique email address within the course. Duplicate emails will be skipped." + }, + "course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId": { + "defaultMessage": "External ID is an optional field for linking a user to an external system. If provided, it must be unique within the course - duplicate external IDs will be skipped." }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfo": { "defaultMessage": "Upload a .csv file with the following format:" @@ -6990,6 +7011,9 @@ "course.userInvitations.InvitationActionButtons.deletionFailure": { "defaultMessage": "Failed to delete user - {error}" }, + "course.userInvitations.InvitationActionButtons.deletionFailureGeneric": { + "defaultMessage": "Failed to delete user." + }, "course.userInvitations.InvitationActionButtons.deletionSuccess": { "defaultMessage": "Invitation for {name} was deleted." }, @@ -6997,7 +7021,7 @@ "defaultMessage": "Delete Invitation" }, "course.userInvitations.InvitationActionButtons.resendFailure": { - "defaultMessage": "Failed to resend invitation - {error}" + "defaultMessage": "Failed to resend invitation." }, "course.userInvitations.InvitationActionButtons.resendSuccess": { "defaultMessage": "Resent email invitation to {email}!" @@ -7072,7 +7096,7 @@ "defaultMessage": "There are no users" }, "course.users.ManageUsersTable.ManageUsersTable.searchText": { - "defaultMessage": "Search by name or email" + "defaultMessage": "Search by name, email or external ID" }, "course.users.ManageUsersTable.assignToTimeline": { "defaultMessage": "Assign to timeline" @@ -7125,6 +7149,24 @@ "course.users.ManageUsersTable.phantomSuccess": { "defaultMessage": "{name} {isPhantom, select, true {is now a phantom user} other {is now a normal user} }." }, + "course.users.ManageUsersTable.addIdFailure": { + "defaultMessage": "Failed to set External ID to {newId}" + }, + "course.users.ManageUsersTable.addIdSuccess": { + "defaultMessage": "External ID set to {newId}" + }, + "course.users.ManageUsersTable.changeIdFailure": { + "defaultMessage": "Failed to change ID from {oldId} to {newId}" + }, + "course.users.ManageUsersTable.changeIdSuccess": { + "defaultMessage": "ID was changed from {oldId} to {newId}" + }, + "course.users.ManageUsersTable.deleteIdFailure": { + "defaultMessage": "Failed to delete External ID" + }, + "course.users.ManageUsersTable.deleteIdSuccess": { + "defaultMessage": "External ID deleted" + }, "course.users.ManageUsersTable.renameFailure": { "defaultMessage": "Failed to rename {oldName} to {newName}" }, @@ -8073,6 +8115,9 @@ "lib.translations.table.column.email": { "defaultMessage": "Email" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "External ID" + }, "lib.translations.table.column.endAt": { "defaultMessage": "End At" }, @@ -8124,6 +8169,9 @@ "lib.translations.table.column.name": { "defaultMessage": "Name" }, + "lib.translations.table.column.optional": { + "defaultMessage": "Optional" + }, "lib.translations.table.column.organization": { "defaultMessage": "Organization" }, diff --git a/client/locales/ko.json b/client/locales/ko.json index 2592efdaf4a..adbd0d5b7cc 100644 --- a/client/locales/ko.json +++ b/client/locales/ko.json @@ -6885,10 +6885,10 @@ "defaultMessage": "닫기" }, "course.userInvitations.InvitationResultDialog.duplicateInfo": { - "defaultMessage": "사용자가 중복 초대되었습니다. 첫 번째 사용자만 초대됩니다." + "defaultMessage": "사용자가 중복 초대되었습니다. 각 사용자의 첫 번째 항목만 초대됩니다." }, "course.userInvitations.InvitationResultDialog.duplicateUsers": { - "defaultMessage": "중복 이메일 사용자 ({count})" + "defaultMessage": "중복 사용자 ({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsers": { "defaultMessage": "기존 과정 사용자 ({count})" @@ -6911,6 +6911,18 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "새 초대장 ({count})" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { + "defaultMessage": "Duplicate email in upload" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { + "defaultMessage": "Duplicate external ID in upload" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { + "defaultMessage": "External ID is already assigned to another course member" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { + "defaultMessage": "Already a course member — external ID could not be applied (already assigned to another member)" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "수락된 초대" }, @@ -6944,11 +6956,20 @@ "course.userInvitations.InviteUsersFileUpload.failure": { "defaultMessage": "사용자 초대에 실패했습니다. 데이터가 올바르게 형식화되었는지 확인하세요 - {error}" }, + "course.userInvitations.InviteUsersFileUpload.failureGeneric": { + "defaultMessage": "사용자 초대에 실패했습니다. 데이터가 올바르게 형식화되었는지 확인하세요." + }, "course.userInvitations.InviteUsersFileUpload.fileUploadExample": { - "defaultMessage": "이름,이메일[,역할,팬텀]{br}John,test1@example.org[,학생,y]{br}Mary,test2@example.org[,티칭 어시스턴트,n]" + "defaultMessage": "이름,이메일,역할,팬텀,외부 ID{br}John,test1@example.org,학생,y,A0123456{br}Mary,test2@example.org,티칭 어시스턴트,n,A0123457" }, "course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline": { - "defaultMessage": "이름,이메일[,역할,팬텀,개인 타임라인]{br}John,test1@example.org[,학생,y,otot]{br}Mary,test2@example.org[,티칭 어시스턴트,n,fixed]" + "defaultMessage": "이름,이메일,역할,팬텀,개인 타임라인,외부 ID{br}John,test1@example.org,학생,y,otot,A0123456{br}Mary,test2@example.org,티칭 어시스턴트,n,fixed,A0123457" + }, + "course.userInvitations.InviteUsersFileUpload.fileUploadInfoEmail": { + "defaultMessage": "각 초대는 과정 내에서 고유한 이메일 주소를 사용해야 합니다. 중복된 이메일은 건너뜁니다." + }, + "course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId": { + "defaultMessage": "외부 ID는 사용자를 외부 시스템과 연결하기 위한 선택적 필드입니다. 입력된 경우 과정 내에서 고유해야 하며, 중복된 외부 ID는 건너뜁니다." }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfo": { "defaultMessage": "다음 형식으로 .csv 파일을 업로드하세요:" @@ -6987,7 +7008,7 @@ "defaultMessage": "초대 삭제" }, "course.userInvitations.InvitationActionButtons.resendFailure": { - "defaultMessage": "초대 재전송에 실패했습니다 - {error}" + "defaultMessage": "초대 재전송에 실패했습니다." }, "course.userInvitations.InvitationActionButtons.resendSuccess": { "defaultMessage": "{email}로 초대 이메일을 다시 보냈습니다!" @@ -7019,6 +7040,9 @@ "course.userInvitations.UserInvitationsTable.noInvitations": { "defaultMessage": "초대가 없습니다." }, + "course.userInvitations.UserInvitationsTable.searchText": { + "defaultMessage": "이름, 이메일 또는 외부 ID로 검색" + }, "course.userInvitations.UserInvitationsTable.pending": { "defaultMessage": "대기 중" }, @@ -7062,7 +7086,7 @@ "defaultMessage": "사용자가 없습니다." }, "course.users.ManageUsersTable.ManageUsersTable.searchText": { - "defaultMessage": "이름 또는 이메일로 검색" + "defaultMessage": "이름, 이메일 또는 외부 ID로 검색" }, "course.users.ManageUsersTable.assignToTimeline": { "defaultMessage": "타임라인에 할당" @@ -7115,6 +7139,24 @@ "course.users.ManageUsersTable.phantomSuccess": { "defaultMessage": "{name} {isPhantom, select, true {은(는) 이제 팬텀 사용자입니다} other {은(는) 이제 일반 사용자입니다}}." }, + "course.users.ManageUsersTable.addIdFailure": { + "defaultMessage": "외부 ID를 {newId}로 설정하지 못했습니다." + }, + "course.users.ManageUsersTable.addIdSuccess": { + "defaultMessage": "외부 ID가 {newId}로 설정되었습니다." + }, + "course.users.ManageUsersTable.changeIdFailure": { + "defaultMessage": "ID를 {oldId}에서 {newId}로 변경하지 못했습니다." + }, + "course.users.ManageUsersTable.changeIdSuccess": { + "defaultMessage": "ID가 {oldId}에서 {newId}로 변경되었습니다." + }, + "course.users.ManageUsersTable.deleteIdFailure": { + "defaultMessage": "외부 ID를 삭제하지 못했습니다." + }, + "course.users.ManageUsersTable.deleteIdSuccess": { + "defaultMessage": "외부 ID가 삭제되었습니다." + }, "course.users.ManageUsersTable.renameFailure": { "defaultMessage": "{oldName}을(를) {newName}(으)로 이름 변경하지 못했습니다." }, @@ -8072,6 +8114,9 @@ "lib.translations.table.column.email": { "defaultMessage": "이메일" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "외부 ID" + }, "lib.translations.table.column.endAt": { "defaultMessage": "종료 시각" }, @@ -8123,6 +8168,9 @@ "lib.translations.table.column.name": { "defaultMessage": "이름" }, + "lib.translations.table.column.optional": { + "defaultMessage": "선택" + }, "lib.translations.table.column.organization": { "defaultMessage": "조직" }, diff --git a/client/locales/zh.json b/client/locales/zh.json index 93d955f80be..0548970c92c 100644 --- a/client/locales/zh.json +++ b/client/locales/zh.json @@ -6879,10 +6879,10 @@ "defaultMessage": "关闭" }, "course.userInvitations.InvitationResultDialog.duplicateInfo": { - "defaultMessage": "在邀请中发现重复用户。只邀请该用户第一个账号。" + "defaultMessage": "在邀请中发现重复用户。只邀请每个用户的第一个实例。" }, "course.userInvitations.InvitationResultDialog.duplicateUsers": { - "defaultMessage": "具有重复电子邮件的用户（{count}）" + "defaultMessage": "重复用户（{count}）" }, "course.userInvitations.InvitationResultDialog.existingCourseUsers": { "defaultMessage": "现有课程用户（{count}）" @@ -6905,6 +6905,18 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "新邀请（{count}）" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { + "defaultMessage": "Duplicate email in upload" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { + "defaultMessage": "Duplicate external ID in upload" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { + "defaultMessage": "External ID is already assigned to another course member" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { + "defaultMessage": "Already a course member — external ID could not be applied (already assigned to another member)" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "已接受邀请" }, @@ -6938,11 +6950,20 @@ "course.userInvitations.InviteUsersFileUpload.failure": { "defaultMessage": "邀请用户失败。请确保你的数据格式正确 - {error}" }, + "course.userInvitations.InviteUsersFileUpload.failureGeneric": { + "defaultMessage": "邀请用户失败。请确保你的数据格式正确。" + }, "course.userInvitations.InviteUsersFileUpload.fileUploadExample": { - "defaultMessage": "姓名，电子邮件[,Role,Phantom]{br}John,test1@example.org[,student,y]{br}Mary,test2@example.org[,teaching_assistant,n]" + "defaultMessage": "姓名，电子邮件,Role,Phantom,外部编号{br}John,test1@example.org,student,y,A0123456{br}Mary,test2@example.org,teaching_assistant,n,A0123457" }, "course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline": { - "defaultMessage": "姓名，电子邮件[,Role,Phantom,PersonalTimeline]{br}John,test1@example.org[,student,y,otot]{br}Mary,test2@example.org[,teaching_assistant,n,fixed]" + "defaultMessage": "姓名，电子邮件,Role,Phantom,PersonalTimeline,外部编号{br}John,test1@example.org,student,y,otot,A0123456{br}Mary,test2@example.org,teaching_assistant,n,fixed,A0123457" + }, + "course.userInvitations.InviteUsersFileUpload.fileUploadInfoEmail": { + "defaultMessage": "每个邀请必须使用课程内唯一的电子邮件地址。重复的电子邮件将被跳过。" + }, + "course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId": { + "defaultMessage": "外部编号是将用户与外部系统关联的可选字段。若提供，则必须在课程内唯一——重复的外部编号将被跳过。" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfo": { "defaultMessage": "上传具有以下格式的 .csv 文件：" @@ -6981,7 +7002,7 @@ "defaultMessage": "删除邀请" }, "course.userInvitations.InvitationActionButtons.resendFailure": { - "defaultMessage": "无法重新发送邀请 - {error}" + "defaultMessage": "无法重新发送邀请。" }, "course.userInvitations.InvitationActionButtons.resendSuccess": { "defaultMessage": "向 {email} 重新发送电子邮件邀请！" @@ -7013,6 +7034,9 @@ "course.userInvitations.UserInvitationsTable.noInvitations": { "defaultMessage": "没有邀请。" }, + "course.userInvitations.UserInvitationsTable.searchText": { + "defaultMessage": "按姓名、电子邮件或外部ID搜索" + }, "course.userInvitations.UserInvitationsTable.pending": { "defaultMessage": "待处理" }, @@ -7056,7 +7080,7 @@ "defaultMessage": "没有用户" }, "course.users.ManageUsersTable.ManageUsersTable.searchText": { - "defaultMessage": "按姓名、电子邮件、角色等搜索。" + "defaultMessage": "按姓名、电子邮件或外部ID搜索" }, "course.users.ManageUsersTable.assignToTimeline": { "defaultMessage": "分配到时间线" @@ -7109,6 +7133,24 @@ "course.users.ManageUsersTable.phantomSuccess": { "defaultMessage": "{name} {isPhantom, select, true {现在是旁听用户} other {现在是普通用户} }。" }, + "course.users.ManageUsersTable.addIdFailure": { + "defaultMessage": "无法将外部编号设置为 {newId}" + }, + "course.users.ManageUsersTable.addIdSuccess": { + "defaultMessage": "外部编号已设置为 {newId}" + }, + "course.users.ManageUsersTable.changeIdFailure": { + "defaultMessage": "无法将编号从 {oldId} 更改为 {newId}" + }, + "course.users.ManageUsersTable.changeIdSuccess": { + "defaultMessage": "编号已从 {oldId} 更改为 {newId}" + }, + "course.users.ManageUsersTable.deleteIdFailure": { + "defaultMessage": "无法删除外部编号" + }, + "course.users.ManageUsersTable.deleteIdSuccess": { + "defaultMessage": "外部编号已删除" + }, "course.users.ManageUsersTable.renameFailure": { "defaultMessage": "无法将 {oldName} 重命名为 {newName}" }, @@ -8066,6 +8108,9 @@ "lib.translations.table.column.email": { "defaultMessage": "电子邮件" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "外部编号" + }, "lib.translations.table.column.endAt": { "defaultMessage": "结束于" }, @@ -8117,6 +8162,9 @@ "lib.translations.table.column.name": { "defaultMessage": "姓名" }, + "lib.translations.table.column.optional": { + "defaultMessage": "选填" + }, "lib.translations.table.column.organization": { "defaultMessage": "组织" }, diff --git a/config/locales/en/activerecord/errors.yml b/config/locales/en/activerecord/errors.yml index cb30644cca7..95f0da1660a 100644 --- a/config/locales/en/activerecord/errors.yml +++ b/config/locales/en/activerecord/errors.yml @@ -196,6 +196,9 @@ en: not_in_course: 'Selected user is not in specified course' course/user_invitation: existing_invitation: 'an open invitation exists for this email address' + attributes: + external_id: + taken: 'has already been taken by another user or pending invitation in this course' course/video: attributes: url: @@ -213,6 +216,8 @@ en: deletion: 'The last tab cannot be deleted' course_user: attributes: + external_id: + taken: 'has already been taken by another user or pending invitation in this course' reference_timeline: belongs_to_course: 'must belong to course' duplication_traceable: diff --git a/config/locales/en/csv.yml b/config/locales/en/csv.yml index 020efec3c82..0d1cf158af1 100644 --- a/config/locales/en/csv.yml +++ b/config/locales/en/csv.yml @@ -62,3 +62,4 @@ en: name: 'Name' type: 'Student Type' email: 'Email' + external_id: 'External ID' diff --git a/config/locales/en/errors.yml b/config/locales/en/errors.yml index e85b2b8f03f..06ca6519b7d 100644 --- a/config/locales/en/errors.yml +++ b/config/locales/en/errors.yml @@ -15,8 +15,11 @@ en: owner: 'Owner' phantom: 'Phantom' user_invitations: - duplicate_user: '%{user} appears more than once in the submission.' - invalid_email: '%{email} is invalid: %{message}.' + already_enrolled: '%{email} could not be processed: already enrolled in this course.' + duplicate_invitation: '%{email} could not be processed: already has a pending invitation.' + invalid_email: '%{email} could not be processed: invalid email: %{message}.' + duplicate_external_id: '%{email} could not be processed: external ID "%{external_id}" is already taken.' + other_error: '%{email} could not be processed: %{message}.' user_registrations: invalid_code: 'You have entered an invalid invitation code.' code_taken: > diff --git a/config/locales/ko/activerecord/errors.yml b/config/locales/ko/activerecord/errors.yml index 21543f1fe1f..62c4016a70f 100644 --- a/config/locales/ko/activerecord/errors.yml +++ b/config/locales/ko/activerecord/errors.yml @@ -194,6 +194,9 @@ ko: not_in_course: '선택한 사용자가 지정된 코스에 없습니다' course/user_invitation: existing_invitation: '이 이메일 주소에는 아직 완료되지 않은 초대장이 있습니다' + attributes: + external_id: + taken: '이 코스에서 다른 사용자 또는 대기 중인 초대에 의해 이미 사용 중입니다' course/video: attributes: url: @@ -211,6 +214,8 @@ ko: deletion: '마지막 탭은 삭제할 수 없습니다' course_user: attributes: + external_id: + taken: '이 코스에서 다른 사용자 또는 대기 중인 초대에 의해 이미 사용 중입니다' reference_timeline: belongs_to_course: '코스에 속해야 합니다' duplication_traceable: diff --git a/config/locales/ko/csv.yml b/config/locales/ko/csv.yml index 6a1a238665c..0a58bcd615a 100644 --- a/config/locales/ko/csv.yml +++ b/config/locales/ko/csv.yml @@ -62,3 +62,4 @@ ko: name: '이름' type: '학생 유형' email: '이메일' + external_id: '외부 ID' diff --git a/config/locales/ko/errors.yml b/config/locales/ko/errors.yml index f57ad1ceb02..ae8c0ce621e 100644 --- a/config/locales/ko/errors.yml +++ b/config/locales/ko/errors.yml @@ -15,8 +15,11 @@ ko: owner: '소유자' phantom: '팬텀' user_invitations: - duplicate_user: '%{user}가 제출물에 여러 번 나타납니다.' - invalid_email: '%{email}이(가) 유효하지 않습니다: %{message}.' + already_enrolled: '%{email}을(를) 처리할 수 없습니다: 이미 이 과정에 등록되어 있습니다.' + duplicate_invitation: '%{email}을(를) 처리할 수 없습니다: 이미 대기 중인 초대가 있습니다.' + invalid_email: '%{email}을(를) 처리할 수 없습니다: 유효하지 않은 이메일: %{message}.' + duplicate_external_id: '%{email}을(를) 처리할 수 없습니다: 외부 ID "%{external_id}"는 이미 사용 중입니다.' + other_error: '%{email}을(를) 처리할 수 없습니다: %{message}.' user_registrations: invalid_code: '잘못된 초대 코드를 입력하셨습니다.' code_taken: > diff --git a/config/locales/zh/activerecord/errors.yml b/config/locales/zh/activerecord/errors.yml index eb9a669b838..5cbb439debc 100644 --- a/config/locales/zh/activerecord/errors.yml +++ b/config/locales/zh/activerecord/errors.yml @@ -194,6 +194,9 @@ zh: not_in_course: '选中的用户不在指定课程中' course/user_invitation: existing_invitation: '此电子邮件地址已存在一个未完成的邀请' + attributes: + external_id: + taken: '该外部编号已被此课程中的其他用户或待处理邀请使用' course/video: attributes: url: @@ -211,6 +214,8 @@ zh: deletion: '最后一项无法被删除' course_user: attributes: + external_id: + taken: '该外部编号已被此课程中的其他用户或待处理邀请使用' reference_timeline: belongs_to_course: '必须隶属于课程' duplication_traceable: diff --git a/config/locales/zh/csv.yml b/config/locales/zh/csv.yml index ac239e862fe..262bdadf441 100644 --- a/config/locales/zh/csv.yml +++ b/config/locales/zh/csv.yml @@ -62,3 +62,4 @@ zh: name: '用户名' type: '学生类型' email: 'Email' + external_id: '外部编号' diff --git a/config/locales/zh/errors.yml b/config/locales/zh/errors.yml index cb4b14af578..368e1787bb4 100644 --- a/config/locales/zh/errors.yml +++ b/config/locales/zh/errors.yml @@ -15,8 +15,11 @@ zh: owner: '拥有者' phantom: '虚拟用户' user_invitations: - duplicate_user: '%{user}在提交中出现了多次。' - invalid_email: '%{email}无效：%{message}。' + already_enrolled: '%{email}无法处理：已加入该课程。' + duplicate_invitation: '%{email}无法处理：已有待处理的邀请。' + invalid_email: '%{email}无法处理：邮箱无效：%{message}。' + duplicate_external_id: '%{email}无法处理：外部编号"%{external_id}"已被使用。' + other_error: '%{email}无法处理：%{message}。' user_registrations: invalid_code: '你输入了一个无效的邀请码。' code_taken: > diff --git a/db/migrate/20260514052933_add_external_id_to_course_users.rb b/db/migrate/20260514052933_add_external_id_to_course_users.rb new file mode 100644 index 00000000000..f1b039f870b --- /dev/null +++ b/db/migrate/20260514052933_add_external_id_to_course_users.rb @@ -0,0 +1,5 @@ +class AddExternalIdToCourseUsers < ActiveRecord::Migration[7.2] + def change + add_column :course_users, :external_id, :string + end +end diff --git a/db/migrate/20260514100000_add_external_id_to_course_user_invitations.rb b/db/migrate/20260514100000_add_external_id_to_course_user_invitations.rb new file mode 100644 index 00000000000..2dbab42f1f1 --- /dev/null +++ b/db/migrate/20260514100000_add_external_id_to_course_user_invitations.rb @@ -0,0 +1,5 @@ +class AddExternalIdToCourseUserInvitations < ActiveRecord::Migration[7.2] + def change + add_column :course_user_invitations, :external_id, :string + end +end diff --git a/db/migrate/20260519000000_add_unique_index_on_external_id.rb b/db/migrate/20260519000000_add_unique_index_on_external_id.rb new file mode 100644 index 00000000000..d7fa4001f83 --- /dev/null +++ b/db/migrate/20260519000000_add_unique_index_on_external_id.rb @@ -0,0 +1,13 @@ +class AddUniqueIndexOnExternalId < ActiveRecord::Migration[7.2] + def change + add_index :course_users, [:course_id, :external_id], + unique: true, + where: 'external_id IS NOT NULL', + name: 'index_course_users_on_course_id_and_external_id' + + add_index :course_user_invitations, [:course_id, :external_id], + unique: true, + where: 'external_id IS NOT NULL', + name: 'index_course_user_invitations_on_course_id_and_external_id' + end +end diff --git a/db/schema.rb b/db/schema.rb index 696d5837a38..d28c3259f1d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_04_06_122130) do +ActiveRecord::Schema[7.2].define(version: 2026_05_19_022334) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "uuid-ossp" @@ -1333,9 +1333,11 @@ t.boolean "phantom", default: false, null: false t.integer "timeline_algorithm" t.boolean "is_retryable", default: true, null: false + t.string "external_id" t.index "lower((email)::text)", name: "index_course_user_invitations_on_email" t.index ["confirmer_id"], name: "fk__course_user_invitations_confirmer_id" t.index ["course_id", "email"], name: "index_course_user_invitations_on_course_id_and_email", unique: true + t.index ["course_id", "external_id"], name: "index_course_user_invitations_on_course_id_and_external_id", unique: true, where: "(external_id IS NOT NULL)" t.index ["course_id"], name: "fk__course_user_invitations_course_id" t.index ["creator_id"], name: "fk__course_user_invitations_creator_id" t.index ["invitation_key"], name: "index_course_user_invitations_on_invitation_key", unique: true @@ -1357,6 +1359,8 @@ t.integer "timeline_algorithm", default: 0, null: false t.datetime "deleted_at" t.boolean "is_suspended", default: false, null: false + t.string "external_id" + t.index ["course_id", "external_id"], name: "index_course_users_on_course_id_and_external_id", unique: true, where: "(external_id IS NOT NULL)" t.index ["course_id", "user_id"], name: "index_course_users_on_course_id_and_user_id", unique: true t.index ["course_id"], name: "fk__course_users_course_id" t.index ["creator_id"], name: "fk__course_users_creator_id" diff --git a/spec/controllers/course/user_invitations_controller_spec.rb b/spec/controllers/course/user_invitations_controller_spec.rb index 5b640aee0e7..34a4f7274f6 100644 --- a/spec/controllers/course/user_invitations_controller_spec.rb +++ b/spec/controllers/course/user_invitations_controller_spec.rb @@ -65,7 +65,7 @@ def replace_with_erroneous_course it 'sets the course errors property' do subject expect(controller.current_course.errors.count).not_to eq(0) - expect(controller.current_course.errors[:invitations_file].length).not_to eq(0) + expect(controller.current_course.errors[:base].length).not_to eq(0) end end end @@ -94,7 +94,20 @@ def replace_with_erroneous_course replace_with_erroneous_course subject current_course = controller.current_course - expect(current_course.errors[:invitations_file]).not_to be_empty + expect(current_course.errors[:base]).not_to be_empty + end + end + + context 'when a form invite has errors' do + subject do + controller.send(:propagate_errors) + controller + end + + it 'propagates each error individually to :base' do + replace_with_erroneous_course + subject + expect(controller.current_course.errors[:base]).not_to be_empty end end end @@ -116,6 +129,75 @@ def replace_with_erroneous_course expect(controller.send(:aggregate_errors).count).to eq(2) end end + + context 'when the CSV has an external ID already taken by an existing course user' do + let!(:existing_user) { create(:course_student, course: course, external_id: 'EXT_DUPE') } + let(:invite_params) do + { invitations_file: fixture_file_upload('course/invitation_duplicate_external_id.csv') } + end + subject { post :create, params: { course_id: course, course: invite_params }, format: :json } + + it 'returns success' do + subject + expect(response).to have_http_status(:ok) + end + + it 'does not surface the conflict in aggregate_errors' do + subject + errors = controller.send(:aggregate_errors) + expect(errors.none? { |e| e.include?('EXT_DUPE') }).to be(true) + end + end + + context 'when a CourseUser has an already-enrolled error' do + before { replace_with_erroneous_course } + + it 'surfaces an already enrolled error message' do + errors = controller.send(:aggregate_errors) + expect(errors.any? { |e| e.include?('already enrolled') }).to be(true) + end + end + + context 'when an invitation has a pending invitation for the same email' do + let(:course_with_dup_invitation) do + create(:course, :enrollable).tap do |c| + c.invitations.create!(name: 'Alice', email: 'pending@example.com') + c.invitations.build(name: 'Alice 2', email: 'pending@example.com') + end + end + + before do + dup_course = course_with_dup_invitation + controller.define_singleton_method(:current_course) { dup_course } + end + + it 'surfaces a duplicate invitation error message' do + errors = controller.send(:aggregate_errors) + expect(errors.any? { |e| e.include?('pending invitation') }).to be(true) + end + end + + context 'when a form invite has a duplicate external ID' do + let!(:existing_user) { create(:course_student, course: course, external_id: 'FORM_DUPE') } + let(:invite_params) do + invitations = { generate(:nested_attribute_new_id) => + { name: generate(:name), email: generate(:email), + role: :student, phantom: false, external_id: 'FORM_DUPE' } } + { invitations_attributes: invitations } + end + subject { post :create, params: { course_id: course, course: invite_params }, format: :json } + + it 'returns success' do + subject + expect(response).to have_http_status(:ok) + end + + it 'does not surface the conflict in aggregate_errors' do + subject + errors = controller.send(:aggregate_errors) + expect(errors.none? { |e| e.include?('FORM_DUPE') }).to be(true) + end + end end describe '#resend_invitation' do diff --git a/spec/features/course/staff_management_spec.rb b/spec/features/course/staff_management_spec.rb index c9f13f308d7..2a058ea6c08 100644 --- a/spec/features/course/staff_management_spec.rb +++ b/spec/features/course/staff_management_spec.rb @@ -65,9 +65,11 @@ # change name within find("tr.course_user_#{staff_to_change.id}") do - find('button.inline-edit-button', visible: false).click - find('input').set(new_name) - find('button.confirm-btn').click + within('.course_user_name') do + find('button.inline-edit-button', visible: :all).click + find('input').set(new_name) + find('button.confirm-btn').click + end end expect_toastify("#{staff_to_change.name} was renamed to #{new_name}") diff --git a/spec/features/course/student_management_spec.rb b/spec/features/course/student_management_spec.rb index 8a104079828..87996211e05 100644 --- a/spec/features/course/student_management_spec.rb +++ b/spec/features/course/student_management_spec.rb @@ -26,9 +26,11 @@ # change name within find("tr.course_user_#{student_to_update.id}") do - find('button.inline-edit-button', visible: false).click - find('input').set(new_name) - find('button.confirm-btn').click + within('.course_user_name') do + find('button.inline-edit-button', visible: :all).click + find('input').set(new_name) + find('button.confirm-btn').click + end end expect_toastify("#{student_to_update.name} was renamed to #{new_name}") diff --git a/spec/fixtures/course/invitation_duplicate_external_id.csv b/spec/fixtures/course/invitation_duplicate_external_id.csv new file mode 100644 index 00000000000..a1ca22f4caa --- /dev/null +++ b/spec/fixtures/course/invitation_duplicate_external_id.csv @@ -0,0 +1,2 @@ +name,email,role,phantom,timeline_algorithm,external_id +Alice,alice_ext_dupe1@example.com,student,false,fixed,EXT_DUPE diff --git a/spec/fixtures/course/invitation_external_id_wrong_header.csv b/spec/fixtures/course/invitation_external_id_wrong_header.csv new file mode 100644 index 00000000000..5e9c2effbe0 --- /dev/null +++ b/spec/fixtures/course/invitation_external_id_wrong_header.csv @@ -0,0 +1,3 @@ +name,email,role,phantom,timeline_algorithm,ext_id +Alice,alice@example.com,student,false,fixed,EXT001 +Bob,bob@example.com,teaching_assistant,false,fomo,EXT002 diff --git a/spec/fixtures/course/invitation_no_external_id_no_timeline.csv b/spec/fixtures/course/invitation_no_external_id_no_timeline.csv new file mode 100644 index 00000000000..d4f4137ff4e --- /dev/null +++ b/spec/fixtures/course/invitation_no_external_id_no_timeline.csv @@ -0,0 +1,3 @@ +name,email,role,phantom +Alice,alice@example.com,student,false +Bob,bob@example.com,teaching_assistant,false diff --git a/spec/fixtures/course/invitation_with_external_id.csv b/spec/fixtures/course/invitation_with_external_id.csv new file mode 100644 index 00000000000..acaeff99b2f --- /dev/null +++ b/spec/fixtures/course/invitation_with_external_id.csv @@ -0,0 +1,4 @@ +name,email,role,phantom,timeline_algorithm,external_id +Alice,alice@example.com,student,false,fixed,EXT001 +Bob,bob@example.com,teaching_assistant,false,fomo,EXT002 +Charlie,charlie@example.com,,,, diff --git a/spec/fixtures/course/invitation_with_external_id_no_timeline.csv b/spec/fixtures/course/invitation_with_external_id_no_timeline.csv new file mode 100644 index 00000000000..612e7601e6d --- /dev/null +++ b/spec/fixtures/course/invitation_with_external_id_no_timeline.csv @@ -0,0 +1,4 @@ +name,email,role,phantom,external_id +Alice,alice@example.com,student,false,EXT001 +Bob,bob@example.com,teaching_assistant,false,EXT002 +Charlie,charlie@example.com,,, diff --git a/spec/models/course/user_invitation_spec.rb b/spec/models/course/user_invitation_spec.rb index 0cb2fc70194..149dfc0a838 100644 --- a/spec/models/course/user_invitation_spec.rb +++ b/spec/models/course/user_invitation_spec.rb @@ -13,6 +13,57 @@ let(:course) { create(:course) } describe 'validations' do + describe 'external_id uniqueness' do + let(:other_course) { create(:course) } + + it 'allows multiple pending invitations with nil external_id in the same course' do + create(:course_user_invitation, course: course, external_id: nil) + invitation = build(:course_user_invitation, course: course, external_id: nil) + expect(invitation).to be_valid + end + + context 'when another pending invitation in the same course has the same external_id' do + let!(:existing) { create(:course_user_invitation, course: course, external_id: 'dup-id') } + + it 'is invalid' do + invitation = build(:course_user_invitation, course: course, external_id: 'dup-id') + expect(invitation).not_to be_valid + expect(invitation.errors[:external_id]). + to include(I18n.t('activerecord.errors.models.course/user_invitation.attributes.external_id.taken')) + end + end + + context 'when a pending invitation in a different course has the same external_id' do + let!(:existing) { create(:course_user_invitation, course: other_course, external_id: 'some-id') } + + it 'is valid' do + invitation = build(:course_user_invitation, course: course, external_id: 'some-id') + expect(invitation).to be_valid + end + end + + context 'when a course user in the same course has the same external_id' do + let!(:existing_user) { create(:course_student, course: course, external_id: 'used-id') } + + it 'is invalid' do + invitation = build(:course_user_invitation, course: course, external_id: 'used-id') + expect(invitation).not_to be_valid + end + end + + context 'when only a confirmed invitation in the same course has the same external_id' do + let!(:confirmed) { create(:course_user_invitation, :confirmed, course: course, external_id: 'past-id') } + + # The uniqueness check (UniqueExternalIdConcern) only queries unconfirmed invitations. + # A confirmed invitation means the user has already joined the course, so their external_id + # is now on the CourseUser record. The invitation row is no longer an active claim on the id. + it 'is valid' do + invitation = build(:course_user_invitation, course: course, external_id: 'past-id') + expect(invitation).to be_valid + end + end + end + context 'when there is a previous invitation with the same email' do let(:email) { generate(:email) } let!(:previous_invitation) do diff --git a/spec/models/course_user_spec.rb b/spec/models/course_user_spec.rb index af3f13bff0c..1f61ae995ed 100644 --- a/spec/models/course_user_spec.rb +++ b/spec/models/course_user_spec.rb @@ -55,6 +55,67 @@ end end + describe 'external_id uniqueness' do + let(:other_course) { create(:course, creator: owner, updater: owner) } + + it 'allows multiple course users with nil external_id in the same course' do + create(:course_student, course: course, external_id: nil) + new_student = build(:course_student, course: course, external_id: nil) + expect(new_student).to be_valid + end + + it 'normalizes blank external_id to nil' do + student = create(:course_student, course: course, external_id: '') + expect(student.reload.external_id).to be_nil + end + + it 'is valid when external_id is unique in the course' do + student = build(:course_student, course: course, external_id: 'unique-id') + expect(student).to be_valid + end + + context 'when another course user in the same course has the same external_id' do + let!(:existing) { create(:course_student, course: course, external_id: 'dup-id') } + + it 'is invalid' do + student = build(:course_student, course: course, external_id: 'dup-id') + 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 + + context 'when a course user in a different course has the same external_id' do + let!(:existing) { create(:course_student, course: other_course, external_id: 'some-id') } + + it 'is valid' do + student = build(:course_student, course: course, external_id: 'some-id') + expect(student).to be_valid + end + end + + context 'when a pending invitation in the same course has the same external_id' do + let!(:invitation) { create(:course_user_invitation, course: course, external_id: 'pending-id') } + + it 'is invalid' do + student = build(:course_student, course: course, external_id: 'pending-id') + expect(student).not_to be_valid + end + end + + context 'when only a confirmed invitation in the same course has the same external_id' do + let!(:invitation) { create(:course_user_invitation, :confirmed, course: course, external_id: 'confirmed-id') } + + # The uniqueness check (UniqueExternalIdConcern) only queries unconfirmed invitations. + # A confirmed invitation means the user has already joined the course, so their external_id + # is now on the CourseUser record. The invitation row is no longer an active claim on the id. + it 'is valid' do + student = build(:course_student, course: course, external_id: 'confirmed-id') + expect(student).to be_valid + end + end + end + describe '.staff' do it 'returns teaching assistant, manager and owner' do expect(course.course_users.staff).to contain_exactly(teaching_assistant, manager, diff --git a/spec/models/user/email_spec.rb b/spec/models/user/email_spec.rb index 6ee1f9e16ba..72f501ddbdd 100644 --- a/spec/models/user/email_spec.rb +++ b/spec/models/user/email_spec.rb @@ -129,6 +129,41 @@ def sign_up_on(sign_up_instance) end end + context 'when the invitation has an external_id' do + let!(:course) { create(:course) } + let!(:pending_invitation) do + create(:course_user_invitation, course: course, email: email_address, external_id: 'EXT-123') + end + + it 'transfers external_id to the created CourseUser' do + email_record = sign_up_on(instance) + course_user = email_record.user.course_users.find_by(course: course) + expect(course_user.external_id).to eq('EXT-123') + end + + context 'when the external_id conflicts with an existing CourseUser at sign-up time' do + # Simulates a race condition: a CourseUser was assigned external_id 'EXT-123' + # after the invitation was created (bypassing model validation). + let!(:conflicting_course_user) do + create(:course_user, course: course).tap do |cu| + cu.update_column(:external_id, 'EXT-123') + end + end + + it 'rolls back the transaction, leaving the invitation unconfirmed' do + sign_up_on(instance) + expect(pending_invitation.reload).not_to be_confirmed + end + + it 'does not create a CourseUser for the invited user' do + email_record = sign_up_on(instance) + ActsAsTenant.without_tenant do + expect(CourseUser.exists?(user: email_record.user, course: course)).to be false + end + end + end + end + context 'when the user is already enrolled in the invited course' do let(:course) { create(:course) } let(:user) { create(:user) } diff --git a/spec/services/course/statistics/assessment_score_summary_download_service_spec.rb b/spec/services/course/statistics/assessment_score_summary_download_service_spec.rb index c65aad32672..0049b3bacf4 100644 --- a/spec/services/course/statistics/assessment_score_summary_download_service_spec.rb +++ b/spec/services/course/statistics/assessment_score_summary_download_service_spec.rb @@ -80,6 +80,83 @@ expect(third_student_record[4]).to eq('') end end + + context 'when students have no external_id set (backwards compatibility)' do + let!(:filepath) { subject } + let!(:csv_lines) { CSV.open(filepath, 'r').readlines } + + it 'CSV has same column count as before (no external_id column)' do + expect(csv_lines[0].size).to eq(5) + end + + it 'CSV header does NOT include external_id header' do + expect(csv_lines[0]).not_to include('external_id') + end + + it 'existing columns (name, email, type, scores) are at same indices' do + header = csv_lines[0] + expect(header[0]).to eq(I18n.t('csv.score_summary.headers.name')) + expect(header[1]).to eq(I18n.t('csv.score_summary.headers.email')) + expect(header[2]).to eq(I18n.t('csv.score_summary.headers.type')) + expect(header[3]).to eq(assessment1.title) + expect(header[4]).to eq(assessment2.title) + end + end + + context 'when some students have external_id set' do + let!(:student_with_ext_id) { create(:course_user, course: course, name: 'Student 4', external_id: 'EXT001') } + let!(:student_without_ext_id) { create(:course_user, course: course, name: 'Student 5') } + + let!(:submission_a1) do + create(:submission, :published, assessment: assessment1, creator: student_with_ext_id.user) + end + let!(:submission_a2) do + create(:submission, :published, assessment: assessment2, creator: student_with_ext_id.user) + end + let!(:submission_b1) do + create(:submission, :published, assessment: assessment1, creator: student_without_ext_id.user) + end + let!(:submission_b2) do + create(:submission, :published, assessment: assessment2, creator: student_without_ext_id.user) + end + + let(:assessment_ids_list) { [assessment1.id, assessment2.id] } + let(:service) { described_class.new(course, assessment_ids_list, file_name) } + let!(:filepath) { service.generate } + let!(:csv_lines) { CSV.open(filepath, 'r').readlines } + + it 'CSV header includes external_id column' do + expect(csv_lines[0].size).to eq(6) + expect(csv_lines[0]).to include(I18n.t('csv.score_summary.headers.external_id')) + end + + it 'CSV header has external_id before assessment scores' do + header = csv_lines[0] + ext_id_index = header.index(I18n.t('csv.score_summary.headers.external_id')) + first_score_index = header.index(assessment1.title) + expect(ext_id_index).to be < first_score_index + end + + it 'student with external_id has it before assessment scores' do + fourth_student_record = csv_lines[4] + expect(fourth_student_record[0]).to eq(student_with_ext_id.name) + expect(fourth_student_record[1]).to eq(student_with_ext_id.user.email) + expect(fourth_student_record[2]).to eq(student_with_ext_id.phantom? ? 'phantom' : 'normal') + expect(fourth_student_record[3]).to eq('EXT001') + expect(fourth_student_record[4]).to eq(submission_a1.grade.to_s) + expect(fourth_student_record[5]).to eq(submission_a2.grade.to_s) + end + + it 'student without external_id has empty string in external_id column before scores' do + fifth_student_record = csv_lines[5] + expect(fifth_student_record[0]).to eq(student_without_ext_id.name) + expect(fifth_student_record[1]).to eq(student_without_ext_id.user.email) + expect(fifth_student_record[2]).to eq(student_without_ext_id.phantom? ? 'phantom' : 'normal') + expect(fifth_student_record[3]).to eq('') + expect(fifth_student_record[4]).to eq(submission_b1.grade.to_s) + expect(fifth_student_record[5]).to eq(submission_b2.grade.to_s) + end + end end end end diff --git a/spec/services/course/user_invitation_service_spec.rb b/spec/services/course/user_invitation_service_spec.rb index aff5da7044e..16b500261f8 100644 --- a/spec/services/course/user_invitation_service_spec.rb +++ b/spec/services/course/user_invitation_service_spec.rb @@ -38,7 +38,7 @@ def temp_csv_from_attributes(records, roles = [], timeline_algorithms = []) let(:existing_user_attributes) do existing_users.each_with_index.map do |user, id| { name: user.name, email: user.email, phantom: false, - role: existing_roles[id], timeline_algorithm: existing_timeline_algorithms[id] } + role: existing_roles[id], timeline_algorithm: existing_timeline_algorithms[id], external_id: nil } end end let(:new_roles) { Course::UserInvitation.roles.keys.sample(3).map(&:to_sym) } @@ -51,7 +51,7 @@ def temp_csv_from_attributes(records, roles = [], timeline_algorithms = []) let(:new_user_attributes) do new_users.each_with_index.map do |user, id| { name: user.name, email: user.email, phantom: false, - role: new_roles[id], timeline_algorithm: new_timeline_algorithms[id] } + role: new_roles[id], timeline_algorithm: new_timeline_algorithms[id], external_id: nil } end end let(:invalid_user_attributes) do @@ -153,6 +153,11 @@ def invite expect(invite.map(&:size)).to eq([new_user_attributes.size - 1, 0, existing_user_attributes.size, 0, 1]) end + it 'tags the duplicate user with a duplicate_email reason' do + _new_invitations, _existing_invitations, _new_course_users, _existing_course_users, duplicate_users = invite + expect(duplicate_users.first[:reason]).to eq(:duplicate_email_in_file) + end + with_active_job_queue_adapter(:test) do it 'sends only one invitation to duplicate users', type: :mailer do expect { invite }.to change { ActionMailer::Base.deliveries.count }. @@ -161,6 +166,441 @@ def invite end end + context 'when two invitations in the same batch share the same external_id' do + let(:form_attributes) do + { generate(:nested_attribute_new_id) => { name: 'User A', email: generate(:email), + role: :student, phantom: false, + external_id: 'shared-id' }, + generate(:nested_attribute_new_id) => { name: 'User B', email: generate(:email), + role: :student, phantom: false, + external_id: 'shared-id' } } + end + + it 'processes only the first and treats the second as a duplicate' do + result = subject.invite(form_attributes) + expect(result).not_to be_nil + new_invitations, _existing_invitations, _new_course_users, _existing_course_users, duplicate_users = result + expect(new_invitations.size).to eq(1) + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:external_id]).to eq('shared-id') + end + + it 'tags the duplicate user with a duplicate_external_id reason' do + result = subject.invite(form_attributes) + _new_invitations, _existing_invitations, _new_course_users, _existing_course_users, duplicate_users = result + expect(duplicate_users.first[:reason]).to eq(:duplicate_external_id_in_file) + end + end + + context 'when an invitation has a duplicate external_id matching an existing course user' do + let!(:existing_course_user) { create(:course_student, course: course, external_id: 'taken-id') } + let(:form_attributes) do + { generate(:nested_attribute_new_id) => { name: 'New User', email: generate(:email), + role: :student, phantom: false, + external_id: 'taken-id' } } + end + subject(:result) { invitation_service.invite(form_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'does not abort the batch' do + expect(result).not_to be_nil + end + + it 'puts the conflicting row in duplicate_users with :external_id_taken reason' do + _, _, _, _, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + expect(duplicate_users.first[:external_id]).to eq('taken-id') + end + + it 'does not create a new invitation' do + new_invitations, = result + expect(new_invitations).to be_empty + end + end + + context 'when an invitation has a duplicate external_id matching a pending invitation' do + let!(:pending_invitation) { create(:course_user_invitation, course: course, external_id: 'taken-id') } + let(:form_attributes) do + { generate(:nested_attribute_new_id) => { name: 'New User', email: generate(:email), + role: :student, phantom: false, + external_id: 'taken-id' } } + end + subject(:result) { invitation_service.invite(form_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'does not abort the batch' do + expect(result).not_to be_nil + end + + it 'puts the conflicting row in duplicate_users with :external_id_taken reason' do + _, _, _, _, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + end + end + + context 'when a CSV (with personalized timelines) has a duplicate external_id for an existing course user' do + let!(:existing_course_user) { create(:course_student, course: course, external_id: 'taken-id') } + + def csv_with_external_id_and_timeline(entries) + Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| + file.write(CSV.generate do |csv| + entries.each do |entry| + csv << [entry[:name], entry[:email], 'student', 'false', 'fixed', entry[:external_id]] + end + end) + file.rewind + end + end + + it 'does not abort the batch' do + csv = csv_with_external_id_and_timeline( + [{ name: 'New User', email: generate(:email), external_id: 'taken-id' }] + ) + result = subject.invite(csv) + expect(result).not_to be_nil + _, _, _, _, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + csv.close! + end + end + + context 'when a CSV (without personalized timelines) has a duplicate external_id for an existing course user' do + before { course.update!(show_personalized_timeline_features: false) } + let!(:existing_course_user) { create(:course_student, course: course, external_id: 'taken-id') } + + def csv_with_external_id_no_timeline(entries) + Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| + file.write(CSV.generate do |csv| + entries.each do |entry| + csv << [entry[:name], entry[:email], 'student', 'false', entry[:external_id]] + end + end) + file.rewind + end + end + + it 'does not abort the batch' do + csv = csv_with_external_id_no_timeline( + [{ name: 'New User', email: generate(:email), external_id: 'taken-id' }] + ) + result = subject.invite(csv) + expect(result).not_to be_nil + _, _, _, _, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + csv.close! + end + end + + context 'when re-inviting a pending invitation with a new external_id' do + let!(:pending_invitation) do + create(:course_user_invitation, course: course, + email: 'pending@example.com', external_id: nil) + end + let(:invitation_attributes) do + { '0' => { name: 'Pending User', email: 'pending@example.com', role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'does not update the pending invitation external_id' do + result + expect(pending_invitation.reload.external_id).to be_nil + end + + it 'puts the user in existing_invitations' do + _, existing_invitations, = result + expect(existing_invitations).to include(pending_invitation) + end + + it 'does not create duplicate_users entry' do + _, _, _, _, duplicate_users = result + expect(duplicate_users).to be_empty + end + end + + context 'when an existing course user is re-invited with a new external_id' do + let!(:enrolled_user) { create(:user) } + let!(:course_user_record) { create(:course_student, course: course, user: enrolled_user, external_id: nil) } + let(:invitation_attributes) do + { '0' => { name: enrolled_user.name, email: enrolled_user.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'does not update the course_user external_id' do + result + expect(course_user_record.reload.external_id).to be_nil + end + + it 'puts the user in existing_course_users' do + _, _, _, existing_course_users, = result + expect(existing_course_users).to include(course_user_record) + end + + it 'produces no duplicate_users entry' do + _, _, _, _, duplicate_users = result + expect(duplicate_users).to be_empty + end + end + + context 'when an existing course user is re-invited and the external_id is already taken' do + let!(:other_user) { create(:course_student, course: course, external_id: 'taken-id') } + let!(:enrolled_user) { create(:user) } + let!(:course_user_record) { create(:course_student, course: course, user: enrolled_user, external_id: nil) } + let(:invitation_attributes) do + { '0' => { name: enrolled_user.name, email: enrolled_user.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'does not abort the batch' do + expect(result).not_to be_nil + end + + it 'puts the user in existing_course_users' do + _, _, _, existing_course_users, = result + expect(existing_course_users).to include(course_user_record) + end + + it 'does not update the course_user external_id' do + result + expect(course_user_record.reload.external_id).to be_nil + end + + it 'does not create a duplicate_users entry' do + _, _, _, _, duplicate_users = result + expect(duplicate_users).to be_empty + end + end + + context 'when a new user is invited but the external_id matches an existing course user' do + let!(:existing_course_user_ext) { create(:course_student, course: course, external_id: 'taken-id') } + let(:new_user) { create(:user) } + let(:invitation_attributes) do + { '0' => { name: new_user.name, email: new_user.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'does not abort the batch' do + expect(result).not_to be_nil + end + + it 'puts the row in duplicate_users with :external_id_taken reason' do + _, _, _, _, duplicate_users = result + expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + end + + it 'does not enroll the user' do + _, _, new_course_users, = result + expect(new_course_users).to be_empty + end + end + + context 'when a new user is enrolled but the external_id matches a pending invitation' do + let!(:pending_inv) { create(:course_user_invitation, course: course, external_id: 'taken-id') } + let(:new_user) { create(:user) } + let(:invitation_attributes) do + { '0' => { name: new_user.name, email: new_user.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'puts the row in duplicate_users with :external_id_taken reason' do + _, _, _, _, duplicate_users = result + expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + end + + it 'does not enroll the user' do + _, _, new_course_users, = result + expect(new_course_users).to be_empty + end + end + + context 'when an existing course user is re-invited and the external_id is taken by a pending invitation' do + let!(:pending_inv) { create(:course_user_invitation, course: course, external_id: 'taken-id') } + let!(:enrolled_user) { create(:user) } + let!(:course_user_record) { create(:course_student, course: course, user: enrolled_user, external_id: nil) } + let(:invitation_attributes) do + { '0' => { name: enrolled_user.name, email: enrolled_user.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'puts the user in existing_course_users' do + _, _, _, existing_course_users, = result + expect(existing_course_users).to include(course_user_record) + end + + it 'does not update the course_user external_id' do + result + expect(course_user_record.reload.external_id).to be_nil + end + + it 'does not create a duplicate_users entry' do + _, _, _, _, duplicate_users = result + expect(duplicate_users).to be_empty + end + end + + context 'when re-inviting a pending invitation whose new external_id is already taken' do + let!(:other_user) { create(:course_student, course: course, external_id: 'taken-id') } + let!(:pending_invitation) do + create(:course_user_invitation, course: course, + email: 'pending@example.com', name: 'Pending User', external_id: 'old-id') + end + let(:invitation_attributes) do + { '0' => { name: 'Pending User', email: 'pending@example.com', role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'puts the user in existing_invitations' do + _, existing_invitations, = result + expect(existing_invitations).to include(pending_invitation) + end + + it 'does not update the pending invitation external_id' do + result + expect(pending_invitation.reload.external_id).to eq('old-id') + end + + it 'does not create a duplicate_users entry' do + _, _, _, _, duplicate_users = result + expect(duplicate_users).to be_empty + end + end + + context 'when a batch reassigns a pending invitation ext_id and a later row claims the freed id' do + let!(:alice_invitation) do + create(:course_user_invitation, course: course, + email: 'alice@example.com', name: 'Alice', external_id: 'freed-id') + end + let(:bob_email) { generate(:email) } + let(:invitation_attributes) do + { '0' => { name: 'Alice', email: 'alice@example.com', role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' }, + '1' => { name: 'Bob', email: bob_email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'freed-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'puts the first row (existing invitation) in existing_invitations unchanged' do + _, existing_invitations, = result + expect(existing_invitations).to include(alice_invitation) + expect(alice_invitation.reload.external_id).to eq('freed-id') + end + + it 'rejects the second row as external_id_taken because the freed-id is never released' do + _, _, _, _, duplicate_users = result + expect(duplicate_users.map { |u| u[:reason] }).to include(:external_id_taken) + end + end + + context 'CSV batch duplicate scenarios' do + def csv_with_timeline(entries) + Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| + file.write(CSV.generate do |csv| + csv << ['Name', 'Email', 'Role', 'Phantom', 'Timeline', 'ExternalId'] + entries.each do |e| + csv << [e[:name], e[:email], e.fetch(:role, 'student'), + e.fetch(:phantom, 'false'), e.fetch(:timeline, 'fixed'), e[:external_id]] + end + end) + file.rewind + end + end + + context 'when a CSV has two rows with the same email' do + it 'puts the second in duplicateUsers with reason duplicate_email' do + csv = csv_with_timeline([ + { name: 'User A', email: 'a@example.com', external_id: 'id-1' }, + { name: 'User B', email: 'a@example.com', external_id: 'id-2' } + ]) + result = subject.invite(csv) + _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:duplicate_email_in_file) + csv.close! + end + end + + context 'when a CSV has two rows with the same external_id' do + it 'puts the second in duplicateUsers with reason duplicate_external_id' do + csv = csv_with_timeline([ + { name: 'User A', email: 'a@example.com', external_id: 'shared-id' }, + { name: 'User B', email: 'b@example.com', external_id: 'shared-id' } + ]) + result = subject.invite(csv) + _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:duplicate_external_id_in_file) + csv.close! + end + end + + context 'when a CSV row has a course-duplicate email AND a batch-duplicate external_id' do + let(:enrolled_user) { create(:instance_user).user } + let!(:course_student) { create(:course_student, course: course, user: enrolled_user) } + + it 'is caught as a batch external_id duplicate and does not fail the batch' do + csv = csv_with_timeline([ + { name: 'User A', email: 'a@example.com', external_id: 'shared-id' }, + { name: 'Enrolled', email: enrolled_user.email, external_id: 'shared-id' } + ]) + result = subject.invite(csv) + expect(result).not_to be_nil + _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:duplicate_external_id_in_file) + csv.close! + end + end + + context 'when a CSV row duplicates both email and external_id within the batch' do + it 'is caught as a duplicate_email (email is checked first)' do + csv = csv_with_timeline([ + { name: 'User A', email: 'a@example.com', external_id: 'id-1' }, + { name: 'User B', email: 'a@example.com', external_id: 'id-1' } + ]) + result = subject.invite(csv) + _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result + expect(duplicate_users.size).to eq(1) + expect(duplicate_users.first[:reason]).to eq(:duplicate_email_in_file) + csv.close! + end + end + end + + context 'when pre-loading taken external_ids' do + let!(:enrolled) { create(:course_student, course: course, external_id: 'enrolled-id') } + let!(:pending_inv) { create(:course_user_invitation, course: course, external_id: 'pending-id') } + let(:invitation_attributes) do + { '0' => { name: 'A', email: generate(:email), role: :student, phantom: false, + timeline_algorithm: :fixed, external_id: 'new-id' } } + end + + it 'does not treat a new unique external_id as taken' do + result = subject.invite(invitation_attributes) + expect(result).not_to be_nil + new_invitations, _, _, _, duplicate_users = result + expect(new_invitations.length).to eq(1) + expect(duplicate_users).to be_empty + end + end + context 'when an invalid email is specified' do let(:invalid_user_attributes) do [{ name: build(:user).name, email: 'xxnot an email', role: :student }] @@ -214,15 +654,7 @@ def invite let(:temp_csv) { temp_csv_from_attributes(users, roles, timeline_algorithms) } after { temp_csv.close! } - it 'accepts a file with name/header' do - result = subject.send(:parse_from_file, temp_csv) - expect(result.length).to eq(users.length) - end - - it 'calls #invite_users with appropriate user attributes' do - result = subject.send(:parse_from_file, temp_csv) - expect(result).to eq(user_attributes) - end + # --- file format edge cases (mode-agnostic) --- context 'when the provided file is invalid' do it 'raises an exception' do @@ -243,149 +675,240 @@ def invite end end - context 'when the provided file has no roles' do - let(:temp_csv_without_role) { temp_csv_from_attributes(users) } - after { temp_csv_without_role.close! } + context 'when the provided file has whitespace in the fields' do + let(:csv_file) { file_fixture('course/invitation_whitespace.csv') } - it 'defaults the role to student' do - result = subject.send(:parse_from_file, temp_csv_without_role) + it 'strips the attributes of whitespace' do + result = subject.send(:parse_from_file, csv_file) result.each do |attr| - expect(attr[:role]).to eq(:student) + expect(attr[:name]).to eq(attr[:name].strip) + expect(attr[:email]).to eq(attr[:email].strip) end end end - context 'when the provided file has no timeline algorithm and \ - default course timeline setting is fomo' do - before do - course.update!(default_timeline_algorithm: 'fomo') + context 'when the provided csv file has blanks' do + subject do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_empty.csv')) end - let(:temp_csv_without_timeline) { temp_csv_from_attributes(users) } - after { temp_csv_without_timeline.close! } - it 'defaults the timeline algorithm to fomo' do - result = subject.send(:parse_from_file, temp_csv_without_timeline) - result.each do |attr| - expect(attr[:timeline_algorithm]).to eq('fomo') - end + it 'does not raise an exception' do + expect { subject }.not_to raise_exception end - end - context 'when the provided file has no timeline algorithm and \ - default course timeline setting is stragglers' do - before do - course.update!(default_timeline_algorithm: 'stragglers') + it 'ignores blank entries and invites users with both name and emails or emails only' do + # Empty invitation CSV only has 1 full entry and 1 entry only with email + expect(subject.flatten.count).to eq(2) end - let(:temp_csv_without_timeline) { temp_csv_from_attributes(users) } - after { temp_csv_without_timeline.close! } + end - it 'defaults the timeline algorithm to stragglers' do - result = subject.send(:parse_from_file, temp_csv_without_timeline) - result.each do |attr| - expect(attr[:timeline_algorithm]).to eq('stragglers') - end + context 'when the provided csv file has no header' do + subject do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_no_header.csv')) end - end - context 'when the provided file has no timeline algorithm and \ - default course timeline setting is otot' do - before do - course.update!(default_timeline_algorithm: 'otot') + it 'does not raise an exception' do + expect { subject }.not_to raise_exception end - let(:temp_csv_without_timeline) { temp_csv_from_attributes(users) } - after { temp_csv_without_timeline.close! } - it 'defaults the timeline algorithm to otot' do - result = subject.send(:parse_from_file, temp_csv_without_timeline) - result.each do |attr| - expect(attr[:timeline_algorithm]).to eq('otot') - end + it 'invites all users including the first row' do + # No header CSV has 2 entries + expect(subject.flatten.count).to eq(2) end end - context 'when the provided file has whitespace in the fields' do - let(:csv_file) { file_fixture('course/invitation_whitespace.csv') } + # --- timeline-aware parsing --- - it 'strips the attributes of whitespace' do - result = subject.send(:parse_from_file, csv_file) - result.each do |attr| - expect(attr[:name]).to eq(attr[:name].strip) - expect(attr[:email]).to eq(attr[:email].strip) - end - end - end + context 'when personal timelines are enabled' do + before { course.update!(show_personalized_timeline_features: true) } - context 'when the csv file has slightly invalid role and/or phantom and/or \ - timeline algorithm specifications' do - subject do - stubbed_user_invitation_service. - send(:parse_from_file, file_fixture('course/invitation_fuzzy_roles_phantom_timeline.csv')) + it 'accepts a file with name/header' do + result = subject.send(:parse_from_file, temp_csv) + expect(result.length).to eq(users.length) end - it 'defaults blank role column to student' do - expect(subject[0][:role]).to eq(:student) + it 'calls #invite_users with appropriate user attributes' do + result = subject.send(:parse_from_file, temp_csv) + expect(result).to eq(user_attributes) end - it 'defaults blank phantom to false' do - expect(subject[0][:phantom]).to be_falsey - end + context 'when the provided file has no roles' do + let(:temp_csv_without_role) { temp_csv_from_attributes(users) } + after { temp_csv_without_role.close! } - it 'defaults blank timeline algorithm to course default (fixed)' do - expect(subject[0][:timeline_algorithm]).to eq('fixed') + it 'defaults the role to student' do + result = subject.send(:parse_from_file, temp_csv_without_role) + result.each do |attr| + expect(attr[:role]).to eq(:student) + end + end end - it 'parses roles correctly anyway' do - expect(subject[1][:role]).to eq(:teaching_assistant) - expect(subject[2][:role]).to eq(:manager) - expect(subject[3][:role]).to eq(:owner) - expect(subject[4][:role]).to eq(:observer) - expect(subject[5][:role]).to eq(:teaching_assistant) - end + context 'when the csv file has slightly invalid role/phantom/timeline algorithm specifications' do + subject do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_fuzzy_roles_phantom_timeline.csv')) + end + + it 'defaults blank role column to student' do + expect(subject[0][:role]).to eq(:student) + end + + it 'defaults blank phantom to false' do + expect(subject[0][:phantom]).to be_falsey + end - it 'parses phantom columns correctly anyway' do - expect(subject[1][:phantom]).to be_falsey - (6..8).each do |i| - expect(subject[i][:phantom]).to be_truthy + it 'defaults blank timeline algorithm to course default (fixed)' do + expect(subject[0][:timeline_algorithm]).to eq('fixed') + end + + it 'parses roles correctly anyway' do + expect(subject[1][:role]).to eq(:teaching_assistant) + expect(subject[2][:role]).to eq(:manager) + expect(subject[3][:role]).to eq(:owner) + expect(subject[4][:role]).to eq(:observer) + expect(subject[5][:role]).to eq(:teaching_assistant) + end + + it 'parses phantom columns correctly anyway' do + expect(subject[1][:phantom]).to be_falsey + (6..8).each do |i| + expect(subject[i][:phantom]).to be_truthy + end + end + + it 'parses timeline algorithms correctly anyway' do + expect(subject[1][:timeline_algorithm]).to eq(:stragglers) + expect(subject[2][:timeline_algorithm]).to eq(:otot) + expect(subject[3][:timeline_algorithm]).to eq(:fomo) + expect(subject[4][:timeline_algorithm]).to eq(:fixed) end end - it 'parses roles correctly anyway' do - expect(subject[1][:timeline_algorithm]).to eq(:stragglers) - expect(subject[2][:timeline_algorithm]).to eq(:otot) - expect(subject[3][:timeline_algorithm]).to eq(:fomo) - expect(subject[4][:timeline_algorithm]).to eq(:fixed) + context 'when no timeline algorithm column is present' do + let(:temp_csv_without_timeline) { temp_csv_from_attributes(users) } + after { temp_csv_without_timeline.close! } + + context 'when the course default is fomo' do + before { course.update!(default_timeline_algorithm: 'fomo') } + + it 'defaults the timeline algorithm to fomo' do + result = subject.send(:parse_from_file, temp_csv_without_timeline) + result.each do |attr| + expect(attr[:timeline_algorithm]).to eq('fomo') + end + end + end + + context 'when the course default is stragglers' do + before { course.update!(default_timeline_algorithm: 'stragglers') } + + it 'defaults the timeline algorithm to stragglers' do + result = subject.send(:parse_from_file, temp_csv_without_timeline) + result.each do |attr| + expect(attr[:timeline_algorithm]).to eq('stragglers') + end + end + end + + context 'when the course default is otot' do + before { course.update!(default_timeline_algorithm: 'otot') } + + it 'defaults the timeline algorithm to otot' do + result = subject.send(:parse_from_file, temp_csv_without_timeline) + result.each do |attr| + expect(attr[:timeline_algorithm]).to eq('otot') + end + end + end end - end - context 'when the provided csv file has blanks' do - subject do - stubbed_user_invitation_service. - send(:parse_from_file, file_fixture('course/invitation_empty.csv')) + context 'when the csv has an external_id column' do + subject do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_with_external_id.csv')) + end + + it 'parses external_id from col 6 correctly' do + expect(subject[0][:external_id]).to eq('EXT001') + expect(subject[1][:external_id]).to eq('EXT002') + end + + it 'sets external_id to nil when blank' do + expect(subject[2][:external_id]).to be_nil + end end - it 'does not raise an exception' do - expect { subject }.not_to raise_exception + context 'when the csv has no external_id column' do + let(:csv_without_external_id) { file_fixture('course/invitation_fuzzy_roles_phantom_timeline.csv') } + + it 'sets external_id to nil for all rows' do + result = stubbed_user_invitation_service.send(:parse_from_file, csv_without_external_id) + result.each do |attr| + expect(attr[:external_id]).to be_nil + end + end end - it 'ignores blank entries and invites users with both name and emails or emails only' do - # Empty invitation CSV only has 1 full entry and 1 entry only with email - expect(subject.flatten.count).to eq(2) + context 'when the csv header uses a slightly wrong external_id column name' do + subject do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_external_id_wrong_header.csv')) + end + + it 'still detects and skips the header row' do + expect(subject.length).to eq(2) + end + + it 'still parses external_id from col 6 correctly' do + expect(subject[0][:external_id]).to eq('EXT001') + expect(subject[1][:external_id]).to eq('EXT002') + end end end - context 'when the provided csv file has no header' do - subject do - stubbed_user_invitation_service. - send(:parse_from_file, file_fixture('course/invitation_no_header.csv')) - end + context 'when personal timelines are disabled' do + before { course.update!(show_personalized_timeline_features: false) } - it 'does not raise an exception' do - expect { subject }.not_to raise_exception + context 'when the csv has an external_id column' do + subject do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_with_external_id_no_timeline.csv')) + end + + it 'parses external_id from col 5 correctly' do + expect(subject[0][:external_id]).to eq('EXT001') + expect(subject[1][:external_id]).to eq('EXT002') + end + + it 'sets external_id to nil when blank' do + expect(subject[2][:external_id]).to be_nil + end + + it 'auto-fills timeline_algorithm with course default' do + result = stubbed_user_invitation_service.send( + :parse_from_file, + file_fixture('course/invitation_with_external_id_no_timeline.csv') + ) + result.each do |attr| + expect(attr[:timeline_algorithm]).to eq(course.default_timeline_algorithm) + end + end end - it 'invites all users including the first row' do - # No header CSV has 2 entries - expect(subject.flatten.count).to eq(2) + context 'when the csv has no external_id column' do + subject do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_no_external_id_no_timeline.csv')) + end + + it 'sets external_id to nil for all rows' do + subject.each do |attr| + expect(attr[:external_id]).to be_nil + end + end end end end @@ -622,6 +1145,8 @@ def invite end it 'adds an invitation to the user' do + subject.instance_variable_set(:@duplicate_users, []) + subject.instance_variable_set(:@taken_external_ids, Set.new) subject.send(:invite_new_users, invitation_params) invitation_params.each do |hash| invitation = course.invitations.find { |i| i.name == hash[:name] } diff --git a/spec/services/course/user_registration_service_spec.rb b/spec/services/course/user_registration_service_spec.rb index 1bfe5011293..9b52f21d048 100644 --- a/spec/services/course/user_registration_service_spec.rb +++ b/spec/services/course/user_registration_service_spec.rb @@ -250,6 +250,22 @@ def self.registration_with_registration_code end.to change { course.course_users.reload.count }.by(0) end end + + context 'when the user is already enrolled and the invitation has an external_id' do + let!(:existing_course_user) { create(:course_student, course: course, user: user, external_id: nil) } + let!(:invitation_with_ext_id) do + create(:course_user_invitation, course: course, email: user.email, external_id: 'stu-001') + end + let(:registration) do + Course::Registration.new(course: course, user: user, + code: invitation_with_ext_id.invitation_key.dup) + end + + it 'does not update the external_id on the existing CourseUser' do + subject.send(:claim_registration_code, registration) + expect(existing_course_user.reload.external_id).to be_nil + end + end end describe '#accept_invitation' do