From 459f47883219eec190c7f1c137b0a5598fca6c2f Mon Sep 17 00:00:00 2001 From: lws49 Date: Tue, 26 May 2026 09:25:29 +0800 Subject: [PATCH 1/3] feat(invitations): add external ID to invite flow and redesign result dialog - add ext_id to CourseUser and UserInvitation with unique-per-course index - accept ext_id in bulk CSV and individual invite form; upsert for existing records (enrolled users and pending invitations) - conflicts (duplicate email or ext_id) surface in Failed with reasons --- .../course/unique_external_id_concern.rb | 12 ++++++------ .../_invitation_result_data.json.jbuilder | 2 +- .../tables/InvitationResultExistingTable.tsx | 12 +++++++++--- .../tables/InvitationResultFailedTable.tsx | 12 +++++++++--- .../tables/InvitationResultPrimaryTable.tsx | 16 +++++++++++++--- client/locales/en.json | 6 ------ client/locales/ko.json | 9 --------- client/locales/zh.json | 9 --------- config/locales/en/errors.yml | 2 +- 9 files changed, 39 insertions(+), 41 deletions(-) diff --git a/app/models/concerns/course/unique_external_id_concern.rb b/app/models/concerns/course/unique_external_id_concern.rb index c06993a764..7d271a3906 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/views/course/user_invitations/_invitation_result_data.json.jbuilder b/app/views/course/user_invitations/_invitation_result_data.json.jbuilder index 166061b22e..19c26c655f 100644 --- a/app/views/course/user_invitations/_invitation_result_data.json.jbuilder +++ b/app/views/course/user_invitations/_invitation_result_data.json.jbuilder @@ -44,7 +44,7 @@ json.existingCourseUsers existing_course_users.each do |course_user| end json.failedUsers failed_users.each.with_index do |failed_user, index| - json.id failed_user[:id] || -(index + 1) + json.id index json.name failed_user[:name] json.email failed_user[:email] json.externalId failed_user[:external_id] diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx index d9d4ffcd50..d93dd09f95 100644 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultExistingTable.tsx @@ -10,11 +10,18 @@ import { TIMELINE_ALGORITHMS, } from 'lib/constants/sharedConstants'; import useTranslation from 'lib/hooks/useTranslation'; -import libTranslations from 'lib/translations'; 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}', @@ -110,8 +117,7 @@ const InvitationResultExistingTable: FC = ({ of: 'phantom', title: t(tableTranslations.phantom), sortable: false, - cell: (row) => - row.phantom ? t(libTranslations.yes) : t(libTranslations.no), + cell: (row) => (row.phantom ? t(translations.yes) : t(translations.no)), csvDownloadable: true, }, ...(showPersonalizedTimelineFeatures diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx index 8b2837c446..83ba8eae88 100644 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultFailedTable.tsx @@ -10,7 +10,6 @@ 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 libTranslations from 'lib/translations'; import roleTranslations from 'lib/translations/course/users/roles'; import tableTranslations from 'lib/translations/table'; @@ -32,6 +31,14 @@ const translations = defineMessages({ 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 { @@ -103,8 +110,7 @@ const InvitationResultFailedTable: FC = ({ title: t(tableTranslations.phantom), sortable: false, searchable: false, - cell: (row) => - row.phantom ? t(libTranslations.yes) : t(libTranslations.no), + cell: (row) => (row.phantom ? t(translations.yes) : t(translations.no)), csvDownloadable: true, }, ...(showPersonalizedTimelineFeatures diff --git a/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx b/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx index ee28c74f1b..e1ec0e89ac 100644 --- a/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx +++ b/client/app/bundles/course/user-invitations/components/tables/InvitationResultPrimaryTable.tsx @@ -1,4 +1,5 @@ import { FC, ReactNode } from 'react'; +import { defineMessages } from 'react-intl'; import { InvitationSuccessRow } from 'types/course/userInvitations'; import { ColumnTemplate } from 'lib/components/table'; @@ -8,10 +9,20 @@ import { TIMELINE_ALGORITHMS, } from 'lib/constants/sharedConstants'; import useTranslation from 'lib/hooks/useTranslation'; -import libTranslations from 'lib/translations'; 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; @@ -65,8 +76,7 @@ const InvitationResultPrimaryTable: FC = ({ of: 'phantom', title: t(tableTranslations.phantom), sortable: false, - cell: (row) => - row.phantom ? t(libTranslations.yes) : t(libTranslations.no), + cell: (row) => (row.phantom ? t(translations.yes) : t(translations.no)), csvDownloadable: true, }, ...(showPersonalizedTimelineFeatures diff --git a/client/locales/en.json b/client/locales/en.json index e5b77a7ff9..15a4d2e756 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -7044,9 +7044,6 @@ "course.userInvitations.InvitationResultDialog.summaryFailed": { "defaultMessage": "{count} failed." }, - "course.userInvitations.InvitationResultDialog.failedRowsSubtitle": { - "defaultMessage": "{count} {count, plural, one {row} other {rows}} highlighted in red could not be sent" - }, "course.userInvitations.InvitationResultFailedTable.duplicateEmailInFile": { "defaultMessage": "Duplicate email in uploaded CSV" }, @@ -7068,9 +7065,6 @@ "course.userInvitations.InvitationResultSkippedTable.yes": { "defaultMessage": "Yes" }, - "course.userInvitations.InvitationResultExistingTable.previouslyLabel": { - "defaultMessage": "Previously: {value}" - }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "Accepted Invitations" }, diff --git a/client/locales/ko.json b/client/locales/ko.json index d4f3096a64..7d82505250 100644 --- a/client/locales/ko.json +++ b/client/locales/ko.json @@ -7028,12 +7028,6 @@ "course.userInvitations.InvitationResultDialog.updatedSubtitle": { "defaultMessage": "{count}개 업데이트됨 · 먼저 표시" }, - "course.userInvitations.InvitationResultDialog.summary": { - "defaultMessage": "새 초대장 {newInvitations}개 발송, {newEnrollments}명 직접 등록, {alreadyInCourse}명은 이미 과정에 있습니다." - }, - "course.userInvitations.InvitationResultDialog.failedRowsSubtitle": { - "defaultMessage": "빨간색으로 강조된 {count}개 행 전송 실패" - }, "course.userInvitations.InvitationResultFailedTable.duplicateEmailInFile": { "defaultMessage": "업로드 파일에 중복된 이메일 주소가 있습니다" }, @@ -7052,9 +7046,6 @@ "course.userInvitations.InvitationResultSkippedTable.previouslyLabel": { "defaultMessage": "이전 값: {value}" }, - "course.userInvitations.InvitationResultExistingTable.previouslyLabel": { - "defaultMessage": "이전 값: {value}" - }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "수락된 초대" }, diff --git a/client/locales/zh.json b/client/locales/zh.json index 5b9af9d34f..9770170996 100644 --- a/client/locales/zh.json +++ b/client/locales/zh.json @@ -7022,12 +7022,6 @@ "course.userInvitations.InvitationResultDialog.updatedSubtitle": { "defaultMessage": "{count} 条已更新 · 优先显示" }, - "course.userInvitations.InvitationResultDialog.summary": { - "defaultMessage": "已发出 {newInvitations} 封新邀请，{newEnrollments} 人直接加入，{alreadyInCourse} 人已在课程中。" - }, - "course.userInvitations.InvitationResultDialog.failedRowsSubtitle": { - "defaultMessage": "红色高亮显示的 {count} 行发送失败" - }, "course.userInvitations.InvitationResultFailedTable.duplicateEmailInFile": { "defaultMessage": "上传文件中存在重复的电子邮件地址" }, @@ -7046,9 +7040,6 @@ "course.userInvitations.InvitationResultSkippedTable.previouslyLabel": { "defaultMessage": "之前的值：{value}" }, - "course.userInvitations.InvitationResultExistingTable.previouslyLabel": { - "defaultMessage": "之前的值：{value}" - }, "course.userInvitations.InvitationsBarChart.accepted": { "defaultMessage": "已接受邀请" }, diff --git a/config/locales/en/errors.yml b/config/locales/en/errors.yml index f7fbcb1052..35b92605ec 100644 --- a/config/locales/en/errors.yml +++ b/config/locales/en/errors.yml @@ -20,7 +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).' + 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: > From 6b148ec0b55d9d0b0d8d6403cdbd113daecfacbf Mon Sep 17 00:00:00 2001 From: lws49 Date: Wed, 3 Jun 2026 15:45:57 +0800 Subject: [PATCH 2/3] feat(invitations): add CSV header validation and external ID conflict resolution - validate CSV column headers explicitly instead of checking column count - detect external ID changes on existing users/invitations before applying; surface a confirmation prompt (Keep Existing / Replace) with a side-by-side Current / New External ID table capped at 320px for large uploads - conflict resolution wired into both file upload and individual invite form; file ref preserved on Go Back so admin need not re-select - add ExternalIdResolution and PendingExternalIdConflict types; update invite API and operations layer to detect and return conflict payload - controller rescues PendingExternalIdUpdates, renders jbuilder partial; concern branches on @resolution to populate pending vs updated arrays - i18n: add EN/KO/ZH translations for conflict prompt and new table columns --- .../course/user_invitations_controller.rb | 26 +++- .../parse_invitation_concern.rb | 37 +++-- .../process_invitation_concern.rb | 44 +++++- .../course/user_invitation_service.rb | 57 ++++--- .../_pending_external_id_data.json.jbuilder | 25 +++ client/app/api/course/UserInvitations.ts | 22 ++- .../components/forms/IndividualInviteForm.tsx | 134 +++++++++++----- .../forms/InviteUsersFileUploadForm.tsx | 2 +- .../misc/ExternalIdConflictPrompt.tsx | 122 +++++++++++++++ .../ExternalIdConflictPrompt.test.tsx | 144 ++++++++++++++++++ .../tables/ExternalIdConflictTable.tsx | 68 +++++++++ .../__test__/ExternalIdConflictTable.test.tsx | 59 +++++++ .../course/user-invitations/operations.ts | 48 ++++-- .../__test__/index.test.tsx | 93 +++++++++++ .../pages/InviteUsersFileUpload/index.tsx | 107 +++++++++---- client/app/types/course/userInvitations.ts | 7 + client/locales/en.json | 32 +++- client/locales/ko.json | 29 +++- client/locales/zh.json | 29 +++- config/locales/en/errors.yml | 2 +- config/locales/ko/errors.yml | 2 +- config/locales/zh/errors.yml | 2 +- .../user_invitations_controller_spec.rb | 114 ++++++++++++++ .../invitation_duplicate_external_id.csv | 4 +- spec/fixtures/course/invitation_empty.csv | 1 - ...nvitation_fuzzy_roles_phantom_timeline.csv | 2 +- .../course/invitation_invalid_email.csv | 2 +- .../invitation_no_external_id_no_timeline.csv | 1 - .../fixtures/course/invitation_whitespace.csv | 1 - .../course/invitation_with_external_id.csv | 2 +- ...nvitation_with_external_id_no_timeline.csv | 2 +- .../course/user_invitation_service_spec.rb | 107 +++++++++---- 32 files changed, 1151 insertions(+), 176 deletions(-) create mode 100644 app/views/course/user_invitations/_pending_external_id_data.json.jbuilder create mode 100644 client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx create mode 100644 client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx create mode 100644 client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx create mode 100644 client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx diff --git a/app/controllers/course/user_invitations_controller.rb b/app/controllers/course/user_invitations_controller.rb index 0ce9d15e9a..4da0dfbe7f 100644 --- a/app/controllers/course/user_invitations_controller.rb +++ b/app/controllers/course/user_invitations_controller.rb @@ -15,8 +15,20 @@ def index def create result = invite - if result + case result + when Array create_invitation_success(result) + when :pending_conflict + respond_to do |format| + format.json do + render partial: 'pending_external_id_data', + locals: { + pending_invitation_updates: @pending_conflict.pending_invitation_updates, + pending_course_user_updates: @pending_conflict.pending_course_user_updates + }, + status: :ok + end + end else propagate_errors errors = current_course.errors[:base] @@ -93,7 +105,7 @@ def resend_invitation_params # 1) Single invitation - specified with the user_invitation_id param # 2) All un-confirmed invitation - if user_invitation_id param was not found def load_invitations - @invitations ||= begin + @load_invitations ||= begin ids = resend_invitation_params ids ||= current_course.invitations.retryable.unconfirmed.select(:id) if ids.blank? @@ -118,12 +130,18 @@ def invite_by_file? # Invites the users via the service object. # - # @return [Boolean] True if the invitation was successful. + # @return [Array] On success. + # @return [Symbol] :pending_conflict when external ID updates require resolution. + # @return [Boolean] false on failure. def invite - invitation_service.invite(invitation_params) + invitation_service.invite(invitation_params, + external_id_resolution: params[:external_id_resolution]) rescue CSV::MalformedCSVError => e current_course.errors.add(:base, e.message) false + rescue Course::UserInvitationService::PendingExternalIdUpdates => e + @pending_conflict = e + :pending_conflict end # Creates a user invitation service object for this object. diff --git a/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb b/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb index ecc094b8e9..b658b9d081 100644 --- a/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb +++ b/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb @@ -9,6 +9,8 @@ module Course::UserInvitationService::ParseInvitationConcern extend ActiveSupport::Autoload TRUE_VALUES = ['t', 'true', 'y', 'yes'].freeze + EXPECTED_HEADERS_WITH_TIMELINE = %w[name email role phantom timeline externalid].freeze + EXPECTED_HEADERS_WITHOUT_TIMELINE = %w[name email role phantom externalid].freeze private @@ -116,8 +118,13 @@ def parse_from_file(file) row_num = row_number row[0] = remove_utf8_byte_order_mark(row[0]) if row_number == 1 row = strip_row(row) - # Ignore first row if it's a header row. - next if row_number == 1 && header_row?(row) + if row_number == 1 && looks_like_header?(row) + unless valid_header_row?(row) + raise I18n.t('errors.course.user_invitations.invalid_headers', + expected: expected_headers.join(',')) + end + next + end invite = parse_file_row(row) invites << invite if invite @@ -127,12 +134,22 @@ def parse_from_file(file) raise CSV::MalformedCSVError.new(e, row_num), e.message end - # Returns a boolean to determine whether the row is a header row. - # - # @param[Array] row Array read from CSV file. - # @return [Boolean] Whether the row is a header row - def header_row?(row) - row[0].casecmp('Name') == 0 && row[1].casecmp('Email') == 0 + def looks_like_header?(row) + row[0]&.casecmp('Name')&.zero? && row[1]&.casecmp('Email')&.zero? + end + + def expected_headers + if @current_course.show_personalized_timeline_features? + EXPECTED_HEADERS_WITH_TIMELINE + else + EXPECTED_HEADERS_WITHOUT_TIMELINE + end + end + + def valid_header_row?(row) + return false unless looks_like_header?(row) + + row.map { |h| h&.downcase }.compact == expected_headers end # Strips a row of whitespaces. @@ -151,10 +168,6 @@ def strip_row(row) def parse_file_row(row) return nil if row[1].blank? - if !@current_course.show_personalized_timeline_features? && row.length > 5 - raise I18n.t('errors.course.user_invitations.timeline_template_mismatch') - end - row[0] = row[1] if row[0].blank? role = parse_file_role(row[2]) phantom = parse_file_phantom(row[3]) diff --git a/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb b/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb index f256d240e8..11e883c2fa 100644 --- a/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb +++ b/app/services/concerns/course/user_invitation_service/process_invitation_concern.rb @@ -84,10 +84,24 @@ def handle_existing_course_user(user, course_user, existing_course_users) elsif @taken_external_ids.include?(csv_ext_id) @failed_users.push(user.merge(reason: :external_id_taken)) else - @taken_external_ids.delete(current_ext_id) if current_ext_id - @taken_external_ids.add(csv_ext_id) - course_user.external_id = csv_ext_id - @updated_course_users << { record: course_user, previous_external_id: current_ext_id } + case @resolution + when :replace_all + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + course_user.external_id = csv_ext_id + @updated_course_users << { record: course_user, previous_external_id: current_ext_id } + when :keep_existing + existing_course_users << course_user + else + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + @pending_course_user_updates << { + record: course_user, + previous_external_id: current_ext_id, + new_external_id: csv_ext_id + } + existing_course_users << course_user + end end end @@ -164,10 +178,24 @@ def handle_existing_invitation(user, invitation, existing_invitations) elsif @taken_external_ids.include?(csv_ext_id) @failed_users.push(user.merge(reason: :external_id_taken)) else - @taken_external_ids.delete(current_ext_id) if current_ext_id - @taken_external_ids.add(csv_ext_id) - invitation.external_id = csv_ext_id - @updated_invitations << { record: invitation, previous_external_id: current_ext_id } + case @resolution + when :replace_all + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + invitation.external_id = csv_ext_id + @updated_invitations << { record: invitation, previous_external_id: current_ext_id } + when :keep_existing + existing_invitations << invitation + else + @taken_external_ids.delete(current_ext_id) if current_ext_id + @taken_external_ids.add(csv_ext_id) + @pending_invitation_updates << { + record: invitation, + previous_external_id: current_ext_id, + new_external_id: csv_ext_id + } + existing_invitations << invitation + end end end diff --git a/app/services/course/user_invitation_service.rb b/app/services/course/user_invitation_service.rb index 5797404d08..7a21b1372a 100644 --- a/app/services/course/user_invitation_service.rb +++ b/app/services/course/user_invitation_service.rb @@ -6,6 +6,16 @@ class Course::UserInvitationService include ProcessInvitationConcern include EmailInvitationConcern + class PendingExternalIdUpdates < StandardError + attr_reader :pending_invitation_updates, :pending_course_user_updates + + def initialize(pending_invitation_updates:, pending_course_user_updates:) + @pending_invitation_updates = pending_invitation_updates + @pending_course_user_updates = pending_course_user_updates + super('Pending external ID updates require confirmation') + end + end + # Constructor for the user invitation service object. # # @param [CourseUser|nil] current_course_user The course user performing this action. @@ -28,33 +38,23 @@ def initialize(current_course_user, current_user, current_course) # existing_course_users, failed_users, updated_invitations, updated_course_users] # if success. nil when fail. # @raise [CSV::MalformedCSVError] When the file provided is invalid. - def invite(users) - new_invitations = nil - existing_invitations = nil - new_course_users = nil - existing_course_users = nil - failed_users = nil - updated_invitations = nil - updated_course_users = nil + def invite(users, external_id_resolution: nil) + @resolution = external_id_resolution&.to_sym + result = nil success = Course.transaction do - new_invitations, existing_invitations, - new_course_users, existing_course_users, - failed_users, updated_invitations, updated_course_users = invite_users(users) - raise ActiveRecord::Rollback unless updated_invitations.all? { |u| u[:record].save } - raise ActiveRecord::Rollback unless updated_course_users.all? { |u| u[:record].save } - raise ActiveRecord::Rollback unless new_invitations.all?(&:save) - raise ActiveRecord::Rollback unless new_course_users.all?(&:save) - + result = invite_users(users) + raise_if_pending_external_id_updates! + save_invitation_records!(result) true end return unless success + new_invitations, _, new_course_users, = result send_registered_emails(new_course_users) send_invitation_emails(new_invitations) - [new_invitations, existing_invitations, new_course_users, existing_course_users, - failed_users, updated_invitations, updated_course_users] + result end # Resends invitation emails to CourseUsers to the given course. @@ -64,11 +64,28 @@ def invite(users) # @return [Boolean] True if there were no errors in sending invitations. # If all provided CourseUsers have already registered, method also returns true. def resend_invitation(invitations) - invitations.blank? ? true : send_invitation_emails(invitations) + invitations.blank? || send_invitation_emails(invitations) end private + def raise_if_pending_external_id_updates! + return unless @pending_invitation_updates.any? || @pending_course_user_updates.any? + + raise PendingExternalIdUpdates.new( + pending_invitation_updates: @pending_invitation_updates, + pending_course_user_updates: @pending_course_user_updates + ) + end + + def save_invitation_records!(result) + new_invitations, _, new_course_users, _, _, updated_invitations, updated_course_users = result + all_records = updated_invitations.map { |u| u[:record] } + + updated_course_users.map { |u| u[:record] } + + new_invitations + new_course_users + raise ActiveRecord::Rollback unless all_records.all?(&:save) + end + # Invites the given users into the course. # # @param [Array|File|TempFile] users Invites the given users. @@ -88,6 +105,8 @@ def invite_users(users) @failed_users = parse_duplicates @updated_invitations = [] @updated_course_users = [] + @pending_invitation_updates = [] + @pending_course_user_updates = [] process_invitations(unique_users) + [@failed_users, @updated_invitations, @updated_course_users] end end diff --git a/app/views/course/user_invitations/_pending_external_id_data.json.jbuilder b/app/views/course/user_invitations/_pending_external_id_data.json.jbuilder new file mode 100644 index 0000000000..8906eeeaf1 --- /dev/null +++ b/app/views/course/user_invitations/_pending_external_id_data.json.jbuilder @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +json.pendingInvitationUpdates pending_invitation_updates do |item| + inv = item[:record] + json.id inv.id + json.name inv.name + json.email inv.email + json.externalId item[:new_external_id] + json.previousExternalId item[:previous_external_id] + json.role inv.role + json.phantom inv.phantom + json.timelineAlgorithm inv.timeline_algorithm +end + +json.pendingCourseUserUpdates pending_course_user_updates do |item| + cu = item[:record] + json.id cu.id + json.name cu.name.strip + json.email cu.user.email + json.externalId item[:new_external_id] + json.previousExternalId item[:previous_external_id] + json.role cu.role + json.phantom cu.phantom? + json.timelineAlgorithm cu.timeline_algorithm +end diff --git a/client/app/api/course/UserInvitations.ts b/client/app/api/course/UserInvitations.ts index 9d042ee905..16fa29d788 100644 --- a/client/app/api/course/UserInvitations.ts +++ b/client/app/api/course/UserInvitations.ts @@ -4,8 +4,10 @@ import { ManageCourseUsersSharedData, } from 'types/course/courseUsers'; import { + ExternalIdResolution, InvitationFileEntity, InvitationListData, + InvitationUpdatedItem, } from 'types/course/userInvitations'; import SubmissionsAPI from './Assessment/Submissions'; @@ -36,11 +38,17 @@ export default class UserInvitationsAPI extends BaseCourseAPI { * @return {Promise} * error response: { errors: [] } - An array of errors will be returned upon validation error. */ - invite(data: InvitationFileEntity | FormData): Promise< - AxiosResponse<{ - newInvitations: number; - invitationResult: string; // string which is JSON.parsed to type InvitationResult - }> + invite( + data: InvitationFileEntity | FormData, + externalIdResolution?: ExternalIdResolution, + ): Promise< + AxiosResponse< + | { newInvitations: number; invitationResult: string } + | { + pendingInvitationUpdates: InvitationUpdatedItem[]; + pendingCourseUserUpdates: InvitationUpdatedItem[]; + } + > > { const config = { headers: { @@ -60,6 +68,10 @@ export default class UserInvitationsAPI extends BaseCourseAPI { formData = data as FormData; } + if (externalIdResolution) { + formData.append('external_id_resolution', externalIdResolution); + } + return this.client.post( `${this.#urlPrefix}/users/invite`, formData, diff --git a/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx b/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx index 56f8a9b133..e6bc40956c 100644 --- a/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx +++ b/client/app/bundles/course/user-invitations/components/forms/IndividualInviteForm.tsx @@ -1,11 +1,13 @@ -import { FC, useEffect, useState } from 'react'; +import { FC, useEffect, useRef, useState } from 'react'; import { useFieldArray, useForm } from 'react-hook-form'; import { defineMessages } from 'react-intl'; import { yupResolver } from '@hookform/resolvers/yup'; import { + ExternalIdResolution, IndividualInvites, InvitationResult, InvitationsPostData, + PendingExternalIdConflict, } from 'types/course/userInvitations'; import * as yup from 'yup'; @@ -20,6 +22,7 @@ import { getManageCourseUserPermissions, getManageCourseUsersSharedData, } from '../../selectors'; +import ExternalIdConflictPrompt from '../misc/ExternalIdConflictPrompt'; import IndividualInvitations from './IndividualInvitations'; @@ -60,6 +63,9 @@ const IndividualInviteForm: FC = (props) => { const { openResultDialog } = props; const { t } = useTranslation(); const [isLoading, setIsLoading] = useState(false); + const [conflictData, setConflictData] = + useState(null); + const dataRef = useRef(null); const dispatch = useAppDispatch(); const sharedData = useAppSelector(getManageCourseUsersSharedData); const permissions = useAppSelector(getManageCourseUserPermissions); @@ -114,54 +120,98 @@ const IndividualInviteForm: FC = (props) => { } }, [invitationsFields.length === 0]); - const onSubmit = (data: InvitationsPostData): Promise => { - setIsLoading(true); - return dispatch(inviteUsersFromForm(data)) + const handleError = (error: unknown): void => { + const rawErrors = (error as { response?: { data?: { errors?: unknown } } }) + ?.response?.data?.errors; + let errorList: string[]; + if (Array.isArray(rawErrors)) errorList = rawErrors; + else if (typeof rawErrors === 'string') errorList = [rawErrors]; + else errorList = []; + const first = errorList[0]; + const overflow = + errorList.length > 1 ? ` (and ${errorList.length - 1} more)` : ''; + if (first) { + toast.error(t(translations.failure, { error: first + overflow }), { + autoClose: false, + }); + } else { + toast.error(t(translations.failureGeneric), { autoClose: false }); + } + }; + + const submitWithResolution = ( + postData: InvitationsPostData, + resolution?: ExternalIdResolution, + ): Promise => + dispatch(inviteUsersFromForm(postData, resolution)) .then((response) => { - reset(initialValues); - openResultDialog(response); - }) - .catch((error) => { - const rawErrors = error.response?.data?.errors; - let errorList: string[]; - if (Array.isArray(rawErrors)) errorList = rawErrors; - else if (typeof rawErrors === 'string') errorList = [rawErrors]; - else errorList = []; - const first = errorList[0]; - const overflow = - errorList.length > 1 ? ` (and ${errorList.length - 1} more)` : ''; - if (first) { - toast.error(t(translations.failure, { error: first + overflow }), { - autoClose: false, - }); + if ('conflict' in response) { + setConflictData(response.conflict); } else { - toast.error(t(translations.failureGeneric), { autoClose: false }); + reset(initialValues); + openResultDialog(response as InvitationResult); } }) - .finally(() => { - setIsLoading(false); - }); + .catch(handleError) + .finally(() => setIsLoading(false)); + + const onSubmit = (data: InvitationsPostData): Promise => { + setIsLoading(true); + dataRef.current = data; + return submitWithResolution(data); + }; + + const handleKeepExisting = (): void => { + setConflictData(null); + if (dataRef.current) { + setIsLoading(true); + submitWithResolution(dataRef.current, 'keep_existing'); + } + }; + + const handleReplaceAll = (): void => { + setConflictData(null); + if (dataRef.current) { + setIsLoading(true); + submitWithResolution(dataRef.current, 'replace_all'); + } + }; + + const handleCancel = (): void => { + setConflictData(null); + dataRef.current = null; }; return ( -
onSubmit(data))} - > - - - + <> + {conflictData && ( + + )} +
onSubmit(data))} + > + + + + ); }; diff --git a/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx b/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx index 3f1dd85c53..1f36ff16ce 100644 --- a/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx +++ b/client/app/bundles/course/user-invitations/components/forms/InviteUsersFileUploadForm.tsx @@ -12,7 +12,7 @@ import useTranslation from 'lib/hooks/useTranslation'; interface Props { open: boolean; onSubmit: ( - data: InvitationFileEntity, + data: { file: InvitationFileEntity }, setError: UseFormSetError, ) => Promise; onClose: () => void; diff --git a/client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx b/client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx new file mode 100644 index 0000000000..6e9f377a3e --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/misc/ExternalIdConflictPrompt.tsx @@ -0,0 +1,122 @@ +import { FC } from 'react'; +import { defineMessages } from 'react-intl'; +import { + Box, + Button, + Dialog, + DialogActions, + DialogContent, + DialogContentText, + DialogTitle, + Typography, +} from '@mui/material'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import useTranslation from 'lib/hooks/useTranslation'; + +import ExternalIdConflictTable from '../tables/ExternalIdConflictTable'; + +interface Props { + pendingInvitationUpdates: InvitationUpdatedItem[]; + pendingCourseUserUpdates: InvitationUpdatedItem[]; + onKeepExisting: () => void; + onReplaceAll: () => void; + onCancel: () => void; +} + +const translations = defineMessages({ + title: { + id: 'course.userInvitations.ExternalIdConflictPrompt.title', + defaultMessage: 'Confirm External ID Updates', + }, + body: { + id: 'course.userInvitations.ExternalIdConflictPrompt.body', + defaultMessage: + 'These users are already enrolled or have pending invitations. No new invitation emails will be sent to them. Would you like to keep their current External IDs, or replace them with the values from your file?', + }, + pendingInvitationUpdates: { + id: 'course.userInvitations.ExternalIdConflictPrompt.pendingInvitationUpdates', + defaultMessage: 'Pending Invitation Updates ({count})', + }, + pendingCourseUserUpdates: { + id: 'course.userInvitations.ExternalIdConflictPrompt.pendingCourseUserUpdates', + defaultMessage: 'Pending Course Member Updates ({count})', + }, + goBack: { + id: 'course.userInvitations.ExternalIdConflictPrompt.goBack', + defaultMessage: 'Go Back', + }, + keepExisting: { + id: 'course.userInvitations.ExternalIdConflictPrompt.keepExisting', + defaultMessage: 'Keep Existing', + }, + replace: { + id: 'course.userInvitations.ExternalIdConflictPrompt.replace', + defaultMessage: 'Replace', + }, +}); + +const ExternalIdConflictPrompt: FC = ({ + pendingInvitationUpdates, + pendingCourseUserUpdates, + onKeepExisting, + onReplaceAll, + onCancel, +}) => { + const { t } = useTranslation(); + + return ( + { + if (reason !== 'backdropClick') onCancel(); + }} + open + > + {t(translations.title)} + + {t(translations.body)} + + {pendingInvitationUpdates.length > 0 && ( + <> + + {t(translations.pendingInvitationUpdates, { + count: pendingInvitationUpdates.length, + })} + + + + + + )} + + {pendingCourseUserUpdates.length > 0 && ( + <> + + {t(translations.pendingCourseUserUpdates, { + count: pendingCourseUserUpdates.length, + })} + + + + + + )} + + + + + + + + ); +}; + +export default ExternalIdConflictPrompt; diff --git a/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx b/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx new file mode 100644 index 0000000000..ff5a827739 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/misc/__test__/ExternalIdConflictPrompt.test.tsx @@ -0,0 +1,144 @@ +import userEvent from '@testing-library/user-event'; +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import ExternalIdConflictPrompt from '../ExternalIdConflictPrompt'; + +const invitationUpdate: InvitationUpdatedItem = { + id: 1, + name: 'Alice Tan', + email: 'alice@example.com', + externalId: 'NEW001', + previousExternalId: 'OLD001', + role: 'student', + phantom: false, +}; + +const courseUserUpdate: InvitationUpdatedItem = { + id: 2, + name: 'Bob Lim', + email: 'bob@example.com', + externalId: 'B042', + previousExternalId: null, + role: 'student', + phantom: false, +}; + +const noop = (): void => {}; + +describe('ExternalIdConflictPrompt', () => { + it('renders the title', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Confirm External ID Updates')).toBeInTheDocument(); + }); + + it('renders the invitation updates section when non-empty', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText(/Pending Invitation Updates/)).toBeInTheDocument(); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + }); + + it('does not render invitation section when empty', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect( + screen.queryByText(/Pending Invitation Updates/), + ).not.toBeInTheDocument(); + expect( + screen.getByText(/Pending Course Member Updates/), + ).toBeInTheDocument(); + }); + + it('renders both sections when both non-empty', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText(/Pending Invitation Updates/)).toBeInTheDocument(); + expect( + screen.getByText(/Pending Course Member Updates/), + ).toBeInTheDocument(); + }); + + it('calls onCancel when Go Back is clicked', async () => { + const onCancel = jest.fn(); + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await userEvent.click(screen.getByRole('button', { name: 'Go Back' })); + expect(onCancel).toHaveBeenCalledTimes(1); + }); + + it('calls onKeepExisting when Keep Existing is clicked', async () => { + const onKeepExisting = jest.fn(); + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await userEvent.click( + screen.getByRole('button', { name: 'Keep Existing' }), + ); + expect(onKeepExisting).toHaveBeenCalledTimes(1); + }); + + it('calls onReplaceAll when Replace is clicked', async () => { + const onReplaceAll = jest.fn(); + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + await userEvent.click(screen.getByRole('button', { name: 'Replace' })); + expect(onReplaceAll).toHaveBeenCalledTimes(1); + }); +}); diff --git a/client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx b/client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx new file mode 100644 index 0000000000..972b33fd68 --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/ExternalIdConflictTable.tsx @@ -0,0 +1,68 @@ +import { FC } from 'react'; +import { defineMessages } from 'react-intl'; +import { + Table, + TableBody, + TableCell, + TableHead, + TableRow, + Typography, +} from '@mui/material'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import useTranslation from 'lib/hooks/useTranslation'; +import tableTranslations from 'lib/translations/table'; + +interface Props { + rows: InvitationUpdatedItem[]; +} + +const translations = defineMessages({ + currentExternalId: { + id: 'lib.translations.table.column.currentExternalId', + defaultMessage: 'Current External ID', + }, + newExternalId: { + id: 'lib.translations.table.column.newExternalId', + defaultMessage: 'New External ID', + }, +}); + +const ExternalIdConflictTable: FC = ({ rows }) => { + const { t } = useTranslation(); + + return ( + + + + {t(tableTranslations.name)} + {t(tableTranslations.email)} + {t(translations.currentExternalId)} + {t(translations.newExternalId)} + + + + {rows.map((row) => ( + + {row.name} + {row.email} + + {row.previousExternalId ?? ( + + — + + )} + + + + {row.externalId} + + + + ))} + +
+ ); +}; + +export default ExternalIdConflictTable; diff --git a/client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx b/client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx new file mode 100644 index 0000000000..48fcb1870e --- /dev/null +++ b/client/app/bundles/course/user-invitations/components/tables/__test__/ExternalIdConflictTable.test.tsx @@ -0,0 +1,59 @@ +import { render, screen, waitForElementToBeRemoved } from 'test-utils'; +import { InvitationUpdatedItem } from 'types/course/userInvitations'; + +import ExternalIdConflictTable from '../ExternalIdConflictTable'; + +const baseItem: InvitationUpdatedItem = { + id: 1, + name: 'Alice Tan', + email: 'alice@example.com', + externalId: 'NEW001', + previousExternalId: 'OLD001', + role: 'student', + phantom: false, +}; + +describe('ExternalIdConflictTable', () => { + it('renders Name, Email, Current External ID, New External ID columns', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Name')).toBeInTheDocument(); + expect(screen.getByText('Email')).toBeInTheDocument(); + expect(screen.getByText('Current External ID')).toBeInTheDocument(); + expect(screen.getByText('New External ID')).toBeInTheDocument(); + }); + + it('renders row data', async () => { + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + expect(screen.getByText('alice@example.com')).toBeInTheDocument(); + expect(screen.getByText('OLD001')).toBeInTheDocument(); + expect(screen.getByText('NEW001')).toBeInTheDocument(); + }); + + it('renders — when previousExternalId is null', async () => { + render( + , + ); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('—')).toBeInTheDocument(); + }); + + it('renders multiple rows', async () => { + const second: InvitationUpdatedItem = { + ...baseItem, + id: 2, + name: 'Bob Lim', + email: 'bob@example.com', + externalId: 'B042', + previousExternalId: null, + }; + render(); + await waitForElementToBeRemoved(() => screen.queryByRole('progressbar')); + expect(screen.getByText('Alice Tan')).toBeInTheDocument(); + expect(screen.getByText('Bob Lim')).toBeInTheDocument(); + }); +}); diff --git a/client/app/bundles/course/user-invitations/operations.ts b/client/app/bundles/course/user-invitations/operations.ts index d9a0c40911..e4668a7602 100644 --- a/client/app/bundles/course/user-invitations/operations.ts +++ b/client/app/bundles/course/user-invitations/operations.ts @@ -1,9 +1,11 @@ import { Operation } from 'store'; import { + ExternalIdResolution, InvitationFileEntity, InvitationPostData, InvitationResult, InvitationsPostData, + PendingExternalIdConflict, } from 'types/course/userInvitations'; import CourseAPI from 'api/course'; @@ -75,25 +77,47 @@ export function fetchPermissionsAndSharedData(): Operation { export function inviteUsersFromFile( fileEntity: InvitationFileEntity, -): Operation { + resolution?: ExternalIdResolution, +): Operation { return async (dispatch) => - CourseAPI.userInvitations.invite(fileEntity).then((response) => { - const data = response.data; - dispatch(actions.updateInvitationCounts(data.newInvitations)); - return JSON.parse(data.invitationResult); - }); + CourseAPI.userInvitations + .invite(fileEntity, resolution) + .then((response) => { + const data = response.data; + if ('pendingInvitationUpdates' in data) { + return { + conflict: { + pendingInvitationUpdates: data.pendingInvitationUpdates, + pendingCourseUserUpdates: data.pendingCourseUserUpdates, + }, + }; + } + dispatch(actions.updateInvitationCounts(data.newInvitations)); + return JSON.parse(data.invitationResult) as InvitationResult; + }); } export function inviteUsersFromForm( postData: InvitationsPostData, -): Operation { + resolution?: ExternalIdResolution, +): Operation { const formattedData = formatInvitations(postData.invitations); return async (dispatch) => - CourseAPI.userInvitations.invite(formattedData).then((response) => { - const data = response.data; - dispatch(actions.updateInvitationCounts(data.newInvitations)); - return JSON.parse(data.invitationResult); - }); + CourseAPI.userInvitations + .invite(formattedData, resolution) + .then((response) => { + const data = response.data; + if ('pendingInvitationUpdates' in data) { + return { + conflict: { + pendingInvitationUpdates: data.pendingInvitationUpdates, + pendingCourseUserUpdates: data.pendingCourseUserUpdates, + }, + }; + } + dispatch(actions.updateInvitationCounts(data.newInvitations)); + return JSON.parse(data.invitationResult) as InvitationResult; + }); } export function resendAllInvitations(): Operation { diff --git a/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx new file mode 100644 index 0000000000..d9b9daeb57 --- /dev/null +++ b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/__test__/index.test.tsx @@ -0,0 +1,93 @@ +import { render, screen, waitFor } from 'test-utils'; +import { InvitationFileEntity } from 'types/course/userInvitations'; + +import InviteUsersFileUpload from '../index'; + +// Capture the onSubmit prop so we can call it directly with IFormInputs-shaped data, +// reproducing exactly what FormDialog passes at runtime. +let capturedOnSubmit: + | ((data: { file: InvitationFileEntity }) => Promise) + | null = null; + +const MockFileUploadForm = ({ + open, + onSubmit, +}: { + open: boolean; + onSubmit: (data: { file: InvitationFileEntity }) => Promise; +}): JSX.Element | null => { + capturedOnSubmit = onSubmit; + return open ?