From 85a592d593a89c1211dfc5742f67695d2e88b800 Mon Sep 17 00:00:00 2001 From: lws49 Date: Tue, 26 May 2026 09:25:29 +0800 Subject: [PATCH 1/3] feat(invitations): add external ID to invite flow and redesign result dialog - add ext_id to CourseUser and UserInvitation with unique-per-course index - accept ext_id in bulk CSV and individual invite form; upsert for existing records (enrolled users and pending invitations) - conflicts (duplicate email or ext_id) surface in Failed with reasons --- .../course/user_invitations_controller.rb | 16 +- .../course/unique_external_id_concern.rb | 12 +- app/models/user.rb | 2 +- .../parse_invitation_concern.rb | 12 +- .../process_invitation_concern.rb | 46 +- ...essments_score_summary_download_service.rb | 2 +- .../course/user_invitation_service.rb | 29 +- .../_invitation_result_data.json.jbuilder | 44 +- .../user_invitations/index.json.jbuilder | 1 + .../bundles/course/enrol-requests/store.ts | 1 + .../misc/InvitationResultDialog.tsx | 398 ++++++++++----- .../__test__/InvitationResultDialog.test.tsx | 439 ++++++++++++++++ .../tables/InvitationResultExistingTable.tsx | 157 ++++++ .../tables/InvitationResultFailedTable.tsx | 163 ++++++ .../InvitationResultInvitationsTable.tsx | 152 ------ .../tables/InvitationResultPrimaryTable.tsx | 113 +++++ .../tables/InvitationResultUsersTable.tsx | 190 ------- .../tables/UserInvitationsTable.tsx | 10 +- .../InvitationResultExistingTable.test.tsx | 188 +++++++ .../InvitationResultFailedTable.test.tsx | 228 +++++++++ .../InvitationResultPrimaryTable.test.tsx | 112 +++++ .../InvitationsIndex/__test__/index.test.tsx | 80 +++ .../pages/InviteUsersFileUpload/index.tsx | 2 +- .../bundles/course/user-invitations/store.ts | 1 + client/app/types/course/courseUsers.ts | 1 + client/app/types/course/userInvitations.ts | 35 +- client/locales/en.json | 52 +- client/locales/ko.json | 42 +- client/locales/zh.json | 42 +- config/locales/en/errors.yml | 1 + config/locales/ko/errors.yml | 1 + config/locales/zh/csv.yml | 8 +- config/locales/zh/errors.yml | 1 + .../user_invitations_controller_spec.rb | 30 ++ spec/models/course_user_spec.rb | 25 +- spec/models/user_spec.rb | 14 + .../course/user_invitation_service_spec.rb | 474 ++++++++++++++---- 37 files changed, 2462 insertions(+), 662 deletions(-) create mode 100644 client/app/bundles/course/user-invitations/components/misc/__test__/InvitationResultDialog.test.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx delete mode 100644 client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx delete mode 100644 client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultExistingTable.test.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultFailedTable.test.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultPrimaryTable.test.tsx create mode 100644 client/app/bundles/course/user-invitations/pages/InvitationsIndex/__test__/index.test.tsx diff --git a/app/controllers/course/user_invitations_controller.rb b/app/controllers/course/user_invitations_controller.rb index 4e1e99bf2ff..0ce9d15e9a9 100644 --- a/app/controllers/course/user_invitations_controller.rb +++ b/app/controllers/course/user_invitations_controller.rb @@ -218,12 +218,16 @@ def invalid_invitations # Returns the invitation response based on file or entry invitation. def parse_invitation_result(new_invitations, existing_invitations, new_course_users, - existing_course_users, duplicate_users) - render_to_string(partial: 'invitation_result_data', locals: { new_invitations: new_invitations, - existing_invitations: existing_invitations, - new_course_users: new_course_users, - existing_course_users: existing_course_users, - duplicate_users: duplicate_users }) + existing_course_users, failed_users, + updated_invitations, updated_course_users) + render_to_string(partial: 'invitation_result_data', + locals: { new_invitations: new_invitations, + existing_invitations: existing_invitations, + new_course_users: new_course_users, + existing_course_users: existing_course_users, + failed_users: failed_users, + updated_invitations: updated_invitations, + updated_course_users: updated_course_users }) end # Enables or disables registration codes in the given course. diff --git a/app/models/concerns/course/unique_external_id_concern.rb b/app/models/concerns/course/unique_external_id_concern.rb index c06993a7649..7d271a39069 100644 --- a/app/models/concerns/course/unique_external_id_concern.rb +++ b/app/models/concerns/course/unique_external_id_concern.rb @@ -33,14 +33,14 @@ def validate_unique_external_id_within_course end def external_id_taken_by_invitation? - query = Course::UserInvitation.unconfirmed.where(course_id: course_id, external_id: external_id) - query = query.where.not(id: id) if is_a?(Course::UserInvitation) - query.exists? + scope = Course::UserInvitation.unconfirmed.where(course_id: course_id, external_id: external_id) + scope = scope.where.not(id: id) if is_a?(Course::UserInvitation) + scope.exists? end def external_id_taken_by_course_user? - query = CourseUser.where(course_id: course_id, external_id: external_id) - query = query.where.not(id: id) if is_a?(CourseUser) - query.exists? + scope = CourseUser.where(course_id: course_id, external_id: external_id) + scope = scope.where.not(id: id) if is_a?(CourseUser) + scope.exists? end end diff --git a/app/models/user.rb b/app/models/user.rb index 85923635741..75aae3fc495 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -129,7 +129,7 @@ def build_course_user_from_invitation(invitation) phantom: invitation.phantom, timeline_algorithm: invitation.timeline_algorithm || invitation.course&.default_timeline_algorithm, - external_id: invitation.external_id, + external_id: invitation.external_id.presence, creator: self, updater: self) 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 d65bf61ec92..ecc094b8e91 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 @@ -50,20 +50,20 @@ def partition_unique_users(users) seen_emails = Set.new seen_external_ids = Set.new unique_users = [] - duplicate_users = [] + failed_users = [] users.each do |user| ext_id = user[:external_id].presence if seen_emails.include?(user[:email]) - duplicate_users.push(user.merge(reason: :duplicate_email_in_file)) + failed_users.push(user.merge(reason: :duplicate_email_in_file)) elsif ext_id && seen_external_ids.include?(ext_id) - duplicate_users.push(user.merge(reason: :duplicate_external_id_in_file)) + failed_users.push(user.merge(reason: :duplicate_external_id_in_file)) else seen_emails.add(user[:email]) seen_external_ids.add(ext_id) if ext_id unique_users << user end end - [unique_users, duplicate_users] + [unique_users, failed_users] end # Change all invitees' roles to :student if inviter is a teaching_assistant. @@ -151,6 +151,10 @@ 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 a93b6831135..f256d240e8a 100644 --- a/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb +++ b/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb @@ -20,7 +20,7 @@ module Course::UserInvitationService::ProcessInvitationConcern # @return # [Array<(Array, Array, Array, Array)>] # A tuple containing the users newly invited, already invited, newly registered and already registered respectively. - # Conflicts are accumulated into +@duplicate_users+ as a side effect. + # Conflicts are accumulated into +@failed_users+ as a side effect. def process_invitations(users) @taken_external_ids = load_existing_external_ids augment_user_objects(users) @@ -67,7 +67,7 @@ def add_existing_users(users) new_course_users = [] users.each do |user| if (course_user = all_course_users[user[:user].id]) - existing_course_users << course_user + handle_existing_course_user(user, course_user, existing_course_users) else enroll_new_user(user, user[:external_id].presence, new_course_users) end @@ -75,9 +75,25 @@ def add_existing_users(users) [new_course_users, existing_course_users] end + 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) + @failed_users.push(user.merge(reason: :external_id_taken)) + else + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + course_user.external_id = csv_ext_id + @updated_course_users << { record: course_user, previous_external_id: current_ext_id } + end + end + def enroll_new_user(user, ext_id, new_course_users) if ext_id && @taken_external_ids.include?(ext_id) - @duplicate_users.push(user.merge(reason: :external_id_taken)) + @failed_users.push(user.merge(reason: :external_id_taken)) else @taken_external_ids.add(ext_id) if ext_id new_course_users << build_course_user(user) @@ -88,7 +104,7 @@ def enroll_new_user(user, ext_id, new_course_users) def build_course_user(user) @current_course.course_users.build(user: user[:user], name: user[:name], role: user[:role], phantom: user[:phantom], - timeline_algorithm: @current_course.default_timeline_algorithm, + timeline_algorithm: user[:timeline_algorithm], external_id: user[:external_id], creator: @current_user, updater: @current_user) end @@ -129,7 +145,7 @@ def invite_new_users(users) users.each do |user| invitation = all_invitations[user[:email]] if invitation - existing_invitations << invitation + handle_existing_invitation(user, invitation, existing_invitations) else add_to_new_invitations(user, user[:external_id].presence, new_invitations) end @@ -137,9 +153,27 @@ def invite_new_users(users) [new_invitations, existing_invitations] end + def handle_existing_invitation(user, invitation, existing_invitations) + csv_ext_id = user[:external_id].presence + current_ext_id = invitation.external_id.presence + + # 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) + @failed_users.push(user.merge(reason: :external_id_taken)) + else + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + invitation.external_id = csv_ext_id + @updated_invitations << { record: invitation, previous_external_id: current_ext_id } + end + end + def add_to_new_invitations(user, ext_id, new_invitations) if ext_id && @taken_external_ids.include?(ext_id) - @duplicate_users.push(user.merge(reason: :external_id_taken)) + @failed_users.push(user.merge(reason: :external_id_taken)) else @taken_external_ids.add(ext_id) if ext_id new_invitations << build_invitation(user) diff --git a/app/services/course/statistics/assessments_score_summary_download_service.rb b/app/services/course/statistics/assessments_score_summary_download_service.rb index f3375bbf6d0..88e98756c6a 100644 --- a/app/services/course/statistics/assessments_score_summary_download_service.rb +++ b/app/services/course/statistics/assessments_score_summary_download_service.rb @@ -75,7 +75,7 @@ def download_score_summary(csv) # content @all_students.each do |student| csv << [student.name, student.user.email, student.phantom? ? 'phantom' : 'normal', - *(@include_external_id ? [student.external_id || ''] : []), + *(@include_external_id ? [student.external_id.presence || ''] : []), *@assessments.flat_map { |a| @submission_grade_hash[[student.id, a.id]] || '' }] end end diff --git a/app/services/course/user_invitation_service.rb b/app/services/course/user_invitation_service.rb index eb77da78879..b7b39e4ddbc 100644 --- a/app/services/course/user_invitation_service.rb +++ b/app/services/course/user_invitation_service.rb @@ -24,28 +24,37 @@ def initialize(current_course_user, current_user, current_course) # because Rails does not handle duplicate nested attribute uniqueness constraints. # # @param [Array|File|TempFile] users Invites the given users. - # @return [Array|nil] An array containing the the size of new_invitations, existing_invitations, - # new_course_users and existing_course_users, duplicate_users respectively if success. nil when fail. + # @return [Array|nil] An array containing the size of new_invitations, existing_invitations, + # new_course_users, existing_course_users, failed_users, updated_invitations, updated_course_users + # respectively 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 - duplicate_users = nil + failed_users = nil + updated_invitations = nil + updated_course_users = nil success = Course.transaction do new_invitations, existing_invitations, - new_course_users, existing_course_users, duplicate_users = invite_users(users) + 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) true end - send_registered_emails(new_course_users) if success - send_invitation_emails(new_invitations) if success - success ? [new_invitations, existing_invitations, new_course_users, existing_course_users, duplicate_users] : nil + return unless success + + 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] end # Resends invitation emails to CourseUsers to the given course. @@ -76,7 +85,9 @@ def resend_invitation(invitations) # @raise [CSV::MalformedCSVError] When the file provided is invalid. def invite_users(users) unique_users, parse_duplicates = parse_invitations(users) - @duplicate_users = parse_duplicates - process_invitations(unique_users) + [@duplicate_users] + @failed_users = parse_duplicates + @updated_invitations = [] + @updated_course_users = [] + process_invitations(unique_users) + [@failed_users, @updated_invitations, @updated_course_users] end end diff --git a/app/views/course/user_invitations/_invitation_result_data.json.jbuilder b/app/views/course/user_invitations/_invitation_result_data.json.jbuilder index 421b05dc99d..19c26c655f2 100644 --- a/app/views/course/user_invitations/_invitation_result_data.json.jbuilder +++ b/app/views/course/user_invitations/_invitation_result_data.json.jbuilder @@ -8,6 +8,7 @@ json.newInvitations new_invitations.each do |invitation| json.role invitation.role json.phantom invitation.phantom json.sentAt invitation.sent_at + json.timelineAlgorithm invitation.timeline_algorithm end json.existingInvitations existing_invitations.each do |invitation| @@ -18,6 +19,8 @@ json.existingInvitations existing_invitations.each do |invitation| json.role invitation.role json.phantom invitation.phantom json.sentAt invitation.sent_at + json.isRetryable invitation.is_retryable + json.timelineAlgorithm invitation.timeline_algorithm end json.newCourseUsers new_course_users.each do |course_user| @@ -27,6 +30,7 @@ json.newCourseUsers new_course_users.each do |course_user| json.externalId course_user.external_id json.role course_user.role json.phantom course_user.phantom? + json.timelineAlgorithm course_user.timeline_algorithm end json.existingCourseUsers existing_course_users.each do |course_user| @@ -36,14 +40,40 @@ json.existingCourseUsers existing_course_users.each do |course_user| json.externalId course_user.external_id json.role course_user.role json.phantom course_user.phantom? + json.timelineAlgorithm course_user.timeline_algorithm end -json.duplicateUsers duplicate_users.each do |duplicate_user, index| +json.failedUsers failed_users.each.with_index do |failed_user, index| json.id index - json.name duplicate_user[:name] - json.email duplicate_user[:email] - json.externalId duplicate_user[:external_id] - json.role duplicate_user[:role] - json.phantom duplicate_user[:phantom] - json.reason duplicate_user[:reason] + json.name failed_user[:name] + json.email failed_user[:email] + json.externalId failed_user[:external_id] + json.role failed_user[:role] + json.phantom failed_user[:phantom] + json.reason failed_user[:reason] + json.timelineAlgorithm failed_user[:timeline_algorithm] +end + +json.updatedInvitations updated_invitations.each do |item| + inv = item[:record] + json.id inv.id + json.name inv.name + json.email inv.email + json.externalId inv.external_id + json.previousExternalId item[:previous_external_id] + json.role inv.role + json.phantom inv.phantom + json.timelineAlgorithm inv.timeline_algorithm +end + +json.updatedCourseUsers updated_course_users.each do |item| + cu = item[:record] + json.id cu.id if cu.id + json.name cu.name.strip + json.email cu.user.email + json.externalId cu.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/app/views/course/user_invitations/index.json.jbuilder b/app/views/course/user_invitations/index.json.jbuilder index f62c23a563c..17ea9e6b4cb 100644 --- a/app/views/course/user_invitations/index.json.jbuilder +++ b/app/views/course/user_invitations/index.json.jbuilder @@ -8,4 +8,5 @@ end json.manageCourseUsersData do json.partial! 'course/users/tabs_data', current_course: current_course json.defaultTimelineAlgorithm current_course.default_timeline_algorithm + json.showPersonalizedTimelineFeatures current_course.show_personalized_timeline_features end diff --git a/client/app/bundles/course/enrol-requests/store.ts b/client/app/bundles/course/enrol-requests/store.ts index c27fd35db95..d1fa07dbdb1 100644 --- a/client/app/bundles/course/enrol-requests/store.ts +++ b/client/app/bundles/course/enrol-requests/store.ts @@ -32,6 +32,7 @@ const initialState: EnrolRequestsState = { requestsCount: 0, invitationsCount: 0, defaultTimelineAlgorithm: 'fixed', + showPersonalizedTimelineFeatures: false, }, }; 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 8c6260c4434..7b7b4510f1a 100644 --- a/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx +++ b/client/app/bundles/course/user-invitations/components/misc/InvitationResultDialog.tsx @@ -1,5 +1,6 @@ import { FC } from 'react'; -import { defineMessages, injectIntl, WrappedComponentProps } from 'react-intl'; +import { defineMessages } from 'react-intl'; +import ErrorOutline from '@mui/icons-material/ErrorOutline'; import HelpIcon from '@mui/icons-material/Help'; import { Button, @@ -10,30 +11,31 @@ import { Tooltip, Typography, } from '@mui/material'; -import { InvitationResult } from 'types/course/userInvitations'; +import { CourseUserData } from 'types/course/courseUsers'; +import { + FailedInvitationRowData, + InvitationListData, + InvitationResult, + InvitationSuccessRow, + InvitationUpdatedItem, +} from 'types/course/userInvitations'; + +import { useAppSelector } from 'lib/hooks/store'; +import useTranslation from 'lib/hooks/useTranslation'; -import InvitationResultInvitationsTable from '../tables/InvitationResultInvitationsTable'; -import InvitationResultUsersTable from '../tables/InvitationResultUsersTable'; +import { getManageCourseUsersSharedData } from '../../selectors'; +import InvitationResultExistingTable, { + ExistingRow, +} from '../tables/InvitationResultExistingTable'; +import InvitationResultFailedTable from '../tables/InvitationResultFailedTable'; +import InvitationResultPrimaryTable from '../tables/InvitationResultPrimaryTable'; -interface Props extends WrappedComponentProps { +interface Props { open: boolean; handleClose: () => void; invitationResult: InvitationResult; } -const styles = { - icon: { - fontSize: '16px', - marginRight: '4px', - }, - dialogStyle: { - top: 40, - '& .MuiDialog-paper': { - overflowY: 'hidden', - }, - }, -}; - const translations = defineMessages({ header: { id: 'course.userInvitations.InvitationResultDialog.header', @@ -43,67 +45,147 @@ const translations = defineMessages({ id: 'course.userInvitations.InvitationResultDialog.close', defaultMessage: 'Close', }, - body: { - id: 'course.userInvitations.InvitationResultDialog.body', + summary: { + id: 'course.userInvitations.InvitationResultDialog.summary', defaultMessage: - '{newInvitationsCount, plural, =0 {No new users were} one {# new user has been} other {# new users have been}} invited to Coursemology. ' + - '{newCourseUsersCount, plural, =0 {No user with Coursemology account has been} one {# new user with existing Coursemology account has been} other {# new users with existing Coursemology accounts have been}} added to this course.', + '{newInvitations} new {newInvitations, plural, one {invitation} other {invitations}} sent, {newEnrollments} directly enrolled, {alreadyInCourse} already in course.', + }, + summaryFailed: { + id: 'course.userInvitations.InvitationResultDialog.summaryFailed', + defaultMessage: '{count} failed.', + }, + actionableTitle: { + id: 'course.userInvitations.InvitationResultDialog.actionableTitle', + defaultMessage: 'Failed ({count})', }, - duplicateInfo: { - id: 'course.userInvitations.InvitationResultDialog.duplicateInfo', + failedRowsSubtitle: { + id: 'course.userInvitations.InvitationResultDialog.failedRowsSubtitle', defaultMessage: - 'Duplicate users were found in the invitation. Only the first instance of each user will be invited.', + '{count} {count, plural, one {row} other {rows}} highlighted in red could not be sent', }, - duplicateUsers: { - id: 'course.userInvitations.InvitationResultDialog.duplicateUsers', - defaultMessage: 'Duplicate Users ({count})', + newInvitations: { + id: 'course.userInvitations.InvitationResultDialog.newInvitations', + defaultMessage: 'New Invitations ({count})', }, - existingCourseUsersInfo: { - id: 'course.userInvitations.InvitationResultDialog.existingCourseUsersInfo', + newCourseUsers: { + id: 'course.userInvitations.InvitationResultDialog.newCourseUsers', + defaultMessage: 'New Course Users ({count})', + }, + existingInvitations: { + id: 'course.userInvitations.InvitationResultDialog.existingInvitations', + defaultMessage: 'Existing Invitations ({count})', + }, + existingInvitationsInfo: { + id: 'course.userInvitations.InvitationResultDialog.existingInvitationsInfo', defaultMessage: - 'Existing course users with this email were found in the invitation. They were not invited.', + 'These users already have a pending invitation. They were not re-invited.', }, existingCourseUsers: { id: 'course.userInvitations.InvitationResultDialog.existingCourseUsers', defaultMessage: 'Existing Course Users ({count})', }, - existingInvitationsInfo: { - id: 'course.userInvitations.InvitationResultDialog.existingInvitationsInfo', + existingCourseUsersInfo: { + id: 'course.userInvitations.InvitationResultDialog.existingCourseUsersInfo', defaultMessage: - 'Existing invitations for these users with this email already exist. They were not invited.', + 'These users are already enrolled in this course. They were not re-enrolled.', }, - existingInvitations: { - id: 'course.userInvitations.InvitationResultDialog.existingInvitations', - defaultMessage: 'Existing Invitations ({count})', + externalIdUpdatedInfo: { + id: 'course.userInvitations.InvitationResultDialog.externalIdUpdatedInfo', + defaultMessage: 'External IDs were updated where specified.', }, - newCourseUsers: { - id: 'course.userInvitations.InvitationResultDialog.newCourseUsers', - defaultMessage: 'New Course Users ({count})', - }, - newInvitations: { - id: 'course.userInvitations.InvitationResultDialog.newInvitations', - defaultMessage: 'New Invitations ({count})', + updatedSubtitle: { + id: 'course.userInvitations.InvitationResultDialog.updatedSubtitle', + defaultMessage: '{count} updated · shown first', }, }); -const InvitationResultDialog: FC = (props) => { - const { open, handleClose, invitationResult, intl } = props; +const toSuccessRow = ( + item: InvitationListData | CourseUserData, + prefix: string, +): InvitationSuccessRow => ({ + id: `${prefix}-${item.id}`, + name: item.name, + email: item.email, + externalId: item.externalId ?? null, + role: item.role ?? '', + phantom: item.phantom ?? false, + timelineAlgorithm: item.timelineAlgorithm, +}); + +const toUpdatedExistingRow = (item: InvitationUpdatedItem): ExistingRow => ({ + id: item.id, + name: item.name, + email: item.email, + externalId: item.externalId, + previousExternalId: item.previousExternalId, + role: item.role, + phantom: item.phantom, + timelineAlgorithm: item.timelineAlgorithm, +}); + +const InvitationResultDialog: FC = ({ + open, + handleClose, + invitationResult, +}) => { + const { t } = useTranslation(); + const { showPersonalizedTimelineFeatures } = useAppSelector( + getManageCourseUsersSharedData, + ); + + if (!open) return null; + const { - duplicateUsers, - existingCourseUsers, - existingInvitations, - newCourseUsers, - newInvitations, + newInvitations = [], + newCourseUsers = [], + existingInvitations = [], + existingCourseUsers = [], + failedUsers = [], + updatedInvitations = [], + updatedCourseUsers = [], } = invitationResult; - if (!open) { - return null; - } + const newInvitationRows = newInvitations.map((i) => toSuccessRow(i, 'inv')); + const newCourseUserRows = newCourseUsers.map((u) => toSuccessRow(u, 'cu')); + + const failedInvitations = existingInvitations.filter( + (i) => i.isRetryable === false, + ); + const normalExistingInvitations = existingInvitations.filter( + (i) => i.isRetryable !== false, + ); + + const failedToSendRows: FailedInvitationRowData[] = failedInvitations.map( + (inv) => ({ + id: inv.id, + name: inv.name, + email: inv.email, + externalId: inv.externalId ?? undefined, + role: inv.role, + phantom: inv.phantom, + reason: 'failed_to_send' as const, + timelineAlgorithm: inv.timelineAlgorithm, + }), + ); + + const allFailedUsers: FailedInvitationRowData[] = [ + ...failedToSendRows, + ...failedUsers, + ]; + + const existingInvitationRows: ExistingRow[] = [ + ...normalExistingInvitations, + ...updatedInvitations.map(toUpdatedExistingRow), + ]; + const existingCourseUserRows: ExistingRow[] = [ + ...existingCourseUsers, + ...updatedCourseUsers.map(toUpdatedExistingRow), + ]; + + const needsAttentionCount = failedUsers.length + failedInvitations.length; const handleDialogClose = (_event: object, reason: string): void => { - if (reason !== 'backdropClick') { - handleClose(); - } + if (reason !== 'backdropClick') handleClose(); }; return ( @@ -113,113 +195,153 @@ const InvitationResultDialog: FC = (props) => { maxWidth="lg" onClose={handleDialogClose} open={open} - sx={styles.dialogStyle} + sx={{ top: 40, '& .MuiDialog-paper': { overflowY: 'auto' } }} > - {intl.formatMessage(translations.header)} + {t(translations.header)} - {intl.formatMessage(translations.body, { - newInvitationsCount: newInvitations?.length ?? 0, - newCourseUsersCount: newCourseUsers?.length ?? 0, + {needsAttentionCount > 0 && + `${t(translations.summaryFailed, { count: needsAttentionCount })} `} + {t(translations.summary, { + newInvitations: newInvitations.length, + newEnrollments: newCourseUsers.length, + alreadyInCourse: + existingInvitationRows.length + existingCourseUserRows.length, })} - {duplicateUsers && duplicateUsers.length > 0 && ( -
- - - - - {intl.formatMessage(translations.duplicateUsers, { - count: duplicateUsers.length, - })} - + + {needsAttentionCount > 0 && ( +
+ + + {t(translations.actionableTitle, { + count: needsAttentionCount, + })} + + {failedInvitations.length > 0 && ( + + {t(translations.failedRowsSubtitle, { + count: failedInvitations.length, + })} + + )} + -
+ )} - {existingInvitations && existingInvitations.length > 0 && ( -
- - - - - {intl.formatMessage(translations.existingInvitations, { - count: existingInvitations.length, - })} - + + {newInvitationRows.length > 0 && ( +
+ + {t(translations.newInvitations, { + count: newInvitationRows.length, + })} + + -
+ )} - {existingCourseUsers && existingCourseUsers.length > 0 && ( -
- - - - - {intl.formatMessage(translations.existingCourseUsers, { - count: existingCourseUsers.length, - })} - + + {newCourseUserRows.length > 0 && ( +
+ + {t(translations.newCourseUsers, { + count: newCourseUserRows.length, + })} + + -
+ )} - {newInvitations && newInvitations.length > 0 && ( -
- - {intl.formatMessage(translations.newInvitations, { - count: newInvitations.length, - })} - + + {existingInvitationRows.length > 0 && ( +
+ + 0 + ? [t(translations.externalIdUpdatedInfo)] + : []), + ].join(' ')} + > + + + {t(translations.existingInvitations, { + count: existingInvitationRows.length, + })} + + {updatedInvitations.length > 0 && ( + + {t(translations.updatedSubtitle, { + count: updatedInvitations.length, + })} + + )} + -
+ )} - {newCourseUsers && newCourseUsers.length > 0 && ( -
- - {intl.formatMessage(translations.newCourseUsers, { - count: newCourseUsers.length, - })} - + + {existingCourseUserRows.length > 0 && ( +
+ + 0 + ? [t(translations.externalIdUpdatedInfo)] + : []), + ].join(' ')} + > + + + {t(translations.existingCourseUsers, { + count: existingCourseUserRows.length, + })} + + {updatedCourseUsers.length > 0 && ( + + {t(translations.updatedSubtitle, { + count: updatedCourseUsers.length, + })} + + )} + -
+ )}
); }; -export default injectIntl(InvitationResultDialog); +export default InvitationResultDialog; diff --git a/client/app/bundles/course/user-invitations/components/misc/__test__/InvitationResultDialog.test.tsx b/client/app/bundles/course/user-invitations/components/misc/__test__/InvitationResultDialog.test.tsx new file mode 100644 index 00000000000..2c460cf1a25 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/misc/__test__/InvitationResultDialog.test.tsx @@ -0,0 +1,439 @@ +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { CourseUserData } from 'types/course/courseUsers'; +import { + FailedInvitationRowData, + InvitationListData, + InvitationResult, + InvitationUpdatedItem, +} from 'types/course/userInvitations'; + +import InvitationResultDialog from '../InvitationResultDialog'; + +const CAROL_EMAIL = 'carol@example.com'; + +const carolDuplicateUser: FailedInvitationRowData = { + id: 3, + name: 'Carol', + email: CAROL_EMAIL, + role: 'student', + reason: 'duplicate_email_in_file', +}; + +const baseInvitation: InvitationListData = { + id: 1, + name: 'Alice', + email: 'alice@example.com', + externalId: null, + role: 'student', + phantom: false, + invitationKey: 'abc', + confirmed: false, + sentAt: null, + confirmedAt: null, + isRetryable: true, +}; + +const failedInvitation: InvitationListData = { + ...baseInvitation, + id: 99, + name: 'FailedUser', + email: 'failed@example.com', + isRetryable: false, +}; + +const baseCourseUser = { + id: 2, + name: 'Bob', + email: 'bob@example.com', + externalId: null, + role: 'student', + phantom: false, + level: 0, + exp: 0, + canReadStatistics: false, +} as CourseUserData; + +const noop = (): void => {}; + +const renderDialog = (result: InvitationResult): void => { + render( + , + ); +}; + +describe('InvitationResultDialog', () => { + // Lifecycle + it('renders nothing when closed', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); + + it('hides all optional sections when data is empty', async () => { + renderDialog({}); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/Failed/)).not.toBeInTheDocument(); + expect(screen.queryByText(/New Invitations/)).not.toBeInTheDocument(); + expect(screen.queryByText(/New Course Users/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Existing Invitations/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Existing Course Users/)).not.toBeInTheDocument(); + }); + + // Summary line + it('shows summary text with correct counts', async () => { + renderDialog({ + newInvitations: [baseInvitation], + newCourseUsers: [baseCourseUser], + existingInvitations: [{ ...baseInvitation, id: 3 }], + existingCourseUsers: [{ ...baseCourseUser, id: 4 }], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.getByText( + /1 new invitation sent, 1 directly enrolled, 2 already in course/, + ), + ).toBeInTheDocument(); + }); + + it('prepends failed count to summary when failures present', async () => { + renderDialog({ failedUsers: [carolDuplicateUser] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText(/1 failed\./)).toBeInTheDocument(); + }); + + it('does not prepend failed count to summary when count is zero', async () => { + renderDialog({ newInvitations: [baseInvitation] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/failed\./)).not.toBeInTheDocument(); + }); + + // Failed section + it('shows Failed section when failedUsers is non-empty', async () => { + renderDialog({ failedUsers: [carolDuplicateUser] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Failed (1)')).toBeInTheDocument(); + expect(screen.getByText('Carol')).toBeInTheDocument(); + }); + + it('renders Failed before New Invitations in DOM', async () => { + renderDialog({ + newInvitations: [baseInvitation], + failedUsers: [carolDuplicateUser], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const headings = screen + .getAllByRole('heading') + .map((h) => h.textContent ?? ''); + const needsIdx = headings.findIndex((h) => h.includes('Failed')); + const newInvIdx = headings.findIndex((h) => h.includes('New Invitations')); + expect(needsIdx).toBeGreaterThanOrEqual(0); + expect(newInvIdx).toBeGreaterThanOrEqual(0); + expect(needsIdx).toBeLessThan(newInvIdx); + }); + + it('shows failed invitation in Failed, not Existing Invitations', async () => { + renderDialog({ existingInvitations: [failedInvitation] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Failed (1)')).toBeInTheDocument(); + expect(screen.getByText('FailedUser')).toBeInTheDocument(); + expect( + screen.getByText( + 'Failed to send invitation email, please try again - if failures persist, contact us for assistance', + ), + ).toBeInTheDocument(); + expect(screen.queryByText(/Existing Invitations/)).not.toBeInTheDocument(); + }); + + it('shows failed invitation with non-null externalId in Failed, not Existing Invitations', async () => { + const failedWithExtId: InvitationListData = { + ...baseInvitation, + id: 100, + name: 'FailedWithExtId', + email: 'failedext@example.com', + externalId: 'old-id', + isRetryable: false, + }; + renderDialog({ existingInvitations: [failedWithExtId] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Failed (1)')).toBeInTheDocument(); + expect(screen.getByText('FailedWithExtId')).toBeInTheDocument(); + expect(screen.queryByText(/Existing Invitations/)).not.toBeInTheDocument(); + }); + + it('keeps retryable existing invitation in Existing Invitations section', async () => { + renderDialog({ + existingInvitations: [{ ...baseInvitation, id: 7, name: 'Retryable' }], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Existing Invitations (1)')).toBeInTheDocument(); + expect(screen.getByText('Retryable')).toBeInTheDocument(); + expect(screen.queryByText(/Failed/)).not.toBeInTheDocument(); + }); + + it('does not count failed invitations in alreadyInCourse summary', async () => { + renderDialog({ + existingInvitations: [failedInvitation], + existingCourseUsers: [baseCourseUser], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.getByText( + /0 new invitations sent, 0 directly enrolled, 1 already in course/, + ), + ).toBeInTheDocument(); + }); + + it('shows failedRowsSubtitle when failed_to_send rows exist', async () => { + renderDialog({ existingInvitations: [failedInvitation] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.getByText('1 row highlighted in red could not be sent'), + ).toBeInTheDocument(); + }); + + it('does not show failedRowsSubtitle when only duplicate users exist', async () => { + renderDialog({ failedUsers: [carolDuplicateUser] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/highlighted in red/)).not.toBeInTheDocument(); + }); + + it('shows failedRowsSubtitle with failed_to_send count only when mixed failures', async () => { + renderDialog({ + existingInvitations: [failedInvitation], + failedUsers: [carolDuplicateUser], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Failed (2)')).toBeInTheDocument(); + expect( + screen.getByText('1 row highlighted in red could not be sent'), + ).toBeInTheDocument(); + }); + + // New Invitations section + it('shows New Invitations section when newInvitations is non-empty', async () => { + renderDialog({ newInvitations: [baseInvitation] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('New Invitations (1)')).toBeInTheDocument(); + expect(screen.getByText('Alice')).toBeInTheDocument(); + }); + + it('hides New Invitations section when empty', async () => { + renderDialog({ newInvitations: [] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/New Invitations/)).not.toBeInTheDocument(); + }); + + // New Course Users section + it('shows New Course Users section when non-empty', async () => { + renderDialog({ newCourseUsers: [baseCourseUser] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('New Course Users (1)')).toBeInTheDocument(); + expect(screen.getByText('Bob')).toBeInTheDocument(); + }); + + it('hides New Course Users section when empty', async () => { + renderDialog({ newCourseUsers: [] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/New Course Users/)).not.toBeInTheDocument(); + }); + + // Existing Invitations section + it('shows Existing Invitations section with name visible', async () => { + renderDialog({ + existingInvitations: [{ ...baseInvitation, id: 5, name: 'Charlie' }], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Existing Invitations (1)')).toBeInTheDocument(); + expect(screen.getByText('Charlie')).toBeInTheDocument(); + }); + + it('hides Existing Invitations section when both existingInvitations and updatedInvitations are empty', async () => { + renderDialog({ newInvitations: [baseInvitation] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/Existing Invitations/)).not.toBeInTheDocument(); + }); + + it('shows updatedSubtitle in Existing Invitations when updatedInvitations is non-empty', async () => { + const updatedItem: InvitationUpdatedItem = { + id: 10, + name: 'Carol', + email: CAROL_EMAIL, + externalId: 'EXT001', + previousExternalId: null, + role: 'student', + phantom: false, + }; + renderDialog({ updatedInvitations: [updatedItem] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Existing Invitations (1)')).toBeInTheDocument(); + expect(screen.getByText('1 updated · shown first')).toBeInTheDocument(); + }); + + it('does not show updatedSubtitle when updatedInvitations is empty', async () => { + renderDialog({ + existingInvitations: [{ ...baseInvitation, id: 5, name: 'Charlie' }], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/updated · shown first/)).not.toBeInTheDocument(); + }); + + it('shows combined count, updated rows before normal rows, and correct alreadyInCourse when both existingInvitations and updatedInvitations are present', async () => { + const updatedItem: InvitationUpdatedItem = { + id: 10, + name: 'UpdatedAlice', + email: 'updated@example.com', + externalId: 'NEW001', + previousExternalId: 'OLD001', + role: 'student', + phantom: false, + }; + renderDialog({ + existingInvitations: [ + { ...baseInvitation, id: 5, name: 'NormalCharlie' }, + ], + updatedInvitations: [updatedItem], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Existing Invitations (2)')).toBeInTheDocument(); + expect(screen.getByText('1 updated · shown first')).toBeInTheDocument(); + expect( + screen.getByText( + /0 new invitations sent, 0 directly enrolled, 2 already in course/, + ), + ).toBeInTheDocument(); + const normalRow = screen.getByText('NormalCharlie'); + const updatedRow = screen.getByText('UpdatedAlice'); + expect( + // eslint-disable-next-line no-bitwise + normalRow.compareDocumentPosition(updatedRow) & + Node.DOCUMENT_POSITION_PRECEDING, + ).toBeTruthy(); + }); + + // Existing Course Users section + it('shows Existing Course Users section with name visible', async () => { + renderDialog({ + existingCourseUsers: [{ ...baseCourseUser, id: 6, name: 'Diana' }], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Existing Course Users (1)')).toBeInTheDocument(); + expect(screen.getByText('Diana')).toBeInTheDocument(); + }); + + it('hides Existing Course Users section when empty', async () => { + renderDialog({ newInvitations: [baseInvitation] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText(/Existing Course Users/)).not.toBeInTheDocument(); + }); + + it('shows updatedSubtitle for Existing Course Users when updatedCourseUsers is non-empty', async () => { + const updatedUser = { + id: 20, + name: 'Dana', + email: 'dana@example.com', + externalId: 'CU001', + previousExternalId: 'CU000', + role: 'student', + phantom: false, + }; + renderDialog({ updatedCourseUsers: [updatedUser] }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Existing Course Users (1)')).toBeInTheDocument(); + expect(screen.getByText('1 updated · shown first')).toBeInTheDocument(); + }); + + it('shows combined count, updated rows before normal rows, and correct alreadyInCourse when both existingCourseUsers and updatedCourseUsers are present', async () => { + const updatedUser = { + id: 20, + name: 'UpdatedDana', + email: 'updated-dana@example.com', + externalId: 'CU001', + previousExternalId: 'CU000', + role: 'student', + phantom: false, + }; + renderDialog({ + existingCourseUsers: [{ ...baseCourseUser, id: 6, name: 'NormalEve' }], + updatedCourseUsers: [updatedUser], + }); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Existing Course Users (2)')).toBeInTheDocument(); + expect(screen.getByText('1 updated · shown first')).toBeInTheDocument(); + expect( + screen.getByText( + /0 new invitations sent, 0 directly enrolled, 2 already in course/, + ), + ).toBeInTheDocument(); + const normalRow = screen.getByText('NormalEve'); + const updatedRow = screen.getByText('UpdatedDana'); + expect( + // eslint-disable-next-line no-bitwise + normalRow.compareDocumentPosition(updatedRow) & + Node.DOCUMENT_POSITION_PRECEDING, + ).toBeTruthy(); + }); + + describe('Personalized Timeline column (showPersonalizedTimelineFeatures)', () => { + const storeWithTimelines = { + 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: '', + }, + }; + + it('shows Personalized Timeline column in new invitations table when feature is on', async () => { + render( + , + { state: storeWithTimelines }, + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + expect(screen.getByText('Otot')).toBeInTheDocument(); + }); + + it('hides Personalized Timeline column when feature is off', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText('Personalized Timeline'), + ).not.toBeInTheDocument(); + }); + }); +}); diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx new file mode 100644 index 00000000000..d93dd09f951 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx @@ -0,0 +1,157 @@ +import { FC, ReactNode } from 'react'; +import { defineMessages } from 'react-intl'; +import { Tooltip } from '@mui/material'; +import { TimelineAlgorithm } from 'types/course/personalTimes'; + +import { ColumnTemplate } from 'lib/components/table'; +import Table from 'lib/components/table/Table'; +import { + DEFAULT_TABLE_ROWS_PER_PAGE, + TIMELINE_ALGORITHMS, +} from 'lib/constants/sharedConstants'; +import useTranslation from 'lib/hooks/useTranslation'; +import roleTranslations from 'lib/translations/course/users/roles'; +import tableTranslations from 'lib/translations/table'; + +const translations = defineMessages({ + yes: { + id: 'course.userInvitations.InvitationResultExistingTable.yes', + defaultMessage: 'Yes', + }, + no: { + id: 'course.userInvitations.InvitationResultExistingTable.no', + defaultMessage: 'No', + }, + previouslyLabel: { + id: 'course.userInvitations.InvitationResultExistingTable.previouslyLabel', + defaultMessage: 'Previously: {value}', + }, +}); + +export interface ExistingRow { + id: number; + name: string; + email: string; + externalId?: string | null; + role?: string; + phantom?: boolean; + previousExternalId?: string | null; + timelineAlgorithm?: TimelineAlgorithm; +} + +interface Props { + rows: ExistingRow[]; + showPersonalizedTimelineFeatures?: boolean; +} + +const InvitationResultExistingTable: FC = ({ + rows, + showPersonalizedTimelineFeatures, +}) => { + const { t } = useTranslation(); + + if (rows.length === 0) return null; + + // Updated rows (ext_id changed) first so admins notice them above unchanged existing rows + const orderedRows = [ + ...rows.filter((r) => r.previousExternalId !== undefined), + ...rows.filter((r) => r.previousExternalId === undefined), + ]; + + const showExternalId = rows.some( + (r) => r.externalId != null || r.previousExternalId !== undefined, + ); + + const columns: ColumnTemplate[] = [ + { + of: 'name', + title: t(tableTranslations.name), + sortable: false, + cell: (row) => row.name, + csvDownloadable: true, + }, + { + of: 'email', + title: t(tableTranslations.email), + sortable: false, + cell: (row) => row.email, + csvDownloadable: true, + }, + ...(showExternalId + ? ([ + { + of: 'externalId', + title: t(tableTranslations.externalId), + sortable: false, + cell: (row): ReactNode => { + if (row.previousExternalId === undefined) + return row.externalId ?? ''; + const previousLabel = t(translations.previouslyLabel, { + value: row.previousExternalId ?? '—', + }); + return ( + + + {row.externalId} + + + ); + }, + csvDownloadable: true, + }, + ] as ColumnTemplate[]) + : []), + { + of: 'role', + title: t(tableTranslations.role), + sortable: false, + cell: (row): ReactNode => { + if (!row.role) return ''; + const desc = + roleTranslations[row.role as keyof typeof roleTranslations]; + return desc ? t(desc) : row.role; + }, + csvDownloadable: true, + }, + { + of: 'phantom', + title: t(tableTranslations.phantom), + sortable: false, + cell: (row) => (row.phantom ? t(translations.yes) : t(translations.no)), + csvDownloadable: true, + }, + ...(showPersonalizedTimelineFeatures + ? ([ + { + of: 'timelineAlgorithm', + title: t(tableTranslations.personalizedTimeline), + sortable: false, + cell: (row): ReactNode => + TIMELINE_ALGORITHMS.find( + (tl) => tl.value === row.timelineAlgorithm, + )?.label ?? '-', + csvDownloadable: true, + }, + ] as ColumnTemplate[]) + : []), + ]; + + return ( + + row.previousExternalId !== undefined ? 'bg-[#e3f2fd]' : '' + } + getRowEqualityData={(row) => row} + getRowId={(row) => String(row.id)} + pagination={{ + rowsPerPage: [DEFAULT_TABLE_ROWS_PER_PAGE], + showAllRows: true, + }} + toolbar={{ show: false }} + /> + ); +}; + +export default InvitationResultExistingTable; diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx new file mode 100644 index 00000000000..83ba8eae886 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx @@ -0,0 +1,163 @@ +import { FC, memo, ReactNode } from 'react'; +import { defineMessages } from 'react-intl'; +import equal from 'fast-deep-equal'; +import { + FailedInvitationRowData, + InvitationFailureReason, +} from 'types/course/userInvitations'; + +import { ColumnTemplate } from 'lib/components/table'; +import Table from 'lib/components/table/Table'; +import { TIMELINE_ALGORITHMS } from 'lib/constants/sharedConstants'; +import useTranslation from 'lib/hooks/useTranslation'; +import roleTranslations from 'lib/translations/course/users/roles'; +import tableTranslations from 'lib/translations/table'; + +const translations = defineMessages({ + duplicateEmailInFile: { + id: 'course.userInvitations.InvitationResultFailedTable.duplicateEmailInFile', + defaultMessage: 'Duplicate email in uploaded CSV', + }, + duplicateExternalIdInFile: { + id: 'course.userInvitations.InvitationResultFailedTable.duplicateExternalIdInFile', + defaultMessage: 'Duplicate external ID in uploaded CSV', + }, + externalIdTaken: { + id: 'course.userInvitations.InvitationResultFailedTable.externalIdTaken', + defaultMessage: 'External ID is taken by another course member', + }, + failedToSend: { + id: 'course.userInvitations.InvitationResultFailedTable.failedToSend', + defaultMessage: + 'Failed to send invitation email, please try again - if failures persist, contact us for assistance', + }, + yes: { + id: 'course.userInvitations.InvitationResultFailedTable.yes', + defaultMessage: 'Yes', + }, + no: { + id: 'course.userInvitations.InvitationResultFailedTable.no', + defaultMessage: 'No', + }, +}); + +interface Props { + users: FailedInvitationRowData[]; + showPersonalizedTimelineFeatures?: boolean; +} + +const REASON_MAP: Record = { + duplicate_email_in_file: 'duplicateEmailInFile', + duplicate_external_id_in_file: 'duplicateExternalIdInFile', + external_id_taken: 'externalIdTaken', + failed_to_send: 'failedToSend', +}; + +const InvitationResultFailedTable: FC = ({ + users, + showPersonalizedTimelineFeatures, +}) => { + const { t } = useTranslation(); + + if (users.length === 0) return null; + + const showExternalId = users.some((u) => u.externalId != null); + + const columns: ColumnTemplate[] = [ + { + of: 'name', + title: t(tableTranslations.name), + sortable: false, + searchable: true, + cell: (row) => row.name, + csvDownloadable: true, + }, + { + of: 'email', + title: t(tableTranslations.email), + sortable: false, + searchable: true, + cell: (row) => row.email, + csvDownloadable: true, + }, + ...(showExternalId + ? ([ + { + of: 'externalId', + title: t(tableTranslations.externalId), + sortable: false, + searchable: false, + cell: (row) => row.externalId ?? '', + csvDownloadable: true, + }, + ] as ColumnTemplate[]) + : []), + { + of: 'role', + title: t(tableTranslations.role), + sortable: false, + searchable: false, + cell: (row): ReactNode => { + if (!row.role) return ''; + const desc = + roleTranslations[row.role as keyof typeof roleTranslations]; + return desc ? t(desc) : row.role; + }, + csvDownloadable: true, + }, + { + of: 'phantom', + title: t(tableTranslations.phantom), + sortable: false, + searchable: false, + cell: (row) => (row.phantom ? t(translations.yes) : t(translations.no)), + csvDownloadable: true, + }, + ...(showPersonalizedTimelineFeatures + ? ([ + { + of: 'timelineAlgorithm', + title: t(tableTranslations.personalizedTimeline), + sortable: false, + searchable: false, + cell: (row): ReactNode => + TIMELINE_ALGORITHMS.find( + (tl) => tl.value === row.timelineAlgorithm, + )?.label ?? '-', + csvDownloadable: true, + }, + ] as ColumnTemplate[]) + : []), + { + of: 'reason', + title: t(tableTranslations.reason), + sortable: false, + searchable: false, + cell: (row): ReactNode => { + return t(translations[REASON_MAP[row.reason]]); + }, + csvDownloadable: true, + }, + ]; + + return ( +
+ row.reason === 'failed_to_send' ? 'bg-[#ffebee]' : '' + } + getRowEqualityData={(row) => row} + getRowId={(row) => String(row.id)} + toolbar={{ show: false }} + /> + ); +}; + +export default memo( + InvitationResultFailedTable, + (prev, next) => + equal(prev.users, next.users) && + prev.showPersonalizedTimelineFeatures === + next.showPersonalizedTimelineFeatures, +); diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx deleted file mode 100644 index fb70f544196..00000000000 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultInvitationsTable.tsx +++ /dev/null @@ -1,152 +0,0 @@ -import { FC, memo } from 'react'; -import { Typography } from '@mui/material'; -import equal from 'fast-deep-equal'; -import { TableColumns, TableOptions } from 'types/components/DataTable'; -import { InvitationListData } from 'types/course/userInvitations'; - -import DataTable from 'lib/components/core/layouts/DataTable'; -import { DEFAULT_TABLE_ROWS_PER_PAGE } from 'lib/constants/sharedConstants'; -import useTranslation from 'lib/hooks/useTranslation'; -import roleTranslations from 'lib/translations/course/users/roles'; -import tableTranslations from 'lib/translations/table'; - -interface Props { - title: JSX.Element; - invitations: InvitationListData[]; -} - -const InvitationResultInvitationsTable: FC = (props) => { - const { title, invitations } = props; - const { t } = useTranslation(); - - if (invitations && invitations.length === 0) return null; - - const showExternalId = invitations.some((i) => i.externalId != null); - - const options: TableOptions = { - download: true, - filter: false, - pagination: true, - print: false, - rowsPerPage: DEFAULT_TABLE_ROWS_PER_PAGE, - rowsPerPageOptions: [DEFAULT_TABLE_ROWS_PER_PAGE], - search: false, - selectableRows: 'none', - setTableProps: (): object => { - return { size: 'small' }; - }, - setRowProps: (_row, dataIndex, _rowIndex): Record => { - return { - key: `invitation_result_invitation_${invitations[dataIndex].id}`, - invitationid: `invitation_result_invitation_${invitations[dataIndex].id}`, - className: `invitation_result_invitation invitation_result_invitation_${invitations[dataIndex].id}`, - }; - }, - viewColumns: false, - }; - - const columns: TableColumns[] = [ - { - name: 'id', - label: t(tableTranslations.id), - options: { - display: false, - filter: false, - sort: false, - }, - }, - { - name: 'name', - label: t(tableTranslations.name), - options: { - alignCenter: false, - sort: false, - }, - }, - { - name: 'email', - label: t(tableTranslations.email), - options: { - alignCenter: false, - sort: false, - }, - }, - ...(showExternalId - ? [ - { - name: 'externalId', - label: t(tableTranslations.externalId), - options: { - alignCenter: true, - sort: false, - }, - }, - ] - : []), - { - name: 'phantom', - label: t(tableTranslations.phantom), - options: { - sort: false, - customBodyRenderLite: (dataIndex): JSX.Element => { - const invitation = invitations[dataIndex]; - return ( - - {invitation.phantom ? 'Yes' : 'No'} - - ); - }, - }, - }, - { - name: 'role', - label: t(tableTranslations.role), - options: { - alignCenter: false, - sort: false, - customBodyRenderLite: (dataIndex): JSX.Element => { - const invitation = invitations[dataIndex]; - return ( - - {t(roleTranslations[invitation.role])} - - ); - }, - }, - }, - { - name: 'sentAt', - label: t(tableTranslations.invitationSentAt), - options: { - alignCenter: false, - sort: false, - }, - }, - ]; - - return ( - - ); -}; - -export default memo( - InvitationResultInvitationsTable, - (prevProps, nextProps) => { - return equal(prevProps.invitations, nextProps.invitations); - }, -); diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx new file mode 100644 index 00000000000..e1ec0e89acd --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx @@ -0,0 +1,113 @@ +import { FC, ReactNode } from 'react'; +import { defineMessages } from 'react-intl'; +import { InvitationSuccessRow } from 'types/course/userInvitations'; + +import { ColumnTemplate } from 'lib/components/table'; +import Table from 'lib/components/table/Table'; +import { + DEFAULT_TABLE_ROWS_PER_PAGE, + TIMELINE_ALGORITHMS, +} from 'lib/constants/sharedConstants'; +import useTranslation from 'lib/hooks/useTranslation'; +import roleTranslations from 'lib/translations/course/users/roles'; +import tableTranslations from 'lib/translations/table'; + +const translations = defineMessages({ + yes: { + id: 'course.userInvitations.InvitationResultPrimaryTable.yes', + defaultMessage: 'Yes', + }, + no: { + id: 'course.userInvitations.InvitationResultPrimaryTable.no', + defaultMessage: 'No', + }, +}); + +interface Props { + rows: InvitationSuccessRow[]; + showPersonalizedTimelineFeatures?: boolean; +} + +const InvitationResultPrimaryTable: FC = ({ + rows, + showPersonalizedTimelineFeatures, +}) => { + const { t } = useTranslation(); + const showExternalId = rows.some((r) => r.externalId != null); + + const columns: ColumnTemplate[] = [ + { + of: 'name', + title: t(tableTranslations.name), + sortable: false, + cell: (row) => row.name, + csvDownloadable: true, + }, + { + of: 'email', + title: t(tableTranslations.email), + sortable: false, + cell: (row) => row.email, + csvDownloadable: true, + }, + ...(showExternalId + ? ([ + { + of: 'externalId', + title: t(tableTranslations.externalId), + sortable: false, + cell: (row) => row.externalId ?? '', + csvDownloadable: true, + }, + ] as ColumnTemplate[]) + : []), + { + of: 'role', + title: t(tableTranslations.role), + sortable: false, + cell: (row): ReactNode => { + const desc = + roleTranslations[row.role as keyof typeof roleTranslations]; + return desc ? t(desc) : row.role; + }, + csvDownloadable: true, + }, + { + of: 'phantom', + title: t(tableTranslations.phantom), + sortable: false, + cell: (row) => (row.phantom ? t(translations.yes) : t(translations.no)), + csvDownloadable: true, + }, + ...(showPersonalizedTimelineFeatures + ? ([ + { + of: 'timelineAlgorithm', + title: t(tableTranslations.personalizedTimeline), + sortable: false, + cell: (row): ReactNode => + TIMELINE_ALGORITHMS.find( + (tl) => tl.value === row.timelineAlgorithm, + )?.label ?? '-', + csvDownloadable: true, + }, + ] as ColumnTemplate[]) + : []), + ]; + + return ( +
row} + getRowId={(row) => row.id} + pagination={{ + rowsPerPage: [DEFAULT_TABLE_ROWS_PER_PAGE], + showAllRows: false, + }} + toolbar={{ show: false }} + /> + ); +}; + +export default InvitationResultPrimaryTable; diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx deleted file mode 100644 index c8b0728fd6c..00000000000 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultUsersTable.tsx +++ /dev/null @@ -1,190 +0,0 @@ -import { FC, memo } from 'react'; -import { defineMessages } from 'react-intl'; -import { Typography } from '@mui/material'; -import equal from 'fast-deep-equal'; -import { TableColumns, TableOptions } from 'types/components/DataTable'; -import { CourseUserListData } from 'types/course/courseUsers'; -import { DuplicateReason } from 'types/course/userInvitations'; - -import DataTable from 'lib/components/core/layouts/DataTable'; -import useTranslation from 'lib/hooks/useTranslation'; -import roleTranslations from 'lib/translations/course/users/roles'; -import tableTranslations from 'lib/translations/table'; - -const translations = defineMessages({ - duplicateEmailInFile: { - id: 'course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile', - defaultMessage: 'Duplicate email in upload', - }, - duplicateExternalIdInFile: { - id: 'course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile', - defaultMessage: 'Duplicate external ID in upload', - }, - externalIdTaken: { - id: 'course.userInvitations.InvitationResultUsersTable.externalIdTaken', - defaultMessage: 'External ID is already assigned to another course member', - }, -}); - -interface Props { - title: JSX.Element; - users: Array; -} - -const InvitationResultUsersTable: FC = (props) => { - const { title, users } = props; - const { t } = useTranslation(); - - if (users && users.length === 0) return null; - - const showExternalId = users.some((u) => u.externalId != null); - const showReason = users.some((u) => u.reason != null); - - const options: TableOptions = { - download: true, - filter: false, - pagination: false, - print: false, - search: false, - selectableRows: 'none', - setTableProps: (): object => { - return { size: 'small' }; - }, - setRowProps: (_row, dataIndex, _rowIndex): Record => { - return { - key: `invitation_result_user_${users[dataIndex].id}`, - userid: `invitation_result_user_${users[dataIndex].id}`, - className: `invitation_result_user invitation_result_user_${users[dataIndex].id}`, - }; - }, - viewColumns: false, - }; - - const columns: TableColumns[] = [ - { - name: 'id', - label: t(tableTranslations.id), - options: { - display: false, - filter: false, - sort: false, - }, - }, - { - name: 'name', - label: t(tableTranslations.name), - options: { - alignCenter: false, - sort: false, - }, - }, - { - name: 'email', - label: t(tableTranslations.email), - options: { - alignCenter: false, - sort: false, - }, - }, - ...(showExternalId - ? [ - { - name: 'externalId', - label: t(tableTranslations.externalId), - options: { - alignCenter: true, - sort: false, - }, - }, - ] - : []), - ...(showReason - ? [ - { - name: 'reason', - label: t(tableTranslations.reason), - options: { - alignCenter: false, - sort: false, - customBodyRenderLite: (dataIndex: number): JSX.Element => { - const user = users[dataIndex]; - const reasonText = - { - duplicate_email_in_file: t( - translations.duplicateEmailInFile, - ), - duplicate_external_id_in_file: t( - translations.duplicateExternalIdInFile, - ), - external_id_taken: t(translations.externalIdTaken), - }[user.reason ?? ''] ?? ''; - return ( - - {reasonText} - - ); - }, - }, - }, - ] - : []), - { - name: 'phantom', - label: t(tableTranslations.phantom), - options: { - sort: false, - customBodyRenderLite: (dataIndex): JSX.Element => { - const user = users[dataIndex]; - return ( - - {user.phantom ? 'Yes' : 'No'} - - ); - }, - }, - }, - { - name: 'role', - label: t(tableTranslations.role), - options: { - alignCenter: false, - sort: false, - customBodyRenderLite: (dataIndex): JSX.Element => { - const user = users[dataIndex]; - return ( - - {t(roleTranslations[user.role])} - - ); - }, - }, - }, - ]; - - return ( - - ); -}; - -export default memo(InvitationResultUsersTable, (prevProps, nextProps) => { - return equal(prevProps.users, nextProps.users); -}); diff --git a/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx b/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx index 86d2f9f69f2..c24b976f1f8 100644 --- a/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/UserInvitationsTable.tsx @@ -6,7 +6,6 @@ import { InvitationStatus, } from 'types/course/userInvitations'; -import { getManageCourseUserPermissions } from 'course/users/selectors'; import Note from 'lib/components/core/Note'; import GhostIcon from 'lib/components/icons/GhostIcon'; import { ColumnTemplate } from 'lib/components/table'; @@ -18,6 +17,7 @@ import { formatMiniDateTime } from 'lib/moment'; import roleTranslations from 'lib/translations/course/users/roles'; import tableTranslations from 'lib/translations/table'; +import { getManageCourseUsersSharedData } from '../../selectors'; import translations from '../../translations'; import InvitationActionButtons from '../buttons/InvitationActionButtons'; import ResendAllInvitationsButton from '../buttons/ResendAllInvitationsButton'; @@ -140,7 +140,9 @@ const UserInvitationsTable: FC = (props) => { ); const { t } = useTranslation(); - const permissions = useAppSelector(getManageCourseUserPermissions); + const { showPersonalizedTimelineFeatures } = useAppSelector( + getManageCourseUsersSharedData, + ); const columns: ColumnTemplate[] = [ { @@ -169,7 +171,7 @@ const UserInvitationsTable: FC = (props) => { title: t(tableTranslations.externalId), sortable: false, searchable: false, - cell: (datum) => datum.externalId ?? null, + cell: (datum) => datum.externalId ?? '', } satisfies ColumnTemplate, ] : []), @@ -186,7 +188,7 @@ const UserInvitationsTable: FC = (props) => { TIMELINE_ALGORITHMS.find( (timeline) => timeline.value === datum.timelineAlgorithm, )?.label ?? '-', - unless: !permissions.canManagePersonalTimes, + unless: !showPersonalizedTimelineFeatures, }, { id: 'status', diff --git a/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultExistingTable.test.tsx b/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultExistingTable.test.tsx new file mode 100644 index 00000000000..c3a9545e6f7 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultExistingTable.test.tsx @@ -0,0 +1,188 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; + +import InvitationResultExistingTable from '../InvitationResultExistingTable'; + +const baseRow = { + id: 1, + name: 'Alice Tan', + email: 'alice@example.com', + externalId: 'aliceExt', + role: 'student', + phantom: false, +}; + +describe('InvitationResultExistingTable', () => { + it('renders nothing when rows is empty', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByRole('table')).not.toBeInTheDocument(); + }); + + it('shows Name, Email, Ext ID, Role, Phantom columns', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + expect(screen.getByText('alice@example.com')).toBeInTheDocument(); + expect(screen.getByText('aliceExt')).toBeInTheDocument(); + expect(screen.getByText('Student')).toBeInTheDocument(); + expect(screen.getByText('No')).toBeInTheDocument(); + }); + + it('hides Ext ID column when no rows have externalId', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText('External ID')).not.toBeInTheDocument(); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + expect(screen.getByText('Student')).toBeInTheDocument(); + }); + + it('localizes role via roleTranslations', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Teaching Assistant')).toBeInTheDocument(); + }); + + it('renders Yes for phantom user', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Yes')).toBeInTheDocument(); + }); + + describe('updated row rendering', () => { + it('renders bold externalId with tooltip "Previously: —" when previousExternalId is null', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const cell = screen.getByText('newId'); + expect(cell.tagName).toBe('STRONG'); + await userEvent.hover(cell); + expect(await screen.findByText('Previously: —')).toBeInTheDocument(); + }); + + it('renders bold externalId with tooltip "Previously: oldId" when previousExternalId is a string', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const cell = screen.getByText('newId'); + expect(cell.tagName).toBe('STRONG'); + await userEvent.hover(cell); + expect(await screen.findByText('Previously: oldId')).toBeInTheDocument(); + }); + + it('does not bold externalId for rows without previousExternalId', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const cell = screen.getByText('aliceExt'); + expect(cell.tagName).not.toBe('STRONG'); + }); + + it('renders updated rows before non-updated rows', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const normalCell = screen.getByText('Normal Bob'); + const updatedCell = screen.getByText('Updated Alice'); + expect( + // eslint-disable-next-line no-bitwise + normalCell.compareDocumentPosition(updatedCell) & + Node.DOCUMENT_POSITION_PRECEDING, + ).toBeTruthy(); + }); + + it('applies highlight class to updated rows', async () => { + const { container } = render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const highlighted = Array.from(container.querySelectorAll('tr')).find( + (tr) => tr.className.includes('bg-[#e3f2fd]'), + ); + expect(highlighted).toBeDefined(); + }); + + it('does not apply highlight class to non-updated rows', async () => { + const { container } = render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const highlighted = Array.from(container.querySelectorAll('tr')).find( + (tr) => tr.className.includes('bg-[#e3f2fd]'), + ); + expect(highlighted).toBeUndefined(); + }); + }); + + describe('Personalized Timeline column', () => { + it('shows column header and algorithm label when showPersonalizedTimelineFeatures is true', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + expect(screen.getByText('Fomo')).toBeInTheDocument(); + }); + + it('hides column when showPersonalizedTimelineFeatures is false', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText('Personalized Timeline'), + ).not.toBeInTheDocument(); + }); + + it('shows dash when timelineAlgorithm is undefined', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + expect(screen.getByText('-')).toBeInTheDocument(); + }); + }); +}); diff --git a/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultFailedTable.test.tsx b/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultFailedTable.test.tsx new file mode 100644 index 00000000000..f28d3f31393 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultFailedTable.test.tsx @@ -0,0 +1,228 @@ +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { FailedInvitationRowData } from 'types/course/userInvitations'; + +import InvitationResultFailedTable from '../InvitationResultFailedTable'; + +const baseUser: FailedInvitationRowData = { + id: 1, + name: 'Alice', + email: 'alice@example.com', + role: 'student', + reason: 'failed_to_send', +}; + +describe('InvitationResultFailedTable', () => { + it('renders nothing when users is empty', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByRole('table')).not.toBeInTheDocument(); + }); + + it('renders name and email for each row', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Alice')).toBeInTheDocument(); + expect(screen.getByText('alice@example.com')).toBeInTheDocument(); + }); + + it('renders reason label for duplicate_email_in_file', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.getByText('Duplicate email in uploaded CSV'), + ).toBeInTheDocument(); + }); + + it('renders reason label for external_id_taken', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.getByText('External ID is taken by another course member'), + ).toBeInTheDocument(); + }); + + it('renders reason label for duplicate_external_id_in_file', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.getByText('Duplicate external ID in uploaded CSV'), + ).toBeInTheDocument(); + }); + + it('renders reason label for failed_to_send', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.getByText( + 'Failed to send invitation email, please try again - if failures persist, contact us for assistance', + ), + ).toBeInTheDocument(); + }); + + it('shows Role and Phantom columns', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Student')).toBeInTheDocument(); + expect(screen.getByText('No')).toBeInTheDocument(); + }); + + it('renders Yes for phantom user', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Yes')).toBeInTheDocument(); + }); + + it('shows External ID column when any user has a non-null externalId', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('External ID')).toBeInTheDocument(); + expect(screen.getByText('ext123')).toBeInTheDocument(); + }); + + it('renders empty cell for null externalId when column is shown by another row', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('External ID')).toBeInTheDocument(); + expect(screen.getByText('ext123')).toBeInTheDocument(); + expect(screen.getByText('Bob')).toBeInTheDocument(); + }); + + it('hides External ID column when all users have no externalId', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText('External ID')).not.toBeInTheDocument(); + }); + + it('applies red highlight class to the failed_to_send row', async () => { + const { container } = render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const rows = Array.from(container.querySelectorAll('tr')); + const aliceRow = rows.find((tr) => tr.textContent?.includes('Alice')); + const bobRow = rows.find((tr) => tr.textContent?.includes('Bob')); + expect(aliceRow?.className).toContain('bg-[#ffebee]'); + expect(bobRow?.className).not.toContain('bg-[#ffebee]'); + }); + + it('does not apply red highlight class to non-failed_to_send rows', async () => { + const { container } = render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + const highlighted = Array.from(container.querySelectorAll('tr')).find( + (tr) => tr.className.includes('bg-[#ffebee]'), + ); + expect(highlighted).toBeUndefined(); + }); + + describe('Personalized Timeline column', () => { + it('shows column header and algorithm label when showPersonalizedTimelineFeatures is true', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + expect(screen.getByText('Stragglers')).toBeInTheDocument(); + }); + + it('hides column when showPersonalizedTimelineFeatures is false', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText('Personalized Timeline'), + ).not.toBeInTheDocument(); + }); + + it('shows dash when timelineAlgorithm is undefined', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + expect(screen.getByText('-')).toBeInTheDocument(); + }); + }); +}); diff --git a/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultPrimaryTable.test.tsx b/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultPrimaryTable.test.tsx new file mode 100644 index 00000000000..f20af531f1f --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/__test__/InvitationResultPrimaryTable.test.tsx @@ -0,0 +1,112 @@ +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; + +import InvitationResultPrimaryTable from '../InvitationResultPrimaryTable'; + +const baseRow = { + id: 'inv-1', + name: 'Alice', + email: 'alice@example.com', + externalId: null, + role: 'student', + phantom: false, +}; + +describe('InvitationResultPrimaryTable', () => { + it('renders Name, Email, Role, and Phantom columns', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Alice')).toBeInTheDocument(); + expect(screen.getByText('alice@example.com')).toBeInTheDocument(); + expect(screen.getByText('Student')).toBeInTheDocument(); + expect(screen.getByText('No')).toBeInTheDocument(); + }); + + it('renders Yes for phantom user', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Yes')).toBeInTheDocument(); + }); + + it('localizes role via roleTranslations', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Teaching Assistant')).toBeInTheDocument(); + }); + + it('shows External ID column when any row has a non-null externalId', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('External ID')).toBeInTheDocument(); + expect(screen.getByText('ext123')).toBeInTheDocument(); + }); + + it('hides External ID column when all rows have null externalId', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.queryByText('External ID')).not.toBeInTheDocument(); + }); + + it('renders empty cell for null externalId when column is shown by another row', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('External ID')).toBeInTheDocument(); + expect(screen.getByText('ext123')).toBeInTheDocument(); + expect(screen.getByText('Bob')).toBeInTheDocument(); + }); + + describe('Personalized Timeline column', () => { + it('shows column header and algorithm label when showPersonalizedTimelineFeatures is true', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + expect(screen.getByText('Otot')).toBeInTheDocument(); + }); + + it('hides column when showPersonalizedTimelineFeatures is false', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText('Personalized Timeline'), + ).not.toBeInTheDocument(); + }); + + it('shows dash when timelineAlgorithm is undefined', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Personalized Timeline')).toBeInTheDocument(); + expect(screen.getByText('-')).toBeInTheDocument(); + }); + }); +}); diff --git a/client/app/bundles/course/user-invitations/pages/InvitationsIndex/__test__/index.test.tsx b/client/app/bundles/course/user-invitations/pages/InvitationsIndex/__test__/index.test.tsx new file mode 100644 index 00000000000..5b601ebf4a6 --- /dev/null +++ b/client/app/bundles/course/user-invitations/pages/InvitationsIndex/__test__/index.test.tsx @@ -0,0 +1,80 @@ +import { createMockAdapter } from 'mocks/axiosMock'; +import { render, waitFor } from 'test-utils'; + +import CourseAPI from 'api/course'; + +import InvitationsIndex from '../index'; + +const mock = createMockAdapter(CourseAPI.userInvitations.client); + +const STUB_INVITATION = { + id: 1, + name: 'Test User', + email: 'test@example.com', + externalId: null, + role: 'student', + phantom: false, + timelineAlgorithm: 'fixed', + invitationKey: 'ABC123', + confirmed: false, + sentAt: '2024-01-01T00:00:00Z', + confirmedAt: null, + isRetryable: true, +}; + +const BASE_PERMISSIONS = { + canManageCourseUsers: true, + canManageEnrolRequests: true, + canManageReferenceTimelines: false, + canManagePersonalTimes: false, + canRegisterWithCode: false, +}; + +const BASE_MANAGE_COURSE_USERS_DATA = { + requestsCount: 0, + invitationsCount: 0, + defaultTimelineAlgorithm: 'fixed', + showPersonalizedTimelineFeatures: false, +}; + +const mockIndexResponse = ( + showPersonalizedTimelineFeatures: boolean, +): object => ({ + invitations: [STUB_INVITATION], + permissions: { + ...BASE_PERMISSIONS, + canManagePersonalTimes: showPersonalizedTimelineFeatures, + }, + manageCourseUsersData: { + ...BASE_MANAGE_COURSE_USERS_DATA, + showPersonalizedTimelineFeatures, + }, +}); + +describe('', () => { + it('shows the Personalized Timeline column when showPersonalizedTimelineFeatures is true', async () => { + mock + .onGet(`/courses/${global.courseId}/user_invitations`) + .reply(200, mockIndexResponse(true)); + + const page = render(); + + // Column only appears once invitations load. state.users is NOT pre-populated — + // the column must come from state.invitations (the correct store). + await waitFor(() => { + expect(page.queryByText('Personalized Timeline')).not.toBeNull(); + }); + }); + + it('hides the Personalized Timeline column when showPersonalizedTimelineFeatures is false', async () => { + mock + .onGet(`/courses/${global.courseId}/user_invitations`) + .reply(200, mockIndexResponse(false)); + + const page = render(); + + await waitFor(() => { + expect(page.queryByText('Personalized Timeline')).toBeNull(); + }); + }); +}); 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 2f5914f61d2..563bca9d858 100644 --- a/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx +++ b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx @@ -60,7 +60,7 @@ const translations = defineMessages({ fileUploadInfoExternalId: { id: 'course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId', defaultMessage: - 'External ID is an optional field for linking a user to an external system. If provided, it must be unique within the course - duplicate external IDs will be skipped.', + 'External ID is optional. If provided, it overwrites any existing external ID for the user and must be unique within the course.', }, exampleHeader: { id: 'course.userInvitations.InviteUsersFileUpload.exampleHeader', diff --git a/client/app/bundles/course/user-invitations/store.ts b/client/app/bundles/course/user-invitations/store.ts index a69fe95561e..514a2698dc4 100644 --- a/client/app/bundles/course/user-invitations/store.ts +++ b/client/app/bundles/course/user-invitations/store.ts @@ -45,6 +45,7 @@ const initialState: InvitationsState = { requestsCount: 0, invitationsCount: 0, defaultTimelineAlgorithm: 'fixed', + showPersonalizedTimelineFeatures: false, }, courseRegistrationKey: '', }; diff --git a/client/app/types/course/courseUsers.ts b/client/app/types/course/courseUsers.ts index 9affb2d7bf7..e316e098155 100644 --- a/client/app/types/course/courseUsers.ts +++ b/client/app/types/course/courseUsers.ts @@ -134,6 +134,7 @@ export interface ManageCourseUsersSharedData { requestsCount: number; invitationsCount: number; defaultTimelineAlgorithm: TimelineAlgorithm; + showPersonalizedTimelineFeatures?: boolean; } export interface LearningRateRecordsData { diff --git a/client/app/types/course/userInvitations.ts b/client/app/types/course/userInvitations.ts index e67607eada9..956d0488fa7 100644 --- a/client/app/types/course/userInvitations.ts +++ b/client/app/types/course/userInvitations.ts @@ -11,21 +11,46 @@ export interface InvitationFileEntity { file?: Blob; } -export type DuplicateReason = +export type InvitationFailureReason = | 'duplicate_email_in_file' | 'duplicate_external_id_in_file' - | 'external_id_taken'; + | 'external_id_taken' + | 'failed_to_send'; -export interface DuplicateUserData extends CourseUserListData { - reason?: DuplicateReason; +export interface FailedInvitationRowData extends CourseUserListData { + reason: InvitationFailureReason; + timelineAlgorithm?: TimelineAlgorithm; } export interface InvitationResult { - duplicateUsers?: DuplicateUserData[]; + failedUsers?: FailedInvitationRowData[]; existingCourseUsers?: CourseUserData[]; existingInvitations?: InvitationListData[]; newCourseUsers?: CourseUserData[]; newInvitations?: InvitationListData[]; + updatedCourseUsers?: InvitationUpdatedItem[]; + updatedInvitations?: InvitationUpdatedItem[]; +} + +export interface InvitationSuccessRow { + id: string; + name: string; + email: string; + externalId: string | null; + role: string; + phantom: boolean; + timelineAlgorithm?: TimelineAlgorithm; +} + +export interface InvitationUpdatedItem { + id: number; + name: string; + email: string; + externalId: string | null; + previousExternalId: string | null; + role: string; + phantom: boolean; + timelineAlgorithm?: TimelineAlgorithm; } export interface IndividualInvites { diff --git a/client/locales/en.json b/client/locales/en.json index 5d98aba0d47..8921ac2ea6d 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -6994,13 +6994,16 @@ "defaultMessage": "Existing Course Users ({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsersInfo": { - "defaultMessage": "Existing course users with this email were found in the invitation. They were not invited." + "defaultMessage": "These users are already enrolled in this course. They were not re-enrolled." + }, + "course.userInvitations.InvitationResultDialog.externalIdUpdatedInfo": { + "defaultMessage": "External IDs were updated where specified." }, "course.userInvitations.InvitationResultDialog.existingInvitations": { "defaultMessage": "Existing Invitations ({count})" }, "course.userInvitations.InvitationResultDialog.existingInvitationsInfo": { - "defaultMessage": "Existing invitations for these users with this email already exist. They were not invited." + "defaultMessage": "These users already have a pending invitation. They were not re-invited." }, "course.userInvitations.InvitationResultDialog.header": { "defaultMessage": "Invitation Summary" @@ -7011,17 +7014,44 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "New Invitations ({count})" }, - "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { - "defaultMessage": "Duplicate email in upload" + "course.userInvitations.InvitationResultDialog.actionableTitle": { + "defaultMessage": "Failed ({count})" + }, + "course.userInvitations.InvitationResultDialog.failedInvitations": { + "defaultMessage": "Failed to Send ({count})" + }, + "course.userInvitations.InvitationResultDialog.failedInvitationsInfo": { + "defaultMessage": "Error occurred when sending invitation email, please try again." + }, + "course.userInvitations.InvitationResultDialog.updatedSubtitle": { + "defaultMessage": "{count} updated · shown first" + }, + "course.userInvitations.InvitationResultDialog.summary": { + "defaultMessage": "{newInvitations} new {newInvitations, plural, one {invitation} other {invitations}} sent, {newEnrollments} directly enrolled, {alreadyInCourse} already in course." + }, + "course.userInvitations.InvitationResultDialog.summaryFailed": { + "defaultMessage": "{count} failed." }, - "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { - "defaultMessage": "Duplicate external ID in upload" + "course.userInvitations.InvitationResultFailedTable.duplicateEmailInFile": { + "defaultMessage": "Duplicate email in uploaded CSV" }, - "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { - "defaultMessage": "External ID is already assigned to another course member" + "course.userInvitations.InvitationResultFailedTable.duplicateExternalIdInFile": { + "defaultMessage": "Duplicate external ID in uploaded CSV" }, - "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { - "defaultMessage": "Already a course member — external ID could not be applied (already assigned to another member)" + "course.userInvitations.InvitationResultFailedTable.externalIdTaken": { + "defaultMessage": "External ID is taken by another course member" + }, + "course.userInvitations.InvitationResultFailedTable.failedToSend": { + "defaultMessage": "Failed to send invitation email, please try again - if failures persist, contact us for assistance" + }, + "course.userInvitations.InvitationResultSkippedTable.no": { + "defaultMessage": "No" + }, + "course.userInvitations.InvitationResultSkippedTable.previouslyLabel": { + "defaultMessage": "Previously: {value}" + }, + "course.userInvitations.InvitationResultSkippedTable.yes": { + "defaultMessage": "Yes" }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "Accepted Invitations" @@ -7066,7 +7096,7 @@ "defaultMessage": "Each invitation must use a unique email address within the course. Duplicate emails will be skipped." }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId": { - "defaultMessage": "External ID is an optional field for linking a user to an external system. If provided, it must be unique within the course - duplicate external IDs will be skipped." + "defaultMessage": "External ID is optional. If provided, it overwrites any existing external ID for the user and must be unique within the course." }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfo": { "defaultMessage": "Upload a .csv file with the following format:" diff --git a/client/locales/ko.json b/client/locales/ko.json index 34196262f2a..b80694eff9b 100644 --- a/client/locales/ko.json +++ b/client/locales/ko.json @@ -6981,13 +6981,16 @@ "defaultMessage": "기존 과정 사용자 ({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsersInfo": { - "defaultMessage": "초대에서 이 이메일의 기존 과정 사용자가 발견되었습니다. 초대되지 않았습니다." + "defaultMessage": "이 사용자들은 이미 과정에 등록되어 있습니다. 재등록되지 않습니다." + }, + "course.userInvitations.InvitationResultDialog.externalIdUpdatedInfo": { + "defaultMessage": "외부 ID가 지정된 경우 업데이트되었습니다." }, "course.userInvitations.InvitationResultDialog.existingInvitations": { "defaultMessage": "기존 초대장 ({count})" }, "course.userInvitations.InvitationResultDialog.existingInvitationsInfo": { - "defaultMessage": "이 이메일의 기존 초대장이 이미 존재합니다. 초대되지 않았습니다." + "defaultMessage": "이 사용자들은 이미 대기 중인 초대장이 있습니다. 재초대되지 않습니다." }, "course.userInvitations.InvitationResultDialog.header": { "defaultMessage": "초대 요약" @@ -6998,17 +7001,38 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "새 초대장 ({count})" }, - "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { + "course.userInvitations.InvitationResultDialog.actionableTitle": { + "defaultMessage": "실패 ({count})" + }, + "course.userInvitations.InvitationResultDialog.failedInvitations": { + "defaultMessage": "전송 실패 ({count})" + }, + "course.userInvitations.InvitationResultDialog.failedInvitationsInfo": { + "defaultMessage": "초대 이메일 전송 중 오류가 발생했습니다. 다시 시도해 주세요." + }, + "course.userInvitations.InvitationResultDialog.summaryFailed": { + "defaultMessage": "{count}건이 실패했습니다." + }, + "course.userInvitations.InvitationResultDialog.updatedSubtitle": { + "defaultMessage": "{count}개 업데이트됨 · 먼저 표시" + }, + "course.userInvitations.InvitationResultFailedTable.duplicateEmailInFile": { "defaultMessage": "업로드 파일에 중복된 이메일 주소가 있습니다" }, - "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { + "course.userInvitations.InvitationResultFailedTable.duplicateExternalIdInFile": { "defaultMessage": "업로드 파일에 중복된 외부 ID가 있습니다" }, - "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { - "defaultMessage": "해당 외부 ID는 이미 다른 강의 구성원에게 할당되어 있습니다" + "course.userInvitations.InvitationResultFailedTable.externalIdTaken": { + "defaultMessage": "해당 외부 ID는 이미 다른 과정 구성원에게 할당되어 있습니다" + }, + "course.userInvitations.InvitationResultFailedTable.failedToSend": { + "defaultMessage": "초대 이메일 전송에 실패했습니다. 다시 시도해 주세요 - 문제가 지속되면 저희에게 문의해 주세요" + }, + "course.userInvitations.InvitationResultFailedTable.externalIdTakenEnrolled": { + "defaultMessage": "이미 과정 구성원입니다 — 외부 ID를 적용할 수 없습니다(이미 다른 구성원에게 할당됨)" }, - "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { - "defaultMessage": "이미 강의 구성원입니다 — 외부 ID를 적용할 수 없습니다(해당 ID가 이미 다른 구성원에게 할당되어 있습니다)" + "course.userInvitations.InvitationResultSkippedTable.previouslyLabel": { + "defaultMessage": "이전 값: {value}" }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "수락된 초대" @@ -7056,7 +7080,7 @@ "defaultMessage": "각 초대는 과정 내에서 고유한 이메일 주소를 사용해야 합니다. 중복된 이메일은 건너뜁니다." }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId": { - "defaultMessage": "외부 ID는 사용자를 외부 시스템과 연결하기 위한 선택적 필드입니다. 입력된 경우 과정 내에서 고유해야 하며, 중복된 외부 ID는 건너뜁니다." + "defaultMessage": "외부 ID는 선택 사항입니다. 입력된 경우 해당 사용자의 기존 외부 ID를 덮어쓰며, 과정 내에서 고유해야 합니다." }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfo": { "defaultMessage": "다음 형식으로 .csv 파일을 업로드하세요:" diff --git a/client/locales/zh.json b/client/locales/zh.json index 37a3aff0227..9d7e72e6a7e 100644 --- a/client/locales/zh.json +++ b/client/locales/zh.json @@ -6975,13 +6975,16 @@ "defaultMessage": "现有课程用户({count})" }, "course.userInvitations.InvitationResultDialog.existingCourseUsersInfo": { - "defaultMessage": "在邀请中找到了此邮箱对应的现有课程用户。他们不会收到邮件。" + "defaultMessage": "这些用户已在课程中注册。他们未被重新注册。" + }, + "course.userInvitations.InvitationResultDialog.externalIdUpdatedInfo": { + "defaultMessage": "如有指定,外部ID已更新。" }, "course.userInvitations.InvitationResultDialog.existingInvitations": { "defaultMessage": "现有邀请({count})" }, "course.userInvitations.InvitationResultDialog.existingInvitationsInfo": { - "defaultMessage": "对于使用此邮箱的用户的邀请已存在。他们不会再次收到邮件。" + "defaultMessage": "这些用户已有待处理的邀请。他们未被重新邀请。" }, "course.userInvitations.InvitationResultDialog.header": { "defaultMessage": "邀请摘要" @@ -6992,17 +6995,38 @@ "course.userInvitations.InvitationResultDialog.newInvitations": { "defaultMessage": "新邀请({count})" }, - "course.userInvitations.InvitationResultUsersTable.duplicateEmailInFile": { - "defaultMessage": "上传文件中存在重复的电子邮箱" + "course.userInvitations.InvitationResultDialog.actionableTitle": { + "defaultMessage": "失败({count})" + }, + "course.userInvitations.InvitationResultDialog.failedInvitations": { + "defaultMessage": "发送失败({count})" + }, + "course.userInvitations.InvitationResultDialog.failedInvitationsInfo": { + "defaultMessage": "发送邀请邮件时出错,请重试。" + }, + "course.userInvitations.InvitationResultDialog.summaryFailed": { + "defaultMessage": "{count} 个失败。" }, - "course.userInvitations.InvitationResultUsersTable.duplicateExternalIdInFile": { + "course.userInvitations.InvitationResultDialog.updatedSubtitle": { + "defaultMessage": "{count} 条已更新 · 优先显示" + }, + "course.userInvitations.InvitationResultFailedTable.duplicateEmailInFile": { + "defaultMessage": "上传文件中存在重复的电子邮件地址" + }, + "course.userInvitations.InvitationResultFailedTable.duplicateExternalIdInFile": { "defaultMessage": "上传文件中存在重复的外部编号" }, - "course.userInvitations.InvitationResultUsersTable.externalIdTaken": { + "course.userInvitations.InvitationResultFailedTable.externalIdTaken": { "defaultMessage": "该外部编号已分配给另一名课程成员" }, - "course.userInvitations.InvitationResultUsersTable.externalIdTakenEnrolled": { - "defaultMessage": "已是课程成员 — 无法应用外部编号(该编号已分配给另一名成员)" + "course.userInvitations.InvitationResultFailedTable.failedToSend": { + "defaultMessage": "邀请邮件发送失败,请重试 - 如问题持续发生,请联系我们获取帮助" + }, + "course.userInvitations.InvitationResultFailedTable.externalIdTakenEnrolled": { + "defaultMessage": "已是课程成员 — 外部编号未能应用(已分配给另一名成员" + }, + "course.userInvitations.InvitationResultSkippedTable.previouslyLabel": { + "defaultMessage": "之前的值:{value}" }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "已接受邀请" @@ -7050,7 +7074,7 @@ "defaultMessage": "每个邀请必须使用课程内唯一的电子邮件地址。重复的电子邮件将被跳过。" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfoExternalId": { - "defaultMessage": "外部编号是将用户与外部系统关联的可选字段。若提供,则必须在课程内唯一——重复的外部编号将被跳过。" + "defaultMessage": "外部编号为可选项。如填写,将取代该用户现有的外部编号,且不得与本课程内其他用户重复。" }, "course.userInvitations.InviteUsersFileUpload.fileUploadInfo": { "defaultMessage": "上传具有以下格式的 .csv 文件:" diff --git a/config/locales/en/errors.yml b/config/locales/en/errors.yml index 63cf8c05085..35b92605ec0 100644 --- a/config/locales/en/errors.yml +++ b/config/locales/en/errors.yml @@ -20,6 +20,7 @@ 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).' user_registrations: invalid_code: 'You have entered an invalid invitation code.' code_taken: > diff --git a/config/locales/ko/errors.yml b/config/locales/ko/errors.yml index 42508712db4..9ad4bc2f498 100644 --- a/config/locales/ko/errors.yml +++ b/config/locales/ko/errors.yml @@ -20,6 +20,7 @@ ko: invalid_email: '%{email}을(를) 처리할 수 없습니다: 유효하지 않은 이메일: %{message}.' duplicate_external_id: '%{email}을(를) 처리할 수 없습니다: 외부 ID "%{external_id}"는 이미 사용 중입니다.' other_error: '%{email}을(를) 처리할 수 없습니다: %{message}.' + timeline_template_mismatch: '5개를 초과하는 열이 감지되었습니다. 이 강좌는 개인화된 일정 기능을 사용하지 않습니다. 5열 템플릿(이름, 이메일, 역할, 팬텀, 외부 ID)을 사용하여 다시 업로드해 주십시오.' user_registrations: invalid_code: '잘못된 초대 코드를 입력하셨습니다.' code_taken: > diff --git a/config/locales/zh/csv.yml b/config/locales/zh/csv.yml index 262bdadf441..0e86f4e740b 100644 --- a/config/locales/zh/csv.yml +++ b/config/locales/zh/csv.yml @@ -4,8 +4,8 @@ zh: assessment_submissions: headers: name: '姓名' - email: 'Email' - role: '身份变更' + email: '电子邮箱' + role: '角色' user_type: '用户类型' status: '状态' question_title: '问题标题' @@ -52,7 +52,7 @@ zh: updated_at: '上一次更新时间戳' course_user_id: '课程用户ID' name: '姓名' - role: '身份变更' + role: '角色' values: unknown_question_type: '未知问题类型' @@ -61,5 +61,5 @@ zh: headers: name: '用户名' type: '学生类型' - email: 'Email' + email: '电子邮箱' external_id: '外部编号' diff --git a/config/locales/zh/errors.yml b/config/locales/zh/errors.yml index 5284d4a5e54..eae926e15b9 100644 --- a/config/locales/zh/errors.yml +++ b/config/locales/zh/errors.yml @@ -20,6 +20,7 @@ zh: invalid_email: '%{email}无法处理:邮箱无效:%{message}。' duplicate_external_id: '%{email}无法处理:外部编号"%{external_id}"已被使用。' other_error: '%{email}无法处理:%{message}。' + timeline_template_mismatch: '检测到超过 5 列。本课程未使用个性化时间线。请使用 5 列模板重新上传(姓名、电子邮箱、角色、虚拟用户、外部编号)。' 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 34a4f7274f6..d9e95a38de3 100644 --- a/spec/controllers/course/user_invitations_controller_spec.rb +++ b/spec/controllers/course/user_invitations_controller_spec.rb @@ -70,6 +70,36 @@ def replace_with_erroneous_course end end + context 'when inviting a user who already has a non-retryable (failed) invitation' do + render_views + + let!(:course_manager) { create(:course_manager, course: course, user: user) } + let!(:failed_invitation) do + create(:course_user_invitation, course: course, is_retryable: false) + end + let(:invite_params) do + invitation = { name: failed_invitation.name, email: failed_invitation.email } + invitations = { generate(:nested_attribute_new_id) => invitation } + { invitations_attributes: invitations } + end + subject { post :create, as: :json, params: { course_id: course, course: invite_params } } + + it 'returns success' do + subject + expect(response).to have_http_status(:ok) + end + + it 'includes isRetryable in existingInvitations of the result' do + subject + body = JSON.parse(response.body) + raw_result = body['invitationResult'] + result = raw_result.is_a?(String) ? JSON.parse(raw_result) : raw_result + existing = result['existingInvitations'] + expect(existing.length).to eq(1) + expect(existing.first['isRetryable']).to be(false) + end + end + context 'when a student visits the page' do let!(:course_student) { create(:course_student, course: course, user: user) } it { expect { subject }.to raise_exception(CanCan::AccessDenied) } diff --git a/spec/models/course_user_spec.rb b/spec/models/course_user_spec.rb index 1f61ae995ed..366583d87a2 100644 --- a/spec/models/course_user_spec.rb +++ b/spec/models/course_user_spec.rb @@ -97,10 +97,33 @@ context 'when a pending invitation in the same course has the same external_id' do let!(:invitation) { create(:course_user_invitation, course: course, external_id: 'pending-id') } - it 'is invalid' do + it 'is invalid for a new record' do student = build(:course_student, course: course, external_id: 'pending-id') expect(student).not_to be_valid end + + it 'is invalid when updating an existing course user to match the pending invitation ext id' do + student = create(:course_student, course: course, external_id: nil) + expect(student.update(external_id: 'pending-id')).to be(false) + expect(student.errors[:external_id]). + to include(I18n.t('activerecord.errors.models.course_user.attributes.external_id.taken')) + end + + it 'is invalid even when the course user DB id happens to equal the pending invitation DB id' do + # Regression guard: validate_unique_external_id_within_course mistakenly applied + # where.not(id: self.id) to the invitations query as well as the course_user query, + # rather than just the course_user query ("don't compare me against myself"). + # So if a pending invitation claimed the same external_id, the check would + # accidentally exclude it and incorrectly allow the external_id through. + # This test ensures that collision is still caught when the IDs happen to match. + student = create(:course_student, course: course, external_id: nil) + allow(student).to receive(:id).and_return(invitation.id) + + student.external_id = 'pending-id' + expect(student).not_to be_valid + expect(student.errors[:external_id]). + to include(I18n.t('activerecord.errors.models.course_user.attributes.external_id.taken')) + end end context 'when only a confirmed invitation in the same course has the same external_id' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2a360ec368d..b7109e39b9d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -193,6 +193,20 @@ end end + describe '#build_course_user_from_invitation' do + let(:course) { create(:course) } + let(:user) { create(:user) } + + context 'when invitation has a blank external_id' do + let(:invitation) { build(:course_user_invitation, course: course, external_id: '') } + + it 'sets external_id to nil on the built CourseUser' do + user.build_course_user_from_invitation(invitation) + expect(user.course_users.last.external_id).to be_nil + end + end + end + describe '#send_reset_password_instructions' do subject { create(:user) } diff --git a/spec/services/course/user_invitation_service_spec.rb b/spec/services/course/user_invitation_service_spec.rb index 16b500261f8..90d868965f6 100644 --- a/spec/services/course/user_invitation_service_spec.rb +++ b/spec/services/course/user_invitation_service_spec.rb @@ -91,7 +91,7 @@ def invite context 'when a list of invitation form attributes are provided' do it 'registers everyone' do - expect(invite.map(&:size)).to eq([new_users.size, 0, existing_users.size, 0, 0]) + expect(invite.map(&:size)).to eq([new_users.size, 0, existing_users.size, 0, 0, 0, 0]) verify_users end @@ -107,7 +107,7 @@ def invite it 'accepts a CSV file with a header' do expect(subject.invite(temp_csv_from_attributes(user_attributes.map do |attributes| OpenStruct.new(attributes) - end)).map(&:size)).to eq([new_users.size, 0, existing_users.size, 0, 0]) + end)).map(&:size)).to eq([new_users.size, 0, existing_users.size, 0, 0, 0, 0]) verify_users end @@ -133,7 +133,7 @@ def invite it 'succeeds' do expect(invite.map(&:size)).to eq([new_users.size - users_invited.size, users_invited.size, - existing_users.size - users_in_course.size, users_in_course.size, 0]) + existing_users.size - users_in_course.size, users_in_course.size, 0, 0, 0]) end with_active_job_queue_adapter(:test) do @@ -150,12 +150,12 @@ def invite end it 'processes duplicate users only once' do - expect(invite.map(&:size)).to eq([new_user_attributes.size - 1, 0, existing_user_attributes.size, 0, 1]) + expect(invite.map(&:size)).to eq([new_user_attributes.size - 1, 0, existing_user_attributes.size, 0, 1, 0, 0]) end it 'tags the duplicate user with a duplicate_email reason' do - _new_invitations, _existing_invitations, _new_course_users, _existing_course_users, duplicate_users = invite - expect(duplicate_users.first[:reason]).to eq(:duplicate_email_in_file) + _new_invitations, _existing_invitations, _new_course_users, _existing_course_users, failed_users = invite + expect(failed_users.first[:reason]).to eq(:duplicate_email_in_file) end with_active_job_queue_adapter(:test) do @@ -179,16 +179,16 @@ def invite it 'processes only the first and treats the second as a duplicate' do result = subject.invite(form_attributes) expect(result).not_to be_nil - new_invitations, _existing_invitations, _new_course_users, _existing_course_users, duplicate_users = result + new_invitations, _existing_invitations, _new_course_users, _existing_course_users, failed_users = result expect(new_invitations.size).to eq(1) - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:external_id]).to eq('shared-id') + expect(failed_users.size).to eq(1) + expect(failed_users.first[:external_id]).to eq('shared-id') end it 'tags the duplicate user with a duplicate_external_id reason' do result = subject.invite(form_attributes) - _new_invitations, _existing_invitations, _new_course_users, _existing_course_users, duplicate_users = result - expect(duplicate_users.first[:reason]).to eq(:duplicate_external_id_in_file) + _new_invitations, _existing_invitations, _new_course_users, _existing_course_users, failed_users = result + expect(failed_users.first[:reason]).to eq(:duplicate_external_id_in_file) end end @@ -206,11 +206,11 @@ def invite expect(result).not_to be_nil end - it 'puts the conflicting row in duplicate_users with :external_id_taken reason' do - _, _, _, _, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:external_id_taken) - expect(duplicate_users.first[:external_id]).to eq('taken-id') + it 'puts the conflicting row in failed_users with :external_id_taken reason' do + _, _, _, _, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:external_id_taken) + expect(failed_users.first[:external_id]).to eq('taken-id') end it 'does not create a new invitation' do @@ -233,10 +233,10 @@ def invite expect(result).not_to be_nil end - it 'puts the conflicting row in duplicate_users with :external_id_taken reason' do - _, _, _, _, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + it 'puts the conflicting row in failed_users with :external_id_taken reason' do + _, _, _, _, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:external_id_taken) end end @@ -260,9 +260,22 @@ def csv_with_external_id_and_timeline(entries) ) result = subject.invite(csv) expect(result).not_to be_nil - _, _, _, _, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + _, _, _, _, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:external_id_taken) + csv.close! + 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) } + + it 'raises CSV::MalformedCSVError to prevent timeline values being stored as external IDs' do + csv = Tempfile.new(File.basename(__FILE__, '.*')).tap do |file| + file.write(CSV.generate { |c| c << ['Alice', generate(:email), 'student', 'false', 'otot', 'EXT001'] }) + file.rewind + end + expect { subject.invite(csv) }.to raise_error(CSV::MalformedCSVError) csv.close! end end @@ -288,14 +301,14 @@ def csv_with_external_id_no_timeline(entries) ) result = subject.invite(csv) expect(result).not_to be_nil - _, _, _, _, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + _, _, _, _, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:external_id_taken) csv.close! end end - context 'when re-inviting a pending invitation with a new external_id' do + context 'when re-inviting a pending invitation with a new external_id (current nil, CSV value free)' do let!(:pending_invitation) do create(:course_user_invitation, course: course, email: 'pending@example.com', external_id: nil) @@ -307,23 +320,61 @@ def csv_with_external_id_no_timeline(entries) subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'does not update the pending invitation external_id' do + it 'updates the pending invitation external_id' do result - expect(pending_invitation.reload.external_id).to be_nil + expect(pending_invitation.reload.external_id).to eq('new-id') end - it 'puts the user in existing_invitations' do - _, existing_invitations, = result - expect(existing_invitations).to include(pending_invitation) + it 'puts the user in updated_invitations' do + updated_invitations = result[5] + expect(updated_invitations.map { |u| u[:record] }).to include(pending_invitation) end - it 'does not create duplicate_users entry' do - _, _, _, _, duplicate_users = result - expect(duplicate_users).to be_empty + 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 + end + + it 'does not create failed_users entry' do + _, _, _, _, failed_users = result + expect(failed_users).to be_empty end end - context 'when an existing course user is re-invited with a new external_id' do + context 'when re-inviting a failed invitation (is_retryable: false) with a new external_id' do + let!(:failed_invitation) do + create(:course_user_invitation, course: course, + email: 'failed@example.com', + external_id: 'old-id', + is_retryable: false) + end + let(:invitation_attributes) do + { '0' => { name: 'Failed User', email: 'failed@example.com', role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'new-id' } } + end + subject(:result) { invitation_service.invite(invitation_attributes) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + it 'does not update the external_id' do + result + expect(failed_invitation.reload.external_id).to eq('old-id') + end + + it 'puts the invitation in existing_invitations, not updated_invitations' do + existing_invitations = result[1] + updated_invitations = result[5] + expect(existing_invitations).to include(failed_invitation) + expect(updated_invitations.map { |u| u[:record] }).not_to include(failed_invitation) + end + + it 'does not create a failed_users entry' do + _, _, _, _, failed_users = result + expect(failed_users).to be_empty + end + end + + context 'when an existing course user is re-invited with a new external_id (current nil, CSV value free)' do let!(:enrolled_user) { create(:user) } let!(:course_user_record) { create(:course_student, course: course, user: enrolled_user, external_id: nil) } let(:invitation_attributes) do @@ -333,19 +384,25 @@ def csv_with_external_id_no_timeline(entries) subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'does not update the course_user external_id' do + it 'updates the course_user external_id' do result - expect(course_user_record.reload.external_id).to be_nil + expect(course_user_record.reload.external_id).to eq('new-id') end - it 'puts the user in existing_course_users' do - _, _, _, existing_course_users, = result - expect(existing_course_users).to include(course_user_record) + 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) end - it 'produces no duplicate_users entry' do - _, _, _, _, duplicate_users = result - expect(duplicate_users).to be_empty + 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 + end + + it 'produces no failed_users entry' do + _, _, _, _, failed_users = result + expect(failed_users).to be_empty end end @@ -364,20 +421,15 @@ def csv_with_external_id_no_timeline(entries) expect(result).not_to be_nil end - it 'puts the user in existing_course_users' do - _, _, _, existing_course_users, = result - expect(existing_course_users).to include(course_user_record) + it 'puts the user in failed_users with :external_id_taken reason' do + _, _, _, _, failed_users = result + expect(failed_users.map { |u| u[:reason] }).to include(:external_id_taken) end it 'does not update the course_user external_id' do result expect(course_user_record.reload.external_id).to be_nil end - - it 'does not create a duplicate_users entry' do - _, _, _, _, duplicate_users = result - expect(duplicate_users).to be_empty - end end context 'when a new user is invited but the external_id matches an existing course user' do @@ -394,9 +446,9 @@ def csv_with_external_id_no_timeline(entries) expect(result).not_to be_nil end - it 'puts the row in duplicate_users with :external_id_taken reason' do - _, _, _, _, duplicate_users = result - expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + it 'puts the row in failed_users with :external_id_taken reason' do + _, _, _, _, failed_users = result + expect(failed_users.first[:reason]).to eq(:external_id_taken) end it 'does not enroll the user' do @@ -415,9 +467,9 @@ def csv_with_external_id_no_timeline(entries) subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'puts the row in duplicate_users with :external_id_taken reason' do - _, _, _, _, duplicate_users = result - expect(duplicate_users.first[:reason]).to eq(:external_id_taken) + it 'puts the row in failed_users with :external_id_taken reason' do + _, _, _, _, failed_users = result + expect(failed_users.first[:reason]).to eq(:external_id_taken) end it 'does not enroll the user' do @@ -437,23 +489,18 @@ def csv_with_external_id_no_timeline(entries) subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'puts the user in existing_course_users' do - _, _, _, existing_course_users, = result - expect(existing_course_users).to include(course_user_record) + it 'puts the user in failed_users with :external_id_taken reason' do + _, _, _, _, failed_users = result + expect(failed_users.map { |u| u[:reason] }).to include(:external_id_taken) end it 'does not update the course_user external_id' do result expect(course_user_record.reload.external_id).to be_nil end - - it 'does not create a duplicate_users entry' do - _, _, _, _, duplicate_users = result - expect(duplicate_users).to be_empty - end end - context 'when re-inviting a pending invitation whose new external_id is already taken' do + context 'when re-inviting a pending invitation whose CSV ext_id is taken by another member' do let!(:other_user) { create(:course_student, course: course, external_id: 'taken-id') } let!(:pending_invitation) do create(:course_user_invitation, course: course, @@ -466,23 +513,52 @@ def csv_with_external_id_no_timeline(entries) subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'puts the user in existing_invitations' do - _, existing_invitations, = result - expect(existing_invitations).to include(pending_invitation) + it 'puts the user in failed_users with :external_id_taken reason' do + _, _, _, _, failed_users = result + expect(failed_users.map { |u| u[:reason] }).to include(:external_id_taken) end it 'does not update the pending invitation external_id' do result expect(pending_invitation.reload.external_id).to eq('old-id') end + end + + context 'when re-inviting a pending invitation with a non-nil ext_id and CSV provides a free different value' do + let!(:pending_invitation) do + create(:course_user_invitation, course: course, + email: 'pending@example.com', name: 'Pending User', external_id: 'old-id') + end + let(:invitation_attributes) do + { '0' => { name: 'Pending User', email: 'pending@example.com', role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: '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') + end + + it 'puts the user in updated_invitations' do + updated_invitations = result[5] + expect(updated_invitations.map { |u| u[:record] }).to include(pending_invitation) + 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') + end - it 'does not create a duplicate_users entry' do - _, _, _, _, duplicate_users = result - expect(duplicate_users).to be_empty + it 'does not create failed_users entry' do + _, _, _, _, failed_users = result + expect(failed_users).to be_empty end end - context 'when a batch reassigns a pending invitation ext_id and a later row claims the freed id' do + context 'when a batch changes an existing invitation ext_id and a later row claims the freed id' do let!(:alice_invitation) do create(:course_user_invitation, course: course, email: 'alice@example.com', name: 'Alice', external_id: 'freed-id') @@ -497,15 +573,113 @@ def csv_with_external_id_no_timeline(entries) subject(:result) { invitation_service.invite(invitation_attributes) } let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } - it 'puts the first row (existing invitation) in existing_invitations unchanged' do - _, existing_invitations, = result - expect(existing_invitations).to include(alice_invitation) - expect(alice_invitation.reload.external_id).to eq('freed-id') + 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 + + 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 + # successfully, but the new invitation has already been rejected. + let!(:enrolled_user_freeing) { create(:user) } + let!(:course_user_freeing) do + create(:course_student, course: course, user: enrolled_user_freeing, external_id: 'freed-id') + end + let(:new_user_email) { generate(:email) } + let(:invitation_attributes) do + { '0' => { name: enrolled_user_freeing.name, email: enrolled_user_freeing.email, + role: :student, phantom: false, timeline_algorithm: :fixed, + external_id: 'new-id' }, + '1' => { name: 'New Person', email: new_user_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 'upserts the existing course user to new-id' do + result + expect(course_user_freeing.reload.external_id).to eq('new-id') end - it 'rejects the second row as external_id_taken because the freed-id is never released' do - _, _, _, _, duplicate_users = result - expect(duplicate_users.map { |u| u[:reason] }).to include(:external_id_taken) + 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 + + context 'when a new direct enrollee (existing account, not yet in course) ' \ + 'has an ext_id already taken by another course member' do + let!(:other_member) { create(:course_student, course: course, external_id: 'taken-id') } + let!(:enrollee) { create(:instance_user).user } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + subject(:result) do + invitation_service.invite( + '0' => { name: enrollee.name, email: enrollee.email, role: :student, + phantom: false, timeline_algorithm: :fixed, external_id: 'taken-id' } + ) + end + + it 'puts the enrollee in failed_users with :external_id_taken' do + _, _, _, _, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:external_id_taken) + end + + it 'does not enroll the user' do + _, _, new_course_users, = result + expect(new_course_users).to be_empty + end + + it 'does not create an invitation' do + new_invitations, = result + expect(new_invitations).to be_empty + end + end + + context 'when a new invitation ext_id conflicts with a course user ext_id (not a pending invitation)' do + let!(:existing_member) { create(:course_student, course: course, external_id: 'cu-taken-id') } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + 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' } + ) + new_invitations, _, _, _, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:external_id_taken) + expect(new_invitations).to be_empty + end + end + + context 'when the email belongs to a user with an unconfirmed email address' do + let!(:unconfirmed_user) { create(:instance_user).user } + before { unconfirmed_user.emails.update_all(confirmed_at: nil) } + let(:invitation_service) { Course::UserInvitationService.new(course_user, user, course) } + + 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 } + ) + new_invitations, _, new_course_users, = result + expect(new_invitations.size).to eq(1) + expect(new_course_users).to be_empty end end @@ -524,29 +698,29 @@ def csv_with_timeline(entries) end context 'when a CSV has two rows with the same email' do - it 'puts the second in duplicateUsers with reason duplicate_email' do + it 'puts the second in failedUsers with reason duplicate_email' do csv = csv_with_timeline([ { name: 'User A', email: 'a@example.com', external_id: 'id-1' }, { name: 'User B', email: 'a@example.com', external_id: 'id-2' } ]) result = subject.invite(csv) - _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:duplicate_email_in_file) + _new_invitations, _existing, _new_cu, _existing_cu, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:duplicate_email_in_file) csv.close! end end context 'when a CSV has two rows with the same external_id' do - it 'puts the second in duplicateUsers with reason duplicate_external_id' do + it 'puts the second in failedUsers with reason duplicate_external_id' do csv = csv_with_timeline([ { name: 'User A', email: 'a@example.com', external_id: 'shared-id' }, { name: 'User B', email: 'b@example.com', external_id: 'shared-id' } ]) result = subject.invite(csv) - _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:duplicate_external_id_in_file) + _new_invitations, _existing, _new_cu, _existing_cu, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:duplicate_external_id_in_file) csv.close! end end @@ -562,9 +736,9 @@ def csv_with_timeline(entries) ]) result = subject.invite(csv) expect(result).not_to be_nil - _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:duplicate_external_id_in_file) + _new_invitations, _existing, _new_cu, _existing_cu, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:duplicate_external_id_in_file) csv.close! end end @@ -576,9 +750,9 @@ def csv_with_timeline(entries) { name: 'User B', email: 'a@example.com', external_id: 'id-1' } ]) result = subject.invite(csv) - _new_invitations, _existing, _new_cu, _existing_cu, duplicate_users = result - expect(duplicate_users.size).to eq(1) - expect(duplicate_users.first[:reason]).to eq(:duplicate_email_in_file) + _new_invitations, _existing, _new_cu, _existing_cu, failed_users = result + expect(failed_users.size).to eq(1) + expect(failed_users.first[:reason]).to eq(:duplicate_email_in_file) csv.close! end end @@ -595,9 +769,9 @@ def csv_with_timeline(entries) it 'does not treat a new unique external_id as taken' do result = subject.invite(invitation_attributes) expect(result).not_to be_nil - new_invitations, _, _, _, duplicate_users = result + new_invitations, _, _, _, failed_users = result expect(new_invitations.length).to eq(1) - expect(duplicate_users).to be_empty + expect(failed_users).to be_empty end end @@ -623,6 +797,110 @@ def csv_with_timeline(entries) end end + describe 'timeline_algorithm assignment on direct enrollment' do + let(:course) { create(:course, default_timeline_algorithm: :fixed) } + let(:manager) { create(:course_manager, course: course) } + let(:existing_user) { create(:instance_user).user } + subject { Course::UserInvitationService.new(manager, manager.user, course) } + + 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 } + ) + _, _, new_course_users, = result + expect(new_course_users.size).to eq(1) + expect(new_course_users.first.timeline_algorithm).to eq('otot') + end + end + + describe 'ext_id handling on existing records' do + let(:course) { create(:course) } + let(:manager) { create(:course_manager, course: course) } + subject { Course::UserInvitationService.new(manager, manager.user, course) } + + # An existing pending invitation with nil ext_id; CSV provides EXT001 (free) + let!(:existing_inv_nil_ext) { create(:course_user_invitation, course: course, external_id: nil) } + let(:inv_nil_ext_user_attrs) do + { '0' => { name: existing_inv_nil_ext.name, email: existing_inv_nil_ext.email, + role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'EXT001' } } + end + + # Another invitation that already holds 'TAKEN' so EXT_TAKEN is not free + let!(:other_inv) { create(:course_user_invitation, course: course, external_id: 'TAKEN') } + # An existing pending invitation with nil ext_id; CSV provides TAKEN (already in DB) + let!(:existing_inv_taken_ext) { create(:course_user_invitation, course: course, external_id: nil) } + let(:inv_taken_ext_user_attrs) do + { '0' => { name: existing_inv_taken_ext.name, email: existing_inv_taken_ext.email, + role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'TAKEN' } } + end + + # An existing enrolled course user with nil ext_id; CSV provides EXT002 (free) + let!(:enrolled_nil) { create(:course_student, course: course, external_id: nil) } + let(:enrolled_nil_user_attrs) do + { '0' => { name: enrolled_nil.name, email: enrolled_nil.user.email, + role: :student, phantom: false, timeline_algorithm: :fixed, external_id: 'EXT002' } } + end + + # An existing enrolled course user with ext_id 'EXISTING'; CSV provides 'DIFFERENT' + let!(:enrolled_conflict) { create(:course_student, course: course, external_id: 'EXISTING') } + let(:enrolled_conflict_user_attrs) do + { '0' => { name: enrolled_conflict.name, email: enrolled_conflict.user.email, + 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) + expect(result).not_to be_nil + updated_invitations = result[5] + expect(updated_invitations.length).to eq(1) + expect(updated_invitations.map { |u| u[:record] }.first.external_id).to eq('EXT001') + expect(existing_inv_nil_ext.reload.external_id).to eq('EXT001') + end + + it 'rejects with :external_id_taken when existing invitation has nil ext_id but CSV value is taken' do + result = subject.invite(inv_taken_ext_user_attrs) + expect(result).not_to be_nil + failed_users = result[4] + 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) + expect(result).not_to be_nil + updated_course_users = result[6] + expect(updated_course_users.length).to eq(1) + expect(updated_course_users.map { |u| u[:record] }.first.external_id).to eq('EXT002') + expect(enrolled_nil.reload.external_id).to eq('EXT002') + 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) + 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) + 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) + 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) + _, _, _, _, failed_users = result + expect(failed_users).to be_empty + end + end + end + describe '#resend_invitation' do let(:previous_sent_time) { 1.day.ago } let(:pending_invitations) do @@ -1145,7 +1423,7 @@ def csv_with_timeline(entries) end it 'adds an invitation to the user' do - subject.instance_variable_set(:@duplicate_users, []) + subject.instance_variable_set(:@failed_users, []) subject.instance_variable_set(:@taken_external_ids, Set.new) subject.send(:invite_new_users, invitation_params) invitation_params.each do |hash| From 67de552f503bab9f6f6fed450c70dfeb27645fce Mon Sep 17 00:00:00 2001 From: lws49 Date: Wed, 3 Jun 2026 15:45:57 +0800 Subject: [PATCH 2/3] 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 | 44 +++++- .../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 | 107 +++++++++---- 32 files changed, 1151 insertions(+), 176 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 ecc094b8e91..b658b9d081f 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 @@ -116,8 +118,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 @@ -127,12 +134,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. @@ -151,10 +168,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 f256d240e8a..11e883c2fab 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 @@ -84,10 +84,24 @@ def handle_existing_course_user(user, course_user, existing_course_users) elsif @taken_external_ids.include?(csv_ext_id) @failed_users.push(user.merge(reason: :external_id_taken)) else - @taken_external_ids.delete(current_ext_id) if current_ext_id - @taken_external_ids.add(csv_ext_id) - course_user.external_id = csv_ext_id - @updated_course_users << { record: course_user, previous_external_id: current_ext_id } + case @resolution + when :replace_all + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + course_user.external_id = csv_ext_id + @updated_course_users << { record: course_user, previous_external_id: current_ext_id } + when :keep_existing + existing_course_users << course_user + else + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + @pending_course_user_updates << { + record: course_user, + previous_external_id: current_ext_id, + new_external_id: csv_ext_id + } + existing_course_users << course_user + end end end @@ -164,10 +178,24 @@ def handle_existing_invitation(user, invitation, existing_invitations) elsif @taken_external_ids.include?(csv_ext_id) @failed_users.push(user.merge(reason: :external_id_taken)) else - @taken_external_ids.delete(current_ext_id) if current_ext_id - @taken_external_ids.add(csv_ext_id) - invitation.external_id = csv_ext_id - @updated_invitations << { record: invitation, previous_external_id: current_ext_id } + case @resolution + when :replace_all + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + invitation.external_id = csv_ext_id + @updated_invitations << { record: invitation, previous_external_id: current_ext_id } + when :keep_existing + existing_invitations << invitation + else + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + @pending_invitation_updates << { + record: invitation, + previous_external_id: current_ext_id, + new_external_id: csv_ext_id + } + existing_invitations << invitation + end end end diff --git a/app/services/course/user_invitation_service.rb b/app/services/course/user_invitation_service.rb index b7b39e4ddbc..d4612317330 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) # new_course_users, existing_course_users, failed_users, updated_invitations, updated_course_users # respectively 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. @@ -88,6 +105,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 ?