feat(invitations): external ID upsert via post-hoc apply in results dialog

app/controllers/course/user_invitations_controller.rb

@@ -15,7 +15,7 @@ def index
 
   def create
     result = invite
-    if result
+    if result.is_a?(Array)
       create_invitation_success(result)
     else
       propagate_errors
@@ -24,6 +24,13 @@ def create
     end
   end
 
+  def update_external_ids
+    invitation_service.update_external_ids(external_id_updates_params)
+    head :ok
+  rescue Course::UserInvitationService::ExternalIdConflict => e
+    render json: { conflictingExternalId: e.external_id }, status: :conflict
+  end
+
   def destroy
     if @invitation.destroy
       destroy_invitation_success
@@ -43,7 +50,8 @@ def resend_invitation
   end
 
   def resend_invitations
-    if invitation_service.resend_invitation(load_invitations)
+    @invitations = load_invitations
+    if invitation_service.resend_invitation(@invitations)
       resend_invitations_success
     else
       resend_invitations_failure
@@ -56,6 +64,12 @@ def toggle_registration
 
   private
 
+  def external_id_updates_params
+    params.permit(updates: [:type, :id, :external_id])[:updates].to_a.map do |u|
+      { type: u[:type], id: u[:id].to_i, external_id: u[:external_id] }
+    end
+  end
+
   def course_user_invitation_params
     @course_user_invitation_params ||= begin
       params[:course] = { invitations_attributes: {} } unless params.key?(:course)
@@ -93,7 +107,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,7 +132,8 @@ def invite_by_file?
 
   # Invites the users via the service object.
   #
-  # @return [Boolean] True if the invitation was successful.
+  # @return [Array] On success.
+  # @return [Boolean] false on failure.
   def invite
     invitation_service.invite(invitation_params)
   rescue CSV::MalformedCSVError => e
@@ -218,12 +233,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,
+                              pending_invitation_updates, pending_course_user_updates)
+    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,
+                               pending_invitation_updates: pending_invitation_updates,
+                               pending_course_user_updates: pending_course_user_updates })
   end
 
   # Enables or disables registration codes in the given course.
@@ -264,7 +283,7 @@ def resend_invitation_failure
   def resend_invitations_success
     respond_to do |format|
       format.json do
-        render partial: 'course_user_invitation_list', locals: { invitations: @invitations.reload }, status: :ok
+        render partial: 'course_user_invitation_list', locals: { invitations: @invitations }, status: :ok
       end
     end
   end

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

app/models/user.rb

@@ -7,6 +7,7 @@ class User < ApplicationRecord
   include UserSearchConcern
   include TimeZoneConcern
   include Generic::CollectionConcern
+
   model_stamper
   acts_as_reader
   mount_uploader :profile_photo, ImageUploader
@@ -93,7 +94,7 @@ def deleted
   #
   # @return [Boolean]
   def built_in?
-    id == User::SYSTEM_USER_ID || id == User::DELETED_USER_ID
+    [User::SYSTEM_USER_ID, User::DELETED_USER_ID].include?(id)
   end
 
   # Pick the default email and set it as primary email. This method would immediately set the
@@ -129,7 +130,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

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
 
@@ -35,35 +37,48 @@ def parse_invitations(users)
         parse_from_form(users)
       end
 
-    partition_unique_users(restrict_invitee_role(result))
+    restrict_invitee_role(result)
   end
 
-  # Partition users into unique (including first duplicate instance) and duplicate users.
+  # Partition users into unique and duplicate users.
+  #
+  # Email dedup applies to all rows. External-ID dedup applies only to rows
+  # whose email is NOT in +existing_account_emails+ — users with existing platform
+  # accounts are handled by the DB-aware process phase, so pre-deduping their
+  # external IDs here would incorrectly fail rows that the process phase would
+  # accept (e.g. an enrolled user re-uploaded with their current external ID).
   #
   # @param [Array<Hash>] users
-  # @return [
-  #   [Array<Hash>],
-  #   [Array<Hash>]
-  # ]
-  def partition_unique_users(users)
+  # @param [Set<String>] existing_account_emails Downcased emails of users who
+  #   already have a platform account. These rows skip external-ID dedup.
+  # @return [[Array<Hash>, Array<Hash>]]
+  def partition_unique_users(users, existing_account_emails = Set.new)
     users.each { |user| user[:email] = user[:email].downcase }
     seen_emails = Set.new
     seen_external_ids = Set.new
-    unique_users = []
-    duplicate_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))
-      elsif ext_id && seen_external_ids.include?(ext_id)
-        duplicate_users.push(user.merge(reason: :duplicate_external_id_in_file))
+    users.each_with_object([[], []]) do |user, (unique, failed)|
+      reason = duplicate_reason(user, seen_emails, seen_external_ids, existing_account_emails)
+      if reason
+        failed << user.merge(reason: reason)
       else
-        seen_emails.add(user[:email])
-        seen_external_ids.add(ext_id) if ext_id
-        unique_users << user
+        track_seen!(user, seen_emails, seen_external_ids, existing_account_emails)
+        unique << user
       end
     end
-    [unique_users, duplicate_users]
+  end
+
+  def duplicate_reason(user, seen_emails, seen_external_ids, existing_account_emails)
+    return :duplicate_email_in_file if seen_emails.include?(user[:email])
+
+    ext_id = user[:external_id].presence
+    return :duplicate_external_id_in_file \
+      if ext_id && !existing_account_emails.include?(user[:email]) && seen_external_ids.include?(ext_id)
+  end
+
+  def track_seen!(user, seen_emails, seen_external_ids, existing_account_emails)
+    seen_emails.add(user[:email])
+    ext_id = user[:external_id].presence
+    seen_external_ids.add(ext_id) if ext_id && !existing_account_emails.include?(user[:email])
   end
 
   # Change all invitees' roles to :student if inviter is a teaching_assistant.
@@ -116,8 +131,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 +147,23 @@ 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)
+
+    provided = row.map { |h| h&.downcase }.compact
+    expected_headers.first(provided.length) == provided
   end
 
   # Strips a row of whitespaces.

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

@@ -20,9 +20,10 @@ module Course::UserInvitationService::ProcessInvitationConcern
   # @return
   #   [Array<(Array<Course::UserInvitation>, Array<Course::UserInvitation>, Array<CourseUser>, Array<CourseUser>)>]
   #   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
+    Rails.logger.info("Processing invitations with taken external IDs: #{@taken_external_ids.to_a}")
     augment_user_objects(users)
     existing_users, new_users = users.partition { |user| user[:user].present? }
     [*invite_new_users(new_users),
@@ -36,7 +37,7 @@ def process_invitations(users)
   # @return [void]
   def augment_user_objects(users)
     email_user_mapping = find_existing_users(users.map { |user| user[:email] })
-    users.each { |user| user[:user] = email_user_mapping[user[:email]] }
+    users.each { |user| user[:user] ||= email_user_mapping[user[:email]] }
   end
 
   # Given a list of email addresses, returns a Hash containing the mappings from email addresses
@@ -67,17 +68,36 @@ 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
     end
     [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.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
+
   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 +108,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,17 +149,38 @@ 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
     end
     [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.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
+
   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)

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