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.

Sign up for GitHub

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

Jump to bottom

Workshop report improvements #32799

Merged
merged 16 commits into from Jan 23, 2020
Merged

Workshop report improvements #32799

merged 16 commits into from Jan 23, 2020

Conversation

bencodeorg
Copy link
Contributor

PLC-609: Update workshop report spreadsheet to facilitate payments processing

  1. Updates order of payment processing spreadsheet to put important columns first.
  2. Adds new columns containing the number of hours in the workshop, and number of attendees who are Code.org scholarship recipients and attended all workshop sessions.
  3. Reorders UI in same way as CSV.

Testing story

Updates existing tests, and adds a new test for the workshop method that returns all attendees who attended all sessions.

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

@bencodeorg
Move columns around in workshop summary report
067a1ed
@bencodeorg
Add method to get all teachers who attended all workshop sessions
c6a21e9
@bencodeorg
Method to get scholarship attendees at a workshop
6611cb2
@bencodeorg
Use mapping of course names to abbreviations
9503892
@bencodeorg
Merge branch 'staging' into workshop-report-improvements
39556e7
@bencodeorg
Add scholarship teacher count to workshop summary report
d182680
@bencodeorg
Merge branch 'staging' into workshop-report-improvements
55551b9
@bencodeorg
Add scholarship info factory
192352c
@bencodeorg
Add test for new workshop method that does not work yet
b905ef6
@bencodeorg
Count teachers attending all sessions of workshop
0ac0230
@bencodeorg
Add new fields to workshop summary report controller test
6a10f0d
@bencodeorg
Reorder columns in workshop summary CSV
08c2c71
@bencodeorg
Reorder columns in workshop summary UI
76fbfd8
@bencodeorg
Test method to count scholarship attendees at workshop
6468e50
@bencodeorg bencodeorg requested review from a team and removed request for a team January 21, 2020 23:36
@bencodeorg bencodeorg mentioned this pull request Jan 22, 2020
Merged
molly-moen
molly-moen reviewed Jan 22, 2020
View reviewed changes
dashboard/app/models/pd/workshop.rb Outdated
@@ -580,6 +580,34 @@ def attending_teachers
sessions.flat_map(&:attendances).flat_map(&:teacher).uniq
end

# Get all teachers who have attended all sessions of this workshop.
def teachers_attending_all_sessions(cdo_scholarship=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I might rename this parameter to something like filter_on_scholarship so it's clear false gives you all the teachers and true gives you just those with scholarships. The current name could be interpreted as false=teachers without scholarships

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, thanks!

islemaster
islemaster reviewed Jan 22, 2020
View reviewed changes
dashboard/app/models/pd/workshop.rb Outdated
# Filter attendances to only scholarship teachers
if cdo_scholarship
teachers_attending.select! do |teacher|
Pd::ScholarshipInfo.exists?(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is sufficient. We have Pd::ScholarshipInfo rows where scholarship_status is 'no', which doesn't seem to match what you're querying for here. In fact, I think all applications get a ScholarshipInfo now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Second concern here - we're nesting a second ActiveRecord query inside a loop, so we'll run the exists? once for every teacher in the workshop. It would be better to add a join to the teachers_attending relation so we're not adding a query... but that could be follow-up work. Let's make sure this is working as designed first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right re: check not being sufficient, will update. Apologies for the oversight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with appropriate check, and changed to be a single ActiveRecord query.

islemaster
islemaster reviewed Jan 22, 2020
View reviewed changes
dashboard/app/models/pd/workshop.rb
]

# Return only teachers who attended all sessions
attendance_count_by_teacher.select {|_, attendances| attendances == sessions.count}.keys
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, this whole thing is quite the query optimization challenge! Since this is just for payments I think you could merge this as-is, but it might be worth revisiting as a good exercise in query optimization within ActiveRecord. Ask Will about his query-counting unit tests sometime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know right, it's hairy! I'll do some research on optimizing queries here tomorrow and share with y'all to review.

islemaster
islemaster reviewed Jan 22, 2020
View reviewed changes
dashboard/app/models/pd/workshop.rb Outdated
@@ -580,6 +580,34 @@ def attending_teachers
sessions.flat_map(&:attendances).flat_map(&:teacher).uniq
end

# Get all teachers who have attended all sessions of this workshop.
def teachers_attending_all_sessions(cdo_scholarship=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole method might be a good candidate for a Query object (read linked doc and ask Elijah for details).

@bencodeorg bencodeorg merged commit 68bfebd into staging Jan 23, 2020
@bencodeorg bencodeorg deleted the workshop-report-improvements branch January 23, 2020 23:21
bencodeorg added a commit that referenced this pull request Jan 31, 2020
@bencodeorg
Revert "Merge pull request #32799 from code-dot-org/workshop-report-i…
14cd9e0 
…mprovements"

This reverts commit 68bfebd, reversing
changes made to 485e1a3.
@bencodeorg bencodeorg mentioned this pull request May 18, 2020
Merged
bencodeorg added a commit that referenced this pull request May 20, 2020
@bencodeorg
Merge pull request #32957 from code-dot-org/revert-32948-revert-works…
b398e61 
…hop-summary-report-updates

Updates to workshop summary report (resuscitated from #32799)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@islemaster islemaster islemaster left review comments

@molly-moen molly-moen molly-moen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bencodeorg @islemaster @molly-moen