Merge pull request #11836 from neinteractiveliterature/fix-ranked-choice-rerun-restore-order

Fix rerun of moderated ranked choice signup rounds requesting the wrong choices

Author: nbudin

.rubocop_todo.yml

@@ -37,6 +37,12 @@ Lint/MissingCopEnableDirective:
   Exclude:
     - 'app/graphql/types/mutation_type.rb'
 
+# Offense count: 2
+Lint/MissingSuper:
+  Exclude:
+    - 'app/services/execute_ranked_choice_signup_service.rb'
+    - 'app/services/rerun_moderated_ranked_choice_signup_round_service.rb'
+
 # Offense count: 1
 Lint/NoReturnInBeginEndBlocks:
   Exclude:
@@ -50,6 +56,7 @@ Metrics/BlockLength:
     - 'config/environments/production.rb'
     - 'config/initializers/doorkeeper.rb'
     - 'test/presenters/signup_count_presenter_test.rb'
+    - 'test/services/rerun_moderated_ranked_choice_signup_round_service_test.rb'
 
 # Offense count: 7
 # Configuration parameters: CountComments, Max, CountAsOne.
@@ -61,13 +68,27 @@ Metrics/ClassLength:
     - 'app/graphql/types/mutation_type.rb'
     - 'app/graphql/types/query_type.rb'
     - 'app/presenters/signup_count_presenter.rb'
+    - 'app/services/execute_ranked_choice_signup_service.rb'
     - 'test/presenters/signup_count_presenter_test.rb'
 
+# Offense count: 1
+# Configuration parameters: AllowedMethods, AllowedPatterns, Max.
+Metrics/CyclomaticComplexity:
+  Exclude:
+    - 'app/services/execute_ranked_choice_signup_service.rb'
+
 # Offense count: 1
 # Configuration parameters: Max.
 Metrics/ParameterLists:
   Exclude:
     - 'app/presenters/signup_count_presenter.rb'
+    - 'app/services/execute_ranked_choice_signup_service.rb'
+
+# Offense count: 1
+# Configuration parameters: AllowedMethods, AllowedPatterns, Max.
+Metrics/PerceivedComplexity:
+  Exclude:
+    - 'app/services/execute_ranked_choice_signup_service.rb'
 
 # Offense count: 1
 # Configuration parameters: ForbiddenPrefixes, AllowedMethods, MethodDefinitionMacros.

app/services/execute_ranked_choice_signup_service.rb

@@ -1,3 +1,4 @@
+# frozen_string_literal: true
 class ExecuteRankedChoiceSignupService < CivilService::Service
   class Result < CivilService::Result
     attr_accessor :decision
@@ -46,6 +47,11 @@ def initialize(
   end
 
   def inner_call
+    # Capture this before doing anything else: accepting the choice changes its state, and
+    # priority is a list position scoped by (user_con_profile, state), so the state change
+    # renumbers the choice itself as well as the remaining pending choices
+    after_signup_ranked_choice
+
     skip = skip_reason
 
     decision =
@@ -176,7 +182,14 @@ def do_waitlist
   end
 
   def after_signup_ranked_choice
-    @after_signup_ranked_choice ||=
-      user_con_profile.signup_ranked_choices.where("priority > ?", signup_ranked_choice.priority).order(:priority).first
+    return @after_signup_ranked_choice if defined?(@after_signup_ranked_choice)
+
+    @after_signup_ranked_choice =
+      user_con_profile
+        .signup_ranked_choices
+        .where(state: "pending")
+        .where("priority > ?", signup_ranked_choice.priority)
+        .order(:priority)
+        .first
   end
 end

app/services/rerun_moderated_ranked_choice_signup_round_service.rb

@@ -1,3 +1,4 @@
+# frozen_string_literal: true
 class RerunModeratedRankedChoiceSignupRoundService < CivilService::Service
   include SkippableAdvisoryLock
 
@@ -19,23 +20,38 @@ def inner_call
   private
 
   def decisions_to_undo
+    # Ordered by id so that choices are restored in the order they were executed; that way, if
+    # multiple decisions for the same user are undone, restore_signup_ranked_choice can count on
+    # earlier choices already being back in the pending list
     @decisions_to_undo ||=
-      signup_round.ranked_choice_decisions.joins(:signup_request).where(signup_requests: { state: "pending" })
+      signup_round
+        .ranked_choice_decisions
+        .joins(:signup_request)
+        .where(signup_requests: { state: "pending" })
+        .order(:id)
   end
 
   def undo_decision(decision)
-    signup_ranked_choice = decision.signup_ranked_choice
-    signup_request = decision.signup_request
+    restore_signup_ranked_choice(decision) if decision.signup_ranked_choice
+    decision.signup_request.destroy!
+  end
+
+  def restore_signup_ranked_choice(decision)
+    after_signup_ranked_choice = decision.after_signup_ranked_choice
     priority =
       (
-        if decision.after_signup_ranked_choice&.state == "pending"
-          { after: decision.after_signup_ranked_choice }
-        else
+        if after_signup_ranked_choice&.state == "pending"
+          { before: after_signup_ranked_choice }
+        elsif after_signup_ranked_choice
+          # The choice that followed this one was executed too; if its decision is also being
+          # undone, it will be re-inserted after this one
           :first
+        else
+          # Nothing followed this choice when it was executed
+          :last
         end
       )
 
-    signup_ranked_choice.update!(state: "pending", priority:)
-    signup_request.destroy!
+    decision.signup_ranked_choice.update!(state: "pending", priority:)
   end
 end

test/services/rerun_moderated_ranked_choice_signup_round_service_test.rb

@@ -0,0 +1,147 @@
+# frozen_string_literal: true
+require "test_helper"
+
+describe RerunModeratedRankedChoiceSignupRoundService do
+  let(:convention) { create(:convention, :with_notification_templates, signup_mode: "moderated") }
+  let(:signup_round) do
+    create(:signup_round, convention:, ranked_choice_order: "asc", maximum_event_signups: "1", start: 1.day.ago)
+  end
+  let(:one_player_registration_policy) do
+    RegistrationPolicy.new({ buckets: [{ key: "only_one_player", total_slots: 1, slots_limited: true }] })
+  end
+
+  def create_ranked_choices(user_con_profile, count)
+    (1..count).map do |i|
+      event = create(:event, convention:, registration_policy: one_player_registration_policy)
+      run = create(:run, event:, starts_at: convention.starts_at + (i * 5).hours)
+      create(:signup_ranked_choice, user_con_profile:, target_run: run)
+    end
+  end
+
+  def execute_round
+    ExecuteRankedChoiceSignupRoundService.new(signup_round:, whodunit: nil).call!
+  end
+
+  def rerun_round
+    max_decision_id = RankedChoiceDecision.maximum(:id) || 0
+    result = RerunModeratedRankedChoiceSignupRoundService.new(signup_round:, whodunit: nil).call!
+    assert result.success?
+    RankedChoiceDecision.where("id > ?", max_decision_id).order(:id).to_a
+  end
+
+  def pending_choice_ids(user_con_profile)
+    user_con_profile.signup_ranked_choices.where(state: "pending").order(:priority).pluck(:id)
+  end
+
+  it "records the next pending choice as after_signup_ranked_choice when executing" do
+    user_con_profile = create(:user_con_profile, convention:, lottery_number: 1, ranked_choice_fallback_action: "none")
+    choices = create_ranked_choices(user_con_profile, 3)
+
+    result = execute_round
+
+    signup_decision = result.decisions.find { |decision| decision.decision == "signup" }
+    assert_equal choices[0], signup_decision.signup_ranked_choice
+    assert_equal choices[1], signup_decision.after_signup_ranked_choice
+  end
+
+  it "re-requests the same choice for attendees whose requests were still pending" do
+    rejected_user = create(:user_con_profile, convention:, lottery_number: 1, ranked_choice_fallback_action: "none")
+    pending_user = create(:user_con_profile, convention:, lottery_number: 2, ranked_choice_fallback_action: "none")
+    rejected_user_choices = create_ranked_choices(rejected_user, 2)
+    pending_user_choices = create_ranked_choices(pending_user, 3)
+
+    execute_round
+    rejected_user_choices[0].reload.result_signup_request.update!(state: "rejected")
+    original_pending_request = pending_user_choices[0].reload.result_signup_request
+
+    rerun_round
+
+    # The attendee with the rejected request should advance to their second choice
+    assert_equal "requested", rejected_user_choices[1].reload.state
+    assert_equal "pending", rejected_user_choices[1].result_signup_request.state
+
+    # The attendee whose request was still pending should have the same choice re-requested
+    assert_equal "requested", pending_user_choices[0].reload.state
+    new_request = pending_user_choices[0].result_signup_request
+    assert_equal "pending", new_request.state
+    assert_not_equal original_pending_request.id, new_request.id
+    assert_equal pending_user_choices[0].target_run, new_request.target_run
+    assert_equal [pending_user_choices[1].id, pending_user_choices[2].id], pending_choice_ids(pending_user)
+  end
+
+  it "does not advance attendees with pending requests across multiple reruns" do
+    rejected_user = create(:user_con_profile, convention:, lottery_number: 1, ranked_choice_fallback_action: "none")
+    pending_user = create(:user_con_profile, convention:, lottery_number: 2, ranked_choice_fallback_action: "none")
+    rejected_user_choices = create_ranked_choices(rejected_user, 3)
+    pending_user_choices = create_ranked_choices(pending_user, 3)
+
+    execute_round
+    rejected_user_choices[0].reload.result_signup_request.update!(state: "rejected")
+    rerun_round
+    rejected_user_choices[1].reload.result_signup_request.update!(state: "rejected")
+    rerun_round
+
+    assert_equal "requested", rejected_user_choices[2].reload.state
+    assert_equal "pending", rejected_user_choices[2].result_signup_request.state
+
+    assert_equal "requested", pending_user_choices[0].reload.state
+    assert_equal "pending", pending_user_choices[0].result_signup_request.state
+    assert_equal pending_user_choices[0].target_run, pending_user_choices[0].result_signup_request.target_run
+    assert_equal [pending_user_choices[1].id, pending_user_choices[2].id], pending_choice_ids(pending_user)
+  end
+
+  it "restores a choice that was the last pending choice to the end of the list" do
+    user_con_profile = create(:user_con_profile, convention:, lottery_number: 1, ranked_choice_fallback_action: "none")
+    choices = create_ranked_choices(user_con_profile, 2)
+    create(:signup, run: choices[0].target_run, state: "confirmed")
+
+    result = execute_round
+    original_decisions = result.decisions.select { |decision| decision.user_con_profile == user_con_profile }
+    assert_equal %w[skip_choice signup], original_decisions.map(&:decision)
+
+    rerun_decisions = rerun_round.select { |decision| decision.user_con_profile == user_con_profile }
+
+    # The full first choice should still be considered (and skipped) before the second choice
+    # is re-requested
+    assert_equal %w[skip_choice signup], rerun_decisions.map(&:decision)
+    assert_equal choices[0], rerun_decisions[0].signup_ranked_choice
+    assert_equal choices[1], rerun_decisions[1].signup_ranked_choice
+    assert_equal [choices[0].id], pending_choice_ids(user_con_profile)
+  end
+
+  it "skips restoring choices that have been deleted" do
+    user_con_profile = create(:user_con_profile, convention:, lottery_number: 1, ranked_choice_fallback_action: "none")
+    choices = create_ranked_choices(user_con_profile, 2)
+
+    execute_round
+    original_request = choices[0].reload.result_signup_request
+    choices[0].destroy!
+
+    rerun_round
+
+    assert_not SignupRequest.exists?(original_request.id)
+    assert_equal "requested", choices[1].reload.state
+    assert_equal "pending", choices[1].result_signup_request.state
+  end
+
+  describe "with a round allowing multiple signups" do
+    let(:signup_round) do
+      create(:signup_round, convention:, ranked_choice_order: "asc", maximum_event_signups: "2", start: 1.day.ago)
+    end
+
+    it "restores multiple undone choices in their original order" do
+      user_con_profile =
+        create(:user_con_profile, convention:, lottery_number: 1, ranked_choice_fallback_action: "none")
+      choices = create_ranked_choices(user_con_profile, 3)
+
+      execute_round
+      assert_equal(%w[requested requested pending], choices.map { |choice| choice.reload.state })
+
+      rerun_decisions = rerun_round
+
+      assert_equal [choices[0], choices[1]], rerun_decisions.map(&:signup_ranked_choice)
+      assert_equal(%w[requested requested pending], choices.map { |choice| choice.reload.state })
+      assert_equal [choices[2].id], pending_choice_ids(user_con_profile)
+    end
+  end
+end