Skip to content

Commit

Permalink
fix grading period report and cache calls to GradingPeriod#last
Browse files Browse the repository at this point in the history
refs CNVS-30880

The expensive grade calculation was moved out of the database access
loop to help avoid the database cursor being open too long.

GradingPeriod#last? is only called when doing assignment filtering. This
change avoids a database call per student or course when doing large
operations like grade results.

Test plan:
  * set up grading periods
  * run the "MGP Grade Export" report
    * try it with all terms and a specific term
  * make sure the grading period grades are correct

Change-Id: I6009ad6541832b180b9f75e1786eabcc1736ea37
Reviewed-on: https://gerrit.instructure.com/88621
Tested-by: Jenkins
Reviewed-by: Cameron Matheson <cameron@instructure.com>
Product-Review: Cameron Matheson <cameron@instructure.com>
Reviewed-by: Neil Gupta <ngupta@instructure.com>
QA-Review: KC Naegle <knaegle@instructure.com>
  • Loading branch information
ktgeek authored and roor0 committed Aug 30, 2016
1 parent f152495 commit d75d22c
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 18 deletions.
8 changes: 5 additions & 3 deletions app/models/grading_period.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,13 @@ def in_date_range?(date)
end

def last?
grading_period_group
# should never be nil, because self is part of the potential set
@last_period ||= grading_period_group
.grading_periods
.active
.sort_by(&:end_date)
.last == self
.order(end_date: :desc)
.first
@last_period == self
end
alias_method :is_last, :last?

Expand Down
49 changes: 37 additions & 12 deletions gems/plugins/account_reports/lib/account_reports/grade_reports.rb
Original file line number Diff line number Diff line change
Expand Up @@ -149,19 +149,13 @@ def mgp_term_csv(term)
headers << I18n.t('final score')
headers << I18n.t('enrollment state')

generate_and_run_report headers do |csv|
firstpass_file = generate_and_run_report(nil, 'firstpass.csv') do |csv|
students.find_in_batches do |student_chunk|
# loading students/courses in chunks to avoid unnecessarily
# re-loading assignments/etc. in the grade calculator
students_by_course = student_chunk.group_by { |x| x.course_id }
courses_by_id = Course.find(students_by_course.keys).index_by(&:id)
students_by_course.each do |course_id, course_students|
grading_period_grades = grading_periods.reduce({}) { |h,gp|
h[gp] = GradeCalculator.new(course_students.map(&:user_id), courses_by_id[course_id],
grading_period: gp).compute_scores
h
}

students_by_course.each do |course_id, course_students|
course_students.each do |student|
arr = []
arr << student["user_name"]
Expand All @@ -178,11 +172,11 @@ def mgp_term_csv(term)
arr << student["term_sis_id"]
arr << gp_set.title
arr << gp_set.id
grading_period_grades.each do |gp, grades|
grading_periods.each do |gp|
arr << gp.id
student_grades = grades.shift
arr << student_grades[:current][:grade]
arr << student_grades[:final][:grade]
# Putting placeholders there to be replaced after expensive grade calculation
arr << nil
arr << nil
end
arr << student["computed_current_score"]
arr << student["computed_final_score"]
Expand All @@ -192,6 +186,37 @@ def mgp_term_csv(term)
end
end
end

generate_and_run_report headers do |report_csv|
read_csv_in_chunks(firstpass_file) do |rows|
rows_by_course = rows.group_by { |arr| arr[4].to_i }
courses_by_id = Course.find(rows_by_course.keys).index_by(&:id)

rows_by_course.each do |course_id, course_rows|
grading_period_grades = grading_periods.reduce({}) do |h,gp|
h[gp] = GradeCalculator.new(course_rows.map { |arr| arr[1] },
courses_by_id[course_id],
grading_period: gp).compute_scores
h
end

course_rows.each do |row|
column = 13
grading_period_grades.each do |gp, grades|
row_grades = grades.shift
# this accounts for gp.id and moves us to where we want
# to drop our grades data.
column += 2
row[column] = row_grades[:current][:grade]
column += 1
row[column] = row_grades[:final][:grade]
end
end

rows.each { |r| report_csv << r }
end
end
end
end

private
Expand Down
20 changes: 17 additions & 3 deletions gems/plugins/account_reports/lib/account_reports/report_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -228,15 +228,29 @@ def write_report(headers, &block)
Shackles.activate(:master) { send_report(file) }
end

def generate_and_run_report(headers)
file = AccountReports.generate_file(@account_report)
def generate_and_run_report(headers = nil, extention = 'csv')
file = AccountReports.generate_file(@account_report, extention)
CSV.open(file, "w") do |csv|
csv << headers
csv << headers unless headers.nil?
Shackles.activate(:slave) { yield csv }
end
file
end

def read_csv_in_chunks(filename, chunk_size = 1000)
CSV.open(filename) do |csv|
rows = []
while (!(row = csv.readline).nil?)
rows << row
if rows.size == chunk_size
yield rows
rows = []
end
end
yield rows unless rows.empty?
end
end

def add_extra_text(text)
if @account_report.has_parameter?('extra_text')
@account_report.parameters["extra_text"] << " #{text}"
Expand Down

0 comments on commit d75d22c

Please sign in to comment.