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
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
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
76 changes: 55 additions & 21 deletions app/views/evaluators/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,39 +2,73 @@
<div data-page="manage-evaluators">
<%= render 'shared/back_link', path: phases_path(@phase) %>
stepchud marked this conversation as resolved.
Show resolved Hide resolved

<h1><%= @challenge.title %> - <%= @phase.title %></h1>
<h1><%= challenge_phase_title(@challenge, @phase) %></h1>
<p>Create and manage a list of evaluators for the challenge.</p>
<h2 class="padding-top-3">Add Evaluators</h2>
<p>Evaluators will not be assigned to submissions until you assign in the submission detail view.</p>
<%= form_with(model: EvaluatorInvitation.new, url: phase_evaluators_path(@phase), method: :post, local: true) do |form| %>
<%= form_with(model: @evaluator_invitation || EvaluatorInvitation.new, url: phase_evaluators_path(@phase), method: :post, local: true, data: { controller: "evaluator-form modal" }, html: { novalidate: true }) do |form| %>
Copy link
Contributor

Choose a reason for hiding this comment

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

the evaluator-form stimulus controller doesn't exist, did you mean to use evaluation-form controller?

Copy link
Contributor

Choose a reason for hiding this comment

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

since you're using the evaluation-form#validatePresence function here, I think it would be good to extract that functionality into a separate controller to be used in both places. the rest of the EvaluationFormController related to the EvaluationForm fields can stay there. that way we'll have a reusable form validation controller for other stimulus forms.

<%= form.hidden_field :challenge_id, value: @challenge.id %>
<%= form.hidden_field :phase_id, value: @phase.id %>
<%= form.hidden_field :last_invite_sent, value: Time.current %>

<% if form.object.errors.any? %>
<div style="color: darkred">
<div class="usa-alert usa-alert--error" role="alert">
<div class="usa-alert__body">
<h3 class="usa-alert__heading">
<%= pluralize(form.object.errors.count, "error") %> prohibited this evaluator from being added:
</h3>
<ul>
<% form.object.errors.each do |error| %>
<li><%= error.full_message %></li>
<% end %>
</ul>
</div>
</div>
</div>
<% end %>

<div class="usa-form-group">
<%= form.label :first_name, class: "usa-label font-sans-md" do %>
<span class="text-bold">First Name</span><span class="text-red">*</span>
<% end %>
<div class="usa-hint font-sans-2xs text-normal">Add evaluator's first name.</div>
<%= form.text_field :first_name, class: "usa-input", required: true, autocomplete: "given-name", style: "border: 1.5px solid #565c65;" %>
</div>
<div class="usa-form-group">
<%= form.label :last_name, class: "usa-label font-sans-md" do %>
<span class="text-bold">Last Name</span><span class="text-red">*</span>
<% end %>
<div class="usa-hint font-sans-2xs text-normal">Add evaluator's last name.</div>
<%= form.text_field :last_name, class: "usa-input", required: true, autocomplete: "family-name", style: "border: 1.5px solid #565c65;" %>
<div class="display-flex flex-align-center text-bold">
<%= form.label :full_name, class: "usa-label font-sans-md #{label_error_class(form, :first_name)}" do %>
<span class="text-bold">First & Last Name</span><span class="text-red">*</span>
<% end %>
</div>
<%= form.text_field :full_name,
class: "usa-input #{input_error_class(form, :last_name)}",
required: true,
autocomplete: "name",
style: "border: 1.5px solid #565c65;",
data: {
action: "evaluator-form#validatePresence focusout->evaluator-form#validatePresence"
},
value: form.object.full_name
%>
<%= inline_error(form, :last_name) %>
</div>

<div class="usa-form-group">
<%= form.label :email, class: "usa-label font-sans-md" do %>
<span class="text-bold">Email Address</span><span class="text-red">*</span>
<% end %>
<div class="usa-hint font-sans-2xs text-normal">Add a single email address for the individual.</div>
<%= form.email_field :email, class: "usa-input", required: true, autocomplete: "email", style: "border: 1.5px solid #565c65;" %>
<div class="display-flex flex-align-center text-bold">
<%= form.label :email, class: "usa-label font-sans-md #{label_error_class(form, :email)}" do %>
<span class="text-bold">Email Address</span><span class="text-red">*</span>
<% end %>
</div>
<%= form.email_field :email,
class: "usa-input #{input_error_class(form, :email)}",
required: true,
autocomplete: "email",
style: "border: 1.5px solid #565c65;",
data: {
action: "evaluator-form#validatePresence focusout->evaluator-form#validatePresence"
}
%>
<%= inline_error(form, :email) %>
</div>

<%= form.button type: 'submit', class: "usa-button margin-top-5 margin-bottom-3" do %>
<%= image_tag('images/usa-icons/person.svg', class: "usa-icon--size-3 icon-white", alt: "Add evaluator") %>
<%= image_tag('images/usa-icons/person.svg', class: "usa-icon--size-3 icon-white", alt: "") %>
<span>Add to evaluator list</span>
<% end %>
<% end %>
<% end %>

<h2>Evaluator List</h2>
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/evaluators_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@
}

expect(response).to render_template(:index)
expect(flash[:alert]).to eq('User does not have a valid evaluator role.')
expect(response.body).to include('User does not have a valid evaluator role.')
end
end
end
Expand Down
28 changes: 27 additions & 1 deletion spec/services/evaluator_management_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,13 @@
let(:evaluator) { create(:user, role: 'evaluator') }

it 'adds the user as an evaluator if not already added' do
result = service.process_evaluator_invitation(evaluator.email, {})
result = service.process_evaluator_invitation(
evaluator.email,
{
email: evaluator.email,
full_name: 'Santos Bickford'
stepchud marked this conversation as resolved.
Show resolved Hide resolved
}
)
expect(result[:success]).to be true
expect(result[:message]).to include('has been added as an evaluator')
expect(ChallengePhasesEvaluator.where(challenge: challenge, phase: phase, user: evaluator).count).to eq(1)
Expand All @@ -31,6 +37,26 @@
expect(result[:success]).to be false
expect(result[:message]).to include('does not have a valid evaluator role')
end

it 'requires full name when adding an existing user' do
result = service.process_evaluator_invitation(evaluator.email, { email: evaluator.email })
expect(result[:success]).to be false
expect(result[:message]).to eq('First and last name are required')
end

it 'updates user name when adding as evaluator' do
result = service.process_evaluator_invitation(
evaluator.email,
{
email: evaluator.email,
full_name: 'Santos Bickford'
}
)
expect(result[:success]).to be true
evaluator.reload
expect(evaluator.first_name).to eq('Santos')
expect(evaluator.last_name).to eq('Bickford')
end
end

context 'with a new user' do
Expand Down
Loading