From 12d7f4a786c9bae4bc2f47266a5c4678957945f8 Mon Sep 17 00:00:00 2001 From: lws49 Date: Wed, 3 Jun 2026 15:45:57 +0800 Subject: [PATCH 1/2] feat(invitations): add CSV header validation and external ID conflict resolution - validate CSV column headers explicitly instead of checking column count - detect external ID changes on existing users/invitations before applying; surface a confirmation prompt (Keep Existing / Replace) with a side-by-side Current / New External ID table capped at 320px for large uploads - conflict resolution wired into both file upload and individual invite form; file ref preserved on Go Back so admin need not re-select - add ExternalIdResolution and PendingExternalIdConflict types; update invite API and operations layer to detect and return conflict payload - controller rescues PendingExternalIdUpdates, renders jbuilder partial; concern branches on @resolution to populate pending vs updated arrays - i18n: add EN/KO/ZH translations for conflict prompt and new table columns --- .../course/user_invitations_controller.rb | 26 +- .../parse_invitation_concern.rb | 37 ++- .../process_invitation_concern.rb | 19 +- .../course/user_invitation_service.rb | 57 ++-- .../_pending_external_id_data.json.jbuilder | 25 ++ client/app/api/course/UserInvitations.ts | 22 +- .../components/forms/IndividualInviteForm.tsx | 134 +++++--- .../forms/InviteUsersFileUploadForm.tsx | 2 +- .../misc/ExternalIdConflictPrompt.tsx | 122 +++++++ .../ExternalIdConflictPrompt.test.tsx | 144 ++++++++ .../tables/ExternalIdConflictTable.tsx | 68 ++++ .../__test__/ExternalIdConflictTable.test.tsx | 59 ++++ .../course/user-invitations/operations.ts | 48 ++- .../__test__/index.test.tsx | 93 ++++++ .../pages/InviteUsersFileUpload/index.tsx | 107 ++++-- client/app/types/course/userInvitations.ts | 7 + client/locales/en.json | 32 +- client/locales/ko.json | 29 +- client/locales/zh.json | 29 +- config/locales/en/errors.yml | 2 +- config/locales/ko/errors.yml | 2 +- config/locales/zh/errors.yml | 2 +- .../user_invitations_controller_spec.rb | 114 +++++++ .../invitation_duplicate_external_id.csv | 4 +- spec/fixtures/course/invitation_empty.csv | 1 - ...nvitation_fuzzy_roles_phantom_timeline.csv | 2 +- .../course/invitation_invalid_email.csv | 2 +- .../invitation_no_external_id_no_timeline.csv | 1 - .../fixtures/course/invitation_whitespace.csv | 1 - .../course/invitation_with_external_id.csv | 2 +- ...nvitation_with_external_id_no_timeline.csv | 2 +- .../course/user_invitation_service_spec.rb | 310 ++++++++++++------ 32 files changed, 1257 insertions(+), 248 deletions(-) create mode 100644 app/views/course/user_invitations/_pending_external_id_data.json.jbuilder create mode 100644 client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx create mode 100644 client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx create mode 100644 client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx diff --git a/app/controllers/course/user_invitations_controller.rb b/app/controllers/course/user_invitations_controller.rb index 0ce9d15e9a9..4da0dfbe7fd 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] @@ -93,7 +105,7 @@ def resend_invitation_params # 1) Single invitation - specified with the user_invitation_id param # 2) All un-confirmed invitation - if user_invitation_id param was not found def load_invitations - @invitations ||= begin + @load_invitations ||= begin ids = resend_invitation_params ids ||= current_course.invitations.retryable.unconfirmed.select(:id) if ids.blank? @@ -118,12 +130,18 @@ def invite_by_file? # 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) + invitation_service.invite(invitation_params, + external_id_resolution: params[:external_id_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. 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..cf34d4d11a6 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 @@ -9,6 +9,8 @@ module Course::UserInvitationService::ParseInvitationConcern extend ActiveSupport::Autoload TRUE_VALUES = ['t', 'true', 'y', 'yes'].freeze + EXPECTED_HEADERS_WITH_TIMELINE = %w[name email role phantom timeline externalid].freeze + EXPECTED_HEADERS_WITHOUT_TIMELINE = %w[name email role phantom externalid].freeze private @@ -129,8 +131,13 @@ def parse_from_file(file) row_num = row_number row[0] = remove_utf8_byte_order_mark(row[0]) if row_number == 1 row = strip_row(row) - # Ignore first row if it's a header row. - next if row_number == 1 && header_row?(row) + if row_number == 1 && looks_like_header?(row) + unless valid_header_row?(row) + raise I18n.t('errors.course.user_invitations.invalid_headers', + expected: expected_headers.join(',')) + end + next + end invite = parse_file_row(row) invites << invite if invite @@ -140,12 +147,22 @@ def parse_from_file(file) 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 + def looks_like_header?(row) + row[0]&.casecmp('Name')&.zero? && row[1]&.casecmp('Email')&.zero? + end + + def expected_headers + if @current_course.show_personalized_timeline_features? + EXPECTED_HEADERS_WITH_TIMELINE + else + EXPECTED_HEADERS_WITHOUT_TIMELINE + end + end + + def valid_header_row?(row) + return false unless looks_like_header?(row) + + row.map { |h| h&.downcase }.compact == expected_headers end # Strips a row of whitespaces. @@ -164,10 +181,6 @@ def strip_row(row) def parse_file_row(row) return nil if row[1].blank? - if !@current_course.show_personalized_timeline_features? && row.length > 5 - raise I18n.t('errors.course.user_invitations.timeline_template_mismatch') - end - row[0] = row[1] if row[0].blank? role = parse_file_role(row[2]) phantom = parse_file_phantom(row[3]) 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..d0ab52b2c96 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) @@ -159,10 +162,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..21d3b659d9a 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,23 @@ 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 # Resends invitation emails to CourseUsers to the given course. @@ -64,11 +64,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 +110,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..16fa29d7887 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,17 @@ 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 } + | { + pendingInvitationUpdates: InvitationUpdatedItem[]; + pendingCourseUserUpdates: InvitationUpdatedItem[]; + } + > > { const config = { headers: { @@ -60,6 +68,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/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx b/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx index 56f8a9b1330..e6bc40956c9 100644 --- a/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx +++ b/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx @@ -1,11 +1,13 @@ -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'; @@ -20,6 +22,7 @@ import { getManageCourseUserPermissions, getManageCourseUsersSharedData, } from '../../selectors'; +import ExternalIdConflictPrompt from '../misc/ExternalIdConflictPrompt'; import IndividualInvitations from './IndividualInvitations'; @@ -60,6 +63,9 @@ const IndividualInviteForm: FC = (props) => { const { openResultDialog } = props; const { t } = useTranslation(); 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 +120,98 @@ const IndividualInviteForm: FC = (props) => { } }, [invitationsFields.length === 0]); - const onSubmit = (data: InvitationsPostData): Promise => { - setIsLoading(true); - return dispatch(inviteUsersFromForm(data)) + const handleError = (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(translations.failure, { error: first + overflow }), { + autoClose: false, + }); + } else { + toast.error(t(translations.failureGeneric), { autoClose: false }); + } + }; + + 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/__test__/ExternalIdConflictPrompt.test.tsx b/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx new file mode 100644 index 00000000000..ff5a8277394 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx @@ -0,0 +1,144 @@ +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('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/__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/operations.ts b/client/app/bundles/course/user-invitations/operations.ts index d9a0c409114..e4668a7602e 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,47 @@ 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; + }); } 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..d9b9daeb577 --- /dev/null +++ b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx @@ -0,0 +1,93 @@ +import { render, screen, waitFor } from 'test-utils'; +import { InvitationFileEntity } from 'types/course/userInvitations'; + +import InviteUsersFileUpload from '../index'; + +// 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; + +const MockFileUploadForm = ({ + open, + onSubmit, +}: { + open: boolean; + onSubmit: (data: { file: InvitationFileEntity }) => Promise; +}): JSX.Element | null => { + capturedOnSubmit = onSubmit; + return open ?