From 85a592d593a89c1211dfc5742f67695d2e88b800 Mon Sep 17 00:00:00 2001 From: lws49 Date: Tue, 26 May 2026 09:25:29 +0800 Subject: [PATCH] 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|