-
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
[200] Build Manage Evaluators List #243
Conversation
remove duplicate node-version line
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.
Left some feedback, I will finish up the review after that's resolved
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 started to make some code suggestions on the PR but it's a little more extensive than I thought and will be easier to create a separate PR that you can review and merge into this one if it makes sense. Trying to make these routes a bit more RESTful.
def set_phase | ||
phase_id = params.dig(:evaluator_invitation, :phase_id) || params[:phase_id] | ||
@phase = phase_id ? @challenge.phases.find(phase_id) : @challenge.phases.order(:start_date).first | ||
end |
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 before_action is overloaded with several routes and conditionals. I'm not sure how each route will work. before_action
s should be simple logic and either check for error conditions in the request or load data in a predictable way, if they're used at all. I'm not sure what kinds of bugs might be caused by an incorrect param. why does it default to the first phase by start_date? which route(s) does that apply to? it gets pretty confusing.
[200] Update the Invitation routes
this.modalDescriptionTarget.textContent = 'Deleting an evaluator from the challenge will remove the evaluator from any submissions of this ' + | ||
'challenge that the evaluator is assigned to. It will also delete any of their completed or in progress evaluations for those submissions.' | ||
this.confirmButtonTarget.textContent = 'Yes' |
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'm not sure why this textContent is being reset, does it ever change?
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.
Tested deleting the resetModal()
function and it seems fine without it, let's remove it for now along with those targets on the divs
app/models/user.rb
Outdated
@@ -1,5 +1,7 @@ | |||
# frozen_string_literal: true | |||
|
|||
require_relative '../services/evaluator_management_service' |
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.
is this line needed? I was under the impression that Rails handles requiring all the code under app/
?
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.
still works without it 👍
def eligible_for_evaluation?(submission) | ||
submission.judging_status.in?(%w[selected winner]) | ||
end | ||
|
||
def selected_to_advance?(submission) | ||
submission.judging_status.in?(%w[winner]) | ||
end | ||
|
||
def phase_has_recused_evaluator?(phase) |
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 already existed on the PhasesHelper so I removed it here
0b1c5f0
to
e41dd2b
Compare
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.
Ship it!
I made a few minor tweaks just to wrap up some loose ends. Nice work! 👏
#200 - Build Manage Evaluators List
#112 - Delete Evaluators
#74 - Enter Evaluators
Build the frontend for the manage evaluators list and the functionality.
This allows a challenge manager to add an existing evaluator to a specific challenge/phase, invite a new evaluator, delete an evaluator or an invitation, resend an invitation to a pending evaluator, or resend an invitation if the challenge manager tries to add an existing evaluator invitation again.
Added tests to cover changes.
Add/delete/resend functionality
Build Manage Evaluator List, form, and delete modal
Update header and challenge status
WAVE
Form fields are required to add evaluator
Using evaluator_submission_assignments for evaluator submission count
Mobile