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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
067a1ed
Move columns around in workshop summary report
bencodeorg Jan 16, 2020
c6a21e9
Add method to get all teachers who attended all workshop sessions
bencodeorg Jan 16, 2020
6611cb2
Method to get scholarship attendees at a workshop
bencodeorg Jan 17, 2020
9503892
Use mapping of course names to abbreviations
bencodeorg Jan 17, 2020
39556e7
Merge branch 'staging' into workshop-report-improvements
bencodeorg Jan 17, 2020
d182680
Add scholarship teacher count to workshop summary report
bencodeorg Jan 17, 2020
55551b9
Merge branch 'staging' into workshop-report-improvements
bencodeorg Jan 17, 2020
192352c
Add scholarship info factory
bencodeorg Jan 18, 2020
b905ef6
Add test for new workshop method that does not work yet
bencodeorg Jan 18, 2020
0ac0230
Count teachers attending all sessions of workshop
bencodeorg Jan 18, 2020
6a10f0d
Add new fields to workshop summary report controller test
bencodeorg Jan 21, 2020
08c2c71
Reorder columns in workshop summary CSV
bencodeorg Jan 21, 2020
76fbfd8
Reorder columns in workshop summary UI
bencodeorg Jan 21, 2020
6468e50
Test method to count scholarship attendees at workshop
bencodeorg Jan 21, 2020
bfe00cf
Better naming and fewer active record queries in attendance retrieval
bencodeorg Jan 23, 2020
1b2df0a
Merge branch 'staging' into workshop-report-improvements
bencodeorg Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 42 additions & 31 deletions apps/src/code-studio/pd/workshop_dashboard/reports/workshop_summary_report.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,6 @@ export class WorkshopSummaryReport extends React.Component {
property: 'organizer_name',
header: {label: 'Organizer Name'}
},
{
property: 'organizer_id',
header: {label: 'Organizer Id'}
},
{
property: 'organizer_email',
header: {label: 'Organizer Email'}
Expand All @@ -129,20 +125,55 @@ export class WorkshopSummaryReport extends React.Component {
header: {label: 'Regional Partner'}
},
{
property: 'workshop_name',
header: {label: 'Workshop Name'}
property: 'workshop_dates',
header: {label: 'Dates'}
},
{
property: 'on_map',
header: {label: 'Shown on Map'}
property: 'num_hours',
header: {label: 'Workshop Total Hours'}
},
{
property: 'funded',
header: {label: 'Funded'}
},
{
property: 'workshop_dates',
header: {label: 'Dates'}
property: 'attendance_url',
header: {label: 'Attendance URL'},
cell: {format: this.formatUrl}
},
{
property: 'num_facilitators',
header: {label: 'Num Facilitators'}
},
{
property: 'num_registered',
header: {label: 'Num Registered'}
},
{
property: 'num_scholarship_teachers_attending_all_sessions',
header: {label: 'Num Scholarship Attending'}
}
];

for (let i = 1; i <= ATTENDANCE_DAYS_COUNT; i++) {
columns.push({
property: `attendance_day_${i}`,
header: {label: `Attendance Day ${i}`}
});
}

columns.push(
{
property: 'organizer_id',
header: {label: 'Organizer Id'}
},
{
property: 'workshop_name',
header: {label: 'Workshop Name'}
},
{
property: 'on_map',
header: {label: 'Shown on Map'}
},
{
property: 'workshop_id',
Expand All @@ -157,20 +188,11 @@ export class WorkshopSummaryReport extends React.Component {
property: 'subject',
header: {label: 'Subject'}
},
{
property: 'attendance_url',
header: {label: 'Attendance URL'},
cell: {format: this.formatUrl}
},
{
property: 'facilitators',
header: {label: 'Facilitators'}
},
{
property: 'num_facilitators',
header: {label: 'Num Facilitators'}
}
];
);

if (this.state.showFacilitatorDetails) {
for (let i = 1; i <= FACILITATOR_DETAILS_COUNT; i++) {
Expand All @@ -188,10 +210,6 @@ export class WorkshopSummaryReport extends React.Component {
}

columns.push(
{
property: 'num_registered',
header: {label: 'Num Registered'}
},
{
property: 'num_qualified_teachers',
header: {label: 'Num Qualified Teachers'}
Expand All @@ -202,13 +220,6 @@ export class WorkshopSummaryReport extends React.Component {
}
);

for (let i = 1; i <= ATTENDANCE_DAYS_COUNT; i++) {
columns.push({
property: `attendance_day_${i}`,
header: {label: `Attendance Day ${i}`}
});
}

if (this.props.permission.has(WorkshopAdmin)) {
columns.push(
{
Expand Down
28 changes: 28 additions & 0 deletions dashboard/app/models/pd/workshop.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!

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).

teachers_attending = sessions.flat_map(&:attendances).flat_map(&:teacher)

# 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.

{
user: teacher,
application_year: school_year,
course: course_key
}
)
end
end

# Get number of sessions attended by teacher
attendance_count_by_teacher = Hash[
teachers_attending.uniq.map do |teacher|
[teacher, teachers_attending.count(teacher)]
end
]

# 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.

end

def local_summer?
subject == SUBJECT_SUMMER_WORKSHOP
end
Expand Down
38 changes: 23 additions & 15 deletions dashboard/lib/pd/payment/workshop_summary.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,36 +75,44 @@ def total_teacher_attendance_days
def generate_organizer_report_line_item(with_payment = false)
line_item = {
organizer_name: workshop.organizer&.name,
organizer_id: workshop.organizer&.id,
organizer_email: workshop.organizer&.email,
regional_partner_name: workshop.regional_partner.try(:name),
workshop_dates: workshop.sessions.map(&:formatted_date).join(' '),
on_map: workshop.on_map,
num_hours: num_hours,
funded: workshop.funding_summary,
attendance_url: attendance_url,
facilitators: workshop.facilitators.pluck(:name).join(', '),
num_facilitators: workshop.facilitators.count,
workshop_id: workshop.id,
workshop_name: workshop.friendly_name,
course: workshop.course,
subject: workshop.subject,
num_registered: workshop.enrollments.count,
num_qualified_teachers: num_qualified_teachers,
days: num_days,
num_scholarship_teachers_attending_all_sessions: workshop.teachers_attending_all_sessions(true).count,
}

# Facilitator names and emails, 1-6
(1..REPORT_FACILITATOR_DETAILS_COUNT).each do |n|
line_item["facilitator_name_#{n}".to_sym] = workshop.facilitators[n - 1].try(&:name)
line_item["facilitator_email_#{n}".to_sym] = workshop.facilitators[n - 1].try(&:email)
end

# Attendance days 1-5
session_attendance_counts = attendance_count_per_session
(1..REPORT_ATTENDANCE_DAY_COUNT).each do |n|
line_item["attendance_day_#{n}".to_sym] = session_attendance_counts[n - 1]
end

# Waiting to add some columns until after attendance to make payment processing easier
line_item.merge!(
{
organizer_id: workshop.organizer&.id,
on_map: workshop.on_map,
facilitators: workshop.facilitators.pluck(:name).join(', '),
workshop_id: workshop.id,
workshop_name: workshop.friendly_name,
course: workshop.course,
subject: workshop.subject,
num_qualified_teachers: num_qualified_teachers,
days: num_days,
}
)

# Facilitator names and emails, 1-6
(1..REPORT_FACILITATOR_DETAILS_COUNT).each do |n|
line_item["facilitator_name_#{n}".to_sym] = workshop.facilitators[n - 1].try(&:name)
line_item["facilitator_email_#{n}".to_sym] = workshop.facilitators[n - 1].try(&:email)
end

if with_payment
line_item.merge!(
{
Expand Down
2 changes: 2 additions & 0 deletions dashboard/test/controllers/api/v1/pd/workshop_summary_report_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class Api::V1::Pd::WorkshopSummaryReportControllerTest < ::ActionController::Tes
num_registered
num_qualified_teachers
days
num_hours
num_scholarship_teachers_attending_all_sessions
).tap do |fields|
(1..Pd::Payment::WorkshopSummary::REPORT_FACILITATOR_DETAILS_COUNT).each do |n|
fields << "facilitator_name_#{n}"
Expand Down
7 changes: 7 additions & 0 deletions dashboard/test/factories/factories.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@
workshop nil
enrolled true
attended false
cdo_scholarship_recipient false
end
after(:create) do |teacher, evaluator|
raise 'workshop required' unless evaluator.workshop
Expand All @@ -170,6 +171,12 @@
create :pd_attendance, session: session, teacher: teacher
end
end
if evaluator.cdo_scholarship_recipient
create :pd_scholarship_info,
user: teacher,
course: Pd::Workshop::COURSE_KEY_MAP[evaluator.workshop.course],
application_year: evaluator.workshop.school_year
end
end
end
transient {pilot_experiment nil}
Expand Down
8 changes: 8 additions & 0 deletions dashboard/test/factories/pd_factories.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,14 @@
end
end

factory :pd_scholarship_info, class: 'Pd::ScholarshipInfo' do
association :user, factory: :teacher

course Pd::Workshop::COURSE_KEY_MAP[Pd::Workshop::COURSE_CSP]
application_year Pd::Application::ApplicationConstants::YEAR_19_20
scholarship_status Pd::ScholarshipInfoConstants::YES_CDO
end

factory :pd_attendance, class: 'Pd::Attendance' do
association :session, factory: :pd_session
association :teacher
Expand Down
15 changes: 15 additions & 0 deletions dashboard/test/models/pd/workshop_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,21 @@ class Pd::WorkshopTest < ActiveSupport::TestCase
assert_equal enrollments.pluck(:id).sort, @workshop.unattended_enrollments.pluck(:id).sort
end

test 'teachers_attending_all_sessions' do
workshop = create :workshop,
course: Pd::Workshop::COURSE_CSP,
sessions_from: Date.new(2019, 7, 1)

2.times do
create :pd_workshop_participant, enrolled: true, workshop: workshop
create :pd_workshop_participant, enrolled: true, attended: true, workshop: workshop
create :pd_workshop_participant, enrolled: true, attended: true, cdo_scholarship_recipient: true, workshop: workshop
end

assert_equal 4, workshop.teachers_attending_all_sessions.count
assert_equal 2, workshop.teachers_attending_all_sessions(true).count
end

# TODO: remove this test when workshop_organizer is deprecated
test 'organizer_or_facilitator?' do
facilitator = create :facilitator
Expand Down