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

Initial Foorm Rollups #33871

Merged
merged 22 commits into from Apr 1, 2020
Merged

Initial Foorm Rollups #33871

merged 22 commits into from Apr 1, 2020

Conversation

molly-moen
Copy link
Contributor

@molly-moen molly-moen commented Mar 26, 2020

Calculates and shows rollups for teacher engagement and overall success matrices for CSD/CSP Foorm survey results on /pd/workshop_dashboard/daily_survey_results_foorm/. Rollups will show average response for the specific workshop and for all workshops for the course. Example view:
Screen Shot 2020-03-26 at 12 59 55 PM

The questions to be rolled up are stored in rollups_by_course.json. Currently only matrix questions that use numeric keys are supported. This PR also includes a new survey configuration for the day 5 CSD/CSP summer workshop, as that survey contains the questions that we currently roll up.

Future work

  • add tests
  • add CSF rollup configuration
  • add per-facilitator rollups (need facilitator surveys first)

Links

Testing story

Tested manually that rollups show up correctly for a workshop with day 5 survey results, and no errors occur with workshops with incomplete/no survey results. The new survey results view page is not linked anywhere so it is low-risk.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@molly-moen molly-moen requested review from breville, hacodeorg and a team March 26, 2020 20:15
@@ -0,0 +1,11 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I love that this is driven by a configuration!

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@breville
Copy link
Member

As discussed, a detailed comment in lib/pd/foorm/survey_results.rb that maps out the call sequencing would be very helpful for future users of this code.

@breville
Copy link
Member

Again looking forward to what @hacodeorg thinks!

@@ -0,0 +1,11 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

dashboard/lib/pd/foorm/survey_results.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@hacodeorg hacodeorg left a comment

Choose a reason for hiding this comment

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

It takes me a while to follow the details in the flow. I think it makes sense. However, it's quite difficult to find issues without a clear understanding of the caveats/corner cases in function inputs. I can take a deeper look in your next PR with tests.
Are we planning to make the rollup pipeline generic or it is completely just for Foorm?

dashboard/lib/pd/foorm/rollup_helper.rb Outdated Show resolved Hide resolved
@molly-moen
Copy link
Contributor Author

thanks @hacodeorg! For now the plan is for it to work with Foorm only, since the formats end up being pretty specific it would be hard to make it generic. I am working on the tests in the follow-up PR, let me know if there are other things I can do to make the code clearer.

@molly-moen molly-moen merged commit fa267e1 into staging Apr 1, 2020
@molly-moen molly-moen deleted the molly/foorm-survey-rollups branch April 1, 2020 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants