Fix rerun of moderated ranked choice rounds requesting the wrong choices

Rerunning a moderated ranked choice signup round advanced every attendee
with a still-pending signup request one notch down their ranked choice
list, instead of re-requesting the same choice. Two compounding bugs
caused this, both rooted in priority being a list position scoped by
(user_con_profile, state):

- ExecuteRankedChoiceSignupService computed after_signup_ranked_choice
  after AcceptSignupRankedChoiceService had already moved the choice into
  the "requested" list, which renumbers the remaining pending choices. The
  unscoped priority comparison then recorded the wrong successor (e.g. the
  attendee's 3rd choice instead of their 2nd). It's now captured before
  the state change and scoped to pending choices.

- RerunModeratedRankedChoiceSignupRoundService#undo_decision restored the
  undone choice after its recorded successor instead of before it. It now
  inserts before the successor, falling back to :first when the successor
  was itself executed and :last when there was no successor. Decisions are
  undone in execution order so multi-signup rounds restore correctly, and
  decisions whose ranked choice has since been deleted no longer crash the
  rerun.

Adds test coverage for RerunModeratedRankedChoiceSignupRoundService, which
previously had none, plus .rubocop_todo.yml entries for pre-existing
offenses in the touched files that started firing with rubocop 1.88.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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