Add facilitator and workshop rollups #29518
Conversation
hacodeorg
requested review from
islemaster,
breville,
clareconstantine and
agealy
|
A note that we are planning to deprecate the workshop_organizer permission (https://codedotorg.atlassian.net/browse/PLC-256), which is mentioned here. It doesn't look anything will break once we remove it, so I wouldn't advocate for any changes. |
There was a problem hiding this comment.
Looks very good, especially as a incremental step while we're behind the experiment. I'd like to address the raw SQL query before merge.
--
A thought on your listed next steps:
Add automated testing in production.
I'd like to discuss this idea before we move forward with it. I think we should do some manual testing in production using the experiment, spot-checking real scenarios, but I'd like to keep automated testing in our test environment, because:
- Tests against production, by definition, can't catch problems before they reach production.
- Tests against production data may either be brittle (looking for specific values that are likely to change) or too general (unable to make assertions about correctness of displayed data).
- Isolation: We don't want our automated tests to cause undue load on our frontends. We don't want to risk tests accidentally writing to the production database. We don't want to risk exposing private production information in test output. We don't want our automated tests to affect any usage metrics on production.
- Maintenance: We don't currently have a test suite that runs against production, and introducing one would add one more system we have to maintain as a team.
| next unless reducer_result.present? | ||
|
|
||
| summaries << group_key.merge({reducer: reducer.name, reducer_result: reducer_result}) | ||
| rescue => e | ||
| errors << e.message |
There was a problem hiding this comment.
Not blocking on this PR, but I'm interested in a discussion about how specific we can usefully get with this (and similar) rescue blocks. I know we want to be resilient against failures, but I wonder if we can define useful boundaries of that resilience that will help us notice very unexpected scenarios faster.
Related: Rescue StandardError, Not Exception - ThoughtBot (especially the "Best-case Scenario" example)
There was a problem hiding this comment.
My train of thought when dealing with errors (StandardError) is
- What code is more likely to produce errors? (Any code could potentially fail, but some has higher risk than the other.)
- What errors are expected (everything else is unexpected)?
- What errors are fatal at what layers? (An error could be fatal and halt execution of a callee function but at the same time is non-fatal to the caller.)
- What to do when catch an expected/unexpected fatal/non-fatal error? A few options are
a) Interrupt the current code flow, throw error up 1 layer and let the upper layer deal with it.
b) Swallow the error and continue with the current code flow.
c) Swallow the error but report it via Honeybadger (externally) or record it and bubble the info up to upper layers.
In this particular case, the rescued block executes one reducer function on one group of data, which is a part of applying multiple reducers to multiple groups. I chose option (c) here, swallow all errors, record and bubble them up. This is best-effort approach, getting usable results but still notify higher layers about caught errors.
| ws_facilitated_query = | ||
| "SELECT DISTINCT pd_workshop_id FROM pd_workshops_facilitators "\ | ||
| "WHERE user_id = #{fac_id}" | ||
| ws_facilitated = ActiveRecord::Base.connection.exec_query(ws_facilitated_query).rows.flatten |
There was a problem hiding this comment.
Although fac_id should only come from our own database, I'd like to be paranoid and pass it as a bind instead of inlining it into the query statement here.
Also, is it possible that Pd::Workshop.managed_by already does what you want, taking a few more edge cases into account?
There was a problem hiding this comment.
I can see the vulnerability here. I have 2 options to fix it
def find_related_workshops(fac_id, course)
Pd::Workshop.left_outer_joins(:facilitators).
where(users: {id: fac_id}, course: course).
distinct.
pluck(:id)
end
This looks cleaner but one thing bothers me is it actually joins 3 tables instead of 2, pd_workshops, pd_workshop_facilitators and users. The last join with users is only to use users.id instead of pd_workshop_facilitators.user_id in filter.
The behind-the-scene SQL command is
SELECT DISTINCT `pd_workshops`.`id` FROM `pd_workshops` LEFT OUTER JOIN `pd_workshops_facilitators` ON `pd_workshops_facilitators`.`pd_workshop_id` = `pd_workshops`.`id` LEFT OUTER JOIN `users` ON `users`.`id` = `pd_workshops_facilitators`.`user_id` AND `users`.`deleted_at` IS NULL WHERE `pd_workshops`.`deleted_at` IS NULL AND `users`.`id` = 124 AND `pd_workshops`.`course` = 'CS Fundamentals'
I can go with option 2 to make the code more succinct, but I'm still interested in learning about better options.
There was a problem hiding this comment.
1 is fine with me - I just want to make sure we're sanitizing our parameters.
|
Having spent a lot of time trying to figure out how the old pipeline did a lot of this, your comments are super helpful!!! |
…ule Pd::WorkshopSurveyResultsHelper
Codecov Report
@@ Coverage Diff @@
## staging #29518 +/- ##
==========================================
Coverage ? 73.15%
==========================================
Files ? 2052
Lines ? 112316
Branches ? 3397
==========================================
Hits ? 82162
Misses ? 26921
Partials ? 3233
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## staging #29518 +/- ##
==========================================
Coverage ? 73.15%
==========================================
Files ? 2052
Lines ? 112316
Branches ? 3397
==========================================
Hits ? 82162
Misses ? 26921
Partials ? 3233
Continue to review full report at Codecov.
|
3rd PR in a series to support workshop surveys rollup. (PLC-330, 1st PR, 2nd PR)
This is not the final PR, it just gets the core functionality in under an experiment flag. There are still quite a few things to iron out.
What added
Facilitator effectiveness average for this workshop.
Facilitator effectiveness average across all workshops they facilitated for the same course.
Overall success and Teacher engagement averages for this workshop.
Overall success and Teacher engagement averages across workshops facilitated by same facilitator for the same course.
User permissions: Workshop admin, program manager, workshop organizer can see all results of facilitators in this workshop. Facilitator can only see their own results.
UI change: The only difference is in the Total responses row. It shows number of responses for Facilitator surveys and Workshop surveys (they are not the same). Previously it was just 1 number without explaining what that number means.
How tested
bundle exec rails test test/controllers/api/v1/pd/workshop_survey_report_controller_test.rbhttp://localhost-studio.code.org:3000/pd/workshop_dashboard/daily_survey_results/6482?enableExperiments=rollupSurveyReporthttp://localhost-studio.code.org:3000/pd/workshop_dashboard/daily_survey_results/6482?disableExperiments=rollupSurveyReportHow to try in production
To enable experiment
?enableExperiments=rollupSurveyReportat the end of a query string. You only have to do this once (the setting is saved to browser local storage). E.g.https://studio.code.org/pd/workshop_dashboard/daily_survey_results/6482?enableExperiments=rollupSurveyReportTo disable experiment
?disableExperiments=rollupSurveyReportat the end of a query string. Only have to do this once.To compare current view and experiment view
To do in the next PR