feat: add optional external_id field

Author: LWS49

app/controllers/concerns/course/users_controller_management_concern.rb

@@ -123,7 +123,7 @@ def should_update_personalized_timeline
 
   def course_user_params
     @course_user_params ||= params.require(:course_user).permit(
-      :user_id, :name, :timeline_algorithm, :role, :phantom, :reference_timeline_id
+      :user_id, :name, :timeline_algorithm, :role, :phantom, :reference_timeline_id, :external_id
     )
   end
 

app/controllers/course/user_invitations_controller.rb

@@ -19,7 +19,8 @@ def create
       create_invitation_success(result)
     else
       propagate_errors
-      render json: { errors: current_course.errors.full_messages.to_sentence }, status: :bad_request
+      errors = current_course.errors[:base]
+      render json: { errors: errors }, status: :bad_request
     end
   end
 
@@ -59,7 +60,8 @@ def course_user_invitation_params
     @course_user_invitation_params ||= begin
       params[:course] = { invitations_attributes: {} } unless params.key?(:course)
       params.require(:course).permit(:invitations_file, :registration_key,
-                                     invitations_attributes: [:name, :email, :role, :phantom, :timeline_algorithm])
+                                     invitations_attributes: [:name, :email, :role, :phantom,
+                                                              :timeline_algorithm, :external_id])
     end
   end
 
@@ -120,7 +122,7 @@ def invite_by_file?
   def invite
     invitation_service.invite(invitation_params)
   rescue CSV::MalformedCSVError => e
-    current_course.errors.add(:invitations_file, e.message)
+    current_course.errors.add(:base, e.message)
     false
   end
 
@@ -135,15 +137,7 @@ def invitation_service
   #
   # @return [void]
   def propagate_errors
-    propagate_errors_to_file if invite_by_file?
-  end
-
-  # Propagates errors from the generated records to the file.
-  #
-  # @return [void]
-  def propagate_errors_to_file
-    errors = aggregate_errors
-    current_course.errors.add(:invitations_file, errors.to_sentence) unless errors.empty?
+    aggregate_errors.each { |msg| current_course.errors.add(:base, msg) }
   end
 
   # Aggregates errors from all the known sources of failure.
@@ -157,9 +151,23 @@ def aggregate_errors
   #
   # @return [Array<String>]
   def invalid_course_user_errors
-    invalid_course_users.map do |course_user|
-      user = self.class.helpers.display_course_user(course_user)
-      t('errors.course.user_invitations.duplicate_user', user: user)
+    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',
+                    email: email, external_id: course_user.external_id)
+      end
+      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
   end
 
@@ -174,9 +182,25 @@ def invalid_course_users
   #
   # @return [Array<String>]
   def invalid_invitation_email_errors
-    invalid_invitations.map do |invitation|
-      message = invitation.errors.full_messages.to_sentence
-      t('errors.course.user_invitations.invalid_email', email: invitation.email, message: message)
+    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',
+                    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
+      errors << t('errors.course.user_invitations.duplicate_invitation', email: email) if invitation.errors[:base].any?
+      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
   end
 
@@ -254,7 +278,7 @@ def destroy_invitation_success
 
   def destroy_invitation_failure
     respond_to do |format|
-      format.json { render json: { errors: @invitation.errors.full_messages.to_sentence }, status: :bad_request }
+      format.json { render json: { errors: @invitation.errors.full_messages }, status: :bad_request }
     end
   end
 

app/models/concerns/course/unique_external_id_concern.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+# This concern validates that external IDs are unique within a course,
+# across both course users and pending invitations.
+#
+# Nil and blank external IDs are allowed.
+module Course::UniqueExternalIdConcern
+  extend ActiveSupport::Concern
+
+  included do
+    before_validation :normalize_external_id
+
+    validate :validate_unique_external_id_within_course
+  end
+
+  private
+
+  # Normalizes blank external IDs to nil.
+  #
+  # @return [void]
+  def normalize_external_id
+    self.external_id = nil if external_id.blank?
+  end
+
+  # Validates that the external ID is unique within the course,
+  # across both course users and invitations.
+  #
+  # @return [void]
+  def validate_unique_external_id_within_course
+    return if external_id.blank?
+
+    invitation_exists = Course::UserInvitation.
+                        unconfirmed.
+                        where(course_id: course_id, external_id: external_id).
+                        where.not(id: id).
+                        exists?
+
+    course_user_exists = CourseUser.
+                         where(course_id: course_id, external_id: external_id).
+                         where.not(id: id).
+                         exists?
+
+    return unless invitation_exists || course_user_exists
+
+    errors.add(:external_id, :taken)
+  end
+end

app/models/course/user_invitation.rb

@@ -4,6 +4,8 @@ class Course::UserInvitation < ApplicationRecord
   after_initialize :set_defaults, if: :new_record?
   before_validation :set_defaults, if: :new_record?
 
+  include Course::UniqueExternalIdConcern
+
   validates :email, format: { with: Devise.email_regexp }, if: :email_changed?
   validates :name, presence: true
   validates :role, presence: true

app/models/course_user.rb

@@ -3,6 +3,7 @@ class CourseUser < ApplicationRecord
   include CourseUser::StaffConcern
   include CourseUser::LevelProgressConcern
   include CourseUser::TodoConcern
+  include Course::UniqueExternalIdConcern
 
   after_initialize :set_defaults, if: :new_record?
   before_validation :set_defaults, if: :new_record?

app/models/user.rb

@@ -129,6 +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,
                        creator: self,
                        updater: self)
   end

app/models/user/email.rb

@@ -35,8 +35,14 @@ def accept_all_pending_invitations
           unconfirmed_invitation.confirm!(confirmer: user)
           next
         end
-        user.build_course_user_from_invitation(unconfirmed_invitation)
-        unconfirmed_invitation.confirm!(confirmer: user) if user.save && user.persisted?
+        # Confirm the invitation before saving the CourseUser so that the
+        # UniqueExternalIdConcern validation doesn't reject the new CourseUser
+        # for sharing an external_id with what is now a confirmed invitation.
+        CourseUser.transaction do
+          unconfirmed_invitation.confirm!(confirmer: user)
+          user.build_course_user_from_invitation(unconfirmed_invitation)
+          raise ActiveRecord::Rollback unless user.save && user.persisted?
+        end
       end
     end
   end

app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb

@@ -48,11 +48,16 @@ def parse_invitations(users)
   def partition_unique_users(users)
     users.each { |user| user[:email] = user[:email].downcase }
     unique_users = {}
+    seen_external_ids = {}
     duplicate_users = []
     users.each do |user|
+      ext_id = user[:external_id].presence
       if unique_users.key?(user[:email])
-        duplicate_users.push(user)
+        duplicate_users.push(user.merge(reason: :duplicate_email))
+      elsif ext_id && seen_external_ids.key?(ext_id)
+        duplicate_users.push(user.merge(reason: :duplicate_external_id))
       else
+        seen_external_ids[ext_id] = true if ext_id
         unique_users[user[:email]] = user
       end
     end
@@ -86,7 +91,8 @@ def parse_from_form(users)
         email: value[:email],
         role: value[:role],
         phantom: phantom,
-        timeline_algorithm: value[:timeline_algorithm] }
+        timeline_algorithm: value[:timeline_algorithm],
+        external_id: value[:external_id] }
     end
   end
 
@@ -144,11 +150,17 @@ def parse_file_row(row)
     return nil if row[1].blank?
 
     row[0] = row[1] if row[0].blank?
-
     role = parse_file_role(row[2])
     phantom = parse_file_phantom(row[3])
-    timeline_algorithm = parse_file_timeline_algorithm(row[4])
-    { name: row[0], email: row[1], role: role, phantom: phantom, timeline_algorithm: timeline_algorithm }
+    if @current_course.show_personalized_timeline_features?
+      timeline_algorithm = parse_file_timeline_algorithm(row[4])
+      external_id = parse_file_external_id(row[5])
+    else
+      external_id = parse_file_external_id(row[4])
+      timeline_algorithm = parse_file_timeline_algorithm(nil)
+    end
+    { name: row[0], email: row[1], role: role, phantom: phantom,
+      timeline_algorithm: timeline_algorithm, external_id: external_id }
   end
 
   # Parses the role column from the CSV file.
@@ -188,6 +200,17 @@ def parse_file_timeline_algorithm(timeline_algorithm)
     symbol || @current_course.default_timeline_algorithm
   end
 
+  # Parses file value for an invitation's external ID.
+  # Returns nil if value is not specified.
+  #
+  # @param [String|nil] external_id External ID as specified in the CSV file.
+  # @return [String|nil] The external ID string, or nil if blank.
+  def parse_file_external_id(external_id)
+    return nil if external_id.blank?
+
+    external_id
+  end
+
   # Removes the UTF-8 byte order mark (BOM) from the string.
   # The BOM exists at the start of in CSVs (optionally) to indicate the
   # encoding of the file.

app/services/concerns/course/user_invitation_service/process_invitation_concern.rb

@@ -73,6 +73,7 @@ def add_existing_users(users)
           @current_course.course_users.build(user: user[:user], name: user[:name],
                                              role: user[:role], phantom: user[:phantom],
                                              timeline_algorithm: @current_course.default_timeline_algorithm,
+                                             external_id: user[:external_id],
                                              creator: @current_user, updater: @current_user)
         @current_course.enrol_requests.pending.find_by(user: user[:user].id)&.destroy!
       end
@@ -113,7 +114,8 @@ def invite_new_users(users)
         new_invitations <<
           @current_course.invitations.build(name: user[:name], email: user[:email],
                                             role: user[:role], phantom: user[:phantom],
-                                            timeline_algorithm: user[:timeline_algorithm])
+                                            timeline_algorithm: user[:timeline_algorithm],
+                                            external_id: user[:external_id])
       end
     end
 

app/services/course/statistics/assessments_score_summary_download_service.rb

@@ -46,6 +46,7 @@ def load_total_grades
 
     @submission_grade_hash = submission_grade_hash
     @all_students = @course.course_users.students.order_alphabetically.preload(user: :emails)
+    @include_external_id = @all_students.any? { |s| s.external_id.present? }
   end
 
   def submission_grade_hash
@@ -67,15 +68,15 @@ def download_score_summary(csv)
       I18n.t('csv.score_summary.headers.name'),
       I18n.t('csv.score_summary.headers.email'),
       I18n.t('csv.score_summary.headers.type'),
-      *@assessments.map(&:title)
+      *@assessments.map(&:title),
+      *(@include_external_id ? [I18n.t('csv.score_summary.headers.external_id')] : [])
     ]
 
     # content
     @all_students.each do |student|
       csv << [student.name, student.user.email, student.phantom? ? 'phantom' : 'normal',
-              *@assessments.flat_map do |assessment|
-                @submission_grade_hash[[student.id, assessment.id]] || ''
-              end]
+              *@assessments.flat_map { |a| @submission_grade_hash[[student.id, a.id]] || '' },
+              *(@include_external_id ? [student.external_id || ''] : [])]
     end
   end
 end