fix: update code based on CI comments

LWS49 · LWS49 · commit 46647012983b · 2026-05-15T15:34:03.000+08:00
diff --git a/app/controllers/course/user_invitations_controller.rb b/app/controllers/course/user_invitations_controller.rb
@@ -170,15 +170,20 @@ def aggregate_errors
   # @return [Array<String>]
   def invalid_course_user_errors
     invalid_course_users.flat_map do |course_user|
+      email = course_user.user&.email || course_user.name
       errors = []
       if course_user.errors[:external_id].any?
         errors << t('errors.course.user_invitations.duplicate_external_id',
-                    external_id: course_user.external_id)
+                    email: email, external_id: course_user.external_id)
       end
-      non_external_id_errors = course_user.errors.reject { |e| e.attribute.to_s == 'external_id' }
-      if non_external_id_errors.any?
-        user = self.class.helpers.display_course_user(course_user)
-        errors << t('errors.course.user_invitations.duplicate_user', user: user)
+      if course_user.errors[:user].any? || course_user.errors[:course].any?
+        errors << t('errors.course.user_invitations.already_enrolled', email: email)
+      end
+      other_errors = course_user.errors.reject { |e| %w[external_id user course].include?(e.attribute.to_s) }
+      if other_errors.any?
+        message = other_errors.map { |e| course_user.errors.full_message(e.attribute, e.message) }.
+                  to_sentence
+        errors << t('errors.course.user_invitations.other_error', email: email, message: message)
       end
       errors
     end
@@ -196,15 +201,24 @@ def invalid_course_users
   # @return [Array<String>]
   def invalid_invitation_email_errors
     invalid_invitations.flat_map do |invitation|
+      email = invitation.email
       errors = []
       if invitation.errors[:external_id].any?
         errors << t('errors.course.user_invitations.duplicate_external_id',
-                    external_id: invitation.external_id)
+                    email: email, external_id: invitation.external_id)
+      end
+      if invitation.errors[:email].any?
+        message = invitation.errors[:email].to_sentence
+        errors << t('errors.course.user_invitations.invalid_email', email: email, message: message)
+      end
+      if invitation.errors[:base].any?
+        errors << t('errors.course.user_invitations.duplicate_invitation', email: email)
       end
-      non_external_id_errors = invitation.errors.reject { |e| e.attribute.to_s == 'external_id' }
-      if non_external_id_errors.any?
-        message = non_external_id_errors.map { |e| invitation.errors.full_message(e.attribute, e.message) }.to_sentence
-        errors << t('errors.course.user_invitations.invalid_email', email: invitation.email, message: message)
+      other_errors = invitation.errors.reject { |e| %w[external_id email base].include?(e.attribute.to_s) }
+      if other_errors.any?
+        message = other_errors.map { |e| invitation.errors.full_message(e.attribute, e.message) }.
+                  to_sentence
+        errors << t('errors.course.user_invitations.other_error', email: email, message: message)
       end
       errors
     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
@@ -182,10 +182,10 @@ def parse_file_phantom(phantom)
     TRUE_VALUES.include?(phantom.downcase)
   end
 
-  # Parses file value for external ID of a student.
-  # Sets external_id as nil if value is not specified.
+  # Parses file value for an invitation's timeline algorithm.
+  # Sets timeline algorithm as course default if value is not specified.
   #
-  # @param [String|nil] External ID as specified in the CSV file.
+  # @param [String|nil] Timeline algorithm as specified in the CSV file.
   # @return [Integer] The enum integer for +Course::UserInvitation.timeline_algorithm+ matching the input.
   #                   current_course.default_timeline_algorithm is returned by default.
   def parse_file_timeline_algorithm(timeline_algorithm)
diff --git a/app/services/course/user_registration_service.rb b/app/services/course/user_registration_service.rb
@@ -52,7 +52,7 @@ def find_or_create_course_user!(registration, invitation = nil)
     role = invitation.try(:role) || :student
     phantom = invitation.try(:phantom) || false
     timeline_algorithm = invitation.try(:timeline_algorithm) || registration.course.default_timeline_algorithm
-    external_id = invitation.try(:external_id)
+    external_id = invitation.try(:external_id) || nil
 
     registration.course_user =
       CourseUser.find_or_create_by!(course: registration.course, user: registration.user,
diff --git a/client/app/bundles/course/user-invitations/components/buttons/InvitationActionButtons.tsx b/client/app/bundles/course/user-invitations/components/buttons/InvitationActionButtons.tsx
@@ -46,6 +46,10 @@ const translations = defineMessages({
     id: 'course.userInvitations.InvitationActionButtons.deletionFailure',
     defaultMessage: 'Failed to delete user - {error}',
   },
+  deletionFailureGeneric: {
+    id: 'course.userInvitations.InvitationActionButtons.deletionFailureGeneric',
+    defaultMessage: 'Failed to delete user.',
+  },
 });
 
 const InvitationActionButtons: FC<Props> = (props) => {
@@ -89,9 +93,11 @@ const InvitationActionButtons: FC<Props> = (props) => {
         if (Array.isArray(rawErrors)) errorList = rawErrors;
         else if (typeof rawErrors === 'string') errorList = [rawErrors];
         else errorList = [];
-        toast.error(
-          t(translations.deletionFailure, { error: errorList[0] ?? '' }),
-        );
+        if (errorList[0]) {
+          toast.error(t(translations.deletionFailure, { error: errorList[0] }));
+        } else {
+          toast.error(t(translations.deletionFailureGeneric));
+        }
         throw error;
       });
   };
diff --git a/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx b/client/app/bundles/course/user-invitations/pages/InviteUsersFileUpload/index.tsx
@@ -84,6 +84,11 @@ const translations = defineMessages({
     defaultMessage:
       'Failed to invite users. Please ensure your data is formatted correctly - {error}',
   },
+  failureGeneric: {
+    id: 'course.userInvitations.InviteUsersFileUpload.failureGeneric',
+    defaultMessage:
+      'Failed to invite users. Please ensure your data is formatted correctly.',
+  },
 });
 
 const InviteUsersFileUpload: FC<Props> = (props) => {
@@ -112,12 +117,16 @@ const InviteUsersFileUpload: FC<Props> = (props) => {
         if (Array.isArray(rawErrors)) errorList = rawErrors;
         else if (typeof rawErrors === 'string') errorList = [rawErrors];
         else errorList = [];
-        const first = errorList[0] ?? '';
+        const first = errorList[0];
         const overflow =
           errorList.length > 1 ? ` (and ${errorList.length - 1} more)` : '';
-        toast.error(t(translations.failure, { error: first + overflow }), {
-          autoClose: false,
-        });
+        if (first) {
+          toast.error(t(translations.failure, { error: first + overflow }), {
+            autoClose: false,
+          });
+        } else {
+          toast.error(t(translations.failureGeneric), { autoClose: false });
+        }
       });
   };
 
diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/ExternalIdField.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/ExternalIdField.tsx
@@ -58,7 +58,7 @@ const ExternalIdField = (props: ExternalIdFieldProps): JSX.Element => {
     <InlineEditTextField
       key={user.id}
       allowEmpty
-      className="course_user_external_id text-neutral-400"
+      className="course_user_external_id"
       onUpdate={(newId): Promise<void> => handleIdUpdate(newId)}
       updateValue={(): void => {}}
       value={user.externalId ?? ''}
diff --git a/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx b/client/app/bundles/course/users/components/tables/ManageUsersTable/index.tsx
@@ -63,7 +63,7 @@ const ManageUsersTable = (props: ManageUsersTableProps): JSX.Element => {
       of: 'externalId',
       title: t(tableTranslations.externalId),
       sortable: false,
-      searchable: false,
+      searchable: true,
       cell: (user) => <ExternalIdField for={user} />,
       csvDownloadable: true,
     },
diff --git a/client/app/types/course/userInvitations.ts b/client/app/types/course/userInvitations.ts
@@ -35,6 +35,7 @@ export interface InvitationPostData {
   id?: string | undefined;
   name: string;
   email: string;
+  externalId?: string | null;
   role: string;
   phantom: boolean;
   timelineAlgorithm?: string | undefined;
@@ -45,6 +46,7 @@ export interface InvitationsPostData {
     id?: string | undefined;
     name: string;
     email: string;
+    externalId?: string | null;
     role: string;
     phantom: boolean;
     timelineAlgorithm?: string | undefined;
diff --git a/client/locales/en.json b/client/locales/en.json
@@ -6945,6 +6945,9 @@
   "course.userInvitations.InviteUsersFileUpload.failure": {
     "defaultMessage": "Failed to invite users. Please ensure your data is formatted correctly - {error}"
   },
+  "course.userInvitations.InviteUsersFileUpload.failureGeneric": {
+    "defaultMessage": "Failed to invite users. Please ensure your data is formatted correctly."
+  },
   "course.userInvitations.InviteUsersFileUpload.fileUploadExample": {
     "defaultMessage": "Name,Email,Role,Phantom,ExternalId{br}John,test1@example.org,student,y,A0123456{br}Mary,test2@example.org,teaching_assistant,n,A0123457"
   },
@@ -6984,6 +6987,9 @@
   "course.userInvitations.InvitationActionButtons.deletionFailure": {
     "defaultMessage": "Failed to delete user - {error}"
   },
+  "course.userInvitations.InvitationActionButtons.deletionFailureGeneric": {
+    "defaultMessage": "Failed to delete user."
+  },
   "course.userInvitations.InvitationActionButtons.deletionSuccess": {
     "defaultMessage": "Invitation for {name} was deleted."
   },
diff --git a/client/locales/ko.json b/client/locales/ko.json
@@ -6935,6 +6935,9 @@
   "course.userInvitations.InviteUsersFileUpload.failure": {
     "defaultMessage": "사용자 초대에 실패했습니다. 데이터가 올바르게 형식화되었는지 확인하세요 - {error}"
   },
+  "course.userInvitations.InviteUsersFileUpload.failureGeneric": {
+    "defaultMessage": "사용자 초대에 실패했습니다. 데이터가 올바르게 형식화되었는지 확인하세요."
+  },
   "course.userInvitations.InviteUsersFileUpload.fileUploadExample": {
     "defaultMessage": "이름,이메일,역할,팬텀,외부 ID{br}John,test1@example.org,학생,y,A0123456{br}Mary,test2@example.org,티칭 어시스턴트,n,A0123457"
   },
diff --git a/client/locales/zh.json b/client/locales/zh.json
@@ -6929,6 +6929,9 @@
   "course.userInvitations.InviteUsersFileUpload.failure": {
     "defaultMessage": "邀请用户失败。请确保你的数据格式正确 - {error}"
   },
+  "course.userInvitations.InviteUsersFileUpload.failureGeneric": {
+    "defaultMessage": "邀请用户失败。请确保你的数据格式正确。"
+  },
   "course.userInvitations.InviteUsersFileUpload.fileUploadExample": {
     "defaultMessage": "姓名，电子邮件,Role,Phantom,外部编号{br}John,test1@example.org,student,y,A0123456{br}Mary,test2@example.org,teaching_assistant,n,A0123457"
   },
diff --git a/config/locales/en/errors.yml b/config/locales/en/errors.yml
@@ -15,9 +15,11 @@ en:
           owner: 'Owner'
           phantom: 'Phantom'
       user_invitations:
-        duplicate_user: '%{user} appears more than once in the submission.'
-        invalid_email: '%{email} is invalid: %{message}.'
-        duplicate_external_id: 'External ID "%{external_id}" is already taken.'
+        already_enrolled: '%{email} could not be processed: already enrolled in this course.'
+        duplicate_invitation: '%{email} could not be processed: already has a pending invitation.'
+        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}.'
       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
@@ -15,9 +15,11 @@ ko:
           owner: '소유자'
           phantom: '팬텀'
       user_invitations:
-        duplicate_user: '%{user}가 제출물에 여러 번 나타납니다.'
-        invalid_email: '%{email}이(가) 유효하지 않습니다: %{message}.'
-        duplicate_external_id: '외부 ID "%{external_id}"는 이미 사용 중입니다.'
+        already_enrolled: '%{email}을(를) 처리할 수 없습니다: 이미 이 과정에 등록되어 있습니다.'
+        duplicate_invitation: '%{email}을(를) 처리할 수 없습니다: 이미 대기 중인 초대가 있습니다.'
+        invalid_email: '%{email}을(를) 처리할 수 없습니다: 유효하지 않은 이메일: %{message}.'
+        duplicate_external_id: '%{email}을(를) 처리할 수 없습니다: 외부 ID "%{external_id}"는 이미 사용 중입니다.'
+        other_error: '%{email}을(를) 처리할 수 없습니다: %{message}.'
       user_registrations:
         invalid_code: '잘못된 초대 코드를 입력하셨습니다.'
         code_taken: >
diff --git a/config/locales/zh/errors.yml b/config/locales/zh/errors.yml
@@ -15,9 +15,11 @@ zh:
           owner: '拥有者'
           phantom: '虚拟用户'
       user_invitations:
-        duplicate_user: '%{user}在提交中出现了多次。'
-        invalid_email: '%{email}无效：%{message}。'
-        duplicate_external_id: '外部编号"%{external_id}"已被使用。'
+        already_enrolled: '%{email}无法处理：已加入该课程。'
+        duplicate_invitation: '%{email}无法处理：已有待处理的邀请。'
+        invalid_email: '%{email}无法处理：邮箱无效：%{message}。'
+        duplicate_external_id: '%{email}无法处理：外部编号"%{external_id}"已被使用。'
+        other_error: '%{email}无法处理：%{message}。'
       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
@@ -148,10 +148,39 @@ def replace_with_erroneous_course
           expect(errors.any? { |e| e.include?('EXT_DUPE') }).to be(true)
         end
 
-        it 'does not report the error as a duplicate user submission error' do
+        it 'does not report the error as an enrolled or pending invitation error' do
           subject
           errors = controller.send(:aggregate_errors)
-          expect(errors.none? { |e| e.include?('appears more than once') }).to be(true)
+          expect(errors.none? { |e| e.include?('already enrolled') || e.include?('pending invitation') }).
+            to be(true)
+        end
+      end
+
+      context 'when a CourseUser has an already-enrolled error' do
+        before { replace_with_erroneous_course }
+
+        it 'surfaces an already enrolled error message' do
+          errors = controller.send(:aggregate_errors)
+          expect(errors.any? { |e| e.include?('already enrolled') }).to be(true)
+        end
+      end
+
+      context 'when an invitation has a pending invitation for the same email' do
+        let(:course_with_dup_invitation) do
+          create(:course, :enrollable).tap do |c|
+            c.invitations.create!(name: 'Alice', email: 'pending@example.com')
+            c.invitations.build(name: 'Alice 2', email: 'pending@example.com')
+          end
+        end
+
+        before do
+          dup_course = course_with_dup_invitation
+          controller.define_singleton_method(:current_course) { dup_course }
+        end
+
+        it 'surfaces a duplicate invitation error message' do
+          errors = controller.send(:aggregate_errors)
+          expect(errors.any? { |e| e.include?('pending invitation') }).to be(true)
         end
       end
 
diff --git a/spec/features/course/student_management_spec.rb b/spec/features/course/student_management_spec.rb
@@ -26,9 +26,11 @@
 
       # change name
       within find("tr.course_user_#{student_to_update.id}") do
-        find('button.inline-edit-button', visible: false).click
-        find('input').set(new_name)
-        find('button.confirm-btn').click
+        within('.course_user_name') do
+          find('button.inline-edit-button', visible: :all).click
+          find('input').set(new_name)
+          find('button.confirm-btn').click
+        end
       end
       expect_toastify("#{student_to_update.name} was renamed to #{new_name}")
 
diff --git a/spec/fixtures/course/invitation_no_external_id_no_timeline.csv b/spec/fixtures/course/invitation_no_external_id_no_timeline.csv
@@ -0,0 +1,3 @@
+name,email,role,phantom
+Alice,alice@example.com,student,false
+Bob,bob@example.com,teaching_assistant,false
diff --git a/spec/models/course/user_invitation_spec.rb b/spec/models/course/user_invitation_spec.rb
@@ -53,6 +53,9 @@
         context 'when only a confirmed invitation in the same course has the same external_id' do
           let!(:confirmed) { create(:course_user_invitation, :confirmed, course: course, external_id: 'past-id') }
 
+          # The uniqueness check (UniqueExternalIdConcern) only queries unconfirmed invitations.
+          # A confirmed invitation means the user has already joined the course, so their external_id
+          # is now on the CourseUser record. The invitation row is no longer an active claim on the id.
           it 'is valid' do
             invitation = build(:course_user_invitation, course: course, external_id: 'past-id')
             expect(invitation).to be_valid
diff --git a/spec/models/course_user_spec.rb b/spec/models/course_user_spec.rb
@@ -106,6 +106,9 @@
       context 'when only a confirmed invitation in the same course has the same external_id' do
         let!(:invitation) { create(:course_user_invitation, :confirmed, course: course, external_id: 'confirmed-id') }
 
+        # The uniqueness check (UniqueExternalIdConcern) only queries unconfirmed invitations.
+        # A confirmed invitation means the user has already joined the course, so their external_id
+        # is now on the CourseUser record. The invitation row is no longer an active claim on the id.
         it 'is valid' do
           student = build(:course_student, course: course, external_id: 'confirmed-id')
           expect(student).to be_valid
diff --git a/spec/services/course/statistics/assessment_score_summary_download_service_spec.rb b/spec/services/course/statistics/assessment_score_summary_download_service_spec.rb
@@ -104,12 +104,9 @@
       end
 
       context 'when some students have external_id set' do
-        # Outer scope creates Student 1, 2, 3. Adding Student A (with ext_id) and Student B (without).
-        # Sorted alphabetically: Student 1[1], Student 2[2], Student 3[3], Student A[4], Student B[5]
-        let!(:student_with_ext_id) { create(:course_user, course: course, name: 'Student A', external_id: 'EXT001') }
-        let!(:student_without_ext_id) { create(:course_user, course: course, name: 'Student B') }
+        let!(:student_with_ext_id) { create(:course_user, course: course, name: 'Student 4', external_id: 'EXT001') }
+        let!(:student_without_ext_id) { create(:course_user, course: course, name: 'Student 5') }
 
-        # Submissions must be created before filepath so the service sees them
         let!(:submission_a1) do
           create(:submission, :published, assessment: assessment1, creator: student_with_ext_id.user)
         end
@@ -134,23 +131,23 @@
         end
 
         it 'student with external_id has it in the correct column' do
-          student_a_record = csv_lines[4]
-          expect(student_a_record[0]).to eq(student_with_ext_id.name)
-          expect(student_a_record[1]).to eq(student_with_ext_id.user.email)
-          expect(student_a_record[2]).to eq(student_with_ext_id.phantom? ? 'phantom' : 'normal')
-          expect(student_a_record[3]).to eq(submission_a1.grade.to_s)
-          expect(student_a_record[4]).to eq(submission_a2.grade.to_s)
-          expect(student_a_record[5]).to eq('EXT001')
+          fourth_student_record = csv_lines[4]
+          expect(fourth_student_record[0]).to eq(student_with_ext_id.name)
+          expect(fourth_student_record[1]).to eq(student_with_ext_id.user.email)
+          expect(fourth_student_record[2]).to eq(student_with_ext_id.phantom? ? 'phantom' : 'normal')
+          expect(fourth_student_record[3]).to eq(submission_a1.grade.to_s)
+          expect(fourth_student_record[4]).to eq(submission_a2.grade.to_s)
+          expect(fourth_student_record[5]).to eq('EXT001')
         end
 
         it 'student without external_id has empty string in external_id column' do
-          student_b_record = csv_lines[5]
-          expect(student_b_record[0]).to eq(student_without_ext_id.name)
-          expect(student_b_record[1]).to eq(student_without_ext_id.user.email)
-          expect(student_b_record[2]).to eq(student_without_ext_id.phantom? ? 'phantom' : 'normal')
-          expect(student_b_record[3]).to eq(submission_b1.grade.to_s)
-          expect(student_b_record[4]).to eq(submission_b2.grade.to_s)
-          expect(student_b_record[5]).to eq('')
+          fifth_student_record = csv_lines[5]
+          expect(fifth_student_record[0]).to eq(student_without_ext_id.name)
+          expect(fifth_student_record[1]).to eq(student_without_ext_id.user.email)
+          expect(fifth_student_record[2]).to eq(student_without_ext_id.phantom? ? 'phantom' : 'normal')
+          expect(fifth_student_record[3]).to eq(submission_b1.grade.to_s)
+          expect(fifth_student_record[4]).to eq(submission_b2.grade.to_s)
+          expect(fifth_student_record[5]).to eq('')
         end
       end
     end
diff --git a/spec/services/course/user_invitation_service_spec.rb b/spec/services/course/user_invitation_service_spec.rb
