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

[179] View Evaluator's Submissions #258

Open
wants to merge 43 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
7abaa15
245 | Update invited evaluator user setup
emmabjj Nov 4, 2024
2093119
245 | Move evaluator invitation to evaluator user logic
emmabjj Nov 5, 2024
6771cf4
245 | Add check for evaluator's role
emmabjj Nov 5, 2024
ef02ade
245 | Remove duplicate logic and handle in/valid evaluator roles
emmabjj Nov 5, 2024
23e5701
245 | Add successfully removed flash notice back in
emmabjj Nov 5, 2024
27f35d0
245 | Update tests
emmabjj Nov 6, 2024
e64fa23
245 | CPE created automatically, adjust redudant conditional
emmabjj Nov 6, 2024
1d36e0d
179 | UI/Add view for evaluator submissions
emmabjj Nov 7, 2024
d576fa0
179 | Add UI for the unassign modal and js
emmabjj Nov 7, 2024
a91e88a
179 | Add routes
emmabjj Nov 7, 2024
a4a9298
179 | Add evaluator_submissions_assignments association to phase thro…
emmabjj Nov 7, 2024
742af61
179 | Add evaluation submissions controller
emmabjj Nov 7, 2024
44c2db5
179 | Add evaluation status text colors
emmabjj Nov 7, 2024
145886a
179 | Add evaluation status order
emmabjj Nov 7, 2024
16866fb
Merge branch '200_manage_evaluators_list' into 179_evaluator_submissi…
emmabjj Nov 7, 2024
4295f4d
179 | Add evaluator submissions message translations
emmabjj Nov 7, 2024
3f803d3
Merge branch '200_manage_evaluators_list' into 179_evaluator_submissi…
emmabjj Nov 19, 2024
8775c82
179 | Update routes
emmabjj Nov 22, 2024
a68e0cb
179 | Update view and use partials
emmabjj Nov 22, 2024
5c5ad7e
179 | Use update for reassign & unassign
emmabjj Nov 22, 2024
e86b23e
179 | Add recused_unassigned status
emmabjj Nov 22, 2024
fbe433a
179 | Back link partial
emmabjj Nov 22, 2024
3ff0089
179 | Adjust js to use patch update on status reassign/unassign
emmabjj Nov 22, 2024
e7882bf
179 | Add to translations
emmabjj Nov 22, 2024
9c89936
Small updates
emmabjj Nov 22, 2024
01b132f
Small fix
emmabjj Nov 22, 2024
6c32c1d
Merge branch 'dev' into 179_evaluator_submissions_view
emmabjj Nov 22, 2024
1a6531e
Update order by status on evaluation submission assignments
emmabjj Nov 22, 2024
3da992d
Remove unused argument
emmabjj Nov 22, 2024
e541c13
Update ordered by status
emmabjj Nov 22, 2024
25f0d6d
Merge branch 'dev' into 179_evaluator_submissions_view
emmabjj Nov 22, 2024
8e207e1
179 | Display score for evaluator submission assignments
emmabjj Nov 22, 2024
f0013f6
179 | Adjust SQL statement for status order
emmabjj Nov 22, 2024
6602fb3
179 | Scope score to evaluator's evaluation score
emmabjj Nov 22, 2024
9f44ad8
179 | Recused unassigned cannot be reassign to the submission
emmabjj Nov 22, 2024
d029c20
179 | Update route for evaluation submission assignments
emmabjj Nov 25, 2024
c890db4
179 | Update js to use assignment
emmabjj Nov 25, 2024
01209a4
179 | Update the display score to check for assignment completion
emmabjj Nov 25, 2024
ec365f9
179 | Update ordered by status query
emmabjj Nov 25, 2024
575eda4
179 | Update tests wip
emmabjj Nov 25, 2024
f824247
179 | Remove unused argument in display_score
emmabjj Nov 25, 2024
d99a8b4
179 | Update tests for display scores
emmabjj Nov 25, 2024
28e77ea
Merge branch 'dev' into 179_evaluator_submissions_view
emmabjj Nov 25, 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
73 changes: 73 additions & 0 deletions app/controllers/evaluator_submission_assignments_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# frozen_string_literal: true

# Controller for evaluator submissions assignments index and update status
class EvaluatorSubmissionAssignmentsController < ApplicationController
stepchud marked this conversation as resolved.
Show resolved Hide resolved
before_action -> { authorize_user('challenge_manager') }
before_action :set_challenge_and_phase
before_action :set_evaluator, only: [:index]
before_action :set_assignment, only: [:update]

def index
@evaluator_assignments = @phase.evaluator_submission_assignments.where(user_id: @evaluator.id)
@assigned_submissions = @evaluator_assignments.
where(status: %i[completed in_progress not_started recused]).
ordered_by_status
@unassigned_submissions = @evaluator_assignments.
where(status: %i[unassigned recused_unassigned]).
ordered_by_status
@submissions_count = @assigned_submissions.group('evaluator_submission_assignments.status').count
end

# update only the status of the evaluation submission assignment to unassign or reassign an evaluator
def update
new_status = params[:status]&.to_sym

if update_assignment_status(new_status)
handle_successful_update(new_status)
else
handle_failed_update(new_status)
end
end

private

def set_challenge_and_phase
@phase = Phase.where(challenge: current_user.challenge_manager_challenges).find(params[:phase_id])
@challenge = @phase.challenge
end

def set_evaluator
@evaluator = @phase.evaluators.find(params[:evaluator_id])
end

def set_assignment
@assignment = @phase.evaluator_submission_assignments.find(params[:id])
end

def update_assignment_status(new_status)
@assignment.update(status: EvaluatorSubmissionAssignment.statuses[new_status])
end

def handle_successful_update(new_status)
flash.now[:success] = t("evaluator_submission_assignments.#{new_status}.success")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the flash.now[:success] should be flash[:success] since you are redirecting to the next page

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I tested this and I don't see the message unless it used flase[:success], unfortunately there is some rubocop linter warning about this but you should disable that cop for these line.

respond_to do |format|
format.html { redirect_to_assignment_path }
format.json { render json: { success: true, message: flash[:success] } }
end
end

def handle_failed_update(new_status)
flash.now[:error] = t("evaluator_submission_assignments.#{new_status}.failure")
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, use flash[:error]

respond_to do |format|
format.html { redirect_to_assignment_path }
format.json { render json: { success: false, message: flash[:error] }, status: :unprocessable_entity }
end
end

def redirect_to_assignment_path
redirect_to phase_evaluator_submission_assignments_path(
@phase,
evaluator_id: params[:evaluator_id]
)
end
end
28 changes: 28 additions & 0 deletions app/helpers/evaluators_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,37 @@ def assigned_submissions_count(evaluator, challenge, phase)
evaluator.evaluator_submission_assignments.
joins(:submission).
where(submissions: { challenge:, phase: }).
where.not(status: [:unassigned, :recused_unassigned]).
count
else
0
end
end

def evaluation_status(status)
case status.to_sym
when :recused
'text-accent-warm-dark'
when :not_started
'text-secondary-dark'
when :in_progress
'text-orange'
when :completed
'text-green'
when :unassigned
'text-accent-cool-darker'
when :recused_unassigned
'text-secondary'
else
'text-base'
end
end

def display_score(assignment)
if assignment.completed? && assignment.evaluation&.total_score
assignment.evaluation.total_score
else
'N/A'
end
end
end
3 changes: 3 additions & 0 deletions app/javascript/controllers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,6 @@ application.register("evaluation-criteria", EvaluationCriteriaController);

import DeleteEvaluatorModalController from "./delete_evaluator_modal_controller"
application.register("delete-evaluator-modal", DeleteEvaluatorModalController)

import UnassignEvaluatorSubmissionModalController from "./unassign_evaluator_submission_modal_controller"
application.register("unassign-evaluator-submission-modal", UnassignEvaluatorSubmissionModalController)
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
static targets = ["modal", "confirmButton"]
static values = {
phaseId: String,
assignmentId: String
}

connect() {
this.modalTarget.addEventListener('click', this.handleOutsideClick.bind(this))
}

disconnect() {
this.modalTarget.removeEventListener('click', this.handleOutsideClick.bind(this))
}

open(event) {
event.preventDefault();
this.setValues(event.currentTarget.dataset);
this.modalTarget.showModal();
}


close() {
this.modalTarget.close()
}

handleOutsideClick(event) {
if (event.target === this.modalTarget) {
this.close()
}
}

confirm() {
this.unassignEvaluatorSubmission()
}

setValues(dataset) {
this.assignmentIdValue = dataset.assignmentId;
this.phaseIdValue = dataset.phaseId;
}

unassignEvaluatorSubmission() {
const csrfToken = document.querySelector('meta[name="csrf-token"]').content;

fetch(`/phases/${this.phaseIdValue}/evaluator_submission_assignments/${this.assignmentIdValue}`, {
method: 'PATCH',
headers: {
'Content-Type': 'application/json',
'X-CSRF-Token': csrfToken,
'Accept': 'application/json'
},
body: JSON.stringify({
status: 'unassigned'
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried modifying this value to send some unknown, invalid status to the backend to trigger an error but it caused a 500 exception instead in the model update. You can remedy this by validating the status param in the rails controller before trying to update the model with an invalid value. if the client sends an invalid status, reject the request with :unprocessable_entity status.

Copy link
Contributor

Choose a reason for hiding this comment

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

.. alternatively you could rescue errors from the @assignment.update call in the controller to return false

})
})
.then(response => response.json())
.then(data => {
if (data.success) {
const evaluatorId = new URLSearchParams(window.location.search).get('evaluator_id');
window.location.href = `/phases/${this.phaseIdValue}/evaluator_submission_assignments?evaluator_id=${evaluatorId}`;
Comment on lines +61 to +62
Copy link
Contributor

@stepchud stepchud Nov 27, 2024

Choose a reason for hiding this comment

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

this is essentially doing location.reload() right? I have a different idea, what if you redirect to the path provided in the response data. you could render the url in the response JSNO as data.redirect_to or something and then reuse that same redirect path for both success and error since the backend will determine where to send the user. I think this will also display the flash message that was stored after the browser redirects, so you may not need the alert() either. if you want you can still log a console.error() for developer debugging help messages.

Suggested change
const evaluatorId = new URLSearchParams(window.location.search).get('evaluator_id');
window.location.href = `/phases/${this.phaseIdValue}/evaluator_submission_assignments?evaluator_id=${evaluatorId}`;
window.location.href = data.redirect_to;

} else {
throw new Error(data.message || 'Failed to unassign evaluator from submission');
}
})
.catch(error => {
console.error('Error:', error);
alert(error.message || 'An error occurred while unassigning the evaluator from the submission');
});
}
}
28 changes: 27 additions & 1 deletion app/models/evaluator_submission_assignment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,31 @@ class EvaluatorSubmissionAssignment < ApplicationRecord
belongs_to :evaluator, class_name: "User", foreign_key: :user_id, inverse_of: :assigned_submissions
has_one :evaluation, dependent: :destroy

enum :status, { assigned: 0, unassigned: 1, recused: 2 }
has_one :phase, through: :submission

enum :status, {
assigned: 0,
unassigned: 1,
recused: 2,
not_started: 3,
Comment on lines +22 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

how are assigned and not_started statuses different? I left a comment here about the status of the assignment/evaluation progress WRT not_started, in_progress and completed. here are some examples of queries that filter on those statuses (that PR isn't merged yet).

I think I'd prefer to stick with that approach for now because it is easier than keeping the status updated in all cases of progress. let me know if it doesn't make sense here.

in_progress: 4,
completed: 5,
recused_unassigned: 6
}

scope :ordered_by_status, lambda {
order(
Arel.sql(
"CASE evaluator_submission_assignments.status
WHEN #{ActiveRecord::Base.connection.quote(statuses[:recused])} THEN 0
WHEN #{ActiveRecord::Base.connection.quote(statuses[:unassigned])} THEN 1
WHEN #{ActiveRecord::Base.connection.quote(statuses[:recused_unassigned])} THEN 2
WHEN #{ActiveRecord::Base.connection.quote(statuses[:not_started])} THEN 3
WHEN #{ActiveRecord::Base.connection.quote(statuses[:in_progress])} THEN 4
WHEN #{ActiveRecord::Base.connection.quote(statuses[:completed])} THEN 5
ELSE 6
END"
)
)
}
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<tr data-evaluator-id="<%= evaluator.id %>" data-evaluator-type="user">
<td data-label="Submission ID" class="text-top text-bold"><%= assignment.submission.id %></td>
<td data-label="Evaluation status">
<span class="text-top text-bold <%= evaluation_status(assignment.status) %>"><%= assignment.status.titleize %></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

did you try using the uswds tag here (with color)?

</td>
<td data-label="Score" class="text-top text-normal"><%= display_score(assignment) %></td>
<td data-label="">
<div class="display-flex flex-justify-end">
<% if assignment.completed? %>
<%= link_to "View Evaluation", submissions_path(@challenge), class: 'usa-button font-body-3xs margin-right-1' %>
<% end %>
<%= button_tag "Unassign", type: 'button', class: 'usa-button usa-button--outline font-body-3xs', data: {
action: "click->unassign-evaluator-submission-modal#open",
assignment_id: assignment.id,
phase_id: @phase.id
} %>
</div>
</td>
</tr>
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 a little nitpicky, but can we rename this _assignments_stats.html.erb just for consistency with the other stats component on #280?

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<div
class="usa-summary-box bg-base-lightest border-1px border-base-dark radius-0"
role="region"
aria-labelledby="summary-box-key-information"
>
<div class="usa-summary-box__body">
<div class="display-flex flex-column tablet:flex-row tablet:flex-align-center">
<div class="tablet:flex-fill">
<div class="display-flex flex-align-center margin-bottom-2 tablet:margin-bottom-0">
<div class="margin-right-2">
<span class="font-sans-3xl text-primary text-bold"><%= @assigned_submissions.count %></span>
</div>
<div>
<h3 class="usa-summary-box__heading text-primary margin-bottom-05" id="summary-box-key-information">
Assigned Submissions
</h3>
<p class="usa-summary-box__text text-primary-darker text-bold">
Evaluations due by <%= @phase.end_date.strftime('%m/%d/%Y') %>
Copy link
Contributor

Choose a reason for hiding this comment

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

evaluations are due by the form's closing_date. there are a few other stats collection examples on #280, although there are subtle differences in the two components.

Suggested change
Evaluations due by <%= @phase.end_date.strftime('%m/%d/%Y') %>
Evaluations due by <%= @phase.evaluation_form.closing_date.strftime('%m/%d/%Y') %>

</p>
</div>
</div>
</div>

<div class="border-top border-base-dark margin-y-2 tablet:display-none"></div>

<div class="display-flex flex-row tablet:margin-left-auto">
<div class="text-center margin-right-5">
<span class="font-sans-xl text-green text-bold"><%= @submissions_count["completed"] || 0 %></span><br>
<span class="text-base-sm text-green text-bold">Completed</span>
</div>
<div class="text-center margin-right-5">
<span class="font-sans-xl text-orange text-bold"><%= @submissions_count["in_progress"] || 0 %></span><br>
<span class="text-base-sm text-orange text-bold">In Progress</span>
</div>
<div class="text-center margin-right-2">
<span class="font-sans-xl text-secondary-dark text-bold"><%= @submissions_count["not_started"] || 0 %></span><br>
<span class="text-base-sm text-secondary-dark text-bold">Not Started</span>
</div>
</div>
</div>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a cool use of layout 👍

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<div class="usa-table-container--scrollable width-full margin-bottom-10" tabindex="0">
<table class="usa-table usa-table--stacked-header usa-table--borderless width-full gray-header">
<thead>
<tr>
<th scope="col">Submission ID</th>
<th scope="col" class="text-black">Evaluation status</th>
<th scope="col">Score</th>
<th scope="col" aria-label="Actions"></th>
</tr>
</thead>
<tbody>
<%= yield %>
</tbody>
</table>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<dialog
id="unassign-evaluator-submission-modal"
data-unassign-evaluator-submission-modal-target="modal"
aria-labelledby="modal-1-heading"
aria-describedby="modal-1-description"
class="radius-lg"
>
<div class="usa-modal__content maxw-mobile-lg">
<div class="usa-modal__main">
<h2 class="usa-modal__heading" id="modal-1-heading">
Are you sure you want to unassign an evaluator from this submission?
</h2>
<div class="usa-prose">
<p id="modal-1-description" data-unassign-evaluator-submission-modal-target="modalDescription">
Unassigning an evaluator will delete any completed or in progress evaluations of this evaluator for this submission. Any other submissions that the evaluator is assigned will not be affected.
</p>
</div>
<div class="usa-modal__footer">
<ul class="usa-button-group">
<li class="usa-button-group__item">
<button type="button" class="usa-button" data-unassign-evaluator-submission-modal-target="confirmButton" data-action="click->unassign-evaluator-submission-modal#confirm">
Yes
</button>
</li>
<li class="usa-button-group__item padding-top-1 text-center">
<a
href="#"
class="usa-link"
data-action="click->unassign-evaluator-submission-modal#close"
>
Back
</a>
</li>
</ul>
</div>
</div>
</div>
</dialog>
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<tr data-evaluator-id="<%= evaluator.id %>" data-evaluator-type="user">
<td data-label="Submission ID" class="text-top text-bold"><%= assignment.submission.id %></td>
<td data-label="Evaluation status">
<span class="text-top text-bold <%= evaluation_status(assignment.status) %>"><%= assignment.status.titleize %></span>
</td>
<td data-label="Score" class="text-top text-light"><%= display_score(assignment) %></td>
<td data-label="">
<div class="display-flex flex-justify-end">
<% if assignment.unassigned? %>
<%= button_to "Reassign", phase_evaluator_submission_assignment_path(@phase, assignment),
method: :patch,
params: { status: :not_started, evaluator_id: evaluator.id },
class: 'usa-button usa-button--outline font-body-3xs' %>
<% end %>
</div>
</td>
</tr>
Loading
Loading