[296] Evaluation Form Validation - Part 2 (#327)

* 296 Inline error unique titles and weighted scores * 296 Newly added criteria collapse existing * 296 Criteria is exapnded if it has errors on save * 296 Simplify validate presence function * 296 Change weighted_scoring to scale_type string * 296 Override default phase presence error for form --------- Co-authored-by: Stephen Chudleigh <[email protected]> Co-authored-by: Stephen Chudleigh <[email protected]>
GSA · Dec 18, 2024 · 0788919 · 0788919
1 parent e1ce5be
commit 0788919
Show file tree

Hide file tree

Showing 25 changed files with 428 additions and 287 deletions.
diff --git a/app/assets/stylesheets/application.sass.scss b/app/assets/stylesheets/application.sass.scss
@@ -21,3 +21,7 @@ dialog::backdrop {
 #add-criteria-button.usa-button {
   background-color: #4d8055;
 }
+
+.usa-combo-box .border-secondary + input {
+  border: 1px solid red;
+}
diff --git a/app/controllers/evaluation_forms_controller.rb b/app/controllers/evaluation_forms_controller.rb
@@ -2,6 +2,8 @@
 
 # Controller for evaluation forms CRUD actions.
 class EvaluationFormsController < ApplicationController
+  helper FormHelper
+
   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]
@@ -99,7 +101,7 @@ def set_available_phases
   def evaluation_form_params
     permitted = params.require(:evaluation_form).
       permit(:title, :instructions, :phase_id, :status, :comments_required,
-             :weighted_scoring, :publication_date, :closing_date, :challenge_id,
+             :scale_type, :publication_date, :closing_date, :challenge_id,
              evaluation_criteria_attributes: [
                :id, :title, :description, :points_or_weight, :scoring_type,
                :option_range_start, :option_range_end, :_destroy,

diff --git a/app/helpers/evaluation_forms_helper.rb b/app/helpers/evaluation_forms_helper.rb
@@ -29,11 +29,6 @@ def evaluation_period(evaluation_form)
     "#{start_date} - #{end_date}"
   end
 
-  def inline_error(evaluation_form, field)
-    error = evaluation_form.errors[field].present? ? evaluation_form.errors[field].first : ""
-    tag.span(error, class: "text-secondary font-body-2xs", id: "evaluation_form_#{field}_error")
-  end
-
   def criteria_field_id(form, attribute, is_template)
     prefix = "evaluation_form_evaluation_criteria_attributes"
 

diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+# Helpers for rendering various form elements and errors
+module FormHelper
+  def label_error_class(form, field)
+    object = form.object
+    object.errors[field].present? ? "text-secondary" : ""
+  end
+
+  def input_error_class(form, field)
+    object = form.object
+    object.errors[field].present? ? "border-secondary" : ""
+  end
+
+  def inline_error(form, field)
+    object = form.object
+    field_id = (form.object_name + "_#{field}_error").gsub(/[\[\]]/, "_").squeeze('_')
+    error = object.errors[field].present? ? object.errors[field].join(", ") : ""
+
+    tag.span(error, class: "text-secondary font-body-2xs", id: field_id)
+  end
+end
diff --git a/app/javascript/controllers/evaluation_criteria_controller.js b/app/javascript/controllers/evaluation_criteria_controller.js
@@ -16,6 +16,9 @@ export default class extends Controller {
     this.replacePlaceholders(newCriteria);
     this.enableInputs(newCriteria);
 
+    this.collapseAllCriteria();
+    this.expandCriterion(newCriteria);
+
     this.criteriaListTarget.appendChild(newCriteria);
 
     this.updateCriteriaTitles();
@@ -40,6 +43,26 @@ export default class extends Controller {
     this.updateCriteriaTitles();
   }
 
+  collapseAllCriteria() {
+    const accordionButtons = this.element.querySelectorAll(
+      ".usa-accordion__button"
+    );
+    const accordions = this.element.querySelectorAll(".usa-accordion__content");
+
+    accordionButtons.forEach((button) =>
+      button.setAttribute("aria-expanded", false)
+    );
+    accordions.forEach((content) => content.setAttribute("hidden", ""));
+  }
+
+  expandCriterion(criterion) {
+    const accordionButton = criterion.querySelector(".usa-accordion__button");
+    const accordionContent = criterion.querySelector(".usa-accordion__content");
+
+    if (accordionButton) accordionButton.setAttribute("aria-expanded", true);
+    if (accordionContent) accordionContent.removeAttribute("hidden");
+  }
+
   toggleScoringType(event) {
     const row = event.target.closest(".criteria-row");
     const scoringType = row.querySelector(".scoring-type-radio:checked").value;
@@ -171,7 +194,7 @@ export default class extends Controller {
       .querySelectorAll(".criteria-option-label-row")
       .forEach((labelRow, index) => {
         labelRow.style.display =
-          index >= start && index <= end ? "flex" : "none";
+          index >= start && index <= end ? "block" : "none";
         const input = labelRow.querySelector("input");
         input.disabled = index < start || index > end;
       });

diff --git a/app/javascript/controllers/evaluation_form_controller.js b/app/javascript/controllers/evaluation_form_controller.js
@@ -21,11 +21,10 @@ export default class extends Controller {
         "data-min-date",
         `${year}-${month}-${day}`
       );
-
-      this.updateErrorMessage("evaluation_form_challenge_id", "");
-      this.updateErrorMessage("evaluation_form_phase_id", "");
     } else {
-      this.updateErrorMessage("evaluation_form_challenge_id", "can't be blank");
+      this.challengeIDTarget.value = null;
+      this.phaseIDTarget.value = null;
+
       this.startDateTarget.innerHTML = "mm/dd/yyyy";
     }
   }
@@ -72,15 +71,43 @@ export default class extends Controller {
   }
 
   validatePresence(e) {
-    if (!e.target.value) {
-      e.target.classList.add("border-secondary");
-      this.updateErrorMessage(e.target.id, "can't be blank");
+    const target = e.target;
+    const formGroup = target.closest(".usa-form-group");
+    const fieldName = target.dataset.fieldName || target.id;
+
+    const label = this.findLabel(target, formGroup);
+
+    if (!target.value) {
+      this.addErrorClasses(target, label);
+      this.updateErrorMessage(fieldName, "can't be blank");
     } else {
-      e.target.classList.remove("border-secondary");
-      this.updateErrorMessage(e.target.id, "");
+      this.removeErrorClasses(target, label);
+      this.updateErrorMessage(fieldName, "");
     }
   }
 
+  findLabel(target, formGroup) {
+    const isSelect =
+      target.tagName === "SELECT" ||
+      target.classList.contains("usa-combo-box__input");
+    const isRadio = target.type === "radio";
+
+    const labelId = isSelect ? target.name : target.id;
+    const labelQuery = isRadio ? "legend" : `label[for="${labelId}"]`;
+
+    return formGroup.querySelector(labelQuery);
+  }
+
+  addErrorClasses(target, label) {
+    target.classList.add("border-secondary");
+    if (label) label.classList.add("text-secondary");
+  }
+
+  removeErrorClasses(target, label) {
+    target.classList.remove("border-secondary");
+    if (label) label.classList.remove("text-secondary");
+  }
+
   updateErrorMessage(field, message) {
     document.getElementById(field + "_error").innerHTML = message;
   }

diff --git a/app/models/evaluation_criterion.rb b/app/models/evaluation_criterion.rb
@@ -37,4 +37,19 @@ class EvaluationCriterion < ApplicationRecord
   validates :title, length: { maximum: 150 }
   validates :description, length: { maximum: 1000 }
   validates :points_or_weight, numericality: { only_integer: true }
+  validates :scoring_type, presence: true
+
+  validate :validate_option_labels_not_blank, if: -> { rating? || binary? }
+
+  private
+
+  def validate_option_labels_not_blank
+    return unless option_labels.is_a?(Hash)
+
+    option_labels.each do |key, value|
+      if value.blank?
+        errors.add("option_labels_#{key}", "can't be blank")
+      end
+    end
+  end
 end
diff --git a/app/models/evaluation_form.rb b/app/models/evaluation_form.rb
@@ -8,7 +8,7 @@
 #  title             :string           not null
 #  instructions      :string           not null
 #  comments_required :boolean          default(FALSE)
-#  weighted_scoring  :boolean          default(FALSE)
+#  scale_type        :string           not null
 #  closing_date      :date             not null
 #  challenge_id      :bigint           not null
 #  created_at        :datetime         not null
@@ -17,41 +17,76 @@
 #
 class EvaluationForm < ApplicationRecord
   belongs_to :challenge
-  belongs_to :phase
+  belongs_to :phase, optional: true # Disables default must exist error message
   has_many :evaluation_criteria, lambda {
     order(:created_at)
   }, class_name: 'EvaluationCriterion', dependent: :destroy, inverse_of: :evaluation_form
   accepts_nested_attributes_for :evaluation_criteria, allow_destroy: true
 
+  enum :scale_type, { point: "point", weight: "weight" }, type: "string"
+
   scope :by_user, lambda { |user|
     joins(challenge: :challenge_manager_users).
       where(challenge_manager_users: { id: user.id })
   }
 
   validates :title, presence: true, length: { maximum: 150 }
   validates :instructions, presence: true
+  validates :scale_type, presence: true
   validates :closing_date, presence: true
 
+  # Adds custom error message for phase presence failure instead of default from above
+  validates :phase, presence: { message: I18n.t("evaluation_form.phase.presence_error") }
   validates :phase_id, uniqueness: true
 
   validate :criteria_weights_must_sum_to_one_hundred
   validate :validate_unique_criteria_titles
 
+  def weighted_scoring?
+    scale_type == "weight"
+  end
+
+  def point_scoring?
+    scale_type == "point"
+  end
+
   private
 
   def validate_unique_criteria_titles
-    titles = evaluation_criteria.reject(&:marked_for_destruction?).map(&:title)
+    duplicate_titles = find_duplicate_titles
+
+    add_criteria_title_errors(duplicate_titles) unless duplicate_titles.empty?
+  end
+
+  def find_duplicate_titles
+    current_criteria_titles = evaluation_criteria.reject(&:marked_for_destruction?).map(&:title)
+    current_criteria_titles.select { |title| current_criteria_titles.count(title) > 1 }.uniq
+  end
 
-    return unless titles.uniq.length != titles.length
+  def add_criteria_title_errors(duplicate_titles)
+    criteria = evaluation_criteria.
+      reject(&:marked_for_destruction?).
+      select { |c| duplicate_titles.include?(c.title) }.
+      reject { |c| c.errors.added?(:title, I18n.t("evaluation_criteria.duplicate_title_error")) }
 
+    criteria.each { |c| c.errors.add(:title, I18n.t("evaluation_criteria.duplicate_title_error")) }
     errors.add(:base, I18n.t("evaluation_criterion_unique_title_in_form_error"))
   end
 
   def criteria_weights_must_sum_to_one_hundred
-    total_weight = evaluation_criteria.reject(&:marked_for_destruction?).sum(&:points_or_weight)
+    return unless weighted_scoring? && total_criteria_weight != 100
 
-    return unless weighted_scoring? && total_weight != 100
+    add_weight_errors
+  end
+
+  def total_criteria_weight
+    evaluation_criteria.reject(&:marked_for_destruction?).sum { |criteria| criteria.points_or_weight.to_i }
+  end
 
+  def add_weight_errors
+    evaluation_criteria.reject(&:marked_for_destruction?).each do |criteria|
+      criteria.errors.add("points_or_weight", I18n.t("evaluation_criteria.must_sum_to_100_error"))
+    end
     errors.add(:base, I18n.t("evaluation_form_criteria_weight_total_error"))
   end
 end