Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[74] Enter Evaluators A11y Fix #343

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
24 changes: 18 additions & 6 deletions app/controllers/evaluators_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,20 @@ def index
end

def create
@evaluator_invitation = EvaluatorInvitation.new(evaluator_invitation_params)

result = evaluator_service.process_evaluator_invitation(
evaluator_invitation_params[:email],
evaluator_invitation_params
)

if result[:success]
redirect_to phase_evaluators_path(@phase), notice: result[:message]
else
flash.now[:alert] = result[:message]
@evaluator_invitations = @phase.evaluator_invitations
@existing_evaluators = @phase.evaluators
render :index
return
end

handle_failed_invitation(result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing some inconsistency with the error message display depending on the path it takes. Can we try to ensure that if there are any validation issues with the name(s) missing, that the First & Last Name label is always highlighted in red (as it does for new users)?

existing user
Screenshot 2025-01-09 at 3 05 15 PM

new user
Screenshot 2025-01-09 at 3 05 38 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first screenshot is also missing the inline error text label describing the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a case where only the first name is entered, First & Last Name label should be red:
Screenshot 2025-01-09 at 2 53 49 PM

render :index, status: :unprocessable_entity
end

def destroy
Expand Down Expand Up @@ -64,7 +65,18 @@ def evaluator_service

def evaluator_invitation_params
params.require(:evaluator_invitation).permit(
:first_name, :last_name, :email, :challenge_id, :phase_id, :last_invite_sent
:full_name, :email, :challenge_id, :phase_id, :last_invite_sent
)
end

def handle_failed_invitation(result)
@evaluator_invitations = @phase.evaluator_invitations
@existing_evaluators = @phase.evaluators

if result[:evaluator_invitation].present?
@evaluator_invitation = result[:evaluator_invitation]
else
@evaluator_invitation.errors.add(:base, result[:message])
end
end
end
42 changes: 0 additions & 42 deletions app/javascript/controllers/evaluation_form_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,46 +69,4 @@ export default class extends Controller {
);
accordions.forEach((content) => content.removeAttribute("hidden"));
}

validatePresence(e) {
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 {
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;
}
}
46 changes: 46 additions & 0 deletions app/javascript/controllers/form_validation_controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { Controller } from "@hotwired/stimulus"

// Connects to data-controller="form-validation"
export default class extends Controller {
validatePresence(e) {
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 {
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) {
const errorElement = document.getElementById(field + "_error");
if (errorElement) {
errorElement.innerHTML = message;
}
}
}
3 changes: 3 additions & 0 deletions app/javascript/controllers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ application.register("evaluation-criteria", EvaluationCriteriaController);
import EvaluationFormController from "./evaluation_form_controller";
application.register("evaluation-form", EvaluationFormController);

import FormValidationController from "./form_validation_controller";
application.register("form-validation", FormValidationController);

import HotdogController from "./hotdog_controller";
application.register("hotdog", HotdogController);

Expand Down
10 changes: 10 additions & 0 deletions app/models/evaluator_invitation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,14 @@ class EvaluatorInvitation < ApplicationRecord
validates :last_invite_sent, presence: true

validates :email, uniqueness: { scope: [:challenge_id, :phase_id] }

def full_name=(name)
names = name.to_s.strip.split(/\s+/, 2)
self.first_name = names[0]
self.last_name = names[1]
end

def full_name
"#{first_name} #{last_name}".strip
end
end
25 changes: 24 additions & 1 deletion app/services/evaluator_management_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ def initialize(challenge, phase)
end

def process_evaluator_invitation(email, invitation_params)
@invitation_params = invitation_params
user = User.find_by(email:)
user ? add_existing_user_as_evaluator(user) : handle_invitation(email, invitation_params)
end
Expand Down Expand Up @@ -44,6 +45,24 @@ def resend_invitation(invitation)

private

def update_name_for_existing_user(user)
names = @invitation_params[:full_name].to_s.strip.split(/\s+/, 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's potentially problematic to duplicate this field parsing logic with the EvaluatorInvitation model, in case things change in the future. Can you extract these into a single function? Something like User.split_full_name(full_name) => [first, last]

For the validation error message, you can just have a method that adds those errors directly to the model if first_name or last_name are blank. Actually it might be even easier to build a fake EvaluatorInvitation model first (without saving), check if it's valid, and if so then update the user. If it's invalid you can return the invalid Invitation with its errors. That way you're reusing the Invitation name splitting logic and you can get the invitation's first_name/last_name to update the User when the name is valid. WDYT?

if names.length < 2
return {
success: false,
message: "First and last name are required"
}
end

first_name, last_name = names
user.update(
first_name:,
last_name:
)

{ success: true }
end

def add_existing_user_as_evaluator(user)
if @phase.evaluators.include?(user)
return {
Expand All @@ -61,6 +80,9 @@ def add_existing_user_as_evaluator(user)
}
end

updated_name = update_name_for_existing_user(user)
return updated_name unless updated_name[:success]

cpe = ChallengePhasesEvaluator.find_or_create_by(challenge: @challenge, phase: @phase, user:)

if cpe.persisted?
Expand Down Expand Up @@ -101,7 +123,8 @@ def create_new_invitation(invitation_params)
else
{
success: false,
message: invitation.errors.full_messages.join(", ")
message: invitation.errors.full_messages.join(", "),
evaluator_invitation: invitation
}
end
end
Expand Down
10 changes: 5 additions & 5 deletions app/views/evaluation_forms/_evaluation_criterion_fields.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
disabled: is_template || form_disabled,
data: {
"evaluation-criteria-target": "titleField",
"action": "evaluation-form#validatePresence focusout->evaluation-form#validatePresence"
"action": "form-validation#validatePresence focusout->form-validation#validatePresence"
}
%>

Expand All @@ -55,7 +55,7 @@
disabled: is_template || form_disabled,
data: {
"evaluation-criteria-target": "descriptionField",
"action": "evaluation-form#validatePresence focusout->evaluation-form#validatePresence"
"action": "form-validation#validatePresence focusout->form-validation#validatePresence"
}
%>
<span class="usa-character-count__message">You can enter up to 1000 characters</span>
Expand Down Expand Up @@ -83,7 +83,7 @@
"evaluation-criteria-target": "pointsOrWeightField",
action: "
input->evaluation-criteria#checkPointsOrWeightMax blur->evaluation-criteria#checkPointsOrWeightMax
evaluation-form#validatePresence focusout->evaluation-form#validatePresence
form-validation#validatePresence focusout->form-validation#validatePresence
"
}
%>
Expand Down Expand Up @@ -111,7 +111,7 @@
disabled: is_template || form_disabled,
data: {
"evaluation-criteria-target": "scoringTypeRadio",
action: "click->evaluation-criteria#toggleScoringType evaluation-form#validatePresence",
action: "click->evaluation-criteria#toggleScoringType form-validation#validatePresence",
field_name: criteria_field_id(f, "scoring_type", is_template)
}
%>
Expand Down Expand Up @@ -187,7 +187,7 @@
value: f.object.option_labels[index.to_s],
disabled: disabledOptionLabel || form_disabled,
data: {
"action": "evaluation-form#validatePresence focusout->evaluation-form#validatePresence",
"action": "form-validation#validatePresence focusout->form-validation#validatePresence",
"evaluation-criteria-target": "optionLabelInput"
}
%>
Expand Down
14 changes: 7 additions & 7 deletions app/views/evaluation_forms/_form.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= form_with(model: evaluation_form, data: { controller: "evaluation-form modal" }) do |form| %>
<%= form_with(model: evaluation_form, data: { controller: "evaluation-form modal form-validation" }) do |form| %>
<% if evaluation_form.errors.any? %>
<div style="color: darkred">
<div class="usa-alert usa-alert--error" role="alert">
Expand Down Expand Up @@ -37,7 +37,7 @@
<%= form.text_field :title,
maxlength: 150,
class: "usa-input #{input_error_class(form, :title)}",
data: {"action": "evaluation-form#validatePresence focusout->evaluation-form#validatePresence"},
data: {"action": "form-validation#validatePresence focusout->form-validation#validatePresence"},
disabled: disabled %>

<%= inline_error(form, :title) %>
Expand All @@ -57,7 +57,7 @@
class: "usa-select #{input_error_class(form, :phase)}",
title: 'challenge-combo',
data: {
action: "evaluation-form#handleChallengeSelect evaluation-form#validatePresence focusout->evaluation-form#validatePresence",
action: "evaluation-form#handleChallengeSelect form-validation#validatePresence focusout->form-validation#validatePresence",
field_name: "evaluation_form_phase"
},
disabled: disabled
Expand All @@ -77,7 +77,7 @@
<span class="margin-right-05" style="color: darkred;">*</span>
</div>

<%= form.text_area :instructions, class: "usa-textarea usa-character-count__field #{input_error_class(form, :instructions)}", maxlength: 3000, rows: "7", data: {"action": "evaluation-form#validatePresence focusout->evaluation-form#validatePresence"}, disabled: disabled%>
<%= form.text_area :instructions, class: "usa-textarea usa-character-count__field #{input_error_class(form, :instructions)}", maxlength: 3000, rows: "7", data: {"action": "form-validation#validatePresence focusout->form-validation#validatePresence"}, disabled: disabled%>
<span id="with-hint-textarea-info" class="usa-character-count__message">You can enter up to 3000 characters</span>
</div>

Expand Down Expand Up @@ -117,7 +117,7 @@
type="radio"
name="evaluation_form[scale_type]"
value="point"
data-action="click->evaluation-form#updateMaxPoints evaluation-form#validatePresence"
data-action="click->evaluation-form#updateMaxPoints form-validation#validatePresence"
data-field-name="evaluation_form_scale_type"
<%= 'checked' if evaluation_form.point_scoring? %>
<%= 'disabled' if disabled %>
Expand All @@ -131,7 +131,7 @@
type="radio"
name="evaluation_form[scale_type]"
value="weight"
data-action="click->evaluation-form#updateMaxPoints evaluation-form#validatePresence"
data-action="click->evaluation-form#updateMaxPoints form-validation#validatePresence"
data-field-name="evaluation_form_scale_type"
<%= 'checked' if evaluation_form.weighted_scoring? %>
<%= 'disabled' if disabled %>
Expand Down Expand Up @@ -208,7 +208,7 @@
name="evaluation_form[closing_date]"
aria-labelledby="evaluation_form_closing_date"
aria-describedby="evaluation_form_closing_date_hint"
data-action="evaluation-form#validatePresence focusout->evaluation-form#validatePresence"
data-action="form-validation#validatePresence focusout->form-validation#validatePresence"
/>
</div>

Expand Down
2 changes: 1 addition & 1 deletion app/views/evaluator_submission_assignments/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<%= render 'shared/back_link', path: phase_evaluators_path(@phase) %>

<h1><%= @challenge.title %> - <%= @phase.title %></h1>
<h1><%= challenge_phase_title(@challenge, @phase) %></h1>
<p>View submissions assigned to an evalutor.</p>

<h2 class="text-primary font-body-md padding-top-1">Evaluator: <%= "#{@evaluator.first_name} #{@evaluator.last_name}" %></h2>
Expand Down
Loading
Loading