-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial feedback on route and controller
assigned: 0, | ||
unassigned: 1, | ||
recused: 2, | ||
not_started: 3, |
There was a problem hiding this comment.
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.
end | ||
|
||
def handle_successful_update(new_status) | ||
flash.now[:success] = t("evaluator_submission_assignments.#{new_status}.success") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
end | ||
|
||
def handle_failed_update(new_status) | ||
flash.now[:error] = t("evaluator_submission_assignments.#{new_status}.failure") |
There was a problem hiding this comment.
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]
const evaluatorId = new URLSearchParams(window.location.search).get('evaluator_id'); | ||
window.location.href = `/phases/${this.phaseIdValue}/evaluator_submission_assignments?evaluator_id=${evaluatorId}`; |
There was a problem hiding this comment.
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.
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; |
'Accept': 'application/json' | ||
}, | ||
body: JSON.stringify({ | ||
status: 'unassigned' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Assigned Submissions | ||
</h3> | ||
<p class="usa-summary-box__text text-primary-darker text-bold"> | ||
Evaluations due by <%= @phase.end_date.strftime('%m/%d/%Y') %> |
There was a problem hiding this comment.
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.
Evaluations due by <%= @phase.end_date.strftime('%m/%d/%Y') %> | |
Evaluations due by <%= @phase.evaluation_form.closing_date.strftime('%m/%d/%Y') %> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
<%= render 'submission_summary', assigned_count: @assigned_submissions.count, | ||
submissions_count: @submissions_count, | ||
phase_end_date: @phase.end_date %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these local assigns are unused and not needed
<%= render 'submission_summary', assigned_count: @assigned_submissions.count, | |
submissions_count: @submissions_count, | |
phase_end_date: @phase.end_date %> | |
<%= render 'submission_summary' %> |
<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> |
There was a problem hiding this comment.
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)?
<div class="margin-bottom-2 margin-top-3"> | ||
<%= link_to path, class: "usa-link display-inline-flex flex-align-center" do %> | ||
<%= image_tag('images/usa-icons/arrow_back.svg', class: "usa-icon--size-3", alt: "Back to previous page") %> | ||
<span class="margin-left-05 text-primary"><%= 'Back' %></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is 'Back' interpolated?
<span class="margin-left-05 text-primary"><%= 'Back' %></span> | |
<span class="margin-left-05 text-primary">Back</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a request spec for the #index route here and also spec/system/evaluator_submission_assignment_spec.rb
with a11y expect(page).to be_axe_clean
and some page interactions assigning and unassigning evaluators with confirmation modal.
Related tickets: #179, #76, #74
Add UI to view evaluators' submissions from the manage evaluator index page.
Add UI and functionality for the Unassign Evaluator from a Submission modal and add functionality to reassign a submission to an evaluator.
I've added controller tests - other tests WIP. The UI is responsive.
Mobile