diff --git a/app/controllers/course/user_invitations_controller.rb b/app/controllers/course/user_invitations_controller.rb index 0ce9d15e9a9..118f7b830dd 100644 --- a/app/controllers/course/user_invitations_controller.rb +++ b/app/controllers/course/user_invitations_controller.rb @@ -15,8 +15,20 @@ def index def create result = invite - if result + case result + when Array create_invitation_success(result) + when :pending_conflict + respond_to do |format| + format.json do + render partial: 'pending_external_id_data', + locals: { + pending_invitation_updates: @pending_conflict.pending_invitation_updates, + pending_course_user_updates: @pending_conflict.pending_course_user_updates + }, + status: :ok + end + end else propagate_errors errors = current_course.errors[:base] @@ -116,14 +128,25 @@ def invite_by_file? invitation_params.is_a?(Tempfile) end + VALID_EXTERNAL_ID_RESOLUTIONS = %w[keep_existing replace_all].freeze + private_constant :VALID_EXTERNAL_ID_RESOLUTIONS + # Invites the users via the service object. # - # @return [Boolean] True if the invitation was successful. + # @return [Array] On success. + # @return [Symbol] :pending_conflict when external ID updates require resolution. + # @return [Boolean] false on failure. def invite - invitation_service.invite(invitation_params) + resolution = params[:external_id_resolution] + resolution = nil unless VALID_EXTERNAL_ID_RESOLUTIONS.include?(resolution) + invitation_service.invite(invitation_params, + external_id_resolution: resolution) rescue CSV::MalformedCSVError => e current_course.errors.add(:base, e.message) false + rescue Course::UserInvitationService::PendingExternalIdUpdates => e + @pending_conflict = e + :pending_conflict end # Creates a user invitation service object for this object. @@ -296,7 +319,8 @@ def create_invitation_success(result) format.json do render json: { newInvitations: result[0].length, - invitationResult: parse_invitation_result(*result) + invitationResult: parse_invitation_result(*result), + blankHeaderWarning: invitation_service.blank_header_warning }, status: :ok 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 cb80966a675..10cc4955d01 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 @@ -1,5 +1,6 @@ # frozen_string_literal: true require 'csv' +require 'set' # This concern includes methods required to parse the invitations data. # This can either be from a form, or a CSV file. @@ -9,6 +10,8 @@ module Course::UserInvitationService::ParseInvitationConcern extend ActiveSupport::Autoload TRUE_VALUES = ['t', 'true', 'y', 'yes'].freeze + SUPPORTED_LOCALES = %i[en zh ko].freeze + CANONICAL_COLUMNS = %i[name email external_id role phantom personal_timeline].freeze private @@ -124,28 +127,25 @@ def parse_from_form(users) # @raise [CSV::MalformedCSVError] When the file provided is invalid, eg. UTF-16 encoding. def parse_from_file(file) row_num = 0 + header_map = nil + blank_header_indices = [] + @blank_header_warning = false [].tap do |invites| CSV.foreach(file, encoding: 'utf-8').with_index(1) do |row, row_number| row_num = row_number - row[0] = remove_utf8_byte_order_mark(row[0]) if row_number == 1 + row[0] = remove_utf8_byte_order_mark(row[0]) if row_number == 1 && row[0] row = strip_row(row) - # Ignore first row if it's a header row. - next if row_number == 1 && header_row?(row) - - invite = parse_file_row(row) + if row_number == 1 + header_map, blank_header_indices = build_header_map!(row) + next + end + @blank_header_warning ||= blank_header_indices.any? { |idx| row[idx].present? } + invite = parse_file_row(row, header_map) invites << invite if invite end end rescue StandardError => e - raise CSV::MalformedCSVError.new(e, row_num), e.message - end - - # Returns a boolean to determine whether the row is a header row. - # - # @param[Array] row Array read from CSV file. - # @return [Boolean] Whether the row is a header row - def header_row?(row) - row[0].casecmp('Name') == 0 && row[1].casecmp('Email') == 0 + raise CSV::MalformedCSVError.new(e.message, row_num), e.message end # Strips a row of whitespaces. @@ -156,32 +156,85 @@ def strip_row(row) row.map { |item| item&.strip } end + def header_alias_map + @header_alias_map ||= SUPPORTED_LOCALES.each_with_object({}) do |locale, map| + CANONICAL_COLUMNS.each do |col| + term = I18n.t("csv.course_user_invitations.headers.#{col}", locale: locale) + map[normalize_header(term)] = col + end + end + end + + def normalize_header(value) + value&.strip&.downcase&.gsub(/[\s_\-]+/, '') + end + + def build_header_map!(row) + resolved = {} + blank_indices = [] + row.each_with_index do |cell, idx| + if cell.blank? + blank_indices << idx + next + end + + canonical = header_alias_map[normalize_header(cell)] + raise_non_canonical_header_error!(cell) unless canonical + raise_duplicate_header_error!(canonical) if resolved.key?(canonical) + + resolved[canonical] = idx + end + + validate_required_headers!(resolved) + [resolved, blank_indices] + end + + def raise_non_canonical_header_error!(cell) + accepted = CANONICAL_COLUMNS.map { |col| I18n.t("csv.course_user_invitations.headers.#{col}") }.join(', ') + raise I18n.t('errors.course.user_invitations.invalid_headers', header: cell, accepted: accepted) + end + + def raise_duplicate_header_error!(canonical) + raise I18n.t('errors.course.user_invitations.duplicate_headers', + column: I18n.t("csv.course_user_invitations.headers.#{canonical}")) + end + + def validate_required_headers!(resolved) + return if resolved.key?(:name) && resolved.key?(:email) + + raise I18n.t('errors.course.user_invitations.missing_required_headers') + end + # Parses the given CSV row (array) and returns attributes for a user invitation. + # - Columns are resolved by position via +header_map+, not by fixed order. # - Sets the name as the given email if a name was not provided. # - # @param [Array] row Array with 3 parameters: name, email and role respectively. + # @param [Array] row The row's cells as read from the CSV file. + # @param [Hash{Symbol=>Integer}] header_map Maps each resolved canonical column + # (a subset of +CANONICAL_COLUMNS+) to its index in +row+. Only +:name+ and + # +:email+ are guaranteed present; optional columns are absent when not in the file. # @return [Hash] The parsed invitation attributes given the row. - def parse_file_row(row) - return nil if row[1].blank? + def parse_file_row(row, header_map) + email = row[header_map[:email]] + return nil if email.blank? - if !@current_course.show_personalized_timeline_features? && row.length > 5 - raise I18n.t('errors.course.user_invitations.timeline_template_mismatch') - end + name = row[header_map[:name]] + name = email if name.blank? - row[0] = row[1] if row[0].blank? - role = parse_file_role(row[2]) - phantom = parse_file_phantom(row[3]) - 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, + role = parse_file_role(row_cell(row, header_map, :role)) + phantom = parse_file_phantom(row_cell(row, header_map, :phantom)) + timeline_algorithm = parse_file_timeline_algorithm(row_cell(row, header_map, :personal_timeline)) + external_id = parse_file_external_id(row_cell(row, header_map, :external_id)) + + { name: name, email: email, role: role, phantom: phantom, timeline_algorithm: timeline_algorithm, external_id: external_id } end + def row_cell(row, header_map, col) + idx = header_map[col] + idx ? row[idx] : nil + end + # Parses the role column from the CSV file. # This method handles the case where the role is not specified too, where "student" will be assumed. # 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 24a37675134..cb6ec3caf9f 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 @@ -79,10 +79,13 @@ def handle_existing_course_user(user, course_user, existing_course_users) csv_ext_id = user[:external_id].presence current_ext_id = course_user.external_id.presence - if csv_ext_id.nil? || csv_ext_id == current_ext_id - existing_course_users << course_user - elsif @taken_external_ids.include?(csv_ext_id) + if @taken_external_ids.include?(csv_ext_id) && csv_ext_id != current_ext_id @failed_users.push(user.merge(reason: :external_id_taken)) + elsif csv_ext_id.nil? || csv_ext_id == current_ext_id || @resolution == :keep_existing + existing_course_users << course_user + elsif @resolution.nil? + @pending_course_user_updates << { record: course_user, new_external_id: csv_ext_id, + previous_external_id: current_ext_id } else @taken_external_ids.delete(current_ext_id) if current_ext_id @taken_external_ids.add(csv_ext_id) @@ -114,6 +117,7 @@ def build_course_user(user) # @param [Array] users The users to ensure have instance users. # @return [void] def ensure_instance_users(users) + # instance_users must be preloaded (done by find_existing_users) — otherwise .any? fires N queries. missing_user_ids = users.reject { |user| user.instance_users.any? }.map(&:id) return if missing_user_ids.empty? @@ -159,10 +163,14 @@ def handle_existing_invitation(user, invitation, existing_invitations) # Non-retryable invitations are surfaced as existing invitations, not errors — # the request succeeded; the prior delivery failure is informational. - if csv_ext_id.nil? || csv_ext_id == current_ext_id || invitation.is_retryable == false - existing_invitations << invitation - elsif @taken_external_ids.include?(csv_ext_id) + if @taken_external_ids.include?(csv_ext_id) && csv_ext_id != current_ext_id && invitation.is_retryable != false @failed_users.push(user.merge(reason: :external_id_taken)) + elsif csv_ext_id.nil? || csv_ext_id == current_ext_id || + invitation.is_retryable == false || @resolution == :keep_existing + existing_invitations << invitation + elsif @resolution.nil? + @pending_invitation_updates << { record: invitation, new_external_id: csv_ext_id, + previous_external_id: current_ext_id } else @taken_external_ids.delete(current_ext_id) if current_ext_id @taken_external_ids.add(csv_ext_id) diff --git a/app/services/course/user_invitation_service.rb b/app/services/course/user_invitation_service.rb index 8a00de98bf7..4adc03098bc 100644 --- a/app/services/course/user_invitation_service.rb +++ b/app/services/course/user_invitation_service.rb @@ -6,6 +6,16 @@ class Course::UserInvitationService include ProcessInvitationConcern include EmailInvitationConcern + class PendingExternalIdUpdates < StandardError + attr_reader :pending_invitation_updates, :pending_course_user_updates + + def initialize(pending_invitation_updates:, pending_course_user_updates:) + @pending_invitation_updates = pending_invitation_updates + @pending_course_user_updates = pending_course_user_updates + super('Pending external ID updates require confirmation') + end + end + # Constructor for the user invitation service object. # # @param [CourseUser|nil] current_course_user The course user performing this action. @@ -28,33 +38,27 @@ def initialize(current_course_user, current_user, current_course) # existing_course_users, failed_users, updated_invitations, updated_course_users] # if success. nil when fail. # @raise [CSV::MalformedCSVError] When the file provided is invalid. - def invite(users) - new_invitations = nil - existing_invitations = nil - new_course_users = nil - existing_course_users = nil - failed_users = nil - updated_invitations = nil - updated_course_users = nil + def invite(users, external_id_resolution: nil) + @resolution = external_id_resolution&.to_sym + result = nil success = Course.transaction do - new_invitations, existing_invitations, - new_course_users, existing_course_users, - failed_users, updated_invitations, updated_course_users = invite_users(users) - raise ActiveRecord::Rollback unless updated_invitations.all? { |u| u[:record].save } - raise ActiveRecord::Rollback unless updated_course_users.all? { |u| u[:record].save } - raise ActiveRecord::Rollback unless new_invitations.all?(&:save) - raise ActiveRecord::Rollback unless new_course_users.all?(&:save) - + result = invite_users(users) + raise_if_pending_external_id_updates! + save_invitation_records!(result) true end return unless success + new_invitations, _, new_course_users, = result send_registered_emails(new_course_users) send_invitation_emails(new_invitations) - [new_invitations, existing_invitations, new_course_users, existing_course_users, - failed_users, updated_invitations, updated_course_users] + result + end + + def blank_header_warning + @blank_header_warning || false end # Resends invitation emails to CourseUsers to the given course. @@ -64,11 +68,28 @@ def invite(users) # @return [Boolean] True if there were no errors in sending invitations. # If all provided CourseUsers have already registered, method also returns true. def resend_invitation(invitations) - invitations.blank? ? true : send_invitation_emails(invitations) + invitations.blank? || send_invitation_emails(invitations) end private + def raise_if_pending_external_id_updates! + return unless @pending_invitation_updates.any? || @pending_course_user_updates.any? + + raise PendingExternalIdUpdates.new( + pending_invitation_updates: @pending_invitation_updates, + pending_course_user_updates: @pending_course_user_updates + ) + end + + def save_invitation_records!(result) + new_invitations, _, new_course_users, _, _, updated_invitations, updated_course_users = result + all_records = updated_invitations.map { |u| u[:record] } + + updated_course_users.map { |u| u[:record] } + + new_invitations + new_course_users + raise ActiveRecord::Rollback unless all_records.all?(&:save) + end + # Invites the given users into the course. # # @param [Array|File|TempFile] users Invites the given users. @@ -93,6 +114,8 @@ def invite_users(users) @failed_users = parse_duplicates @updated_invitations = [] @updated_course_users = [] + @pending_invitation_updates = [] + @pending_course_user_updates = [] process_invitations(unique_users) + [@failed_users, @updated_invitations, @updated_course_users] end end diff --git a/app/views/course/user_invitations/_pending_external_id_data.json.jbuilder b/app/views/course/user_invitations/_pending_external_id_data.json.jbuilder new file mode 100644 index 00000000000..8906eeeaf15 --- /dev/null +++ b/app/views/course/user_invitations/_pending_external_id_data.json.jbuilder @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +json.pendingInvitationUpdates pending_invitation_updates do |item| + inv = item[:record] + json.id inv.id + json.name inv.name + json.email inv.email + json.externalId item[:new_external_id] + json.previousExternalId item[:previous_external_id] + json.role inv.role + json.phantom inv.phantom + json.timelineAlgorithm inv.timeline_algorithm +end + +json.pendingCourseUserUpdates pending_course_user_updates do |item| + cu = item[:record] + json.id cu.id + json.name cu.name.strip + json.email cu.user.email + json.externalId item[:new_external_id] + json.previousExternalId item[:previous_external_id] + json.role cu.role + json.phantom cu.phantom? + json.timelineAlgorithm cu.timeline_algorithm +end diff --git a/client/app/api/course/UserInvitations.ts b/client/app/api/course/UserInvitations.ts index 9d042ee905b..1e2f51ca4b5 100644 --- a/client/app/api/course/UserInvitations.ts +++ b/client/app/api/course/UserInvitations.ts @@ -4,8 +4,10 @@ import { ManageCourseUsersSharedData, } from 'types/course/courseUsers'; import { + ExternalIdResolution, InvitationFileEntity, InvitationListData, + InvitationUpdatedItem, } from 'types/course/userInvitations'; import SubmissionsAPI from './Assessment/Submissions'; @@ -36,11 +38,21 @@ export default class UserInvitationsAPI extends BaseCourseAPI { * @return {Promise} * error response: { errors: [] } - An array of errors will be returned upon validation error. */ - invite(data: InvitationFileEntity | FormData): Promise< - AxiosResponse<{ - newInvitations: number; - invitationResult: string; // string which is JSON.parsed to type InvitationResult - }> + invite( + data: InvitationFileEntity | FormData, + externalIdResolution?: ExternalIdResolution, + ): Promise< + AxiosResponse< + | { + newInvitations: number; + invitationResult: string; + blankHeaderWarning: boolean; + } + | { + pendingInvitationUpdates: InvitationUpdatedItem[]; + pendingCourseUserUpdates: InvitationUpdatedItem[]; + } + > > { const config = { headers: { @@ -60,6 +72,10 @@ export default class UserInvitationsAPI extends BaseCourseAPI { formData = data as FormData; } + if (externalIdResolution) { + formData.append('external_id_resolution', externalIdResolution); + } + return this.client.post( `${this.#urlPrefix}/users/invite`, formData, diff --git a/client/app/assets/templates/course-user-invitation-template-en-no-timeline.csv b/client/app/assets/templates/course-user-invitation-template-en-no-timeline.csv new file mode 100644 index 00000000000..0371f1771ec --- /dev/null +++ b/client/app/assets/templates/course-user-invitation-template-en-no-timeline.csv @@ -0,0 +1,3 @@ +Name,Email,External ID,Role,Phantom +John,test1@example.com,A0123456,student,y +Mary,test2@example.com,A0123457,teaching_assistant,n diff --git a/client/app/assets/templates/course-user-invitation-template-en.csv b/client/app/assets/templates/course-user-invitation-template-en.csv new file mode 100644 index 00000000000..c8a6607eacd --- /dev/null +++ b/client/app/assets/templates/course-user-invitation-template-en.csv @@ -0,0 +1,3 @@ +Name,Email,External ID,Role,Phantom,Personal Timeline +John,test1@example.com,A0123456,student,y,otot +Mary,test2@example.com,A0123457,teaching_assistant,n,fixed diff --git a/client/app/assets/templates/course-user-invitation-template-ko-no-timeline.csv b/client/app/assets/templates/course-user-invitation-template-ko-no-timeline.csv new file mode 100644 index 00000000000..ded13bb595b --- /dev/null +++ b/client/app/assets/templates/course-user-invitation-template-ko-no-timeline.csv @@ -0,0 +1,3 @@ +이름,이메일,외부 ID,역할,팬텀 +John,test1@example.com,A0123456,student,y +Mary,test2@example.com,A0123457,teaching_assistant,n diff --git a/client/app/assets/templates/course-user-invitation-template-ko.csv b/client/app/assets/templates/course-user-invitation-template-ko.csv new file mode 100644 index 00000000000..52d59e6306f --- /dev/null +++ b/client/app/assets/templates/course-user-invitation-template-ko.csv @@ -0,0 +1,3 @@ +이름,이메일,외부 ID,역할,팬텀,개인 타임라인 +John,test1@example.com,A0123456,student,y,otot +Mary,test2@example.com,A0123457,teaching_assistant,n,fixed 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 deleted file mode 100644 index 6874f099d4c..00000000000 --- a/client/app/assets/templates/course-user-invitation-template-no-timeline.csv +++ /dev/null @@ -1,3 +0,0 @@ -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-zh-no-timeline.csv b/client/app/assets/templates/course-user-invitation-template-zh-no-timeline.csv new file mode 100644 index 00000000000..3806611b2c5 --- /dev/null +++ b/client/app/assets/templates/course-user-invitation-template-zh-no-timeline.csv @@ -0,0 +1,3 @@ +姓名,电子邮件,外部编号,角色,旁听学生 +John,test1@example.com,A0123456,student,y +Mary,test2@example.com,A0123457,teaching_assistant,n diff --git a/client/app/assets/templates/course-user-invitation-template-zh.csv b/client/app/assets/templates/course-user-invitation-template-zh.csv new file mode 100644 index 00000000000..4556ac674bb --- /dev/null +++ b/client/app/assets/templates/course-user-invitation-template-zh.csv @@ -0,0 +1,3 @@ +姓名,电子邮件,外部编号,角色,旁听学生,个人时间线 +John,test1@example.com,A0123456,student,y,otot +Mary,test2@example.com,A0123457,teaching_assistant,n,fixed diff --git a/client/app/assets/templates/course-user-invitation-template.csv b/client/app/assets/templates/course-user-invitation-template.csv deleted file mode 100644 index 4e0ee4106ed..00000000000 --- a/client/app/assets/templates/course-user-invitation-template.csv +++ /dev/null @@ -1,3 +0,0 @@ -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 01eb52ba487..aee2ca3ead3 100644 --- a/client/app/bundles/course/helper/index.ts +++ b/client/app/bundles/course/helper/index.ts @@ -1,6 +1,10 @@ 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'; +import courseUserInvitationTemplateEnUrl from 'assets/templates/course-user-invitation-template-en.csv?url'; +import courseUserInvitationTemplateEnNoTimelineUrl from 'assets/templates/course-user-invitation-template-en-no-timeline.csv?url'; +import courseUserInvitationTemplateKoUrl from 'assets/templates/course-user-invitation-template-ko.csv?url'; +import courseUserInvitationTemplateKoNoTimelineUrl from 'assets/templates/course-user-invitation-template-ko-no-timeline.csv?url'; +import courseUserInvitationTemplateZhUrl from 'assets/templates/course-user-invitation-template-zh.csv?url'; +import courseUserInvitationTemplateZhNoTimelineUrl from 'assets/templates/course-user-invitation-template-zh-no-timeline.csv?url'; export const getCourseLogoUrl = (url?: string | null): string => { if (!url) { @@ -9,10 +13,26 @@ export const getCourseLogoUrl = (url?: string | null): string => { return url; }; +const TEMPLATE_MAP: Record> = { + en: { + timeline: courseUserInvitationTemplateEnUrl, + noTimeline: courseUserInvitationTemplateEnNoTimelineUrl, + }, + zh: { + timeline: courseUserInvitationTemplateZhUrl, + noTimeline: courseUserInvitationTemplateZhNoTimelineUrl, + }, + ko: { + timeline: courseUserInvitationTemplateKoUrl, + noTimeline: courseUserInvitationTemplateKoNoTimelineUrl, + }, +}; + export const getCourseUserInviteTemplatePath = ( hasPersonalTimelines: boolean, + locale?: string, ): string => { - return hasPersonalTimelines - ? courseUserInvitationTemplateUrl - : courseUserInvitationTemplateNoTimelineUrl; + const normalizedLocale = locale?.split('-')[0] ?? 'en'; + const localeMap = TEMPLATE_MAP[normalizedLocale] ?? TEMPLATE_MAP.en; + return hasPersonalTimelines ? localeMap.timeline : localeMap.noTimeline; }; diff --git a/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx b/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx index 56f8a9b1330..113613374e5 100644 --- a/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx +++ b/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx @@ -1,25 +1,27 @@ -import { FC, useEffect, useState } from 'react'; +import { FC, useEffect, useRef, useState } from 'react'; import { useFieldArray, useForm } from 'react-hook-form'; import { defineMessages } from 'react-intl'; import { yupResolver } from '@hookform/resolvers/yup'; import { + ExternalIdResolution, IndividualInvites, InvitationResult, InvitationsPostData, + PendingExternalIdConflict, } from 'types/course/userInvitations'; import * as yup from 'yup'; import ErrorText from 'lib/components/core/ErrorText'; import { useAppDispatch, useAppSelector } from 'lib/hooks/store'; -import toast from 'lib/hooks/toast'; -import useTranslation from 'lib/hooks/useTranslation'; import formTranslations from 'lib/translations/form'; +import useInviteErrorHandler from '../../hooks/useInviteErrorHandler'; import { inviteUsersFromForm } from '../../operations'; import { getManageCourseUserPermissions, getManageCourseUsersSharedData, } from '../../selectors'; +import ExternalIdConflictPrompt from '../misc/ExternalIdConflictPrompt'; import IndividualInvitations from './IndividualInvitations'; @@ -58,8 +60,14 @@ const validationSchema = yup.object({ const IndividualInviteForm: FC = (props) => { const { openResultDialog } = props; - const { t } = useTranslation(); + const handleError = useInviteErrorHandler( + translations.failure, + translations.failureGeneric, + ); const [isLoading, setIsLoading] = useState(false); + const [conflictData, setConflictData] = + useState(null); + const dataRef = useRef(null); const dispatch = useAppDispatch(); const sharedData = useAppSelector(getManageCourseUsersSharedData); const permissions = useAppSelector(getManageCourseUserPermissions); @@ -114,54 +122,79 @@ const IndividualInviteForm: FC = (props) => { } }, [invitationsFields.length === 0]); - const onSubmit = (data: InvitationsPostData): Promise => { - setIsLoading(true); - return dispatch(inviteUsersFromForm(data)) + const submitWithResolution = ( + postData: InvitationsPostData, + resolution?: ExternalIdResolution, + ): Promise => + dispatch(inviteUsersFromForm(postData, resolution)) .then((response) => { - reset(initialValues); - openResultDialog(response); - }) - .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, - }); + if ('conflict' in response) { + setConflictData(response.conflict); } else { - toast.error(t(translations.failureGeneric), { autoClose: false }); + reset(initialValues); + openResultDialog(response as InvitationResult); } }) - .finally(() => { - setIsLoading(false); - }); + .catch(handleError) + .finally(() => setIsLoading(false)); + + const onSubmit = (data: InvitationsPostData): Promise => { + setIsLoading(true); + dataRef.current = data; + return submitWithResolution(data); + }; + + const handleKeepExisting = (): void => { + setConflictData(null); + if (dataRef.current) { + setIsLoading(true); + submitWithResolution(dataRef.current, 'keep_existing'); + } + }; + + const handleReplaceAll = (): void => { + setConflictData(null); + if (dataRef.current) { + setIsLoading(true); + submitWithResolution(dataRef.current, 'replace_all'); + } + }; + + const handleCancel = (): void => { + setConflictData(null); + dataRef.current = null; }; return ( -
onSubmit(data))} - > - - - + <> + {conflictData && ( + + )} +
onSubmit(data))} + > + + + + ); }; diff --git a/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx b/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx index 3f1dd85c539..1f36ff16cea 100644 --- a/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx +++ b/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx @@ -12,7 +12,7 @@ import useTranslation from 'lib/hooks/useTranslation'; interface Props { open: boolean; onSubmit: ( - data: InvitationFileEntity, + data: { file: InvitationFileEntity }, setError: UseFormSetError, ) => Promise; onClose: () => void; diff --git a/client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx b/client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx new file mode 100644 index 00000000000..6e9f377a3ef --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx @@ -0,0 +1,122 @@ +import { FC } from 'react'; +import { defineMessages } from 'react-intl'; +import { + Box, + Button, + Dialog, + DialogActions, + DialogContent, + DialogContentText, + DialogTitle, + Typography, +} from '@mui/material'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import useTranslation from 'lib/hooks/useTranslation'; + +import ExternalIdConflictTable from '../tables/ExternalIdConflictTable'; + +interface Props { + pendingInvitationUpdates: InvitationUpdatedItem[]; + pendingCourseUserUpdates: InvitationUpdatedItem[]; + onKeepExisting: () => void; + onReplaceAll: () => void; + onCancel: () => void; +} + +const translations = defineMessages({ + title: { + id: 'course.userInvitations.ExternalIdConflictPrompt.title', + defaultMessage: 'Confirm External ID Updates', + }, + body: { + id: 'course.userInvitations.ExternalIdConflictPrompt.body', + defaultMessage: + 'These users are already enrolled or have pending invitations. No new invitation emails will be sent to them. Would you like to keep their current External IDs, or replace them with the values from your file?', + }, + pendingInvitationUpdates: { + id: 'course.userInvitations.ExternalIdConflictPrompt.pendingInvitationUpdates', + defaultMessage: 'Pending Invitation Updates ({count})', + }, + pendingCourseUserUpdates: { + id: 'course.userInvitations.ExternalIdConflictPrompt.pendingCourseUserUpdates', + defaultMessage: 'Pending Course Member Updates ({count})', + }, + goBack: { + id: 'course.userInvitations.ExternalIdConflictPrompt.goBack', + defaultMessage: 'Go Back', + }, + keepExisting: { + id: 'course.userInvitations.ExternalIdConflictPrompt.keepExisting', + defaultMessage: 'Keep Existing', + }, + replace: { + id: 'course.userInvitations.ExternalIdConflictPrompt.replace', + defaultMessage: 'Replace', + }, +}); + +const ExternalIdConflictPrompt: FC = ({ + pendingInvitationUpdates, + pendingCourseUserUpdates, + onKeepExisting, + onReplaceAll, + onCancel, +}) => { + const { t } = useTranslation(); + + return ( + { + if (reason !== 'backdropClick') onCancel(); + }} + open + > + {t(translations.title)} + + {t(translations.body)} + + {pendingInvitationUpdates.length > 0 && ( + <> + + {t(translations.pendingInvitationUpdates, { + count: pendingInvitationUpdates.length, + })} + + + + + + )} + + {pendingCourseUserUpdates.length > 0 && ( + <> + + {t(translations.pendingCourseUserUpdates, { + count: pendingCourseUserUpdates.length, + })} + + + + + + )} + + + + + + + + ); +}; + +export default ExternalIdConflictPrompt; 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 7b7b4510f1a..3a2a8b6450e 100644 --- a/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx +++ b/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx @@ -3,6 +3,7 @@ import { defineMessages } from 'react-intl'; import ErrorOutline from '@mui/icons-material/ErrorOutline'; import HelpIcon from '@mui/icons-material/Help'; import { + Alert, Button, Dialog, DialogActions, @@ -97,6 +98,11 @@ const translations = defineMessages({ id: 'course.userInvitations.InvitationResultDialog.updatedSubtitle', defaultMessage: '{count} updated · shown first', }, + blankHeaderWarning: { + id: 'course.userInvitations.InvitationResultDialog.blankHeaderWarning', + defaultMessage: + 'One or more columns had no header - their data was ignored.', + }, }); const toSuccessRow = ( @@ -143,6 +149,7 @@ const InvitationResultDialog: FC = ({ failedUsers = [], updatedInvitations = [], updatedCourseUsers = [], + blankHeaderWarning = false, } = invitationResult; const newInvitationRows = newInvitations.map((i) => toSuccessRow(i, 'inv')); @@ -199,6 +206,11 @@ const InvitationResultDialog: FC = ({ > {t(translations.header)} + {blankHeaderWarning && ( + + {t(translations.blankHeaderWarning)} + + )} {needsAttentionCount > 0 && `${t(translations.summaryFailed, { count: needsAttentionCount })} `} diff --git a/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx b/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx new file mode 100644 index 00000000000..095ea379f80 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx @@ -0,0 +1,161 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import ExternalIdConflictPrompt from '../ExternalIdConflictPrompt'; + +const invitationUpdate: InvitationUpdatedItem = { + id: 1, + name: 'Alice Tan', + email: 'alice@example.com', + externalId: 'NEW001', + previousExternalId: 'OLD001', + role: 'student', + phantom: false, +}; + +const courseUserUpdate: InvitationUpdatedItem = { + id: 2, + name: 'Bob Lim', + email: 'bob@example.com', + externalId: 'B042', + previousExternalId: null, + role: 'student', + phantom: false, +}; + +const noop = (): void => {}; + +describe('ExternalIdConflictPrompt', () => { + it('renders the title', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Confirm External ID Updates')).toBeInTheDocument(); + }); + + it('renders the invitation updates section when non-empty', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText(/Pending Invitation Updates/)).toBeInTheDocument(); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + }); + + it('does not render invitation section when empty', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText(/Pending Invitation Updates/), + ).not.toBeInTheDocument(); + expect( + screen.getByText(/Pending Course Member Updates/), + ).toBeInTheDocument(); + }); + + it('does not render course member section when pendingCourseUserUpdates is empty', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText(/Pending Course Member Updates/), + ).not.toBeInTheDocument(); + expect(screen.getByText(/Pending Invitation Updates/)).toBeInTheDocument(); + }); + + it('renders both sections when both non-empty', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText(/Pending Invitation Updates/)).toBeInTheDocument(); + expect( + screen.getByText(/Pending Course Member Updates/), + ).toBeInTheDocument(); + }); + + it('calls onCancel when Go Back is clicked', async () => { + const onCancel = jest.fn(); + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await userEvent.click(screen.getByRole('button', { name: 'Go Back' })); + expect(onCancel).toHaveBeenCalledTimes(1); + }); + + it('calls onKeepExisting when Keep Existing is clicked', async () => { + const onKeepExisting = jest.fn(); + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await userEvent.click( + screen.getByRole('button', { name: 'Keep Existing' }), + ); + expect(onKeepExisting).toHaveBeenCalledTimes(1); + }); + + it('calls onReplaceAll when Replace is clicked', async () => { + const onReplaceAll = jest.fn(); + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await userEvent.click(screen.getByRole('button', { name: 'Replace' })); + expect(onReplaceAll).toHaveBeenCalledTimes(1); + }); +}); diff --git a/client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx b/client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx new file mode 100644 index 00000000000..972b33fd68a --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx @@ -0,0 +1,68 @@ +import { FC } from 'react'; +import { defineMessages } from 'react-intl'; +import { + Table, + TableBody, + TableCell, + TableHead, + TableRow, + Typography, +} from '@mui/material'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import useTranslation from 'lib/hooks/useTranslation'; +import tableTranslations from 'lib/translations/table'; + +interface Props { + rows: InvitationUpdatedItem[]; +} + +const translations = defineMessages({ + currentExternalId: { + id: 'lib.translations.table.column.currentExternalId', + defaultMessage: 'Current External ID', + }, + newExternalId: { + id: 'lib.translations.table.column.newExternalId', + defaultMessage: 'New External ID', + }, +}); + +const ExternalIdConflictTable: FC = ({ rows }) => { + const { t } = useTranslation(); + + return ( + + + + {t(tableTranslations.name)} + {t(tableTranslations.email)} + {t(translations.currentExternalId)} + {t(translations.newExternalId)} + + + + {rows.map((row) => ( + + {row.name} + {row.email} + + {row.previousExternalId ?? ( + + — + + )} + + + + {row.externalId} + + + + ))} + +
+ ); +}; + +export default ExternalIdConflictTable; diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx index d9d4ffcd50c..51915776959 100644 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx @@ -6,6 +6,7 @@ import { TimelineAlgorithm } from 'types/course/personalTimes'; import { ColumnTemplate } from 'lib/components/table'; import Table from 'lib/components/table/Table'; import { + DEFAULT_MINI_TABLE_ROWS_PER_PAGE, DEFAULT_TABLE_ROWS_PER_PAGE, TIMELINE_ALGORITHMS, } from 'lib/constants/sharedConstants'; @@ -140,7 +141,7 @@ const InvitationResultExistingTable: FC = ({ getRowEqualityData={(row) => row} getRowId={(row) => String(row.id)} pagination={{ - rowsPerPage: [DEFAULT_TABLE_ROWS_PER_PAGE], + rowsPerPage: [DEFAULT_MINI_TABLE_ROWS_PER_PAGE], showAllRows: true, }} toolbar={{ show: false }} diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx index ee28c74f1b5..3da6e4d2da0 100644 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx @@ -4,6 +4,7 @@ import { InvitationSuccessRow } from 'types/course/userInvitations'; import { ColumnTemplate } from 'lib/components/table'; import Table from 'lib/components/table/Table'; import { + DEFAULT_MINI_TABLE_ROWS_PER_PAGE, DEFAULT_TABLE_ROWS_PER_PAGE, TIMELINE_ALGORITHMS, } from 'lib/constants/sharedConstants'; @@ -92,7 +93,7 @@ const InvitationResultPrimaryTable: FC = ({ getRowEqualityData={(row) => row} getRowId={(row) => row.id} pagination={{ - rowsPerPage: [DEFAULT_TABLE_ROWS_PER_PAGE], + rowsPerPage: [DEFAULT_MINI_TABLE_ROWS_PER_PAGE], showAllRows: false, }} toolbar={{ show: false }} diff --git a/client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx b/client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx new file mode 100644 index 00000000000..48fcb1870e0 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx @@ -0,0 +1,59 @@ +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import ExternalIdConflictTable from '../ExternalIdConflictTable'; + +const baseItem: InvitationUpdatedItem = { + id: 1, + name: 'Alice Tan', + email: 'alice@example.com', + externalId: 'NEW001', + previousExternalId: 'OLD001', + role: 'student', + phantom: false, +}; + +describe('ExternalIdConflictTable', () => { + it('renders Name, Email, Current External ID, New External ID columns', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Name')).toBeInTheDocument(); + expect(screen.getByText('Email')).toBeInTheDocument(); + expect(screen.getByText('Current External ID')).toBeInTheDocument(); + expect(screen.getByText('New External ID')).toBeInTheDocument(); + }); + + it('renders row data', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + expect(screen.getByText('alice@example.com')).toBeInTheDocument(); + expect(screen.getByText('OLD001')).toBeInTheDocument(); + expect(screen.getByText('NEW001')).toBeInTheDocument(); + }); + + it('renders — when previousExternalId is null', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('—')).toBeInTheDocument(); + }); + + it('renders multiple rows', async () => { + const second: InvitationUpdatedItem = { + ...baseItem, + id: 2, + name: 'Bob Lim', + email: 'bob@example.com', + externalId: 'B042', + previousExternalId: null, + }; + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + expect(screen.getByText('Bob Lim')).toBeInTheDocument(); + }); +}); diff --git a/client/app/bundles/course/user-invitations/components/tables/__test__/UserInvitationsTable.test.tsx b/client/app/bundles/course/user-invitations/components/tables/__test__/UserInvitationsTable.test.tsx index 62bde5be532..35b711dda00 100644 --- a/client/app/bundles/course/user-invitations/components/tables/__test__/UserInvitationsTable.test.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/__test__/UserInvitationsTable.test.tsx @@ -19,9 +19,274 @@ const baseInvitation: InvitationMiniEntity = { }; const SEARCH_PLACEHOLDER = 'Search by name, email or external ID'; +const EMAIL_BOB = 'bob@example.com'; + +const storeWithTimeline = { + invitations: { + invitations: { ids: [], entities: {}, byId: {} }, + permissions: { + canManageCourseUsers: false, + canManageEnrolRequests: false, + canManageReferenceTimelines: false, + canManagePersonalTimes: false, + canRegisterWithCode: false, + }, + manageCourseUsersData: { + requestsCount: 0, + invitationsCount: 0, + defaultTimelineAlgorithm: 'fixed' as const, + showPersonalizedTimelineFeatures: true, + }, + courseRegistrationKey: '', + }, +}; describe('', () => { + it('renders empty state when invitations is empty', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('There are no invitations.')).toBeInTheDocument(); + expect(screen.queryByRole('table')).not.toBeInTheDocument(); + }); + + describe('External ID column', () => { + it('shows column when any invitation has a non-null externalId', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('External ID')).toBeInTheDocument(); + }); + + it('hides column when all invitations have null externalId', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText('External ID')).not.toBeInTheDocument(); + }); + }); + + describe('Personalized Timeline column', () => { + it('shows column when showPersonalizedTimelineFeatures is true', async () => { + render(, { + state: storeWithTimeline, + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + }); + + it('hides column when showPersonalizedTimelineFeatures is false', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText('Personalized Timeline'), + ).not.toBeInTheDocument(); + }); + }); + + describe('phantom icon', () => { + it('renders GhostIcon in name cell when phantom is true', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const nameEl = screen.getByText('Alice Lim'); + expect(nameEl.querySelector('[class*="MuiSvgIcon"]')).toBeInTheDocument(); + }); + + it('does not render GhostIcon in name cell when phantom is false', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const nameEl = screen.getByText('Alice Lim'); + expect( + nameEl.querySelector('[class*="MuiSvgIcon"]'), + ).not.toBeInTheDocument(); + }); + }); + + describe('status chips', () => { + it('renders Accepted chip when confirmed is true', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Accepted')).toBeInTheDocument(); + }); + + it('renders Pending chip when confirmed is false and isRetryable is true', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Pending')).toBeInTheDocument(); + }); + + it('renders Failed chip when confirmed is false and isRetryable is false', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Failed')).toBeInTheDocument(); + }); + }); + + describe('actions cell', () => { + it('renders action buttons for pending invitations', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + document.querySelector('.invitation-delete-42'), + ).toBeInTheDocument(); + }); + + it('renders action buttons for failed invitations', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + document.querySelector('.invitation-delete-43'), + ).toBeInTheDocument(); + }); + + it('renders no action buttons for accepted invitations', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + document.querySelector('.invitation-delete-44'), + ).not.toBeInTheDocument(); + }); + }); + + describe('sort order', () => { + it('sorts rows failed first, pending second, accepted last', async () => { + const invitations: InvitationMiniEntity[] = [ + { + ...baseInvitation, + id: 1, + name: 'AcceptedUser', + confirmed: true, + confirmedAt: '2024-02-01T00:00:00Z', + }, + { + ...baseInvitation, + id: 2, + name: 'FailedUser', + email: 'failed@example.com', + confirmed: false, + isRetryable: false, + }, + { + ...baseInvitation, + id: 3, + name: 'PendingUser', + email: 'pending@example.com', + confirmed: false, + isRetryable: true, + }, + ]; + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + + const failedEl = screen.getByText('FailedUser'); + const pendingEl = screen.getByText('PendingUser'); + const acceptedEl = screen.getByText('AcceptedUser'); + + // Failed appears before Pending in DOM + expect( + // eslint-disable-next-line no-bitwise + failedEl.compareDocumentPosition(pendingEl) & + Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + // Pending appears before Accepted in DOM + expect( + // eslint-disable-next-line no-bitwise + pendingEl.compareDocumentPosition(acceptedEl) & + Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + }); + }); + describe('search', () => { + it('filters invitations by name', async () => { + const user = userEvent.setup(); + const invitations: InvitationMiniEntity[] = [ + { ...baseInvitation, id: 1, name: 'Alice Lim' }, + { + ...baseInvitation, + id: 2, + name: 'Bob Tan', + email: EMAIL_BOB, + }, + ]; + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await user.type(screen.getByPlaceholderText(SEARCH_PLACEHOLDER), 'Alice'); + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + }); + + it('filters invitations by email', async () => { + const user = userEvent.setup(); + const invitations: InvitationMiniEntity[] = [ + { ...baseInvitation, id: 1, name: 'Alice Lim' }, + { + ...baseInvitation, + id: 2, + name: 'Bob Tan', + email: EMAIL_BOB, + }, + ]; + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await user.type( + screen.getByPlaceholderText(SEARCH_PLACEHOLDER), + 'alice@', + ); + expect(screen.getByText('Alice Lim')).toBeInTheDocument(); + expect(screen.queryByText('Bob Tan')).not.toBeInTheDocument(); + }); + it('filters invitations by external ID', async () => { const user = userEvent.setup(); const invitations: InvitationMiniEntity[] = [ @@ -35,7 +300,7 @@ describe('', () => { ...baseInvitation, id: 2, name: 'Bob Tan', - email: 'bob@example.com', + email: EMAIL_BOB, externalId: 'EXT-BOB', }, ]; @@ -65,7 +330,7 @@ describe('', () => { ...baseInvitation, id: 2, name: 'Bob Tan', - email: 'bob@example.com', + email: EMAIL_BOB, externalId: 'EXT-BOB', }, ]; @@ -95,7 +360,7 @@ describe('', () => { ...baseInvitation, id: 2, name: 'Bob Tan', - email: 'bob@example.com', + email: EMAIL_BOB, externalId: 'EXT-BOB', }, ]; diff --git a/client/app/bundles/course/user-invitations/hooks/useInviteErrorHandler.ts b/client/app/bundles/course/user-invitations/hooks/useInviteErrorHandler.ts new file mode 100644 index 00000000000..cd45442c90b --- /dev/null +++ b/client/app/bundles/course/user-invitations/hooks/useInviteErrorHandler.ts @@ -0,0 +1,32 @@ +import { MessageDescriptor } from 'react-intl'; + +import toast from 'lib/hooks/toast'; +import useTranslation from 'lib/hooks/useTranslation'; + +const useInviteErrorHandler = ( + failure: MessageDescriptor, + failureGeneric: MessageDescriptor, +): ((error: unknown) => void) => { + const { t } = useTranslation(); + + return (error: unknown): void => { + const rawErrors = (error as { response?: { data?: { errors?: unknown } } }) + ?.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(failure, { error: first + overflow }), { + autoClose: false, + }); + } else { + toast.error(t(failureGeneric), { autoClose: false }); + } + }; +}; + +export default useInviteErrorHandler; diff --git a/client/app/bundles/course/user-invitations/operations.ts b/client/app/bundles/course/user-invitations/operations.ts index d9a0c409114..b81b416c77f 100644 --- a/client/app/bundles/course/user-invitations/operations.ts +++ b/client/app/bundles/course/user-invitations/operations.ts @@ -1,9 +1,11 @@ import { Operation } from 'store'; import { + ExternalIdResolution, InvitationFileEntity, InvitationPostData, InvitationResult, InvitationsPostData, + PendingExternalIdConflict, } from 'types/course/userInvitations'; import CourseAPI from 'api/course'; @@ -75,25 +77,50 @@ export function fetchPermissionsAndSharedData(): Operation { export function inviteUsersFromFile( fileEntity: InvitationFileEntity, -): Operation { + resolution?: ExternalIdResolution, +): Operation { return async (dispatch) => - CourseAPI.userInvitations.invite(fileEntity).then((response) => { - const data = response.data; - dispatch(actions.updateInvitationCounts(data.newInvitations)); - return JSON.parse(data.invitationResult); - }); + CourseAPI.userInvitations + .invite(fileEntity, resolution) + .then((response) => { + const data = response.data; + if ('pendingInvitationUpdates' in data) { + return { + conflict: { + pendingInvitationUpdates: data.pendingInvitationUpdates, + pendingCourseUserUpdates: data.pendingCourseUserUpdates, + }, + }; + } + dispatch(actions.updateInvitationCounts(data.newInvitations)); + return { + ...(JSON.parse(data.invitationResult) as InvitationResult), + blankHeaderWarning: data.blankHeaderWarning ?? false, + }; + }); } export function inviteUsersFromForm( postData: InvitationsPostData, -): Operation { + resolution?: ExternalIdResolution, +): Operation { const formattedData = formatInvitations(postData.invitations); return async (dispatch) => - CourseAPI.userInvitations.invite(formattedData).then((response) => { - const data = response.data; - dispatch(actions.updateInvitationCounts(data.newInvitations)); - return JSON.parse(data.invitationResult); - }); + CourseAPI.userInvitations + .invite(formattedData, resolution) + .then((response) => { + const data = response.data; + if ('pendingInvitationUpdates' in data) { + return { + conflict: { + pendingInvitationUpdates: data.pendingInvitationUpdates, + pendingCourseUserUpdates: data.pendingCourseUserUpdates, + }, + }; + } + dispatch(actions.updateInvitationCounts(data.newInvitations)); + return JSON.parse(data.invitationResult) as InvitationResult; + }); } export function resendAllInvitations(): Operation { diff --git a/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx new file mode 100644 index 00000000000..905f7e94c81 --- /dev/null +++ b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx @@ -0,0 +1,382 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen, waitFor } from 'test-utils'; +import { InvitationFileEntity } from 'types/course/userInvitations'; + +import InviteUsersFileUpload from '../index'; + +const mockToastError = jest.fn(); +jest.mock('lib/hooks/toast', () => ({ + __esModule: true, + default: { + error: (...args: unknown[]): void => mockToastError(...args), + success: jest.fn(), + }, +})); + +// Capture the onSubmit prop so we can call it directly with IFormInputs-shaped data, +// reproducing exactly what FormDialog passes at runtime. +let capturedOnSubmit: + | ((data: { file: InvitationFileEntity }) => Promise) + | null = null; + +// MockFileUploadForm must be defined inside the factory to avoid jest.mock hoisting: +// babel-jest hoists jest.mock above all module-level code, so any variable referenced +// in the factory would be undefined when the factory runs during import resolution. +jest.mock('../../../components/forms/InviteUsersFileUploadForm', () => { + const MockFileUploadForm = ({ + open, + onSubmit, + formSubtitle, + }: { + open: boolean; + onSubmit: (data: { file: InvitationFileEntity }) => Promise; + formSubtitle?: React.ReactNode; + }): JSX.Element | null => { + capturedOnSubmit = onSubmit; + return open ? ( +
+
+ ) : null; + }; + MockFileUploadForm.displayName = 'MockFileUploadForm'; + return MockFileUploadForm; +}); + +const mockInviteUsersFromFile = jest.fn( + (): ((_dispatch: unknown) => Promise) => + (_dispatch: unknown): Promise => + Promise.resolve({ + newInvitations: [], + existingInvitations: [], + newCourseUsers: [], + existingCourseUsers: [], + failedUsers: [], + updatedInvitations: [], + updatedCourseUsers: [], + }), +); + +jest.mock('../../../operations', () => ({ + inviteUsersFromFile: (...args: unknown[]): unknown => + (mockInviteUsersFromFile as (...a: unknown[]) => unknown)(...args), + fetchInvitations: jest.fn( + (): (() => Promise) => (): Promise => Promise.resolve(), + ), + fetchPermissionsAndSharedData: jest.fn( + (): (() => Promise) => (): Promise => Promise.resolve(), + ), +})); + +const MOCK_FILE_SUBMIT_TESTID = 'mock-file-submit'; + +const TEST_FILE_ENTITY: InvitationFileEntity = { + name: 'invites.csv', + url: 'blob:http://localhost/test-123', + file: new Blob(['Name,Email\nAlice,alice@test.com'], { type: 'text/csv' }), +}; + +const conflictResponse = { + conflict: { + pendingInvitationUpdates: [ + { + id: 1, + name: 'Alice', + email: 'alice@example.com', + externalId: 'NEW001', + previousExternalId: 'OLD001', + role: 'student', + phantom: false, + }, + ], + pendingCourseUserUpdates: [], + }, +}; + +const successResponse = { + newInvitations: [], + existingInvitations: [], + newCourseUsers: [], + existingCourseUsers: [], + failedUsers: [], + updatedInvitations: [], + updatedCourseUsers: [], +}; + +const storeWithPersonalTimes = { + invitations: { + invitations: { ids: [], entities: {}, byId: {} }, + permissions: { + canManageCourseUsers: false, + canManageEnrolRequests: false, + canManageReferenceTimelines: false, + canManagePersonalTimes: true, + canRegisterWithCode: false, + }, + manageCourseUsersData: { + requestsCount: 0, + invitationsCount: 0, + defaultTimelineAlgorithm: 'fixed' as const, + showPersonalizedTimelineFeatures: false, + }, + courseRegistrationKey: '', + }, +}; + +const noop = jest.fn(); + +describe('', () => { + beforeEach(() => { + capturedOnSubmit = null; + mockInviteUsersFromFile.mockClear(); + mockToastError.mockClear(); + noop.mockClear(); + }); + + it('renders nothing when open is false', () => { + render( + , + ); + expect( + screen.queryByTestId(MOCK_FILE_SUBMIT_TESTID), + ).not.toBeInTheDocument(); + }); + + it('passes InvitationFileEntity (not the IFormInputs wrapper) to inviteUsersFromFile', async () => { + render( + , + ); + + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + expect(capturedOnSubmit).not.toBeNull(); + + // Simulate what FormDialog passes: the full IFormInputs wrapper { file: InvitationFileEntity } + await capturedOnSubmit!({ file: TEST_FILE_ENTITY }); + + expect(mockInviteUsersFromFile).toHaveBeenCalledTimes(1); + const firstArg = ( + mockInviteUsersFromFile.mock.calls as unknown[][] + )[0][0] as InvitationFileEntity; + + // Regression guard: the page must extract .file from the IFormInputs wrapper. + // If the bug regresses, firstArg would be { file: InvitationFileEntity } and + // firstArg.name would be undefined. + expect(firstArg.name).toBe('invites.csv'); + expect(firstArg.file).toBeInstanceOf(Blob); + }); + + it('shows ExternalIdConflictPrompt when server returns a conflict', async () => { + mockInviteUsersFromFile.mockImplementationOnce( + () => + (_dispatch: unknown): Promise => + Promise.resolve(conflictResponse), + ); + + render( + , + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + + await capturedOnSubmit!({ file: TEST_FILE_ENTITY }); + + await waitFor(() => + expect( + screen.getByText('Confirm External ID Updates'), + ).toBeInTheDocument(), + ); + }); + + it('calls openResultDialog and onClose on successful upload', async () => { + const openResultDialog = jest.fn(); + const onClose = jest.fn(); + + render( + , + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + + await capturedOnSubmit!({ file: TEST_FILE_ENTITY }); + + expect(openResultDialog).toHaveBeenCalledTimes(1); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('shows error toast with first error and overflow count when response has array of errors', async () => { + mockInviteUsersFromFile.mockImplementationOnce( + () => + (_dispatch: unknown): Promise => + Promise.reject( + Object.assign(new Error('Upload failed'), { + response: { + data: { errors: ['Header mismatch', 'Second error'] }, + }, + }), + ), + ); + + render( + , + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + + await capturedOnSubmit!({ file: TEST_FILE_ENTITY }); + + await waitFor(() => expect(mockToastError).toHaveBeenCalled()); + expect(mockToastError).toHaveBeenCalledWith( + expect.stringContaining('Header mismatch (and 1 more)'), + expect.anything(), + ); + }); + + it('shows generic error toast when response has no recognized errors', async () => { + mockInviteUsersFromFile.mockImplementationOnce( + () => + (_dispatch: unknown): Promise => + Promise.reject( + Object.assign(new Error('Upload failed'), { + response: { data: {} }, + }), + ), + ); + + render( + , + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + + await capturedOnSubmit!({ file: TEST_FILE_ENTITY }); + + await waitFor(() => expect(mockToastError).toHaveBeenCalled()); + expect(mockToastError).toHaveBeenCalledWith( + expect.stringContaining( + 'Failed to invite users. Please ensure your data is formatted correctly.', + ), + expect.anything(), + ); + }); + + describe('personal timeline hint', () => { + it('shows personal timeline bullet when canManagePersonalTimes is true', async () => { + render( + , + { state: storeWithPersonalTimes }, + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + expect(screen.getByText(/Personal Timelines can be/)).toBeInTheDocument(); + }); + + it('hides personal timeline bullet when canManagePersonalTimes is false', async () => { + render( + , + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + expect( + screen.queryByText(/Personal Timelines can be/), + ).not.toBeInTheDocument(); + }); + + it('shows 6-col example when canManagePersonalTimes is true', async () => { + render( + , + { state: storeWithPersonalTimes }, + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + expect( + screen.getByText( + /Name,Email,External ID,Role,Phantom,Personal Timeline/, + ), + ).toBeInTheDocument(); + }); + + it('shows 5-col example when canManagePersonalTimes is false', async () => { + render( + , + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + const preEl = document.querySelector('pre'); + expect(preEl?.textContent).toContain( + 'Name,Email,External ID,Role,Phantom', + ); + expect(preEl?.textContent).not.toContain('Personal Timeline'); + }); + }); + + describe('conflict resolution', () => { + const setupConflict = async (): Promise => { + mockInviteUsersFromFile.mockImplementationOnce( + () => + (_dispatch: unknown): Promise => + Promise.resolve(conflictResponse), + ); + mockInviteUsersFromFile.mockImplementationOnce( + () => + (_dispatch: unknown): Promise => + Promise.resolve(successResponse), + ); + + render( + , + ); + await waitFor(() => + expect(screen.getByTestId(MOCK_FILE_SUBMIT_TESTID)).toBeInTheDocument(), + ); + await capturedOnSubmit!({ file: TEST_FILE_ENTITY }); + await waitFor(() => + expect( + screen.getByText('Confirm External ID Updates'), + ).toBeInTheDocument(), + ); + }; + + it('passes keep_existing resolution when Keep Existing is clicked', async () => { + await setupConflict(); + await userEvent.click( + screen.getByRole('button', { name: 'Keep Existing' }), + ); + expect(mockInviteUsersFromFile).toHaveBeenCalledTimes(2); + expect((mockInviteUsersFromFile.mock.calls as unknown[][])[1][1]).toBe( + 'keep_existing', + ); + }); + + it('passes replace_all resolution when Replace is clicked', async () => { + await setupConflict(); + await userEvent.click(screen.getByRole('button', { name: 'Replace' })); + expect(mockInviteUsersFromFile).toHaveBeenCalledTimes(2); + expect((mockInviteUsersFromFile.mock.calls as unknown[][])[1][1]).toBe( + 'replace_all', + ); + }); + }); +}); 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 563bca9d858..0cdd1105b4a 100644 --- a/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx +++ b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx @@ -1,16 +1,21 @@ -import { FC, ReactNode } from 'react'; -import { defineMessages, FormattedMessage } from 'react-intl'; +import { FC, ReactNode, useRef, useState } from 'react'; +import { defineMessages, FormattedMessage, useIntl } from 'react-intl'; import DownloadIcon from '@mui/icons-material/Download'; import { Typography } from '@mui/material'; -import { InvitationResult } from 'types/course/userInvitations'; +import { + InvitationFileEntity, + InvitationResult, + PendingExternalIdConflict, +} from 'types/course/userInvitations'; import { getCourseUserInviteTemplatePath } from 'course/helper'; import Link from 'lib/components/core/Link'; import { useAppDispatch, useAppSelector } from 'lib/hooks/store'; -import toast from 'lib/hooks/toast'; import useTranslation from 'lib/hooks/useTranslation'; import FileUploadForm from '../../components/forms/InviteUsersFileUploadForm'; +import ExternalIdConflictPrompt from '../../components/misc/ExternalIdConflictPrompt'; +import useInviteErrorHandler from '../../hooks/useInviteErrorHandler'; import { inviteUsersFromFile } from '../../operations'; import { getManageCourseUserPermissions, @@ -73,16 +78,16 @@ const translations = defineMessages({ fileUploadExample: { id: 'course.userInvitations.InviteUsersFileUpload.fileUploadExample', defaultMessage: - 'Name,Email,Role,Phantom,ExternalId' + - '{br}John,test1@example.org,student,y,A0123456' + - '{br}Mary,test2@example.org,teaching_assistant,n,A0123457', + 'Name,Email,External ID,Role,Phantom' + + '{br}John,test1@example.com,A0123456,student,y' + + '{br}Mary,test2@example.com,A0123457,teaching_assistant,n', }, fileUploadExamplePersonalTimeline: { id: 'course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline', 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', + 'Name,Email,External ID,Role,Phantom,Personal Timeline' + + '{br}John,test1@example.com,A0123456,student,y,otot' + + '{br}Mary,test2@example.com,A0123457,teaching_assistant,n,fixed', }, failure: { id: 'course.userInvitations.InviteUsersFileUpload.failure', @@ -99,40 +104,59 @@ const translations = defineMessages({ const InviteUsersFileUpload: FC = (props) => { const { open, onClose, openResultDialog } = props; const { t } = useTranslation(); + const intl = useIntl(); const dispatch = useAppDispatch(); + const fileRef = useRef(null); + const [conflictData, setConflictData] = + useState(null); + const sharedData = useAppSelector(getManageCourseUsersSharedData); const permissions = useAppSelector(getManageCourseUserPermissions); const defaultTimelineAlgorithm = sharedData.defaultTimelineAlgorithm; + const handleError = useInviteErrorHandler( + translations.failure, + translations.failureGeneric, + ); - if (!open) { + if (!open && !conflictData) { return null; } - const onSubmit = (data): Promise => { - return dispatch(inviteUsersFromFile(data.file)) + const submitWithResolution = ( + fileEntity: InvitationFileEntity, + resolution?: 'keep_existing' | 'replace_all', + ): Promise => + dispatch(inviteUsersFromFile(fileEntity, resolution)) .then((response) => { - onClose(); - openResultDialog(response); - }) - .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, - }); + if ('conflict' in response) { + setConflictData(response.conflict); } else { - toast.error(t(translations.failureGeneric), { autoClose: false }); + onClose(); + openResultDialog(response as InvitationResult); } - }); + }) + .catch(handleError); + + const onSubmit = (data: { file: InvitationFileEntity }): Promise => { + fileRef.current = data.file; + return submitWithResolution(data.file); + }; + + const handleKeepExisting = (): void => { + setConflictData(null); + if (fileRef.current) submitWithResolution(fileRef.current, 'keep_existing'); + }; + + const handleReplaceAll = (): void => { + setConflictData(null); + if (fileRef.current) submitWithResolution(fileRef.current, 'replace_all'); + }; + + const handleCancel = (): void => { + setConflictData(null); + fileRef.current = null; }; const formSubtitle = ( @@ -144,6 +168,11 @@ const InviteUsersFileUpload: FC = (props) => { {t(translations.fileUploadInfoEmail)} +
  • + + + +
  • @@ -167,11 +196,6 @@ const InviteUsersFileUpload: FC = (props) => {
    • )} -
  • - - - -
    • {t(translations.exampleHeader)} @@ -179,6 +203,7 @@ const InviteUsersFileUpload: FC = (props) => { download="template.csv" href={getCourseUserInviteTemplatePath( permissions.canManagePersonalTimes, + intl.locale, )} opensInNewTab style={{ textDecoration: 'none', cursor: 'pointer' }} @@ -203,12 +228,23 @@ const InviteUsersFileUpload: FC = (props) => { ); return ( - + <> + {conflictData && ( + + )} + + ); }; 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 72bf3413222..76f30227a4f 100644 --- a/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx +++ b/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx @@ -59,14 +59,6 @@ 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, @@ -75,6 +67,14 @@ const ManageUsersTable = (props: ManageUsersTableProps): JSX.Element => { cell: (user) => user.email, csvDownloadable: true, }, + { + of: 'externalId', + title: t(tableTranslations.externalId), + sortable: false, + searchable: true, + cell: (user) => , + csvDownloadable: true, + }, { of: 'phantom', sortable: true, diff --git a/client/app/types/course/userInvitations.ts b/client/app/types/course/userInvitations.ts index 956d0488fa7..c2054ebcb94 100644 --- a/client/app/types/course/userInvitations.ts +++ b/client/app/types/course/userInvitations.ts @@ -30,6 +30,14 @@ export interface InvitationResult { newInvitations?: InvitationListData[]; updatedCourseUsers?: InvitationUpdatedItem[]; updatedInvitations?: InvitationUpdatedItem[]; + blankHeaderWarning?: boolean; +} + +export type ExternalIdResolution = 'keep_existing' | 'replace_all'; + +export interface PendingExternalIdConflict { + pendingInvitationUpdates: InvitationUpdatedItem[]; + pendingCourseUserUpdates: InvitationUpdatedItem[]; } export interface InvitationSuccessRow { diff --git a/client/locales/en.json b/client/locales/en.json index dd6c94ad676..94a73f56244 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -522,8 +522,7 @@ "defaultMessage": "The AI model used by Codaveri to generate help conversations with students for programming questions." }, "course.admin.CodaveriSettings.codaveriSystemPromptDescription": { - "defaultMessage": - "You may customize the behavior of the Codaveri model by providing instructions here. {br} When assisting students, these instructions will be followed in addition to any you have set on the question itself.{br}To reference question-specific details, you may use the following variables within the prompt, writing them with brackets as shown below:" + "defaultMessage": "You may customize the behavior of the Codaveri model by providing instructions here. {br} When assisting students, these instructions will be followed in addition to any you have set on the question itself.{br}To reference question-specific details, you may use the following variables within the prompt, writing them with brackets as shown below:" }, "course.admin.CodaveriSettings.codaveriSystemPromptProblemDescriptionLine": { "defaultMessage": "{problemDescriptionVar} : The full description of the coding problem." @@ -6000,7 +5999,7 @@ "course.plagiarism.PlagiarismIndex.assessments.noPlagiarismCheckableQuestions": { "defaultMessage": "No checkable questions" }, - "course.plagiarism.PlagiarismIndex.assessments.notEnoughSubmissions": { + "course.plagiarism.PlagiarismIndex.assessments.notEnoughSubmissions": { "defaultMessage": "Not enough submissions" }, "course.plagiarism.PlagiarismIndex.assessments.runAssessmentsPlagiarism": { @@ -7038,6 +7037,9 @@ "course.userInvitations.InvitationResultDialog.updatedSubtitle": { "defaultMessage": "{count} updated · shown first" }, + "course.userInvitations.InvitationResultDialog.blankHeaderWarning": { + "defaultMessage": "One or more columns had no header - their data was ignored." + }, "course.userInvitations.InvitationResultDialog.summary": { "defaultMessage": "{newInvitations} new {newInvitations, plural, one {invitation} other {invitations}} sent, {newEnrollments} directly enrolled, {alreadyInCourse} already in course." }, @@ -7105,10 +7107,10 @@ "defaultMessage": "Failed to invite users. Please ensure your data is formatted correctly." }, "course.userInvitations.InviteUsersFileUpload.fileUploadExample": { - "defaultMessage": "Name,Email,Role,Phantom,ExternalId{br}John,test1@example.org,student,y,A0123456{br}Mary,test2@example.org,teaching_assistant,n,A0123457" + "defaultMessage": "Name,Email,External ID,Role,Phantom{br}John,test1@example.com,A0123456,student,y{br}Mary,test2@example.com,A0123457,teaching_assistant,n" }, "course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline": { - "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" + "defaultMessage": "Name,Email,External ID,Role,Phantom,Personal Timeline{br}John,test1@example.com,A0123456,student,y,otot{br}Mary,test2@example.com,A0123457,teaching_assistant,n,fixed" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoEmail": { "defaultMessage": "Each invitation must use a unique email address within the course. Duplicate emails will be skipped." @@ -9158,5 +9160,32 @@ }, "users.troubleSigningIn": { "defaultMessage": "Trouble signing in?" + }, + "course.userInvitations.ExternalIdConflictPrompt.title": { + "defaultMessage": "Confirm External ID Updates" + }, + "course.userInvitations.ExternalIdConflictPrompt.body": { + "defaultMessage": "These users are already enrolled or have pending invitations. No new invitation emails will be sent to them. Would you like to keep their current External IDs, or replace them with the values from your file?" + }, + "course.userInvitations.ExternalIdConflictPrompt.pendingInvitationUpdates": { + "defaultMessage": "Pending Invitation Updates ({count})" + }, + "course.userInvitations.ExternalIdConflictPrompt.pendingCourseUserUpdates": { + "defaultMessage": "Pending Course Member Updates ({count})" + }, + "course.userInvitations.ExternalIdConflictPrompt.goBack": { + "defaultMessage": "Go Back" + }, + "course.userInvitations.ExternalIdConflictPrompt.keepExisting": { + "defaultMessage": "Keep Existing" + }, + "course.userInvitations.ExternalIdConflictPrompt.replace": { + "defaultMessage": "Replace" + }, + "lib.translations.table.column.currentExternalId": { + "defaultMessage": "Current External ID" + }, + "lib.translations.table.column.newExternalId": { + "defaultMessage": "New External ID" } } diff --git a/client/locales/ko.json b/client/locales/ko.json index aa466d9880f..1cc2584bf5c 100644 --- a/client/locales/ko.json +++ b/client/locales/ko.json @@ -5981,7 +5981,7 @@ "course.plagiarism.PlagiarismIndex.assessments.noPlagiarismCheckableQuestions": { "defaultMessage": "검사 가능한 질문이 없습니다" }, - "course.plagiarism.PlagiarismIndex.assessments.notEnoughSubmissions": { + "course.plagiarism.PlagiarismIndex.assessments.notEnoughSubmissions": { "defaultMessage": "제출이 충분하지 않습니다" }, "course.plagiarism.PlagiarismIndex.assessments.runAssessmentsPlagiarism": { @@ -7028,6 +7028,9 @@ "course.userInvitations.InvitationResultDialog.updatedSubtitle": { "defaultMessage": "{count}개 업데이트됨 · 먼저 표시" }, + "course.userInvitations.InvitationResultDialog.blankHeaderWarning": { + "defaultMessage": "헤더가 없는 열이 하나 이상 있어 해당 데이터는 무시되었습니다." + }, "course.userInvitations.InvitationResultDialog.summary": { "defaultMessage": "새 초대장 {newInvitations}개 발송, {newEnrollments}명 직접 등록, {alreadyInCourse}명은 이미 과정에 있습니다." }, @@ -7092,10 +7095,10 @@ "defaultMessage": "사용자 초대에 실패했습니다. 데이터가 올바르게 형식화되었는지 확인하세요." }, "course.userInvitations.InviteUsersFileUpload.fileUploadExample": { - "defaultMessage": "이름,이메일,역할,팬텀,외부 ID{br}John,test1@example.org,학생,y,A0123456{br}Mary,test2@example.org,티칭 어시스턴트,n,A0123457" + "defaultMessage": "이름,이메일,외부 ID,역할,팬텀{br}John,test1@example.com,A0123456,student,y{br}Mary,test2@example.com,A0123457,teaching_assistant,n" }, "course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline": { - "defaultMessage": "이름,이메일,역할,팬텀,개인 타임라인,외부 ID{br}John,test1@example.org,학생,y,otot,A0123456{br}Mary,test2@example.org,티칭 어시스턴트,n,fixed,A0123457" + "defaultMessage": "이름,이메일,외부 ID,역할,팬텀,개인 타임라인{br}John,test1@example.com,A0123456,student,y,otot{br}Mary,test2@example.com,A0123457,teaching_assistant,n,fixed" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoEmail": { "defaultMessage": "각 초대는 과정 내에서 고유한 이메일 주소를 사용해야 합니다. 중복된 이메일은 건너뜁니다." @@ -9154,5 +9157,32 @@ }, "users.troubleSigningIn": { "defaultMessage": "로그인에 문제가 있으신가요?" + }, + "course.userInvitations.ExternalIdConflictPrompt.title": { + "defaultMessage": "외부 ID 업데이트 확인" + }, + "course.userInvitations.ExternalIdConflictPrompt.body": { + "defaultMessage": "이 사용자들은 이미 등록되었거나 초대가 대기 중입니다. 새로운 초대 이메일은 발송되지 않습니다. 현재 외부 ID를 유지하거나 파일의 값으로 교체하시겠습니까?" + }, + "course.userInvitations.ExternalIdConflictPrompt.pendingInvitationUpdates": { + "defaultMessage": "대기 중인 초대 업데이트 ({count}개)" + }, + "course.userInvitations.ExternalIdConflictPrompt.pendingCourseUserUpdates": { + "defaultMessage": "대기 중인 수강생 업데이트 ({count}개)" + }, + "course.userInvitations.ExternalIdConflictPrompt.goBack": { + "defaultMessage": "돌아가기" + }, + "course.userInvitations.ExternalIdConflictPrompt.keepExisting": { + "defaultMessage": "현재 유지" + }, + "course.userInvitations.ExternalIdConflictPrompt.replace": { + "defaultMessage": "교체" + }, + "lib.translations.table.column.currentExternalId": { + "defaultMessage": "현재 외부 ID" + }, + "lib.translations.table.column.newExternalId": { + "defaultMessage": "새 외부 ID" } } diff --git a/client/locales/zh.json b/client/locales/zh.json index 9a82bf3ce6b..6084292548d 100644 --- a/client/locales/zh.json +++ b/client/locales/zh.json @@ -5975,7 +5975,7 @@ "course.plagiarism.PlagiarismIndex.assessments.noPlagiarismCheckableQuestions": { "defaultMessage": "没有可检查的问题" }, - "course.plagiarism.PlagiarismIndex.assessments.notEnoughSubmissions": { + "course.plagiarism.PlagiarismIndex.assessments.notEnoughSubmissions": { "defaultMessage": "提交数量不足" }, "course.plagiarism.PlagiarismIndex.assessments.runAssessmentsPlagiarism": { @@ -7022,6 +7022,9 @@ "course.userInvitations.InvitationResultDialog.updatedSubtitle": { "defaultMessage": "{count} 条已更新 · 优先显示" }, + "course.userInvitations.InvitationResultDialog.blankHeaderWarning": { + "defaultMessage": "有一个或多个列缺少标题，其数据已被忽略。" + }, "course.userInvitations.InvitationResultDialog.summary": { "defaultMessage": "已发出 {newInvitations} 封新邀请，{newEnrollments} 人直接加入，{alreadyInCourse} 人已在课程中。" }, @@ -7086,10 +7089,10 @@ "defaultMessage": "邀请用户失败。请确保你的数据格式正确。" }, "course.userInvitations.InviteUsersFileUpload.fileUploadExample": { - "defaultMessage": "姓名，电子邮件,Role,Phantom,外部编号{br}John,test1@example.org,student,y,A0123456{br}Mary,test2@example.org,teaching_assistant,n,A0123457" + "defaultMessage": "姓名,电子邮件,外部编号,角色,旁听学生{br}John,test1@example.com,A0123456,student,y{br}Mary,test2@example.com,A0123457,teaching_assistant,n" }, "course.userInvitations.InviteUsersFileUpload.fileUploadExamplePersonalTimeline": { - "defaultMessage": "姓名，电子邮件,Role,Phantom,PersonalTimeline,外部编号{br}John,test1@example.org,student,y,otot,A0123456{br}Mary,test2@example.org,teaching_assistant,n,fixed,A0123457" + "defaultMessage": "姓名,电子邮件,外部编号,角色,旁听学生,个人时间线{br}John,test1@example.com,A0123456,student,y,otot{br}Mary,test2@example.com,A0123457,teaching_assistant,n,fixed" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoEmail": { "defaultMessage": "每个邀请必须使用课程内唯一的电子邮件地址。重复的电子邮件将被跳过。" @@ -7104,7 +7107,7 @@ "defaultMessage": "个人时间线可以是[fixed, otot, stragglers, fomo]，若省略则默认为 {defaultTimelineAlgorithm}。" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoPhantom": { - "defaultMessage": "旁听用户可以是 true/false，具有以下值 ['t', 'true', 'y', 'yes']（不区分大小写），若省略则默认为 false。" + "defaultMessage": "旁听学生可以是 true/false，具有以下值 ['t', 'true', 'y', 'yes']（不区分大小写），若省略则默认为 false。" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoRole": { "defaultMessage": "角色可以是[student, observer, teaching_assistant, manager, owner]，如果省略则默认为学生。用户只能被助教邀请为学生。" @@ -9148,5 +9151,32 @@ }, "users.troubleSigningIn": { "defaultMessage": "登录遇到问题？" + }, + "course.userInvitations.ExternalIdConflictPrompt.title": { + "defaultMessage": "确认外部编号更新" + }, + "course.userInvitations.ExternalIdConflictPrompt.body": { + "defaultMessage": "这些用户已经注册或有待处理的邀请。不会向他们发送新的邀请邮件。您希望保留其当前的外部编号，还是用文件中的值替换它们？" + }, + "course.userInvitations.ExternalIdConflictPrompt.pendingInvitationUpdates": { + "defaultMessage": "待处理邀请更新（{count}）" + }, + "course.userInvitations.ExternalIdConflictPrompt.pendingCourseUserUpdates": { + "defaultMessage": "待处理课程成员更新（{count}）" + }, + "course.userInvitations.ExternalIdConflictPrompt.goBack": { + "defaultMessage": "返回" + }, + "course.userInvitations.ExternalIdConflictPrompt.keepExisting": { + "defaultMessage": "保留现有" + }, + "course.userInvitations.ExternalIdConflictPrompt.replace": { + "defaultMessage": "替换" + }, + "lib.translations.table.column.currentExternalId": { + "defaultMessage": "当前外部编号" + }, + "lib.translations.table.column.newExternalId": { + "defaultMessage": "新外部编号" } } diff --git a/config/locales/en/csv.yml b/config/locales/en/csv.yml index 0d1cf158af1..a2d89645408 100644 --- a/config/locales/en/csv.yml +++ b/config/locales/en/csv.yml @@ -63,3 +63,13 @@ en: type: 'Student Type' email: 'Email' external_id: 'External ID' + + # Course user invitations CSV header terms (parser source of truth) + course_user_invitations: + headers: + name: "Name" + email: "Email" + external_id: "External ID" + role: "Role" + phantom: "Phantom" + personal_timeline: "Personal Timeline" diff --git a/config/locales/en/errors.yml b/config/locales/en/errors.yml index f7fbcb10527..f4d67e5a94e 100644 --- a/config/locales/en/errors.yml +++ b/config/locales/en/errors.yml @@ -20,7 +20,9 @@ en: 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}.' - timeline_template_mismatch: 'More than 5 columns detected. This course does not use Personalized Timelines. Please re-upload using the 5-column template (Name, Email, Role, Phantom, External ID).' + invalid_headers: 'Unrecognized CSV header "%{header}". Accepted headers: %{accepted}.' + missing_required_headers: 'CSV must include both a "Name" and "Email" column.' + duplicate_headers: 'Duplicate CSV column: "%{column}". Each column must appear only once.' user_registrations: invalid_code: 'You have entered an invalid invitation code.' code_taken: > diff --git a/config/locales/ko/csv.yml b/config/locales/ko/csv.yml index 0a58bcd615a..73c6ec9390f 100644 --- a/config/locales/ko/csv.yml +++ b/config/locales/ko/csv.yml @@ -63,3 +63,13 @@ ko: type: '학생 유형' email: '이메일' external_id: '외부 ID' + + # Course user invitations CSV header terms (parser source of truth) + course_user_invitations: + headers: + name: "이름" + email: "이메일" + external_id: "외부 ID" + role: "역할" + phantom: "팬텀" + personal_timeline: "개인 타임라인" diff --git a/config/locales/ko/errors.yml b/config/locales/ko/errors.yml index 9ad4bc2f498..bb9a293c558 100644 --- a/config/locales/ko/errors.yml +++ b/config/locales/ko/errors.yml @@ -20,7 +20,9 @@ ko: invalid_email: '%{email}을(를) 처리할 수 없습니다: 유효하지 않은 이메일: %{message}.' duplicate_external_id: '%{email}을(를) 처리할 수 없습니다: 외부 ID "%{external_id}"는 이미 사용 중입니다.' other_error: '%{email}을(를) 처리할 수 없습니다: %{message}.' - timeline_template_mismatch: '5개를 초과하는 열이 감지되었습니다. 이 강좌는 개인화된 일정 기능을 사용하지 않습니다. 5열 템플릿(이름, 이메일, 역할, 팬텀, 외부 ID)을 사용하여 다시 업로드해 주십시오.' + invalid_headers: '인식되지 않는 CSV 헤더: "%{header}". 허용되는 헤더: %{accepted}.' + missing_required_headers: 'CSV에는 "이름"과 "이메일" 열이 모두 포함되어야 합니다.' + duplicate_headers: '중복된 CSV 열: "%{column}". 각 열은 한 번만 나타나야 합니다.' user_registrations: invalid_code: '잘못된 초대 코드를 입력하셨습니다.' code_taken: > diff --git a/config/locales/zh/csv.yml b/config/locales/zh/csv.yml index 0e86f4e740b..b0e02401b2f 100644 --- a/config/locales/zh/csv.yml +++ b/config/locales/zh/csv.yml @@ -63,3 +63,13 @@ zh: type: '学生类型' email: '电子邮箱' external_id: '外部编号' + + # Course user invitations CSV header terms (parser source of truth) + course_user_invitations: + headers: + name: "姓名" + email: "电子邮件" + external_id: "外部编号" + role: "角色" + phantom: "旁听学生" + personal_timeline: "个人时间线" diff --git a/config/locales/zh/errors.yml b/config/locales/zh/errors.yml index eae926e15b9..44416200e3b 100644 --- a/config/locales/zh/errors.yml +++ b/config/locales/zh/errors.yml @@ -20,7 +20,9 @@ zh: invalid_email: '%{email}无法处理：邮箱无效：%{message}。' duplicate_external_id: '%{email}无法处理：外部编号"%{external_id}"已被使用。' other_error: '%{email}无法处理：%{message}。' - timeline_template_mismatch: '检测到超过 5 列。本课程未使用个性化时间线。请使用 5 列模板重新上传（姓名、电子邮箱、角色、虚拟用户、外部编号）。' + invalid_headers: 'CSV 标题 "%{header}" 无法识别。接受的标题：%{accepted}。' + missing_required_headers: 'CSV 必须包含"姓名"和"电子邮件"列。' + duplicate_headers: '重复的 CSV 列："%{column}"。每一列只能出现一次。' user_registrations: invalid_code: '你输入了一个无效的邀请码。' code_taken: > diff --git a/spec/controllers/course/user_invitations_controller_spec.rb b/spec/controllers/course/user_invitations_controller_spec.rb index d9e95a38de3..07991b5cd5a 100644 --- a/spec/controllers/course/user_invitations_controller_spec.rb +++ b/spec/controllers/course/user_invitations_controller_spec.rb @@ -68,6 +68,156 @@ def replace_with_erroneous_course expect(controller.current_course.errors[:base].length).not_to eq(0) end end + + context 'when uploading a CSV with external ID updates and no resolution param' do + render_views + + let(:existing_user) { create(:user) } + let!(:existing_course_user) do + create(:course_student, course: course, user: existing_user, external_id: 'OLD001') + end + let(:csv_content) do + "Name,Email,External ID,Role,Phantom,Personal Timeline\n" \ + "#{existing_user.name},#{existing_user.email},NEW001,student,false," + end + let(:csv_file) do + file = Tempfile.new(['invite', '.csv']) + file.write(csv_content) + file.rewind + Rack::Test::UploadedFile.new(file.path, 'text/csv').tap { file.close } + end + + subject do + post :create, format: :json, params: { course_id: course, course: { invitations_file: csv_file } } + end + + it 'returns 200 with pendingCourseUserUpdates' do + subject + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json.keys).to include('pendingCourseUserUpdates') + expect(json['pendingCourseUserUpdates']).not_to be_empty + end + + it 'does not change external_id in the database' do + expect { subject }.not_to(change { existing_course_user.reload.external_id }) + end + end + + context 'when uploading with resolution=keep_existing' do + render_views + + let(:existing_user) { create(:user) } + let!(:existing_course_user) do + create(:course_student, course: course, user: existing_user, external_id: 'OLD001') + end + let(:csv_content) do + "Name,Email,External ID,Role,Phantom,Personal Timeline\n" \ + "#{existing_user.name},#{existing_user.email},NEW001,student,false," + end + let(:csv_file) do + file = Tempfile.new(['invite', '.csv']) + file.write(csv_content) + file.rewind + Rack::Test::UploadedFile.new(file.path, 'text/csv').tap { file.close } + end + + subject do + post :create, format: :json, params: { + course_id: course, + course: { invitations_file: csv_file }, + external_id_resolution: 'keep_existing' + } + end + + it 'returns 200 with invitationResult' do + subject + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json).to have_key('invitationResult') + end + + it 'does not change the external_id' do + expect { subject }.not_to(change { existing_course_user.reload.external_id }) + end + end + + context 'when uploading with resolution=replace_all' do + render_views + + let(:existing_user) { create(:user) } + let!(:existing_course_user) do + create(:course_student, course: course, user: existing_user, external_id: 'OLD001') + end + let(:csv_content) do + "Name,Email,External ID,Role,Phantom,Personal Timeline\n" \ + "#{existing_user.name},#{existing_user.email},NEW001,student,false," + end + let(:csv_file) do + file = Tempfile.new(['invite', '.csv']) + file.write(csv_content) + file.rewind + Rack::Test::UploadedFile.new(file.path, 'text/csv').tap { file.close } + end + + subject do + post :create, format: :json, params: { + course_id: course, + course: { invitations_file: csv_file }, + external_id_resolution: 'replace_all' + } + end + + it 'returns 200 with invitationResult' do + subject + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json).to have_key('invitationResult') + end + + it 'updates the external_id to the new value' do + subject + expect(existing_course_user.reload.external_id).to eq('NEW001') + end + end + + context 'when uploading with an unrecognized resolution value' do + render_views + + let(:existing_user) { create(:user) } + let!(:existing_course_user) do + create(:course_student, course: course, user: existing_user, external_id: 'OLD001') + end + let(:csv_content) do + "Name,Email,External ID,Role,Phantom,Personal Timeline\n" \ + "#{existing_user.name},#{existing_user.email},NEW001,student,false," + end + let(:csv_file) do + file = Tempfile.new(['invite', '.csv']) + file.write(csv_content) + file.rewind + Rack::Test::UploadedFile.new(file.path, 'text/csv').tap { file.close } + end + + subject do + post :create, format: :json, params: { + course_id: course, + course: { invitations_file: csv_file }, + external_id_resolution: 'anything_at_all' + } + end + + it 'treats the invalid resolution as nil and returns pendingCourseUserUpdates' do + subject + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json.keys).to include('pendingCourseUserUpdates') + end + + it 'does not overwrite the external_id' do + expect { subject }.not_to(change { existing_course_user.reload.external_id }) + end + end end context 'when inviting a user who already has a non-retryable (failed) invitation' do diff --git a/spec/features/course/invitation_management_spec.rb b/spec/features/course/invitation_management_spec.rb index deade746734..4cfe15bc2b8 100644 --- a/spec/features/course/invitation_management_spec.rb +++ b/spec/features/course/invitation_management_spec.rb @@ -34,7 +34,8 @@ users = build_list(:user, 2) invitation_file = Tempfile.new(['invitation', '.csv']) invitation_file. - write("Name,Email\n#{users.map { |u| [u.name, u.email].join(',') }.join("\n")}") + write("Name,Email,Phantom,Role,External ID,Personal Timeline\n" \ + "#{users.map { |u| [u.name, u.email, '', '', '', ''].join(',') }.join("\n")}") invitation_file.close visit invite_course_users_path(course) diff --git a/spec/fixtures/course/invitation_duplicate_external_id.csv b/spec/fixtures/course/invitation_duplicate_external_id.csv index a1ca22f4caa..cbf7133958a 100644 --- a/spec/fixtures/course/invitation_duplicate_external_id.csv +++ b/spec/fixtures/course/invitation_duplicate_external_id.csv @@ -1,2 +1,2 @@ -name,email,role,phantom,timeline_algorithm,external_id -Alice,alice_ext_dupe1@example.com,student,false,fixed,EXT_DUPE +Name,Email,External ID,Role,Phantom +Alice,alice_ext_dupe1@example.com,EXT_DUPE,student,false diff --git a/spec/fixtures/course/invitation_empty.csv b/spec/fixtures/course/invitation_empty.csv index 435a29647fa..3d834561cd2 100644 --- a/spec/fixtures/course/invitation_empty.csv +++ b/spec/fixtures/course/invitation_empty.csv @@ -1,4 +1,4 @@ -name,email +Name,Email,External ID,Role,Phantom Foo,bar@foo.com Blank Email, ,blank@name.com diff --git a/spec/fixtures/course/invitation_fuzzy_roles_phantom_timeline.csv b/spec/fixtures/course/invitation_fuzzy_roles_phantom_timeline.csv index 25e55fa3588..c38d32434e1 100644 --- a/spec/fixtures/course/invitation_fuzzy_roles_phantom_timeline.csv +++ b/spec/fixtures/course/invitation_fuzzy_roles_phantom_timeline.csv @@ -1,10 +1,10 @@ -name,email,role,phantom,timeline_algorithm -Foo,bar@foo.com,,, -Non-Phantom Staff 1,foo1@bar.com,Teaching Assistant,no,stragGlers -Non-Phantom Staff 2,foo2@bar.com,manager,no,OTOT -Non-Phantom Staff 3,foo3@bar.com,oWNER,no,Fomo -Non-Phantom Staff 4,foo4@bar.com,Observer,no,Fixed -Phantom Baz,foo5@bar.com,teaching_AssisTant,y,otot -Phantom Student1,foo6@bar.com,student,t,fixed -Phantom Student2,foo7@bar.com,student,yes,foMo -Phantom Student3,foo8@bar.com,student,true,stragglers +Name,Email,External ID,Role,Phantom,Personal Timeline +Foo,bar@foo.com,,,,, +Non-Phantom Staff 1,foo1@bar.com,,Teaching Assistant,no,stragGlers +Non-Phantom Staff 2,foo2@bar.com,,manager,no,OTOT +Non-Phantom Staff 3,foo3@bar.com,,oWNER,no,Fomo +Non-Phantom Staff 4,foo4@bar.com,,Observer,no,Fixed +Phantom Baz,foo5@bar.com,,teaching_AssisTant,y,otot +Phantom Student1,foo6@bar.com,,student,t,fixed +Phantom Student2,foo7@bar.com,,student,yes,foMo +Phantom Student3,foo8@bar.com,,student,true,stragglers diff --git a/spec/fixtures/course/invitation_invalid_email.csv b/spec/fixtures/course/invitation_invalid_email.csv index e8838ad89a3..f24994852fb 100644 --- a/spec/fixtures/course/invitation_invalid_email.csv +++ b/spec/fixtures/course/invitation_invalid_email.csv @@ -1,3 +1,3 @@ -name,email,role +Name,Email,External ID,Role,Phantom baz,foo.com bar,twang.com diff --git a/spec/fixtures/course/invitation_no_external_id_no_timeline.csv b/spec/fixtures/course/invitation_no_external_id_no_timeline.csv index d4f4137ff4e..ed0df70aa68 100644 --- a/spec/fixtures/course/invitation_no_external_id_no_timeline.csv +++ b/spec/fixtures/course/invitation_no_external_id_no_timeline.csv @@ -1,3 +1,3 @@ -name,email,role,phantom -Alice,alice@example.com,student,false -Bob,bob@example.com,teaching_assistant,false +Name,Email,External ID,Role,Phantom +Alice,alice@example.com,,student,false +Bob,bob@example.com,,teaching_assistant,false diff --git a/spec/fixtures/course/invitation_external_id_wrong_header.csv b/spec/fixtures/course/invitation_unrecognized_header.csv similarity index 68% rename from spec/fixtures/course/invitation_external_id_wrong_header.csv rename to spec/fixtures/course/invitation_unrecognized_header.csv index 5e9c2effbe0..b0bae22efb5 100644 --- a/spec/fixtures/course/invitation_external_id_wrong_header.csv +++ b/spec/fixtures/course/invitation_unrecognized_header.csv @@ -1,3 +1,3 @@ -name,email,role,phantom,timeline_algorithm,ext_id +Name,Email,Role,Phantom,Personal Timeline,ExtID Alice,alice@example.com,student,false,fixed,EXT001 Bob,bob@example.com,teaching_assistant,false,fomo,EXT002 diff --git a/spec/fixtures/course/invitation_whitespace.csv b/spec/fixtures/course/invitation_whitespace.csv index 7f67af54eb8..85067f25ab9 100644 --- a/spec/fixtures/course/invitation_whitespace.csv +++ b/spec/fixtures/course/invitation_whitespace.csv @@ -1,4 +1,4 @@ -name,email,role - Foo ,bar@foo.com ,student - Bar, baz@bar.com,teaching_assistant -Baz , foo@baz.com ,student +Name,Email,External ID,Role,Phantom + Foo ,bar@foo.com ,,student + Bar, baz@bar.com,,teaching_assistant +Baz , foo@baz.com ,,student diff --git a/spec/fixtures/course/invitation_with_external_id.csv b/spec/fixtures/course/invitation_with_external_id.csv index acaeff99b2f..4e97c523899 100644 --- a/spec/fixtures/course/invitation_with_external_id.csv +++ b/spec/fixtures/course/invitation_with_external_id.csv @@ -1,4 +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 +Name,Email,External ID,Role,Phantom,Personal Timeline +Alice,alice@example.com,EXT001,student,false,fixed +Bob,bob@example.com,EXT002,teaching_assistant,false,fomo 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 index 612e7601e6d..4ed6e100833 100644 --- a/spec/fixtures/course/invitation_with_external_id_no_timeline.csv +++ b/spec/fixtures/course/invitation_with_external_id_no_timeline.csv @@ -1,4 +1,4 @@ -name,email,role,phantom,external_id -Alice,alice@example.com,student,false,EXT001 -Bob,bob@example.com,teaching_assistant,false,EXT002 +Name,Email,External ID,Role,Phantom +Alice,alice@example.com,EXT001,student,false +Bob,bob@example.com,EXT002,teaching_assistant,false Charlie,charlie@example.com,,, diff --git a/spec/fixtures/course/invitation_with_utf_bom.csv b/spec/fixtures/course/invitation_with_utf_bom.csv index adc9eff3ecd..65d12ec555d 100644 --- a/spec/fixtures/course/invitation_with_utf_bom.csv +++ b/spec/fixtures/course/invitation_with_utf_bom.csv @@ -1 +1,2 @@ -Test,baz@foo.com,student \ No newline at end of file +Name,Email,External ID,Role,Phantom,Personal Timeline +Test,baz@foo.com diff --git a/spec/services/course/user_invitation_service_spec.rb b/spec/services/course/user_invitation_service_spec.rb index a6e842cc812..6bc62bd3a08 100644 --- a/spec/services/course/user_invitation_service_spec.rb +++ b/spec/services/course/user_invitation_service_spec.rb @@ -5,11 +5,20 @@ let(:instance) { create(:instance) } with_tenant(:instance) do def temp_csv_from_attributes(records, roles = [], timeline_algorithms = []) + include_timeline = course.show_personalized_timeline_features? Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| file.write(CSV.generate do |csv| - csv << [:name, :email, :role] + headers = ['Name', 'Email', 'External ID', 'Role', 'Phantom'] + headers << 'Personal Timeline' if include_timeline + csv << headers records.zip(roles, timeline_algorithms).each do |user, role, timeline_algorithm| - csv << (role.blank? ? [user.name, user.email] : [user.name, user.email, role, false, timeline_algorithm]) + csv << if role.blank? + [user.name, user.email] + elsif include_timeline + [user.name, user.email, nil, role, false, timeline_algorithm] + else + [user.name, user.email, nil, role, false] + end end end) file.rewind @@ -144,6 +153,23 @@ def invite end end + context 'when duplicate emails differ only in case' do + let(:form_attributes) do + { generate(:nested_attribute_new_id) => { name: 'Alice', email: 'Alice@Example.COM', + role: :student, phantom: false, + external_id: nil }, + generate(:nested_attribute_new_id) => { name: 'Alice2', email: 'alice@example.com', + role: :student, phantom: false, + external_id: nil } } + end + + it 'treats them as duplicate and processes only the first' do + _new_invitations, _existing, _new_cu, _existing_cu, failed_users = subject.invite(form_attributes) + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:duplicate_email_in_file) + end + end + context 'when duplicate users are specified' do before do new_users.push(new_users.last) @@ -246,8 +272,9 @@ def invite def csv_with_external_id_and_timeline(entries) Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| file.write(CSV.generate do |csv| + csv << ['Name', 'Email', 'External ID', 'Role', 'Phantom', 'Personal Timeline'] entries.each do |entry| - csv << [entry[:name], entry[:email], 'student', 'false', 'fixed', entry[:external_id]] + csv << [entry[:name], entry[:email], entry[:external_id], 'student', 'false', 'fixed'] end end) file.rewind @@ -256,7 +283,7 @@ def csv_with_external_id_and_timeline(entries) it 'does not abort the batch' do csv = csv_with_external_id_and_timeline( - [{ name: 'New User', email: generate(:email), external_id: 'taken-id' }] + [name: 'New User', email: generate(:email), external_id: 'taken-id'] ) result = subject.invite(csv) expect(result).not_to be_nil @@ -267,12 +294,25 @@ def csv_with_external_id_and_timeline(entries) end end - context 'when a 6-column CSV (timeline template) is uploaded to a non-timeline course' do - before { course.update!(show_personalized_timeline_features: false) } + context 'when the CSV has duplicate column headers' do + it 'raises CSV::MalformedCSVError for same-language duplicates' do + csv = Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| + file.write(CSV.generate do |c| + c << ['Name', 'Name', 'Email', 'External ID', 'Role', 'Phantom'] + c << ['Alice', 'Alice', generate(:email), 'EXT001', 'student', 'false'] + end) + file.rewind + end + expect { subject.invite(csv) }.to raise_error(CSV::MalformedCSVError) + csv.close! + end - it 'raises CSV::MalformedCSVError to prevent timeline values being stored as external IDs' do + it 'raises CSV::MalformedCSVError for cross-language duplicates' do csv = Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| - file.write(CSV.generate { |c| c << ['Alice', generate(:email), 'student', 'false', 'otot', 'EXT001'] }) + file.write(CSV.generate do |c| + c << ['Name', '姓名', 'Email', 'External ID', 'Role', 'Phantom'] + c << ['Alice', 'Alice', generate(:email), 'EXT001', 'student', 'false'] + end) file.rewind end expect { subject.invite(csv) }.to raise_error(CSV::MalformedCSVError) @@ -280,6 +320,37 @@ def csv_with_external_id_and_timeline(entries) end end + context 'when the CSV begins with a blank line' do + it 'raises a header-validation error rather than a nil error' do + csv = Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| + file.write("\nName,Email\nAlice,#{generate(:email)}\n") + file.rewind + end + expect { subject.invite(csv) }.to raise_error( + CSV::MalformedCSVError, + /#{Regexp.escape(I18n.t('errors.course.user_invitations.missing_required_headers'))}/ + ) + csv.close! + end + end + + context 'when a timeline-template-header CSV is uploaded to a non-timeline course' do + before { course.update!(show_personalized_timeline_features: false) } + + it 'accepts a timeline-format CSV even when the course has timelines disabled' do + csv = Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| + file.write(CSV.generate do |c| + c << ['Name', 'Email', 'External ID', 'Role', 'Phantom', 'Personal Timeline'] + c << ['Alice', generate(:email), 'EXT001', 'student', 'false', 'otot'] + end) + file.rewind + end + new_invitations, = subject.invite(csv) + expect(new_invitations.first.timeline_algorithm).to eq('otot') + 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') } @@ -287,8 +358,9 @@ def csv_with_external_id_and_timeline(entries) def csv_with_external_id_no_timeline(entries) Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| file.write(CSV.generate do |csv| + csv << ['Name', 'Email', 'External ID', 'Role', 'Phantom'] entries.each do |entry| - csv << [entry[:name], entry[:email], 'student', 'false', entry[:external_id]] + csv << [entry[:name], entry[:email], entry[:external_id], 'student', 'false'] end end) file.rewind @@ -297,7 +369,7 @@ def csv_with_external_id_no_timeline(entries) it 'does not abort the batch' do csv = csv_with_external_id_no_timeline( - [{ name: 'New User', email: generate(:email), external_id: 'taken-id' }] + [name: 'New User', email: generate(:email), external_id: 'taken-id'] ) result = subject.invite(csv) expect(result).not_to be_nil @@ -317,28 +389,32 @@ def csv_with_external_id_no_timeline(entries) { '0' => { name: 'Pending User', email: 'pending@example.com', role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' } } end - subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'updates the pending invitation external_id' do - result - expect(pending_invitation.reload.external_id).to eq('new-id') + it 'raises PendingExternalIdUpdates without resolution' do + expect { invitation_service.invite(invitation_attributes) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) end - it 'puts the user in updated_invitations' do - updated_invitations = result[5] - expect(updated_invitations.map { |u| u[:record] }).to include(pending_invitation) + it 'surfaces the pending invitation with correct IDs in the error' do + err = begin + invitation_service.invite(invitation_attributes) + rescue Course::UserInvitationService::PendingExternalIdUpdates => e + e + end + entry = err.pending_invitation_updates.find { |u| u[:record] == pending_invitation } + expect(entry[:new_external_id]).to eq('new-id') + expect(entry[:previous_external_id]).to be_nil end - it 'captures nil as previous_external_id' do - updated_invitations = result[5] - entry = updated_invitations.find { |u| u[:record] == pending_invitation } - expect(entry[:previous_external_id]).to be_nil + it 'updates the invitation external_id with resolution :replace_all' do + invitation_service.invite(invitation_attributes, external_id_resolution: :replace_all) + expect(pending_invitation.reload.external_id).to eq('new-id') end - it 'does not create failed_users entry' do - _, _, _, _, failed_users = result - expect(failed_users).to be_empty + it 'keeps nil external_id with resolution :keep_existing' do + invitation_service.invite(invitation_attributes, external_id_resolution: :keep_existing) + expect(pending_invitation.reload.external_id).to be_nil end end @@ -381,28 +457,54 @@ def csv_with_external_id_no_timeline(entries) { '0' => { name: enrolled_user.name, email: enrolled_user.email, role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' } } end - subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'updates the course_user external_id' do - result + it 'raises PendingExternalIdUpdates without resolution' do + expect { invitation_service.invite(invitation_attributes) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) + end + + it 'surfaces the course user with correct IDs in the error' do + err = begin + invitation_service.invite(invitation_attributes) + rescue Course::UserInvitationService::PendingExternalIdUpdates => e + e + end + entry = err.pending_course_user_updates.find { |u| u[:record] == course_user_record } + expect(entry[:new_external_id]).to eq('new-id') + expect(entry[:previous_external_id]).to be_nil + end + + it 'updates the course_user external_id with resolution :replace_all' do + invitation_service.invite(invitation_attributes, external_id_resolution: :replace_all) expect(course_user_record.reload.external_id).to eq('new-id') end - it 'puts the user in updated_course_users' do - updated_course_users = result[6] - expect(updated_course_users.map { |u| u[:record] }).to include(course_user_record) + it 'keeps nil external_id with resolution :keep_existing' do + invitation_service.invite(invitation_attributes, external_id_resolution: :keep_existing) + expect(course_user_record.reload.external_id).to be_nil end + end - it 'captures nil as previous_external_id' do - updated_course_users = result[6] - entry = updated_course_users.find { |u| u[:record] == course_user_record } - expect(entry[:previous_external_id]).to be_nil + context 'when an existing course user with a non-nil external_id is re-invited with a different free value' do + let!(:enrolled_user) { create(:user) } + let!(:course_user_record) do + create(:course_student, course: course, user: enrolled_user, external_id: 'old-id') end + let(:invitation_attributes) do + { '0' => { name: enrolled_user.name, email: enrolled_user.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' } } + end + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'produces no failed_users entry' do - _, _, _, _, failed_users = result - expect(failed_users).to be_empty + it 'updates the course_user external_id with resolution :replace_all' do + invitation_service.invite(invitation_attributes, external_id_resolution: :replace_all) + expect(course_user_record.reload.external_id).to eq('new-id') + end + + it 'keeps the old external_id with resolution :keep_existing' do + invitation_service.invite(invitation_attributes, external_id_resolution: :keep_existing) + expect(course_user_record.reload.external_id).to eq('old-id') end end @@ -533,28 +635,32 @@ def csv_with_external_id_no_timeline(entries) { '0' => { name: 'Pending User', email: 'pending@example.com', role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' } } end - subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'updates the pending invitation external_id' do - result - expect(pending_invitation.reload.external_id).to eq('new-id') + it 'raises PendingExternalIdUpdates without resolution' do + expect { invitation_service.invite(invitation_attributes) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) end - it 'puts the user in updated_invitations' do - updated_invitations = result[5] - expect(updated_invitations.map { |u| u[:record] }).to include(pending_invitation) + it 'surfaces the pending invitation with correct IDs in the error' do + err = begin + invitation_service.invite(invitation_attributes) + rescue Course::UserInvitationService::PendingExternalIdUpdates => e + e + end + entry = err.pending_invitation_updates.find { |u| u[:record] == pending_invitation } + expect(entry[:new_external_id]).to eq('new-id') + expect(entry[:previous_external_id]).to eq('old-id') end - it 'captures the previous ext_id' do - updated_invitations = result[5] - entry = updated_invitations.find { |u| u[:record] == pending_invitation } - expect(entry[:previous_external_id]).to eq('old-id') + it 'updates the invitation external_id with resolution :replace_all' do + invitation_service.invite(invitation_attributes, external_id_resolution: :replace_all) + expect(pending_invitation.reload.external_id).to eq('new-id') end - it 'does not create failed_users entry' do - _, _, _, _, failed_users = result - expect(failed_users).to be_empty + it 'keeps the old external_id with resolution :keep_existing' do + invitation_service.invite(invitation_attributes, external_id_resolution: :keep_existing) + expect(pending_invitation.reload.external_id).to eq('old-id') end end @@ -570,28 +676,36 @@ def csv_with_external_id_no_timeline(entries) '1' => { name: 'Bob', email: bob_email, role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'freed-id' } } end - subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'updates Alice to new-id' do - result - expect(alice_invitation.reload.external_id).to eq('new-id') + it 'raises PendingExternalIdUpdates without resolution (Alice has existing ext_id)' do + expect { invitation_service.invite(invitation_attributes) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) end - it 'creates a new invitation for Bob with the freed id' do - new_invitations, = result - expect(new_invitations.map(&:external_id)).to include('freed-id') - end + context 'with resolution :replace_all' do + subject(:result) { invitation_service.invite(invitation_attributes, external_id_resolution: :replace_all) } - it 'produces no failed_users entries' do - _, _, _, _, failed_users = result - expect(failed_users).to be_empty + it 'updates Alice to new-id' do + result + expect(alice_invitation.reload.external_id).to eq('new-id') + end + + it 'creates a new invitation for Bob with the freed id' do + new_invitations, = result + expect(new_invitations.map(&:external_id)).to include('freed-id') + end + + it 'produces no failed_users entries' do + _, _, _, _, failed_users = result + expect(failed_users).to be_empty + end end end context 'cross-type freed-id: existing course user frees id, new invitation claims it in same batch' do # invite_new_users runs before add_existing_users, so the new invitation is processed - # first and sees the id as still taken. The existing course user is then upserted + # first and sees the id as still taken. The existing course user is then updated # successfully, but the new invitation has already been rejected. let!(:enrolled_user_freeing) { create(:user) } let!(:course_user_freeing) do @@ -606,18 +720,26 @@ def csv_with_external_id_no_timeline(entries) role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'freed-id' } } end - subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'upserts the existing course user to new-id' do - result - expect(course_user_freeing.reload.external_id).to eq('new-id') + it 'raises PendingExternalIdUpdates without resolution' do + expect { invitation_service.invite(invitation_attributes) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) end - it 'rejects the new invitation because it is processed before the id is freed' do - _, _, _, _, failed_users = result - expect(failed_users.map { |u| u[:email] }).to include(new_user_email) - expect(failed_users.find { |u| u[:email] == new_user_email }[:reason]).to eq(:external_id_taken) + context 'with resolution :replace_all' do + subject(:result) { invitation_service.invite(invitation_attributes, external_id_resolution: :replace_all) } + + it 'updates the existing course user to new-id' do + result + expect(course_user_freeing.reload.external_id).to eq('new-id') + end + + it 'rejects the new invitation because it is processed before the id is freed' do + _, _, _, _, failed_users = result + expect(failed_users.map { |u| u[:email] }).to include(new_user_email) + expect(failed_users.find { |u| u[:email] == new_user_email }[:reason]).to eq(:external_id_taken) + end end end @@ -629,8 +751,8 @@ def csv_with_external_id_no_timeline(entries) subject(:result) do invitation_service.invite( - '0' => { name: enrollee.name, email: enrollee.email, role: :student, - phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } + { '0' => { name: enrollee.name, email: enrollee.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } } ) end @@ -657,8 +779,8 @@ def csv_with_external_id_no_timeline(entries) it 'puts the row in failed_users with :external_id_taken and does not create an invitation' do result = invitation_service.invite( - '0' => { name: 'New Person', email: generate(:email), role: :student, - phantom: false, timeline_algorithm: :fixed, external_id: 'cu-taken-id' } + { '0' => { name: 'New Person', email: generate(:email), role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'cu-taken-id' } } ) new_invitations, _, _, _, failed_users = result expect(failed_users.size).to eq(1) @@ -674,8 +796,8 @@ def csv_with_external_id_no_timeline(entries) it 'sends an invitation instead of directly enrolling the user' do result = invitation_service.invite( - '0' => { name: unconfirmed_user.name, email: unconfirmed_user.email, - role: :student, phantom: false, timeline_algorithm: :fixed, external_id: nil } + { '0' => { name: unconfirmed_user.name, email: unconfirmed_user.email, + role: :student, phantom: false, timeline_algorithm: :fixed, external_id: nil } } ) new_invitations, _, new_course_users, = result expect(new_invitations.size).to eq(1) @@ -687,10 +809,10 @@ def csv_with_external_id_no_timeline(entries) 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'] + csv << ['Name', 'Email', 'External ID', 'Role', 'Phantom', 'Personal Timeline'] entries.each do |e| - csv << [e[:name], e[:email], e.fetch(:role, 'student'), - e.fetch(:phantom, 'false'), e.fetch(:timeline, 'fixed'), e[:external_id]] + csv << [e[:name], e[:email], e[:external_id], + e.fetch(:role, 'student'), e.fetch(:phantom, 'false'), e.fetch(:timeline, 'fixed')] end end) file.rewind @@ -793,7 +915,7 @@ def csv_with_timeline(entries) context 'when an invalid email is specified' do let(:invalid_user_attributes) do - [{ name: build(:user).name, email: 'xxnot an email', role: :student }] + [name: build(:user).name, email: 'xxnot an email', role: :student] end it 'fails' do @@ -821,8 +943,8 @@ def csv_with_timeline(entries) it 'assigns the CSV timeline_algorithm to the enrolled CourseUser, not the course default' do result = subject.invite( - '0' => { name: existing_user.name, email: existing_user.email, - role: :student, phantom: false, timeline_algorithm: :otot, external_id: nil } + { '0' => { name: existing_user.name, email: existing_user.email, + role: :student, phantom: false, timeline_algorithm: :otot, external_id: nil } } ) _, _, new_course_users, = result expect(new_course_users.size).to eq(1) @@ -865,8 +987,13 @@ def csv_with_timeline(entries) role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'DIFFERENT' } } end - it 'sets ext_id on existing invitation when current is nil and CSV value is free' do - result = subject.invite(inv_nil_ext_user_attrs) + it 'raises PendingExternalIdUpdates when existing invitation has nil ext_id and CSV value is free' do + expect { subject.invite(inv_nil_ext_user_attrs) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) + end + + it 'sets ext_id on existing invitation when current is nil and CSV value is free with :replace_all' do + result = subject.invite(inv_nil_ext_user_attrs, external_id_resolution: :replace_all) expect(result).not_to be_nil updated_invitations = result[5] expect(updated_invitations.length).to eq(1) @@ -881,8 +1008,13 @@ def csv_with_timeline(entries) expect(failed_users.map { |u| u[:reason] }).to include(:external_id_taken) end - it 'sets ext_id on existing course user when current is nil and CSV value is free' do - result = subject.invite(enrolled_nil_user_attrs) + it 'raises PendingExternalIdUpdates when existing course user has nil ext_id and CSV value is free' do + expect { subject.invite(enrolled_nil_user_attrs) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) + end + + it 'sets ext_id on existing course user when current is nil and CSV value is free with :replace_all' do + result = subject.invite(enrolled_nil_user_attrs, external_id_resolution: :replace_all) expect(result).not_to be_nil updated_course_users = result[6] expect(updated_course_users.length).to eq(1) @@ -891,26 +1023,31 @@ def csv_with_timeline(entries) end context 'when an existing course user has non-nil ext_id and CSV provides a free different value' do - it 'updates the course_user external_id' do - subject.invite(enrolled_conflict_user_attrs) + it 'raises PendingExternalIdUpdates without resolution' do + expect { subject.invite(enrolled_conflict_user_attrs) }. + to raise_error(Course::UserInvitationService::PendingExternalIdUpdates) + end + + it 'updates the course_user external_id with resolution :replace_all' do + subject.invite(enrolled_conflict_user_attrs, external_id_resolution: :replace_all) expect(enrolled_conflict.reload.external_id).to eq('DIFFERENT') end - it 'puts the user in updated_course_users' do - result = subject.invite(enrolled_conflict_user_attrs) + it 'puts the user in updated_course_users with resolution :replace_all' do + result = subject.invite(enrolled_conflict_user_attrs, external_id_resolution: :replace_all) updated_course_users = result[6] expect(updated_course_users.map { |u| u[:record] }).to include(enrolled_conflict) end - it 'captures the previous ext_id' do - result = subject.invite(enrolled_conflict_user_attrs) + it 'captures the previous ext_id with resolution :replace_all' do + result = subject.invite(enrolled_conflict_user_attrs, external_id_resolution: :replace_all) updated_course_users = result[6] entry = updated_course_users.find { |u| u[:record] == enrolled_conflict } expect(entry[:previous_external_id]).to eq('EXISTING') end - it 'produces no failed_users entry' do - result = subject.invite(enrolled_conflict_user_attrs) + it 'produces no failed_users entry with resolution :replace_all' do + result = subject.invite(enrolled_conflict_user_attrs, external_id_resolution: :replace_all) _, _, _, _, failed_users = result expect(failed_users).to be_empty end @@ -970,6 +1107,7 @@ def csv_with_timeline(entries) end context 'when the provided file has whitespace in the fields' do + before { course.update!(show_personalized_timeline_features: false) } let(:csv_file) { file_fixture('course/invitation_whitespace.csv') } it 'strips the attributes of whitespace' do @@ -982,6 +1120,7 @@ def csv_with_timeline(entries) end context 'when the provided csv file has blanks' do + before { course.update!(show_personalized_timeline_features: false) } subject do stubbed_user_invitation_service. send(:parse_from_file, file_fixture('course/invitation_empty.csv')) @@ -1003,13 +1142,40 @@ def csv_with_timeline(entries) send(:parse_from_file, file_fixture('course/invitation_no_header.csv')) end - it 'does not raise an exception' do - expect { subject }.not_to raise_exception + before { course.update!(show_personalized_timeline_features: false) } + + it 'raises invalid_headers (headerless CSVs are rejected)' do + expect { subject }.to raise_error(CSV::MalformedCSVError) 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 a blank-header column has data in data rows' do + before { course.update!(show_personalized_timeline_features: false) } + + it 'proceeds and sets blank_header_warning to true' do + csv_content = "Name,Email,,Role\n" \ + "Alice,alice@example.com,mystery_data,student\n" + Tempfile.create(['blank_header_with_data', '.csv']) do |f| + f.write(csv_content) + f.rewind + stubbed_user_invitation_service.send(:parse_from_file, f) + expect(stubbed_user_invitation_service.blank_header_warning).to be true + end + end + end + + context 'when a blank-header column has no data (trailing spreadsheet artifact)' do + before { course.update!(show_personalized_timeline_features: false) } + + it 'proceeds with blank_header_warning false' do + csv_content = "Name,Email,\n" \ + "Alice,alice@example.com,\n" + Tempfile.create(['blank_header_empty', '.csv']) do |f| + f.write(csv_content) + f.rewind + stubbed_user_invitation_service.send(:parse_from_file, f) + expect(stubbed_user_invitation_service.blank_header_warning).to be false + end end end @@ -1018,6 +1184,130 @@ def csv_with_timeline(entries) context 'when personal timelines are enabled' do before { course.update!(show_personalized_timeline_features: true) } + context 'Phase 3 header acceptance — with timeline' do + it 'accepts canonical EN header' do + csv_content = "Name,Email,External ID,Role,Phantom,Personal Timeline\n" \ + "Alice,alice@example.com,EXT001,student,false,fixed\n" + Tempfile.create(['p3_en_timeline', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + expect(result[0][:timeline_algorithm]).to eq(:fixed) + end + end + + it 'accepts canonical ZH header' do + csv_content = "姓名,电子邮件,外部编号,角色,旁听学生,个人时间线\n" \ + "Alice,alice@example.com,EXT001,student,false,fixed\n" + Tempfile.create(['p3_zh_timeline', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + end + end + + it 'accepts canonical KO header' do + csv_content = "이름,이메일,외부 ID,역할,팬텀,개인 타임라인\n" \ + "Alice,alice@example.com,EXT001,student,false,fixed\n" + Tempfile.create(['p3_ko_timeline', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + end + end + + it 'accepts mixed-language header' do + csv_content = "姓名,Email,외부 ID,Role,旁听学生,Personal Timeline\n" \ + "Alice,alice@example.com,EXT001,student,false,fixed\n" + Tempfile.create(['p3_mixed', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + end + end + + it 'parses correctly regardless of column order' do + csv_content = "Personal Timeline,External ID,Email,Role,Name,Phantom\n" \ + "fixed,EXT001,alice@example.com,student,Alice,false\n" + Tempfile.create(['p3_shuffled', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + expect(result[0][:name]).to eq('Alice') + expect(result[0][:timeline_algorithm]).to eq(:fixed) + end + end + + it 'raises invalid_headers for headerless CSV' do + Tempfile.create(['p3_no_header', '.csv']) do |f| + f.write("Alice,alice@example.com,EXT001,student,false,fixed\n") + f.rewind + expect { stubbed_user_invitation_service.send(:parse_from_file, f) }. + to raise_error(CSV::MalformedCSVError) + end + end + + it 'includes accepted headers in UI locale in the error message' do + Tempfile.create(['p3_err_locale', '.csv']) do |f| + f.write("bad,headers\n") + f.rewind + expect { stubbed_user_invitation_service.send(:parse_from_file, f) }. + to raise_error(CSV::MalformedCSVError, /External ID/) + end + end + + it 'accepts a CSV with only name and email columns' do + Tempfile.create(['p3_name_email_only', '.csv']) do |f| + f.write("Name,Email\nAlice,alice@example.com\n") + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:name]).to eq('Alice') + expect(result[0][:email]).to eq('alice@example.com') + expect(result[0][:role]).to eq(:student) + expect(result[0][:phantom]).to eq(false) + end + end + + it 'raises missing_required_headers when email column is absent' do + Tempfile.create(['p3_no_email', '.csv']) do |f| + f.write("Name\nAlice\n") + f.rewind + expect { stubbed_user_invitation_service.send(:parse_from_file, f) }. + to raise_error(CSV::MalformedCSVError) + end + end + + it 'raises missing_required_headers when name column is absent' do + Tempfile.create(['p3_no_name', '.csv']) do |f| + f.write("Email\nalice@example.com\n") + f.rewind + expect { stubbed_user_invitation_service.send(:parse_from_file, f) }. + to raise_error(CSV::MalformedCSVError) + end + end + end + + it 'silently skips blank-header columns and parses remaining columns correctly' do + Tempfile.create(['blank_header_col', '.csv']) do |f| + f.write("Name,,Email\nAlice,,alice@example.com\n") + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:name]).to eq('Alice') + expect(result[0][:email]).to eq('alice@example.com') + end + end + it 'accepts a file with name/header' do result = subject.send(:parse_from_file, temp_csv) expect(result.length).to eq(users.length) @@ -1146,19 +1436,12 @@ def csv_with_timeline(entries) end end - 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') + context 'when the csv header uses unrecognized column names' do + it 'raises an error for the unrecognized header' do + expect do + stubbed_user_invitation_service. + send(:parse_from_file, file_fixture('course/invitation_unrecognized_header.csv')) + end.to raise_error(CSV::MalformedCSVError) end end end @@ -1166,6 +1449,99 @@ def csv_with_timeline(entries) context 'when personal timelines are disabled' do before { course.update!(show_personalized_timeline_features: false) } + context 'Phase 3 header acceptance — without timeline' do + it 'accepts canonical EN header (no timeline)' do + csv_content = "Name,Email,External ID,Role,Phantom\n" \ + "Alice,alice@example.com,EXT001,student,false\n" + Tempfile.create(['p3_en_notimeline', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + end + end + + it 'accepts canonical ZH header (no timeline)' do + csv_content = "姓名,电子邮件,外部编号,角色,旁听学生\n" \ + "Alice,alice@example.com,EXT001,student,false\n" + Tempfile.create(['p3_zh_notimeline', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + end + end + + it 'accepts canonical KO header (no timeline)' do + csv_content = "이름,이메일,외부 ID,역할,팬텀\n" \ + "Alice,alice@example.com,EXT001,student,false\n" + Tempfile.create(['p3_ko_notimeline', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + end + end + + it 'accepts timeline-format CSV even when the course has timelines disabled' do + csv_content = "Name,Email,External ID,Role,Phantom,Personal Timeline\n" \ + "Alice,alice@example.com,EXT001,student,false,otot\n" + Tempfile.create(['p3_timeline_on_notime', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + end + end + + it 'raises invalid_headers for headerless CSV (no timeline)' do + Tempfile.create(['p3_no_header_notime', '.csv']) do |f| + f.write("Alice,alice@example.com,EXT001,student,false\n") + f.rewind + expect { stubbed_user_invitation_service.send(:parse_from_file, f) }. + to raise_error(CSV::MalformedCSVError) + end + end + + it 'accepts external_id header spelled with underscore instead of space' do + csv_content = "Name,Email,external_id,Role,Phantom\n" \ + "Alice,alice@example.com,EXT001,student,false\n" + Tempfile.create(['p3_ext_underscore', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + end + end + + it 'accepts external_id header spelled with no separator (externalid)' do + csv_content = "Name,Email,externalid,Role,Phantom\n" \ + "Alice,alice@example.com,EXT001,student,false\n" + Tempfile.create(['p3_ext_nospace', '.csv']) do |f| + f.write(csv_content) + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:external_id]).to eq('EXT001') + end + end + + it 'accepts headers in any casing' do + Tempfile.create(['p3_uppercase', '.csv']) do |f| + f.write("NAME,EMAIL\nAlice,alice@example.com\n") + f.rewind + result = stubbed_user_invitation_service.send(:parse_from_file, f) + expect(result.length).to eq(1) + expect(result[0][:name]).to eq('Alice') + expect(result[0][:email]).to eq('alice@example.com') + end + end + end + context 'when the csv has an external_id column' do subject do stubbed_user_invitation_service. @@ -1207,6 +1583,20 @@ def csv_with_timeline(entries) end end + describe 'template guard' do + it 'accepts every static invite template with the parser' do + template_dir = Rails.root.join('client/app/assets/templates') + Dir.glob(template_dir.join('course-user-invitation-template*.csv')).each do |path| + include_timeline = !path.include?('no-timeline') + course.update!(show_personalized_timeline_features: include_timeline) + + expect do + stubbed_user_invitation_service.send(:parse_from_file, path) + end.not_to raise_error, "Template #{File.basename(path)} failed parser validation" + end + end + end + describe '#parse_from_form' do subject { stubbed_user_invitation_service } diff --git a/spec/support/i18n.rb b/spec/support/i18n.rb index 038adebe8fe..811726a1d7f 100644 --- a/spec/support/i18n.rb +++ b/spec/support/i18n.rb @@ -35,7 +35,7 @@ def always_return_key?(key) # @return [Boolean] def always_return_actual?(key) key.start_with?('errors.', 'support.', 'number.', 'javascript.', 'date.formats.', - 'time.formats.') || + 'time.formats.', 'csv.course_user_invitations.headers.') || # The html passed to the key should always be returned. key.end_with?('_html') end