Merge pull request #11216 from neinteractiveliterature/11215-fix-merge-users-signup-ranked-choices

Fix FK violation when merging users with signup_ranked_choices

Author: nbudin

.github/workflows/ci.yml

@@ -308,6 +308,8 @@ jobs:
       - minitest
       - minitest-system
     steps:
+      - name: Check out repository
+        uses: actions/checkout@v5
       - name: Download Minitest coverage
         uses: actions/download-artifact@v6
         with:
@@ -323,8 +325,6 @@ jobs:
         with:
           name: vitest-coverage
           path: vitest-coverage
-      - name: Check out repository
-        uses: actions/checkout@v5
       - name: Merge coverage reports
         run: ruby scripts/merge_coverage.rb merged-coverage.xml minitest-coverage/coverage.xml vitest-coverage/cobertura-coverage.xml minitest-system-coverage/coverage.xml
       - name: Generate Coverage Report

app/services/merge_users_service.rb

@@ -25,6 +25,7 @@ class Result < CivilService::Result
     { model: Run, field: :updated_by_id },
     { model: Signup, field: :updated_by_id },
     { model: SignupChange, field: :updated_by_id },
+    { model: SignupRankedChoice, field: :updated_by_id },
     { model: SignupRequest, field: :updated_by_id },
     { model: UserActivityAlert, field: :user_id }
   ].freeze
@@ -75,11 +76,25 @@ def merge_profile_if_losing(profile)
       model.where(field => profile.id).update_all(field => winning_profile_for_convention.id)
     end
 
+    merge_signup_ranked_choices(profile, winning_profile_for_convention)
+
     profile.staff_positions.each { |staff_position| winning_profile_for_convention.staff_positions << staff_position }
 
     profile.destroy!
   end
 
+  def merge_signup_ranked_choices(losing_profile, winning_profile)
+    losing_choices_by_state =
+      SignupRankedChoice.where(user_con_profile: losing_profile).order(:priority).group_by(&:state)
+
+    losing_choices_by_state.each do |state, choices|
+      max_priority = SignupRankedChoice.where(user_con_profile: winning_profile, state: state).maximum(:priority) || 0
+      choices.each_with_index do |choice, index|
+        choice.update_columns(user_con_profile_id: winning_profile.id, priority: max_priority + index + 1)
+      end
+    end
+  end
+
   def merge_record_keeping_fields
     RECORD_KEEPING_FIELDS.each do |record_keeping_field|
       model = record_keeping_field[:model]

scripts/merge_coverage.rb

@@ -18,6 +18,8 @@
 file_lines = Hash.new { |h, k| h[k] = Hash.new(0) }
 
 input_paths.each do |path|
+  abort "Error: coverage file not found: #{path}" unless File.exist?(path)
+
   doc = REXML::Document.new(File.read(path))
   doc
     .elements
@@ -27,7 +29,7 @@
         .elements
         .each("lines/line") { |line| file_lines[fname][line.attributes["number"].to_i] += line.attributes["hits"].to_i }
     end
-rescue StandardError => e
+rescue REXML::ParseException => e
   warn "Warning: could not parse #{path}: #{e}"
 end
 

test/services/merge_users_service_test.rb

@@ -1,14 +1,15 @@
-require 'test_helper'
+require "test_helper"
 
 class MergeUsersServiceTest < ActiveSupport::TestCase
-  it 'merges two users' do
+  it "merges two users" do
     profile1, profile2 = create_list(:user_con_profile, 2)
     user_id1, user_id2 = [profile1, profile2].map(&:user_id)
 
     MergeUsersService.new(
       user_ids: [user_id1, user_id2],
       winning_user_id: user_id1,
-      winning_user_con_profile_ids_by_convention_id: {}
+      winning_user_con_profile_ids_by_convention_id: {
+      }
     ).call!
 
     assert UserConProfile.find_by(id: profile1.id)
@@ -18,7 +19,83 @@ class MergeUsersServiceTest < ActiveSupport::TestCase
     assert_nil User.find_by(id: user_id2)
   end
 
-  it 'merges two profiles from the same convention' do
+  it "reassigns signup_ranked_choices updated_by to winning user" do
+    profile1, profile2 = create_list(:user_con_profile, 2)
+    user1 = profile1.user
+    user2 = profile2.user
+    run = create(:run, event: create(:event, convention: profile2.convention))
+    choice = create(:signup_ranked_choice, user_con_profile: profile2, target_run: run, updated_by: user2)
+
+    MergeUsersService.new(
+      user_ids: [user1.id, user2.id],
+      winning_user_id: user1.id,
+      winning_user_con_profile_ids_by_convention_id: {
+      }
+    ).call!
+
+    assert_equal user1.id, choice.reload.updated_by_id
+    assert_nil User.find_by(id: user2.id)
+  end
+
+  it "transfers signup_ranked_choices from losing profile to winning profile" do
+    profile1, profile2 = create_list(:user_con_profile, 2)
+    user_id1, user_id2 = [profile1, profile2].map(&:user_id)
+    profile3 = create(:user_con_profile, convention: profile1.convention, user: profile2.user)
+
+    # profile1 is the losing profile for the shared convention
+    # profile3 is the winning profile for the shared convention
+    run1 = create(:run, event: create(:event, convention: profile1.convention))
+    run2 = create(:run, event: create(:event, convention: profile1.convention))
+    choice1 = create(:signup_ranked_choice, user_con_profile: profile1, target_run: run1, updated_by: profile1.user)
+    choice2 = create(:signup_ranked_choice, user_con_profile: profile1, target_run: run2, updated_by: profile1.user)
+
+    MergeUsersService.new(
+      user_ids: [user_id1, user_id2],
+      winning_user_id: user_id1,
+      winning_user_con_profile_ids_by_convention_id: {
+        profile3.convention_id => profile3.id
+      }
+    ).call!
+
+    assert_equal profile3.id, choice1.reload.user_con_profile_id
+    assert_equal profile3.id, choice2.reload.user_con_profile_id
+    assert_equal [1, 2], [choice1.reload.priority, choice2.reload.priority].sort
+  end
+
+  it "transfers signup_ranked_choices when both profiles have choices" do
+    profile1, profile2 = create_list(:user_con_profile, 2)
+    user_id1, user_id2 = [profile1, profile2].map(&:user_id)
+    profile3 = create(:user_con_profile, convention: profile1.convention, user: profile2.user)
+
+    run1 = create(:run, event: create(:event, convention: profile1.convention))
+    run2 = create(:run, event: create(:event, convention: profile1.convention))
+    run3 = create(:run, event: create(:event, convention: profile1.convention))
+
+    # losing profile has priorities 1, 2; winning profile already has priority 1
+    winning_choice =
+      create(:signup_ranked_choice, user_con_profile: profile3, target_run: run3, updated_by: profile3.user)
+    losing_choice1 =
+      create(:signup_ranked_choice, user_con_profile: profile1, target_run: run1, updated_by: profile1.user)
+    losing_choice2 =
+      create(:signup_ranked_choice, user_con_profile: profile1, target_run: run2, updated_by: profile1.user)
+
+    MergeUsersService.new(
+      user_ids: [user_id1, user_id2],
+      winning_user_id: user_id1,
+      winning_user_con_profile_ids_by_convention_id: {
+        profile3.convention_id => profile3.id
+      }
+    ).call!
+
+    all_choices = SignupRankedChoice.where(user_con_profile: profile3).order(:priority)
+    assert_equal 3, all_choices.count
+    assert_equal [1, 2, 3], all_choices.map(&:priority)
+    assert_includes all_choices, winning_choice.reload
+    assert_includes all_choices, losing_choice1.reload
+    assert_includes all_choices, losing_choice2.reload
+  end
+
+  it "merges two profiles from the same convention" do
     profile1, profile2 = create_list(:user_con_profile, 2)
     user_id1, user_id2 = [profile1, profile2].map(&:user_id)
     profile3 = create(:user_con_profile, convention: profile1.convention, user: profile2.user)