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
Autoscoring #19550
Autoscoring #19550
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.
Can you link the spec so I can see the rule definitions?
Presuming that's in line, this LGTM with a couple small suggestions.
@@ -430,5 +430,57 @@ def meets_criteria | |||
def total_score | |||
response_scores_hash.values.map {|x| x.try(:to_i)}.compact.reduce(:+) | |||
end | |||
|
|||
# Called once after the application is submitted, and the principal approval is done | |||
def auto_score |
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.
Please add a comment about what it does in addition to when it should be called, and also mention that it's idempotent and won't override existing scores.
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.
Since this mutates the application object, should we call it auto_score!
) | ||
|
||
application = create :pd_teacher1819_application, course: 'csp', form_data: application_hash.to_json | ||
application.update(regional_partner: (create :regional_partner)) |
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 update rather than inlining this in the create? If we are to use update, use the bang version so we'll catch failures.
end | ||
|
||
if course == 'csp' | ||
scores[:csp_which_grades] = responses[:csp_which_grades].any? ? YES : NO |
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.
Should these types of scores be boolean, and then the client can choose what to display? Why store "Yes"/"No"?
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 went back and forth on this. I went with string values instead of bools for scores so I wouldn't have to write extra logic for Yes/No questions versus scores with numeric values.
When the teacher application is submitted, and when the principal approval is submitted, auto-score some parts of the application. Users will be able to override these scores.