From 9c390c497c6b1641cdcf09c7d2c83fa23c3b2f28 Mon Sep 17 00:00:00 2001 From: lws49 Date: Thu, 14 May 2026 14:19:21 +0800 Subject: [PATCH] feat(users): add optional external_id field to course members and invitations Allows institutions to link Coursemology enrolments to their own student records or LMS identifiers. The field is stored on CourseUser and Course::UserInvitation and validated unique per course across both tables via a cross-table concern and partial DB index - a pending invitation holds its ext_id until confirmed, preventing collisions with direct enrolments. Surfaces: - Individual invite form: ext_id input field - Bulk CSV invite: ext_id column in both template variants (with and without Timeline column); set on new records only - existing pending invitations and enrolled users are read-only in this flow - Manage users table: inline editable ext_id column (always visible) - Score summary export: ext_id column included when any student has one - Statistics > Students table: ext_id column, sortable and searchable, shown only when at least one student has a non-null ext_id View-only tables suppress the ext_id column when no course members have one set, consistent with how group manager, gamification, and video columns are conditionally shown. Edit tables always show it. Also changes error responses from the invitations controller from a single concatenated string to an array of per-record strings, enabling the frontend to render overflow counts without truncating meaningful error detail. --- .../users_controller_management_concern.rb | 2 +- .../course/user_invitations_controller.rb | 67 +- .../course/unique_external_id_concern.rb | 46 ++ app/models/course/user_invitation.rb | 2 + app/models/course_user.rb | 1 + app/models/user.rb | 1 + app/models/user/email.rb | 10 +- .../parse_invitation_concern.rb | 43 +- .../process_invitation_concern.rb | 67 +- ...essments_score_summary_download_service.rb | 7 +- .../course/user_invitation_service.rb | 5 +- .../course/user_registration_service.rb | 25 +- .../aggregate/all_students.json.jbuilder | 3 + ...se_user_invitation_list_data.json.jbuilder | 1 + .../_invitation_result_data.json.jbuilder | 6 + .../users/_user_list_data.json.jbuilder | 1 + ...e-user-invitation-template-no-timeline.csv | 3 + .../course-user-invitation-template.csv | 6 +- client/app/bundles/course/helper/index.ts | 9 +- .../students/StudentStatisticsTable.tsx | 13 + client/app/bundles/course/statistics/types.ts | 2 + .../buttons/InvitationActionButtons.tsx | 37 +- .../components/forms/IndividualInvitation.tsx | 14 + .../components/forms/IndividualInviteForm.tsx | 40 +- .../misc/InvitationResultDialog.tsx | 4 +- .../InvitationResultInvitationsTable.tsx | 14 + .../tables/InvitationResultUsersTable.tsx | 70 +- .../tables/UserInvitationsTable.tsx | 15 + .../course/user-invitations/operations.ts | 42 +- .../pages/InviteUsersFileUpload/index.tsx | 65 +- .../ManageUsersTable/ExternalIdField.tsx | 72 ++ .../tables/ManageUsersTable/UserNameField.tsx | 1 + .../tables/ManageUsersTable/index.tsx | 9 + client/app/bundles/course/users/operations.ts | 1 + .../app/bundles/course/users/translations.ts | 24 + .../DataTableInlineEditable/TextField.tsx | 4 +- client/app/lib/translations/table.ts | 8 + client/app/types/course/courseUsers.ts | 3 + client/app/types/course/userInvitations.ts | 22 +- client/locales/en.json | 58 +- client/locales/ko.json | 55 +- client/locales/zh.json | 55 +- config/locales/en/activerecord/errors.yml | 5 + config/locales/en/csv.yml | 1 + config/locales/en/errors.yml | 7 +- config/locales/ko/activerecord/errors.yml | 5 + config/locales/ko/csv.yml | 1 + config/locales/ko/errors.yml | 7 +- config/locales/zh/activerecord/errors.yml | 5 + config/locales/zh/csv.yml | 1 + config/locales/zh/errors.yml | 7 +- ...rnal_id_to_course_users_and_invitations.rb | 16 + db/schema.rb | 6 +- .../user_invitations_controller_spec.rb | 86 +- spec/features/course/staff_management_spec.rb | 8 +- .../course/student_management_spec.rb | 8 +- .../invitation_duplicate_external_id.csv | 2 + .../invitation_external_id_wrong_header.csv | 3 + .../invitation_no_external_id_no_timeline.csv | 3 + .../course/invitation_with_external_id.csv | 4 + ...nvitation_with_external_id_no_timeline.csv | 4 + spec/models/course/user_invitation_spec.rb | 51 ++ spec/models/course_user_spec.rb | 61 ++ spec/models/user/email_spec.rb | 35 + ...ent_score_summary_download_service_spec.rb | 77 ++ .../course/user_invitation_service_spec.rb | 745 +++++++++++++++--- .../course/user_registration_service_spec.rb | 16 + 67 files changed, 1826 insertions(+), 271 deletions(-) create mode 100644 app/models/concerns/course/unique_external_id_concern.rb create mode 100644 client/app/assets/templates/course-user-invitation-template-no-timeline.csv create mode 100644 client/app/bundles/course/users/components/tables/ManageUsersTable/ExternalIdField.tsx create mode 100644 db/migrate/20260514052933_add_external_id_to_course_users_and_invitations.rb create mode 100644 spec/fixtures/course/invitation_duplicate_external_id.csv create mode 100644 spec/fixtures/course/invitation_external_id_wrong_header.csv create mode 100644 spec/fixtures/course/invitation_no_external_id_no_timeline.csv create mode 100644 spec/fixtures/course/invitation_with_external_id.csv create mode 100644 spec/fixtures/course/invitation_with_external_id_no_timeline.csv 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..c06993a7649 --- /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? + return unless external_id_taken_by_invitation? || external_id_taken_by_course_user? + + errors.add(:external_id, :taken) + end + + def external_id_taken_by_invitation? + query = Course::UserInvitation.unconfirmed.where(course_id: course_id, external_id: external_id) + query = query.where.not(id: id) if is_a?(Course::UserInvitation) + query.exists? + end + + def external_id_taken_by_course_user? + query = CourseUser.where(course_id: course_id, external_id: external_id) + query = query.where.not(id: id) if is_a?(CourseUser) + query.exists? + 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..86d2f9f69f2 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), + sortable: false, + searchable: false, + cell: (datum) => datum.externalId ?? null, + } satisfies ColumnTemplate, + ] + : []), { of: 'role', title: t(tableTranslations.role), 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..2f5914f61d2 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/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/index.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx index 2343c8f10f2..31da6ed8c16 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, 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..cb7aec0635d 100644 --- a/client/app/bundles/course/users/translations.ts +++ b/client/app/bundles/course/users/translations.ts @@ -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 04a7fdd3810..5d98aba0d47 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -6985,10 +6985,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})" @@ -7011,6 +7011,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" }, @@ -7041,11 +7053,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:" @@ -7077,6 +7098,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." }, @@ -7084,7 +7108,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}!" @@ -7212,6 +7236,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}" }, @@ -8178,6 +8220,9 @@ "lib.translations.table.column.email": { "defaultMessage": "Email" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "External ID" + }, "lib.translations.table.column.endAt": { "defaultMessage": "End At" }, @@ -8229,6 +8274,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 98def922a54..34196262f2a 100644 --- a/client/locales/ko.json +++ b/client/locales/ko.json @@ -6972,10 +6972,10 @@ "defaultMessage": "닫기" }, "course.userInvitations.InvitationResultDialog.duplicateInfo": { - "defaultMessage": "사용자가 중복 초대되었습니다. 첫 번째 사용자만 초대됩니다." + "defaultMessage": "사용자가 중복 초대되었습니다. 각 사용자의 첫 번째 항목만 초대됩니다." }, "course.userInvitations.InvitationResultDialog.duplicateUsers": { - "defaultMessage": "중복 이메일 사용자 ({count})" + "defaultMessage": "중복 사용자 ({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsers": { "defaultMessage": "기존 과정 사용자 ({count})" @@ -6998,6 +6998,18 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "새 초대장 ({count})" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { + "defaultMessage": "업로드 파일에 중복된 이메일 주소가 있습니다" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { + "defaultMessage": "업로드 파일에 중복된 외부 ID가 있습니다" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { + "defaultMessage": "해당 외부 ID는 이미 다른 강의 구성원에게 할당되어 있습니다" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { + "defaultMessage": "이미 강의 구성원입니다 — 외부 ID를 적용할 수 없습니다(해당 ID가 이미 다른 구성원에게 할당되어 있습니다)" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "수락된 초대" }, @@ -7031,11 +7043,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 파일을 업로드하세요:" @@ -7074,7 +7095,7 @@ "defaultMessage": "초대 삭제" }, "course.userInvitations.InvitationActionButtons.resendFailure": { - "defaultMessage": "초대 재전송에 실패했습니다 - {error}" + "defaultMessage": "초대 재전송에 실패했습니다." }, "course.userInvitations.InvitationActionButtons.resendSuccess": { "defaultMessage": "{email}로 초대 이메일을 다시 보냈습니다!" @@ -7202,6 +7223,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}(으)로 이름 변경하지 못했습니다." }, @@ -8177,6 +8216,9 @@ "lib.translations.table.column.email": { "defaultMessage": "이메일" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "외부 ID" + }, "lib.translations.table.column.endAt": { "defaultMessage": "종료 시각" }, @@ -8228,6 +8270,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 667b7326009..37a3aff0227 100644 --- a/client/locales/zh.json +++ b/client/locales/zh.json @@ -6966,10 +6966,10 @@ "defaultMessage": "关闭" }, "course.userInvitations.InvitationResultDialog.duplicateInfo": { - "defaultMessage": "在邀请中发现重复用户。只邀请该用户第一个账号。" + "defaultMessage": "在邀请中发现重复用户。只邀请每个用户的第一个实例。" }, "course.userInvitations.InvitationResultDialog.duplicateUsers": { - "defaultMessage": "具有重复电子邮件的用户({count})" + "defaultMessage": "重复用户({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsers": { "defaultMessage": "现有课程用户({count})" @@ -6992,6 +6992,18 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "新邀请({count})" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { + "defaultMessage": "上传文件中存在重复的电子邮箱" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { + "defaultMessage": "上传文件中存在重复的外部编号" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { + "defaultMessage": "该外部编号已分配给另一名课程成员" + }, + "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { + "defaultMessage": "已是课程成员 — 无法应用外部编号(该编号已分配给另一名成员)" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "已接受邀请" }, @@ -7025,11 +7037,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 文件:" @@ -7068,7 +7089,7 @@ "defaultMessage": "删除邀请" }, "course.userInvitations.InvitationActionButtons.resendFailure": { - "defaultMessage": "无法重新发送邀请 - {error}" + "defaultMessage": "无法重新发送邀请。" }, "course.userInvitations.InvitationActionButtons.resendSuccess": { "defaultMessage": "向 {email} 重新发送电子邮件邀请!" @@ -7196,6 +7217,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}" }, @@ -8171,6 +8210,9 @@ "lib.translations.table.column.email": { "defaultMessage": "电子邮件" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "外部编号" + }, "lib.translations.table.column.endAt": { "defaultMessage": "结束于" }, @@ -8222,6 +8264,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 bb21d913fe5..63cf8c05085 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 0e635b1f40c..42508712db4 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 1186605a7e4..5284d4a5e54 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_and_invitations.rb b/db/migrate/20260514052933_add_external_id_to_course_users_and_invitations.rb new file mode 100644 index 00000000000..f04b0359fe6 --- /dev/null +++ b/db/migrate/20260514052933_add_external_id_to_course_users_and_invitations.rb @@ -0,0 +1,16 @@ +class AddExternalIdToCourseUsersAndInvitations < ActiveRecord::Migration[7.2] + def change + add_column :course_users, :external_id, :string + add_column :course_user_invitations, :external_id, :string + + 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 AND confirmed_at IS 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 30d00863a32..1d320432f9a 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_23_065615) do +ActiveRecord::Schema[7.2].define(version: 2026_05_14_052933) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "uuid-ossp" @@ -1345,9 +1345,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) AND (confirmed_at IS 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 @@ -1369,6 +1371,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