[237] Evaluation Form validates uniqueness of Phase (#294)

* 237 Add unique phase constraint on eval forms

---------

Co-authored-by: Stephen Chudleigh <[email protected]>

app/controllers/evaluation_forms_controller.rb

@@ -5,6 +5,7 @@ class EvaluationFormsController < ApplicationController
   before_action -> { authorize_user('challenge_manager') }
   before_action :set_evaluation_form, only: %i[show edit update destroy]
   before_action :set_evaluation_forms, only: %i[index]
+  before_action :set_available_phases, only: %i[new create edit update]
 
   # GET /evaluation_forms or /evaluation_forms.json
   def index; end
@@ -80,6 +81,20 @@ def set_evaluation_forms
       includes([:challenge, :phase])
   end
 
+  def set_available_phases
+    current_phase_id = @evaluation_form&.phase_id
+
+    @available_phases =
+      current_user.challenge_manager_challenges.includes(:phases).map do |challenge|
+        {
+          challenge:,
+          phases: challenge.phases.reject do |phase|
+            current_phase_id != phase.id && EvaluationForm.exists?(phase_id: phase.id)
+          end
+        }
+      end
+  end
+
   # Only allow a list of trusted parameters through.
   def evaluation_form_params
     permitted = params.require(:evaluation_form).

app/helpers/evaluation_forms_helper.rb

@@ -10,6 +10,18 @@ def challenge_phase_title(challenge, phase)
     "#{challenge.title} - Phase #{phase_number(challenge, phase)}"
   end
 
+  def options_for_available_phases(available_phases)
+    available_phases.flat_map do |entry|
+      challenge = entry[:challenge]
+      entry[:phases].map do |phase|
+        [
+          challenge_phase_title(challenge, phase).to_s,
+          "#{challenge.id}.#{phase.id}.#{phase.end_date.strftime('%m/%d/%Y')}"
+        ]
+      end
+    end
+  end
+
   def evaluation_period(evaluation_form)
     start_date = evaluation_form.phase.end_date.strftime("%m/%d/%Y")
     end_date = evaluation_form.closing_date.strftime("%m/%d/%Y")

app/models/evaluation_form.rb

@@ -32,9 +32,13 @@ class EvaluationForm < ApplicationRecord
   validates :instructions, presence: true
   validates :closing_date, presence: true
 
+  validates :phase_id, uniqueness: true
+
   validate :criteria_weights_must_sum_to_one_hundred
   validate :validate_unique_criteria_titles
 
+  private
+
   def validate_unique_criteria_titles
     titles = evaluation_criteria.reject(&:marked_for_destruction?).map(&:title)
 

app/views/evaluation_forms/_form.html.erb

@@ -44,13 +44,15 @@
       </label>
       <div class="usa-hint">Choose the challenge this form will evaluate.</div>
       <div class="usa-combo-box" data-default-value="<%= combo_box_default_value %>">
-        <select class="usa-select" required <%= if disabled then "disabled" end %> id="challenge-combo" title="challenge-combo" data-action="evaluation-form#handleChallengeSelect">
-          <% current_user.challenge_manager_challenges.includes(:phases).each do |challenge| %>
-            <% challenge.phases.each do |phase| %>
-              <option value="<%= "#{challenge.id}.#{phase.id}.#{phase.end_date.strftime("%m/%d/%Y")}" %>"><%= challenge_phase_title(challenge, phase) %></option>
-            <% end %>
-          <% end %>
-        </select>
+        <%= select_tag(
+          'challenge-combo',
+          options_for_select(options_for_available_phases(@available_phases)),
+          class: 'usa-select',
+          title: 'challenge-combo',
+          data: { action: 'evaluation-form#handleChallengeSelect' },
+          required: true,
+          disabled: disabled
+        ) %>
       </div>
 
       <%= inline_error(evaluation_form, :challenge_id) %>

db/migrate/20241125060011_add_unique_phase_constraint_to_evaluation_forms.rb

@@ -0,0 +1,13 @@
+class AddUniquePhaseConstraintToEvaluationForms < ActiveRecord::Migration[7.2]
+  def change
+    ActiveRecord::Base.connection.truncate_tables(
+      EvaluationForm.table_name,
+      EvaluationCriterion.table_name,
+      Evaluation.table_name,
+      EvaluationScore.table_name
+    )
+
+    remove_index :evaluation_forms, :phase_id, if_exists: true
+    add_index :evaluation_forms, :phase_id, unique: true
+  end
+end

db/structure.sql

@@ -1897,7 +1897,7 @@ CREATE INDEX index_evaluation_forms_on_challenge_id ON public.evaluation_forms U
 -- Name: index_evaluation_forms_on_phase_id; Type: INDEX; Schema: public; Owner: -
 --
 
-CREATE INDEX index_evaluation_forms_on_phase_id ON public.evaluation_forms USING btree (phase_id);
+CREATE UNIQUE INDEX index_evaluation_forms_on_phase_id ON public.evaluation_forms USING btree (phase_id);
 
 
 --
@@ -2465,6 +2465,7 @@ ALTER TABLE ONLY public.winners
 SET search_path TO "$user", public;
 
 INSERT INTO "schema_migrations" (version) VALUES
+(20241125060011),
 (20241120024946),
 (20241120024939),
 (20241115193801),

spec/factories/evaluation_form.rb

@@ -23,7 +23,11 @@
     # Assures proper points sum of 100 when weighted_scoring = 100
     # Skips initial validation on eval form create because of dependency
     before(:create) do
-      EvaluationForm.skip_callback(:validate, :before, :criteria_weights_must_sum_to_one_hundred)
+      if EvaluationForm._validate_callbacks.any? do |cb|
+        cb.kind == :before && cb.filter == :criteria_weights_must_sum_to_one_hundred
+      end
+        EvaluationForm.skip_callback(:validate, :before, :criteria_weights_must_sum_to_one_hundred)
+      end
     end
 
     after(:build) do |evaluation_form|
@@ -52,6 +56,7 @@
       else
         create_list(:evaluation_criterion, num_criteria, evaluation_form:)
       end
+
       evaluation_form.reload
     ensure
       EvaluationForm.set_callback(:validate, :before, :criteria_weights_must_sum_to_one_hundred)

spec/models/evaluation_form_spec.rb

@@ -34,6 +34,21 @@
       expect(evaluation_form).not_to be_valid
       expect(evaluation_form.errors[:closing_date]).to include("can't be blank")
     end
+
+    it 'validates the presence of a phase' do
+      phase = create(:phase)
+      evaluation_form = create(:evaluation_form, phase:)
+      expect(evaluation_form.phase).to eq(phase)
+    end
+
+    it 'requires a phase to be unique' do
+      phase = create(:phase)
+      create(:evaluation_form, phase:)
+
+      expect do
+        create(:evaluation_form, phase:)
+      end.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Phase has already been taken")
+    end
   end
 
   describe "scope" do

spec/system/evaluation_form_spec.rb

@@ -4,7 +4,7 @@
   let(:user) { create_user(role: "challenge_manager", status: "active") }
 
   describe "new evaluation form page" do
-    let!(:challenge) { create(:challenge, user:) }
+    let!(:challenge) { create(:challenge, user:, is_multi_phase: true) }
 
     before do
       system_login_user(user)
@@ -49,6 +49,36 @@
       expect(page).to(be_axe_clean)
     end
 
+    it "only shows phases without existing evaluation forms as options for phase select" do
+      create(:evaluation_form, phase: challenge.phases[0])
+      new_challenge = create(:challenge, user:)
+
+      # Generate taken and non taken challenge_phase_title values
+      invalid_phase = challenge.phases[0]
+      invalid_challenge_phase_title = challenge_phase_title(challenge, invalid_phase)
+      valid_phase = challenge.phases[1]
+      valid_challenge_phase_title = challenge_phase_title(challenge, valid_phase)
+      new_challenge.phases[0]
+      new_challenge_phase_title = challenge_phase_title(challenge, valid_phase)
+
+      visit new_evaluation_form_path
+
+      expect_form_phase_select_to_not_contain(invalid_challenge_phase_title)
+      expect_form_phase_select_to_contain(valid_challenge_phase_title)
+      expect_form_phase_select_to_contain(new_challenge_phase_title)
+    end
+
+    it "shows no results found if all phases are taken" do
+      # Make an evaluation form for all 3 initial factory challenge phases
+      create(:evaluation_form, phase: challenge.phases[0])
+      create(:evaluation_form, phase: challenge.phases[1])
+      create(:evaluation_form, phase: challenge.phases[2])
+
+      visit new_evaluation_form_path
+
+      expect_form_phase_select_to_be_empty
+    end
+
     it "prevents form submission and focuses first missing required field" do
       title = "Prevents form submission #{Faker::Lorem.sentence(word_count: 3)}"
       visit new_evaluation_form_path
@@ -675,6 +705,30 @@ def expect_form_phase_to_equal(value)
   expect(find_by_id('challenge-combo').value).to eq(value)
 end
 
+def expect_form_phase_select_to_not_contain(value)
+  expect(page).to have_no_select(
+    class: "usa-combo-box__select",
+    with_options: [value],
+    visible: :all
+  )
+end
+
+def expect_form_phase_select_to_contain(value)
+  expect(page).to have_select(
+    class: "usa-combo-box__select",
+    with_options: [value],
+    visible: :all
+  )
+end
+
+def expect_form_phase_select_to_be_empty
+  expect(page).to have_select(
+    class: "usa-combo-box__select",
+    options: [],
+    visible: :all
+  )
+end
+
 def expect_form_instructions_to_equal(value)
   expect(find_by_id('evaluation_form_instructions').value).to eq(value)
 end