Foorm: Minor rollups fixes #34890
Foorm: Minor rollups fixes #34890
Conversation
|
|
||
| # Sort summaries so that survey data is ordered in the order surveys | ||
| # should be taken in. i.e. Pre-Workshop, Day 1, Day 2,.. Post-Workshop | ||
| def self.sort_summary(workshop_summary) |
There was a problem hiding this comment.
I'm not sure how efficient this is...reading says sort_by is not great, but there are probably a maximum of 12 values to sort here in the worst case so it's probably a non-issue. Any suggestions for improvement are appreciated.
| ] | ||
| }, | ||
| "CS Discoveries": { | ||
| "general": [ | ||
| {"question_id": "teacher_engagement", "header_text": "Teacher Engagement"}, | ||
| {"question_id": "overall_success", "header_text": "Overall Success"} | ||
| ], | ||
| "facilitator": [ | ||
| {"question_id": "facilitator_effectiveness", "header_text": "Facilitator Effectiveness"} |
There was a problem hiding this comment.
I love that this is driven by a simple configuration!
| if choice_hash.class == Hash && choice_hash.key?(:value) && choice_hash.key?(:text) | ||
| choices_obj[choice_hash[:value]] = fill_question_placeholders(choice_hash[:text]) | ||
| elsif choice_hash.class == String | ||
| choices_obj[choice_hash] = fill_question_placeholders(choice_hash) |
There was a problem hiding this comment.
Should we emit a warning anywhere when we notice this?
There was a problem hiding this comment.
good question--should I add a honeybadger warning? Something else?
There was a problem hiding this comment.
It does seem that we use honeybadger for these kinds of alerts sometimes, with the idea that we will remedy the issue to try and drive instances to 0 each time it happens. The only downside is reporting the issue every time we render a form, when we really just need to capture it once. For that reason, it could make more sense to detect during seeding (and later, in the authoring UI itself too). But with our current usage levels it doesn't seem unreasonable to just catch in honeybadger.
There was a problem hiding this comment.
added honeybadger alert. Let me know if the wording makes sense.
| # Sort summaries so that survey data is ordered in the order surveys | ||
| # should be taken in. i.e. Pre-Workshop, Day 1, Day 2,.. Post-Workshop | ||
| def self.sort_summary(workshop_summary) | ||
| temp_summary = workshop_summary.sort_by {|survey_key, _| get_index_for_survey_key(survey_key)} |
There was a problem hiding this comment.
Do you need the _ parameter here?
There was a problem hiding this comment.
If I don't then the single variable will be a 2 element array containing the survey_key and summary, so adding _ extracts out the survey_key
A couple rollups fixes to make things work for daily workshop pattern:
bypass_dateso we can force-show daily surveys for testingTesting story
Updated unit tests and ensured existing functionality (CSF Intro) was not affected. This change enables viewing of CSD/CSP rollups, as they were throwing an error due to the new configuration sometimes using strings instead of hashes for answer options.
Reviewer Checklist: