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

[75] Assign Evaluators to a Submission (WIP) #329

Open
wants to merge 30 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
40ce276
working
stonefilipczak Dec 9, 2024
3f1e16d
disabling and verbage
stonefilipczak Dec 10, 2024
acf9d04
specs
stonefilipczak Dec 10, 2024
ff410cc
reorder
stonefilipczak Dec 10, 2024
7ac20ae
Merge branch 'dev' into 305_submission_details
stonefilipczak Dec 10, 2024
2145636
render everything
stonefilipczak Dec 10, 2024
a10efbb
fix spec
stonefilipczak Dec 10, 2024
7e8e97b
Merge branch 'dev' into 305_submission_details
stonefilipczak Dec 11, 2024
2a5de81
Merge branch 'dev' into 305_submission_details
stonefilipczak Dec 12, 2024
55401a6
initialize
stonefilipczak Dec 12, 2024
e321bf7
missing evaluation form
stonefilipczak Dec 12, 2024
c405259
Update spec/system/submission_details_spec.rb
stonefilipczak Dec 16, 2024
9d313a1
Update spec/system/submission_details_spec.rb
stonefilipczak Dec 16, 2024
81a8020
Update spec/system/submission_details_spec.rb
stonefilipczak Dec 16, 2024
f4231db
Update app/views/submissions/_judging_status.html.erb
stonefilipczak Dec 16, 2024
1d11c91
not working single form attempt
stonefilipczak Dec 17, 2024
ff79c48
weird WIP
stonefilipczak Dec 18, 2024
ef9808f
working single form approach
stonefilipczak Dec 18, 2024
9d6de87
fix test
stonefilipczak Dec 18, 2024
22d736c
Merge branch 'dev' into 305_submission_details
stonefilipczak Dec 18, 2024
faf6aba
Merge branch '305_submission_details' into 75_evaluator_submission_as…
stonefilipczak Dec 19, 2024
efb3ed5
WIP
stonefilipczak Dec 19, 2024
7779784
both accordions working
stonefilipczak Dec 19, 2024
ad1d718
succesful assignment
stonefilipczak Dec 20, 2024
63aa36b
working everything
stonefilipczak Dec 20, 2024
f0548ee
unassign
stonefilipczak Dec 20, 2024
3f4b79b
Merge branch 'dev' into 75_evaluator_submission_assignment
stepchud Dec 20, 2024
c942ba2
move form to partial
stepchud Dec 20, 2024
85a8193
Merge branch 'dev' into 75_evaluator_submission_assignment
stonefilipczak Dec 30, 2024
2e9236e
extract available evaluators to helper method
stonefilipczak Dec 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .zshrc
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably remove this file, not sure why we need it?

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export PHOENIX_URI="localhost:4000"
Copy link
Contributor

Choose a reason for hiding this comment

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

are you using nix? this value is defined in .env_dev so you can source that directly instead if you don't use nix

24 changes: 23 additions & 1 deletion app/controllers/evaluator_submission_assignments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,26 @@
class EvaluatorSubmissionAssignmentsController < ApplicationController
before_action -> { authorize_user('challenge_manager') }
before_action :set_challenge_and_phase
before_action :set_evaluator, only: [:index]
before_action :set_evaluator, only: [:index, :create]
before_action :set_assignment, only: [:update]
before_action :set_submission, only: [:create]

def create
@evaluator_submission_assignment = EvaluatorSubmissionAssignment.new(
user_id: params["evaluator_id"],
submission_id: @submission.id,
status: :assigned
)
if @evaluator_submission_assignment.save
redirect_to submission_path(@submission), notice: I18n.t("evaluator_submission_assignment_saved")
else
redirect_to confirmation_evaluation_form_path(@evaluator_submission_assignment), notice: I18n.t("evaluation_form_saved")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the wrong path to redirect to when it failed to save. it should probably redirect to the current evaluator_submission_assignment with a warning error message instead of evaluation_form_saved flash notice.

end
end

def unassign

end
Comment on lines +24 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

this is unused, we can rely on the update route for unassigning


def index
@evaluator_assignments = @phase.evaluator_submission_assignments.includes(:submission).where(user_id: @evaluator.id)
Expand Down Expand Up @@ -49,6 +67,10 @@ def set_assignment
@assignment = @phase.evaluator_submission_assignments.find(params[:id])
end

def set_submission
@submission = @phase.submissions.find(params[:submission_id])
stonefilipczak marked this conversation as resolved.
Show resolved Hide resolved
end

def status_from_params
status = params[:status] || params.dig(:evaluator_submission_assignment, :status)
status&.to_sym
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/submissions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ class SubmissionsController < ApplicationController
before_action -> { authorize_user('challenge_manager') }
before_action :set_submission, only: [:show, :update]

def show; end
def show
@available_evaluators = @submission.phase.evaluators - @submission.evaluators
Copy link
Contributor

Choose a reason for hiding this comment

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

this list needs a bit of tweaking to account for different statuses. available evaluators should include evaluators that were assigned and then unassigned. when you add specs we should test all these scenarios where the evaluators are in different states of assigned/unassigned/recused/recused_unassigned and make sure it's working for each case.

Copy link
Contributor

Choose a reason for hiding this comment

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

See this Figma comment thread.. if the evaluator was unassigned the button CTA should read "Reassign to Submission".

end

def update
if @submission.update!(submission_params)
Expand Down
2 changes: 1 addition & 1 deletion app/views/shared/_alert_error.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<div class="grid-row margin-top-105">
<div class="grid-col-12 tablet:grid-col-6">
<div class="maxw-mobile-lg">
<div class="usa-alert usa-alert--error padding-top-105" role="alert">
<div class="usa-alert__body">
<h4 class="usa-alert__heading"><%= alert_heading %></h4>
Expand Down
46 changes: 46 additions & 0 deletions app/views/submissions/_assigned_evaluators.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<div class="usa-accordion">
<% rand = SecureRandom.hex(8) %>
<h2 class="usa-accordion__heading" id="<%= "#{rand}-accordion-heading" %>">
<button
type="button"
class="usa-accordion__button display-flex flex-justify icon-white"
aria-expanded="true"
aria-controls="<%= rand %>"
>
<div>Assigned Evaluators</div>
</button>
</h2>

<div class="usa-accordion__content" id="<%= rand %>">
<table class="usa-table usa-table--stacked-header usa-table--borderless gray-header">
<thead>
<tr>
<th scope="col">Evaluator name</th>
<th scope="col">Evaluation Status</th>
<th scope="col">Score</th>
<th scope="col" aria-label="Actions"></th>
</tr>
</thead>
<tbody>
<% @submission.evaluators.each do |evaluator| %>
Copy link
Contributor

Choose a reason for hiding this comment

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

we should filter this by evaluator_submission_assignments with assigned or recused status. (this should work, or you can assign the filtered list in the controller like you do with @available_evaluators)

Suggested change
<% @submission.evaluators.each do |evaluator| %>
<% @submission.evaluators.where("evaluator_submission_assignments.status" => ["assigned", "recused"]).each do |evaluator| %>

<tr data-evaluator-id="<%= evaluator.id %>" data-evaluator-type="user">
<th scope="row" data-label="Evaluator name" class="text-top">
<span class="text-bold text-ls-1 line-height-sans-4"><%= "#{evaluator.first_name} #{evaluator.last_name}" %></span>
</th>
<td data-label="Evaluation Status" class="text-top"></td>
<td data-label="Score" class="text-top"></td>
<td data-label="">
<div class="display-flex flex-no-wrap grid-row">
<%= form_with url: phase_evaluator_submission_assignments_path(@submission.phase) do |f| %>
<%= f.hidden_field :evaluator_id, value: evaluator.id %>
<%= f.hidden_field :submission_id, value: @submission.id %>
<%= f.submit "Unassign" %>
Comment on lines +34 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

this form routes to the create action when it should issue an update to the existing evaluator_submission_assignment to mark it unassigned if it's assigned and recused_unassigned if it is recused status.
assigned => unassigned
recused => recused_unassigned
because it creates a new evaluator_submission_assignment, that also creates duplicate assignment records. we should also validate uniqueness on the combination of evaluator_id/phase_id/submission_id in the EvaluatorSubmissionAssignment model, so we don't end up with duplicate records (just in case someone tries to create one).

<% end %>
</div>
</td>
</tr>
<% end %>
</tbody>
</table>
</div>
</div>
30 changes: 30 additions & 0 deletions app/views/submissions/_available_evaluators.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<div class="usa-accordion">
<% rand = SecureRandom.hex(8) %>
<h2 class="usa-accordion__heading" id="<%= "#{rand}-accordion-heading" %>">
<button
type="button"
class="usa-accordion__button display-flex flex-justify icon-white"
aria-expanded="true"
aria-controls="<%= rand %>"
>
<div>Available Evaluators</div>
</button>
</h2>

<div class="usa-accordion__content background-primary-lighter" id="<%= rand %>">
<% @available_evaluators.each do |evaluator| %>
<div class="float-left">
<%= "#{evaluator.first_name} #{evaluator.last_name}" %> <br/>
<%= evaluator.email %>
</div>
<div class="float-right">
<%= form_with url: phase_evaluator_submission_assignments_path(@submission.phase) do |f| %>
<%= f.hidden_field :evaluator_id, value: evaluator.id %>
<%= f.hidden_field :submission_id, value: @submission.id %>
<%= f.submit "Assign" %>
<% end %>
</div>

<% end %>
</div>
</div>
53 changes: 53 additions & 0 deletions app/views/submissions/_details_form.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<div data-controller="submission-details">
<% rand = SecureRandom.hex(8) %>
<%= form_with(model: @submission, url: submission_path(@submission), class: "maxw-mobile-lg") do |form| %>
<div class="usa-checkbox" id="eligible-for-evaluation">
<input
class="usa-checkbox__input usa-checkbox__input--tile"
type="checkbox"
id="<%= "#{rand}-judging-status-selected" %>"
value="selected"
<%= "disabled" if @submission.judging_status == "winner" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like #321 will merge first so I wanted to leave a reminder to update this disabled state from that logic once it's merged:

def eligibility_deselection_disabled?
evaluator_submission_assignments.exists?(status: [:assigned, :recused])
end
def advancement_checkbox_disabled?
!eligible_for_evaluation? || !all_evaluations_completed? || evaluators.empty?
end

(also the winner status disabled below)

<%= "checked" if @submission.judging_status.in?(["selected", "winner"]) %>
data-action="change->submission-details#eligibleCheck"
data-submission-details-target="eligibleCheckbox"
/>
<label class="usa-checkbox__label" for="<%= "#{rand}-judging-status-selected" %>"
>This submission is eligible for evaluation.
<span class="usa-checkbox__label-description"
>Check the box if the submission is pre-screened and is eligible for the evaluation process.</span
></label
>
</div>
<div class="usa-checkbox" id="selected-to-advance">
<input
class="usa-checkbox__input usa-checkbox__input--tile"
id="<%= "#{rand}-judging-status-winner" %>"
type="checkbox"
<%= "disabled" unless @submission.judging_status.in?(["selected", "winner"]) %>
<%= "checked" if @submission.judging_status == "winner" %>
data-action="change->submission-details#selectedCheck"
data-submission-details-target="winnerCheckbox"
/>
<label class="usa-checkbox__label" for="<%= "#{rand}-judging-status-winner" %>"
>This submission is selected to advance.
<span class="usa-checkbox__label-description"
>Check the box to select this submission to advance or for award after all evaluations are completed.</span
></label
>
</div>
<input
id="judging-status-hidden"
type="hidden"
name="submission[judging_status]"
data-submission-details-target="judgingStatusHidden"
/>
<div class="usa-form-group">
<%= form.label :comments, "Comments and notes:", class: "usa-label" %>
<%= form.text_area :comments, class: "usa-textarea", default: @submission.comments %>
</div>
<button type="submit" name="commit" class="usa-button font-body-2xs text-no-wrap margin-y-2">
Save
</button>
<% end %>
</div>
57 changes: 4 additions & 53 deletions app/views/submissions/_submission_details.html.erb
Original file line number Diff line number Diff line change
@@ -1,53 +1,4 @@
<div data-controller="submission-details">
<% rand = SecureRandom.hex(8) %>
<%= form_with(model: @submission, url: submission_path(@submission), class: "maxw-mobile-lg") do |form| %>
<div class="usa-checkbox" id="eligible-for-evaluation">
<input
class="usa-checkbox__input usa-checkbox__input--tile"
type="checkbox"
id="<%= "#{rand}-judging-status-selected" %>"
value="selected"
<%= if @submission.judging_status == "winner" then "disabled" end %>
<%= "checked" if @submission.judging_status.in?(["selected", "winner"]) %>
data-action="change->submission-details#eligibleCheck"
data-submission-details-target="eligibleCheckbox"
/>
<label class="usa-checkbox__label" for="<%= "#{rand}-judging-status-selected" %>"
>This submission is eligible for evaluation.
<span class="usa-checkbox__label-description"
>Check the box if the submission is pre-screened and is eligible for the evaluation process.</span
></label
>
</div>
<div class="usa-checkbox" id="selected-to-advance">
<input
class="usa-checkbox__input usa-checkbox__input--tile"
id="<%= "#{rand}-judging-status-winner" %>"
type="checkbox"
<%= "disabled" unless @submission.judging_status.in?(["selected", "winner"]) %>
<%= "checked" if @submission.judging_status == "winner" %>
data-action="change->submission-details#selectedCheck"
data-submission-details-target="winnerCheckbox"
/>
<label class="usa-checkbox__label" for="<%= "#{rand}-judging-status-winner" %>"
>This submission is selected to advance.
<span class="usa-checkbox__label-description"
>Check the box to select this submission to advance or for award after all evaluations are completed.</span
></label
>
</div>
<input
id="judging-status-hidden"
type="hidden"
name="submission[judging_status]"
data-submission-details-target="judgingStatusHidden"
/>
<div class="usa-form-group">
<%= form.label :comments, "Comments and notes:", class: "usa-label" %>
<%= form.text_area :comments, class: "usa-textarea", default: @submission.comments %>
</div>
<button type="submit" name="commit" class="usa-button font-body-2xs text-no-wrap margin-y-2">
Save
</button>
<% end %>
</div>
<%= render partial: 'details_form' %>
<% if @submission.judging_status.in?(["selected", "winner"]) %>
<%= render partial: 'submission_evaluation' %>
<% end %>
7 changes: 7 additions & 0 deletions app/views/submissions/_submission_evaluation.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<h2>Submission Evaluation</h2>
<% if @submission.phase.evaluation_form.nil? %>
<%= render 'shared/alert_error', alert_heading: t('alerts.no_evaluation_form.heading'), alert_text: t('alerts.no_evaluation_form.text') %>
<% else %>
<%= render partial: 'assigned_evaluators' %>
<%= render partial: 'available_evaluators' %>
<% end %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget the link to "Manage my evaluation panel"
Screenshot 2024-12-20 at 12 01 33 PM

1 change: 1 addition & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ en:
already_logged_in_notice: "You are already logged in."
evaluation_form_destroyed: "Evaluation form was successfully destroyed."
evaluation_form_saved: "Evaluation form was saved successfully."
evaluator_submission_assignment_saved: "Submission assignment was saved succesfully."
comments_saved: "Comments saved succesfully."
submission_updated: "Submission was updated succesfully."
login_error: "There was an issue with logging in. Please try again."
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
post 'resend_invite'
end
end
resources :evaluator_submission_assignments, only: [:index, :update]
resources :evaluator_submission_assignments, only: [:index, :update, :create]
end
resources :submissions, only: [:index, :show, :update]

Expand Down
2 changes: 1 addition & 1 deletion db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ CREATE TABLE public.submissions (
description_delta text,
brief_description_delta text,
pdf_reference character varying(255),
comments text
comments character varying
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed here

);


Expand Down
Loading