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..abd143158f0 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,23 @@ 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[:external_id].any? + errors << t('errors.course.user_invitations.duplicate_external_id', + email: email, external_id: course_user.external_id) + end + if course_user.errors[:user].any? || course_user.errors[:course].any? + 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) } + 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 +182,25 @@ 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[:external_id].any? + errors << t('errors.course.user_invitations.duplicate_external_id', + email: email, external_id: invitation.external_id) + end + if invitation.errors[:email].any? + message = invitation.errors[:email].to_sentence + errors << t('errors.course.user_invitations.invalid_email', email: email, message: message) + end + errors << t('errors.course.user_invitations.duplicate_invitation', email: email) if invitation.errors[:base].any? + other_errors = invitation.errors.reject { |e| %w[external_id email base].include?(e.attribute.to_s) } + 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 +278,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..303c36b83a0 --- /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 + end + + private + + # Normalizes blank external IDs to nil. + # + # @return [void] + def normalize_external_id + self.external_id = nil if external_id.blank? + end + + # Validates that the external ID is unique within the course, + # across both course users and invitations. + # + # @return [void] + def validate_unique_external_id_within_course + return if external_id.blank? + + invitation_exists = Course::UserInvitation. + unconfirmed. + where(course_id: course_id, external_id: external_id). + where.not(id: id). + exists? + + course_user_exists = CourseUser. + where(course_id: course_id, external_id: external_id). + where.not(id: id). + exists? + + return unless invitation_exists || course_user_exists + + errors.add(:external_id, :taken) + end +end diff --git a/app/models/course/user_invitation.rb b/app/models/course/user_invitation.rb index bfbede2d9d8..e7dab5a9caa 100644 --- a/app/models/course/user_invitation.rb +++ b/app/models/course/user_invitation.rb @@ -4,6 +4,8 @@ class Course::UserInvitation < ApplicationRecord after_initialize :set_defaults, if: :new_record? before_validation :set_defaults, if: :new_record? + include Course::UniqueExternalIdConcern + validates :email, format: { with: Devise.email_regexp }, if: :email_changed? validates :name, presence: true validates :role, presence: true diff --git a/app/models/course_user.rb b/app/models/course_user.rb index d4c9bd9bfc4..944fe25e09c 100644 --- a/app/models/course_user.rb +++ b/app/models/course_user.rb @@ -3,6 +3,7 @@ class CourseUser < ApplicationRecord include CourseUser::StaffConcern include CourseUser::LevelProgressConcern include CourseUser::TodoConcern + include Course::UniqueExternalIdConcern after_initialize :set_defaults, if: :new_record? before_validation :set_defaults, if: :new_record? diff --git a/app/models/user.rb b/app/models/user.rb index 9415e285035..85923635741 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -129,6 +129,7 @@ def build_course_user_from_invitation(invitation) phantom: invitation.phantom, timeline_algorithm: invitation.timeline_algorithm || invitation.course&.default_timeline_algorithm, + external_id: invitation.external_id, creator: self, updater: self) end diff --git a/app/models/user/email.rb b/app/models/user/email.rb index bcb71d25b0a..ab9920c9bf6 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 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..f13bd79f463 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 @@ -48,11 +48,16 @@ def parse_invitations(users) def partition_unique_users(users) users.each { |user| user[:email] = user[:email].downcase } unique_users = {} + seen_external_ids = {} duplicate_users = [] users.each do |user| + ext_id = user[:external_id].presence if unique_users.key?(user[:email]) - duplicate_users.push(user) + duplicate_users.push(user.merge(reason: :duplicate_email)) + elsif ext_id && seen_external_ids.key?(ext_id) + duplicate_users.push(user.merge(reason: :duplicate_external_id)) else + seen_external_ids[ext_id] = true if ext_id unique_users[user[:email]] = user end end @@ -86,7 +91,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 +150,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 +200,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..e37076efd81 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 @@ -73,6 +73,7 @@ def add_existing_users(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, + external_id: user[:external_id], creator: @current_user, updater: @current_user) @current_course.enrol_requests.pending.find_by(user: user[:user].id)&.destroy! end @@ -113,7 +114,8 @@ def invite_new_users(users) new_invitations << @current_course.invitations.build(name: user[:name], email: user[:email], role: user[:role], phantom: user[:phantom], - timeline_algorithm: user[:timeline_algorithm]) + 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..bd31596a655 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 = @all_students.any? { |s| s.external_id.present? } 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'), - *@assessments.map(&:title) + *@assessments.map(&:title), + *(@include_external_id ? [I18n.t('csv.score_summary.headers.external_id')] : []) ] # 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] + *@assessments.flat_map { |a| @submission_grade_hash[[student.id, a.id]] || '' }, + *(@include_external_id ? [student.external_id || ''] : [])] end end end diff --git a/app/services/course/user_registration_service.rb b/app/services/course/user_registration_service.rb index c397c19f025..11f0ecb9dac 100644 --- a/app/services/course/user_registration_service.rb +++ b/app/services/course/user_registration_service.rb @@ -52,10 +52,12 @@ def find_or_create_course_user!(registration, invitation = nil) 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) || nil registration.course_user = CourseUser.find_or_create_by!(course: registration.course, user: registration.user, - name: name, role: role, phantom: phantom, timeline_algorithm: timeline_algorithm) + name: name, role: role, phantom: phantom, timeline_algorithm: timeline_algorithm, + external_id: external_id) end # Claims a given registration code. The correct type of code is deduced from the code itself and 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/user-invitations/components/buttons/InvitationActionButtons.tsx b/client/app/bundles/course/user-invitations/components/buttons/InvitationActionButtons.tsx index 303bb904153..cccd846fae1 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,14 +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, - }), - ); + 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)); + } throw error; }); }; 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)); + } }) .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..197dbe17f44 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,30 @@ 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({ + duplicateEmail: { + id: 'course.userInvitations.InvitationResultUsersTable.duplicateEmail', + defaultMessage: 'Duplicate email', + }, + duplicateExternalId: { + id: 'course.userInvitations.InvitationResultUsersTable.duplicateExternalId', + defaultMessage: 'Duplicate external ID', + }, +}); + interface Props { title: JSX.Element; - users: CourseUserData[]; + users: Array; } const InvitationResultUsersTable: FC = (props) => { @@ -20,6 +33,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 +82,44 @@ 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]; + return ( + + {user.reason === 'duplicate_external_id' + ? t(translations.duplicateExternalId) + : t(translations.duplicateEmail)} + + ); + }, + }, + }, + ] + : []), { 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..74d6604df88 100644 --- a/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx @@ -135,6 +135,10 @@ const FailedChip: FC<{ sentAt: string }> = ({ sentAt }) => { const UserInvitationsTable: FC = (props) => { const { invitations } = props; + const showExternalId = invitations.some( + (invitation) => invitation.externalId != null, + ); + const { t } = useTranslation(); const permissions = useAppSelector(getManageCourseUserPermissions); @@ -158,6 +162,17 @@ const UserInvitationsTable: FC = (props) => { searchable: true, cell: (datum) => datum.email, }, + ...(showExternalId + ? [ + { + of: 'externalId', + title: t(tableTranslations.externalId ?? null), + sortable: false, + searchable: 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..691ef3b828c 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..65cdd2284aa 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,14 @@ export interface InvitationFileEntity { file?: Blob; } +export type DuplicateReason = 'duplicate_email' | 'duplicate_external_id'; + +export interface DuplicateUserData extends CourseUserListData { + reason?: DuplicateReason; +} + export interface InvitationResult { - duplicateUsers?: CourseUserData[]; + duplicateUsers?: DuplicateUserData[]; existingCourseUsers?: CourseUserData[]; existingInvitations?: InvitationListData[]; newCourseUsers?: CourseUserData[]; @@ -22,6 +32,7 @@ export interface IndividualInvite { id?: string; name: string; email: string; + externalId?: string; role: string; phantom: boolean; timelineAlgorithm?: string; @@ -34,6 +45,7 @@ export interface InvitationPostData { id?: string | undefined; name: string; email: string; + externalId?: string | null; role: string; phantom: boolean; timelineAlgorithm?: string | undefined; @@ -44,6 +56,7 @@ export interface InvitationsPostData { id?: string | undefined; name: string; email: string; + externalId?: string | null; role: string; phantom: boolean; timelineAlgorithm?: string | undefined; @@ -54,6 +67,7 @@ export interface InvitationMiniEntity { id: number; name: string; email: string; + externalId: string | null; role: CourseUserRole; phantom: boolean; timelineAlgorithm?: TimelineAlgorithm; @@ -68,6 +82,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 9010ae111cf..9f18b48ecae 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -6889,10 +6889,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})" @@ -6915,6 +6915,12 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "New Invitations ({count})" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmail": { + "defaultMessage": "Duplicate email" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalId": { + "defaultMessage": "Duplicate external ID" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "Accepted Invitations" }, @@ -6945,11 +6951,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:" @@ -6981,6 +6996,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." }, @@ -6988,7 +7006,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}!" @@ -7116,6 +7134,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}" }, @@ -8055,6 +8091,9 @@ "lib.translations.table.column.email": { "defaultMessage": "Email" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "External ID" + }, "lib.translations.table.column.endAt": { "defaultMessage": "End At" }, @@ -8106,6 +8145,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 1c765561a6d..d7c5d0af01c 100644 --- a/client/locales/ko.json +++ b/client/locales/ko.json @@ -6876,10 +6876,10 @@ "defaultMessage": "닫기" }, "course.userInvitations.InvitationResultDialog.duplicateInfo": { - "defaultMessage": "사용자가 중복 초대되었습니다. 첫 번째 사용자만 초대됩니다." + "defaultMessage": "사용자가 중복 초대되었습니다. 각 사용자의 첫 번째 항목만 초대됩니다." }, "course.userInvitations.InvitationResultDialog.duplicateUsers": { - "defaultMessage": "중복 이메일 사용자 ({count})" + "defaultMessage": "중복 사용자 ({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsers": { "defaultMessage": "기존 과정 사용자 ({count})" @@ -6902,6 +6902,12 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "새 초대장 ({count})" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmail": { + "defaultMessage": "중복 이메일" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalId": { + "defaultMessage": "중복 외부 ID" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "수락된 초대" }, @@ -6935,11 +6941,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 파일을 업로드하세요:" @@ -6978,7 +6993,7 @@ "defaultMessage": "초대 삭제" }, "course.userInvitations.InvitationActionButtons.resendFailure": { - "defaultMessage": "초대 재전송에 실패했습니다 - {error}" + "defaultMessage": "초대 재전송에 실패했습니다." }, "course.userInvitations.InvitationActionButtons.resendSuccess": { "defaultMessage": "{email}로 초대 이메일을 다시 보냈습니다!" @@ -7106,6 +7121,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}(으)로 이름 변경하지 못했습니다." }, @@ -8054,6 +8087,9 @@ "lib.translations.table.column.email": { "defaultMessage": "이메일" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "외부 ID" + }, "lib.translations.table.column.endAt": { "defaultMessage": "종료 시각" }, @@ -8105,6 +8141,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 cf919268bb3..8cbebe4689c 100644 --- a/client/locales/zh.json +++ b/client/locales/zh.json @@ -6870,10 +6870,10 @@ "defaultMessage": "关闭" }, "course.userInvitations.InvitationResultDialog.duplicateInfo": { - "defaultMessage": "在邀请中发现重复用户。只邀请该用户第一个账号。" + "defaultMessage": "在邀请中发现重复用户。只邀请每个用户的第一个实例。" }, "course.userInvitations.InvitationResultDialog.duplicateUsers": { - "defaultMessage": "具有重复电子邮件的用户({count})" + "defaultMessage": "重复用户({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsers": { "defaultMessage": "现有课程用户({count})" @@ -6896,6 +6896,12 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "新邀请({count})" }, + "course.userInvitations.InvitationResultUsersTable.duplicateEmail": { + "defaultMessage": "重复电子邮件" + }, + "course.userInvitations.InvitationResultUsersTable.duplicateExternalId": { + "defaultMessage": "重复外部 ID" + }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "已接受邀请" }, @@ -6929,11 +6935,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 文件:" @@ -6972,7 +6987,7 @@ "defaultMessage": "删除邀请" }, "course.userInvitations.InvitationActionButtons.resendFailure": { - "defaultMessage": "无法重新发送邀请 - {error}" + "defaultMessage": "无法重新发送邀请。" }, "course.userInvitations.InvitationActionButtons.resendSuccess": { "defaultMessage": "向 {email} 重新发送电子邮件邀请!" @@ -7100,6 +7115,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}" }, @@ -8048,6 +8081,9 @@ "lib.translations.table.column.email": { "defaultMessage": "电子邮件" }, + "lib.translations.table.column.externalId": { + "defaultMessage": "外部编号" + }, "lib.translations.table.column.endAt": { "defaultMessage": "结束于" }, @@ -8099,6 +8135,9 @@ "lib.translations.table.column.name": { "defaultMessage": "姓名" }, + "lib.translations.table.column.optional": { + "defaultMessage": "选填" + }, "lib.translations.table.column.organization": { "defaultMessage": "组织" }, diff --git a/config/locales/en/activerecord/errors.yml b/config/locales/en/activerecord/errors.yml index cb30644cca7..95f0da1660a 100644 --- a/config/locales/en/activerecord/errors.yml +++ b/config/locales/en/activerecord/errors.yml @@ -196,6 +196,9 @@ en: not_in_course: 'Selected user is not in specified course' course/user_invitation: existing_invitation: 'an open invitation exists for this email address' + attributes: + external_id: + taken: 'has already been taken by another user or pending invitation in this course' course/video: attributes: url: @@ -213,6 +216,8 @@ en: deletion: 'The last tab cannot be deleted' course_user: attributes: + external_id: + taken: 'has already been taken by another user or pending invitation in this course' reference_timeline: belongs_to_course: 'must belong to course' duplication_traceable: diff --git a/config/locales/en/csv.yml b/config/locales/en/csv.yml index 020efec3c82..0d1cf158af1 100644 --- a/config/locales/en/csv.yml +++ b/config/locales/en/csv.yml @@ -62,3 +62,4 @@ en: name: 'Name' type: 'Student Type' email: 'Email' + external_id: 'External ID' diff --git a/config/locales/en/errors.yml b/config/locales/en/errors.yml index e85b2b8f03f..06ca6519b7d 100644 --- a/config/locales/en/errors.yml +++ b/config/locales/en/errors.yml @@ -15,8 +15,11 @@ en: owner: 'Owner' phantom: 'Phantom' user_invitations: - duplicate_user: '%{user} appears more than once in the submission.' - invalid_email: '%{email} is invalid: %{message}.' + already_enrolled: '%{email} could not be processed: already enrolled in this course.' + duplicate_invitation: '%{email} could not be processed: already has a pending invitation.' + invalid_email: '%{email} could not be processed: invalid email: %{message}.' + duplicate_external_id: '%{email} could not be processed: external ID "%{external_id}" is already taken.' + other_error: '%{email} could not be processed: %{message}.' user_registrations: invalid_code: 'You have entered an invalid invitation code.' code_taken: > diff --git a/config/locales/ko/activerecord/errors.yml b/config/locales/ko/activerecord/errors.yml index 21543f1fe1f..62c4016a70f 100644 --- a/config/locales/ko/activerecord/errors.yml +++ b/config/locales/ko/activerecord/errors.yml @@ -194,6 +194,9 @@ ko: not_in_course: '선택한 사용자가 지정된 코스에 없습니다' course/user_invitation: existing_invitation: '이 이메일 주소에는 아직 완료되지 않은 초대장이 있습니다' + attributes: + external_id: + taken: '이 코스에서 다른 사용자 또는 대기 중인 초대에 의해 이미 사용 중입니다' course/video: attributes: url: @@ -211,6 +214,8 @@ ko: deletion: '마지막 탭은 삭제할 수 없습니다' course_user: attributes: + external_id: + taken: '이 코스에서 다른 사용자 또는 대기 중인 초대에 의해 이미 사용 중입니다' reference_timeline: belongs_to_course: '코스에 속해야 합니다' duplication_traceable: diff --git a/config/locales/ko/csv.yml b/config/locales/ko/csv.yml index 6a1a238665c..0a58bcd615a 100644 --- a/config/locales/ko/csv.yml +++ b/config/locales/ko/csv.yml @@ -62,3 +62,4 @@ ko: name: '이름' type: '학생 유형' email: '이메일' + external_id: '외부 ID' diff --git a/config/locales/ko/errors.yml b/config/locales/ko/errors.yml index f57ad1ceb02..ae8c0ce621e 100644 --- a/config/locales/ko/errors.yml +++ b/config/locales/ko/errors.yml @@ -15,8 +15,11 @@ ko: owner: '소유자' phantom: '팬텀' user_invitations: - duplicate_user: '%{user}가 제출물에 여러 번 나타납니다.' - invalid_email: '%{email}이(가) 유효하지 않습니다: %{message}.' + already_enrolled: '%{email}을(를) 처리할 수 없습니다: 이미 이 과정에 등록되어 있습니다.' + duplicate_invitation: '%{email}을(를) 처리할 수 없습니다: 이미 대기 중인 초대가 있습니다.' + invalid_email: '%{email}을(를) 처리할 수 없습니다: 유효하지 않은 이메일: %{message}.' + duplicate_external_id: '%{email}을(를) 처리할 수 없습니다: 외부 ID "%{external_id}"는 이미 사용 중입니다.' + other_error: '%{email}을(를) 처리할 수 없습니다: %{message}.' user_registrations: invalid_code: '잘못된 초대 코드를 입력하셨습니다.' code_taken: > diff --git a/config/locales/zh/activerecord/errors.yml b/config/locales/zh/activerecord/errors.yml index eb9a669b838..5cbb439debc 100644 --- a/config/locales/zh/activerecord/errors.yml +++ b/config/locales/zh/activerecord/errors.yml @@ -194,6 +194,9 @@ zh: not_in_course: '选中的用户不在指定课程中' course/user_invitation: existing_invitation: '此电子邮件地址已存在一个未完成的邀请' + attributes: + external_id: + taken: '该外部编号已被此课程中的其他用户或待处理邀请使用' course/video: attributes: url: @@ -211,6 +214,8 @@ zh: deletion: '最后一项无法被删除' course_user: attributes: + external_id: + taken: '该外部编号已被此课程中的其他用户或待处理邀请使用' reference_timeline: belongs_to_course: '必须隶属于课程' duplication_traceable: diff --git a/config/locales/zh/csv.yml b/config/locales/zh/csv.yml index ac239e862fe..262bdadf441 100644 --- a/config/locales/zh/csv.yml +++ b/config/locales/zh/csv.yml @@ -62,3 +62,4 @@ zh: name: '用户名' type: '学生类型' email: 'Email' + external_id: '外部编号' diff --git a/config/locales/zh/errors.yml b/config/locales/zh/errors.yml index cb4b14af578..368e1787bb4 100644 --- a/config/locales/zh/errors.yml +++ b/config/locales/zh/errors.yml @@ -15,8 +15,11 @@ zh: owner: '拥有者' phantom: '虚拟用户' user_invitations: - duplicate_user: '%{user}在提交中出现了多次。' - invalid_email: '%{email}无效:%{message}。' + already_enrolled: '%{email}无法处理:已加入该课程。' + duplicate_invitation: '%{email}无法处理:已有待处理的邀请。' + invalid_email: '%{email}无法处理:邮箱无效:%{message}。' + duplicate_external_id: '%{email}无法处理:外部编号"%{external_id}"已被使用。' + other_error: '%{email}无法处理:%{message}。' user_registrations: invalid_code: '你输入了一个无效的邀请码。' code_taken: > diff --git a/db/migrate/20260514052933_add_external_id_to_course_users.rb b/db/migrate/20260514052933_add_external_id_to_course_users.rb new file mode 100644 index 00000000000..f1b039f870b --- /dev/null +++ b/db/migrate/20260514052933_add_external_id_to_course_users.rb @@ -0,0 +1,5 @@ +class AddExternalIdToCourseUsers < ActiveRecord::Migration[7.2] + def change + add_column :course_users, :external_id, :string + end +end diff --git a/db/migrate/20260514100000_add_external_id_to_course_user_invitations.rb b/db/migrate/20260514100000_add_external_id_to_course_user_invitations.rb new file mode 100644 index 00000000000..2dbab42f1f1 --- /dev/null +++ b/db/migrate/20260514100000_add_external_id_to_course_user_invitations.rb @@ -0,0 +1,5 @@ +class AddExternalIdToCourseUserInvitations < ActiveRecord::Migration[7.2] + def change + add_column :course_user_invitations, :external_id, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 696d5837a38..41b3a10a1b9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_04_06_122130) do +ActiveRecord::Schema[7.2].define(version: 2026_05_14_100000) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "uuid-ossp" @@ -1333,6 +1333,7 @@ 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 @@ -1357,6 +1358,7 @@ 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", "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..82490bc5e5e 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,85 @@ 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 + + it 'returns a bad request' do + subject + expect(response).to have_http_status(:bad_request) + end + + it 'surfaces a duplicate external ID error message' do + subject + errors = controller.send(:aggregate_errors) + expect(errors).not_to be_empty + expect(errors.any? { |e| e.include?('EXT_DUPE') }).to be(true) + end + + it 'does not report the error as an enrolled or pending invitation error' do + subject + errors = controller.send(:aggregate_errors) + expect(errors.none? { |e| e.include?('already enrolled') || e.include?('pending invitation') }). + 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 + + it 'returns a bad request' do + subject + expect(response).to have_http_status(:bad_request) + end + + it 'returns errors as an array' do + subject + expect(response.parsed_body['errors']).to be_an(Array) + end + + it 'includes the duplicate external ID in the error' do + subject + expect(response.parsed_body['errors'].any? { |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..9af625a210a 100644 --- a/spec/models/user/email_spec.rb +++ b/spec/models/user/email_spec.rb @@ -129,6 +129,19 @@ 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 + 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..d77d4891236 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,76 @@ 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 'student with external_id has it in the correct column' 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(submission_a1.grade.to_s) + expect(fourth_student_record[4]).to eq(submission_a2.grade.to_s) + expect(fourth_student_record[5]).to eq('EXT001') + end + + it 'student without external_id has empty string in external_id column' 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(submission_b1.grade.to_s) + expect(fifth_student_record[4]).to eq(submission_b2.grade.to_s) + expect(fifth_student_record[5]).to eq('') + 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..858ecfac839 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) + 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,180 @@ 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) + 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 + + it 'returns nil' do + expect(subject.invite(form_attributes)).to be_nil + 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 + + it 'returns nil' do + expect(subject.invite(form_attributes)).to be_nil + 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 'returns nil' do + csv = csv_with_external_id_and_timeline( + [{ name: 'New User', email: generate(:email), external_id: 'taken-id' }] + ) + expect(subject.invite(csv)).to be_nil + 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 'returns nil' do + csv = csv_with_external_id_no_timeline( + [{ name: 'New User', email: generate(:email), external_id: 'taken-id' }] + ) + expect(subject.invite(csv)).to be_nil + csv.close! + 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) + 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) + 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) + 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) + csv.close! + end + 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 +393,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 +414,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 'defaults blank timeline algorithm to course default (fixed)' do + expect(subject[0][:timeline_algorithm]).to eq('fixed') + 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 '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