Skip to content

Commit a362dcd

Browse files
adi-herwana-nusadi-herwana-nus
committed
fix(statistics): fix sql query gaps
- fixed handling of grades / times calculation when there are no students - fixed CsvDownloadJob not working in 'My Students' view - refactored course user selections by type to common model method
1 parent 10db90f commit a362dcd

16 files changed

Lines changed: 208 additions & 129 deletions

File tree

‎app/controllers/concerns/course/statistics/grades_concern.rb‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ module Course::Statistics::GradesConcern
33
private
44

55
def grade_statistics_hash
6+
return {} if @assessments.empty? || @all_students.empty?
7+
68
grades_info = ActiveRecord::Base.connection.execute("
79
SELECT ca.assessment_id AS id, AVG(ca.grade) AS avg, STDDEV(ca.grade) AS stdev
810
FROM (
@@ -21,6 +23,8 @@ def grade_statistics_hash
2123
end
2224

2325
def max_grade_statistics_hash
26+
return {} if @assessments.empty?
27+
2428
max_grades = Course::Assessment.find_by_sql(<<-SQL.squish
2529
SELECT assessment_id, SUM(maximum_grade) AS maximum_grade
2630
FROM (

‎app/controllers/concerns/course/statistics/times_concern.rb‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ module Course::Statistics::TimesConcern
33
private
44

55
def duration_statistics_hash
6+
return {} if @assessments.empty? || @all_students.empty?
7+
68
durations_info = ActiveRecord::Base.connection.execute("
79
SELECT ca.assessment_id AS id, AVG(ca.duration) AS avg, STDDEV(ca.duration) AS stdev
810
FROM (

‎app/controllers/course/assessment/assessments_controller.rb‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,6 @@ class Course::Assessment::AssessmentsController < Course::Assessment::Controller
1515

1616
before_action :load_question_duplication_data, only: [:show, :reorder]
1717

18-
COURSE_USERS = { my_students: 'my_students',
19-
my_students_w_phantom: 'my_students_w_phantom',
20-
students: 'students',
21-
students_w_phantom: 'students_w_phantom' }.freeze
22-
2318
def index
2419
@assessments = @assessments.ordered_by_date_and_title.with_submissions_by(current_user)
2520

@@ -198,7 +193,7 @@ def authenticate
198193

199194
def remind
200195
authorize!(:manage, @assessment)
201-
return head :bad_request unless student_course_users
196+
return head :bad_request unless CourseUser.valid_course_user_type?(params[:course_users])
202197

203198
Course::Assessment::ReminderService.
204199
send_closing_reminder(@assessment, student_course_users.pluck(:id), include_unsubscribed: true)
@@ -210,7 +205,6 @@ def remind
210205
# if more feedback types are added this function should be extended.
211206
def auto_feedback_count
212207
authorize!(:manage, @assessment)
213-
return head :bad_request unless student_course_users
214208

215209
render json: {
216210
count: draft_file_annotation_posts(student_course_users).count
@@ -220,7 +214,6 @@ def auto_feedback_count
220214
# Publish all automated feedback in this assessment's submissions.
221215
def publish_auto_feedback
222216
authorize!(:manage, @assessment)
223-
return head :bad_request unless student_course_users
224217

225218
ActiveRecord::Base.transaction do
226219
posts = draft_file_annotation_posts(student_course_users)
@@ -448,18 +441,7 @@ def ordered_assessments_by_tab
448441
end
449442

450443
def student_course_users
451-
case params[:course_users]
452-
when COURSE_USERS[:my_students]
453-
current_course_user.my_students.without_phantom_users
454-
when COURSE_USERS[:my_students_w_phantom]
455-
current_course_user.my_students
456-
when COURSE_USERS[:students_w_phantom]
457-
@assessment.course.course_users.students
458-
when COURSE_USERS[:students]
459-
@assessment.course.course_users.students.without_phantom_users
460-
else
461-
false
462-
end
444+
current_course_user.users_in_course_by_type(params[:course_users])
463445
end
464446

465447
def can_access_assessment?

‎app/controllers/course/assessment/submission/submissions_controller.rb‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,6 @@ class Course::Assessment::Submission::SubmissionsController < # rubocop:disable
2727
delegate_to_service(:load_or_create_answers)
2828
delegate_to_service(:load_or_create_submission_questions)
2929

30-
COURSE_USERS = { my_students: 'my_students',
31-
my_students_w_phantom: 'my_students_w_phantom',
32-
students: 'students',
33-
students_w_phantom: 'students_w_phantom',
34-
staff: 'staff',
35-
staff_w_phantom: 'staff_w_phantom' }.freeze
36-
3730
def index
3831
authorize!(:view_all_submissions, @assessment)
3932

@@ -400,21 +393,8 @@ def check_zombie_jobs # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComple
400393
end
401394
end
402395

403-
def course_user_ids # rubocop:disable Metrics/AbcSize
404-
@course_user_ids ||= case params[:course_users]
405-
when COURSE_USERS[:my_students]
406-
current_course_user.my_students.without_phantom_users
407-
when COURSE_USERS[:my_students_w_phantom]
408-
current_course_user.my_students
409-
when COURSE_USERS[:students_w_phantom]
410-
@assessment.course.course_users.students
411-
when COURSE_USERS[:staff]
412-
@assessment.course.course_users.staff.without_phantom_users
413-
when COURSE_USERS[:staff_w_phantom]
414-
@assessment.course.course_users.staff
415-
else
416-
@assessment.course.course_users.students.without_phantom_users
417-
end.select(:user_id)
396+
def course_user_ids
397+
@course_user_ids ||= current_course_user.users_in_course_by_type(params[:course_users]).select(:user_id)
418398
end
419399

420400
def user_ids_without_submission

‎app/controllers/course/survey/surveys_controller.rb‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,6 @@ class Course::Survey::SurveysController < Course::Survey::Controller
55
skip_load_and_authorize_resource :survey, only: [:new, :create]
66
build_and_authorize_new_lesson_plan_item :survey, class: Course::Survey, through: :course, only: [:new, :create]
77

8-
COURSE_USERS = { my_students: 'my_students',
9-
my_students_w_phantom: 'my_students_w_phantom',
10-
students: 'students',
11-
students_w_phantom: 'students_w_phantom' }.freeze
12-
138
def index
149
@surveys = @surveys.includes(responses: { experience_points_record: :course_user })
1510
preload_student_submitted_responses_counts
@@ -50,7 +45,7 @@ def results
5045

5146
def remind
5247
authorize!(:manage, @survey)
53-
return head :bad_request unless student_course_users
48+
return head :bad_request unless CourseUser.valid_course_user_type?(params[:course_users])
5449

5550
Course::Survey::ReminderService.
5651
send_closing_reminder(
@@ -73,18 +68,7 @@ def download
7368
private
7469

7570
def student_course_users
76-
case params[:course_users]
77-
when COURSE_USERS[:my_students]
78-
current_course_user.my_students.without_phantom_users
79-
when COURSE_USERS[:my_students_w_phantom]
80-
current_course_user.my_students
81-
when COURSE_USERS[:students_w_phantom]
82-
@survey.course.course_users.students
83-
when COURSE_USERS[:students]
84-
@survey.course.course_users.students.without_phantom_users
85-
else
86-
false
87-
end
71+
current_course_user.users_in_course_by_type(params[:course_users])
8872
end
8973

9074
def render_survey_with_questions_json

‎app/models/course_user.rb‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,15 @@ class CourseUser < ApplicationRecord
170170
where(user_id: user.id)
171171
end)
172172

173+
COURSE_USER_TYPES = {
174+
my_students: 'my_students',
175+
my_students_w_phantom: 'my_students_w_phantom',
176+
students: 'students',
177+
students_w_phantom: 'students_w_phantom',
178+
staff: 'staff',
179+
staff_w_phantom: 'staff_w_phantom'
180+
}.freeze
181+
173182
# Test whether the current scope includes the current user.
174183
#
175184
# @param [User] user The user to check
@@ -229,7 +238,7 @@ def suspended_from_course?(ability)
229238
#
230239
# @return[Array<CourseUser>]
231240
def my_students
232-
CourseUser.joins(group_users: :group).merge(Course::GroupUser.normal).
241+
CourseUser.joins(group_users: :group).merge(Course::GroupUser.normal).where(role: :student).
233242
where(Course::Group.arel_table[:id].in(group_users.manager.pluck(:group_id))).distinct
234243
end
235244

@@ -246,6 +255,27 @@ def latest_learning_rate_record
246255
learning_rate_records.limit(1).first
247256
end
248257

258+
def self.valid_course_user_type?(type)
259+
COURSE_USER_TYPES.value?(type)
260+
end
261+
262+
def users_in_course_by_type(type)
263+
case type
264+
when COURSE_USER_TYPES[:my_students]
265+
my_students.without_phantom_users
266+
when COURSE_USER_TYPES[:my_students_w_phantom]
267+
my_students
268+
when COURSE_USER_TYPES[:students_w_phantom]
269+
course.students
270+
when COURSE_USER_TYPES[:staff]
271+
course.staff.without_phantom_users
272+
when COURSE_USER_TYPES[:staff_w_phantom]
273+
course.staff
274+
else
275+
course.students.without_phantom_users # :students is the default type
276+
end
277+
end
278+
249279
private
250280

251281
def set_defaults

‎app/services/course/assessment/submission/csv_download_service.rb‎

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,6 @@ def generate_csv
4747
csv_file_path
4848
end
4949

50-
COURSE_USERS = { my_students: 'my_students',
51-
my_students_w_phantom: 'my_students_w_phantom',
52-
students: 'students',
53-
students_w_phantom: 'students_w_phantom',
54-
staff: 'staff',
55-
staff_w_phantom: 'staff_w_phantom' }.freeze
56-
5750
private
5851

5952
def cleanup_entries
@@ -113,21 +106,9 @@ def generate_answer_row(question, answer)
113106
answer.specific.csv_download
114107
end
115108

116-
def course_users # rubocop:disable Metrics/AbcSize
117-
@course_users ||=
118-
case @course_users_type
119-
when COURSE_USERS[:my_students]
120-
@current_course_user.my_students.without_phantom_users
121-
when COURSE_USERS[:my_students_w_phantom]
122-
@current_course_user.my_students
123-
when COURSE_USERS[:students_w_phantom]
124-
@assessment.course.course_users.students
125-
when COURSE_USERS[:staff]
126-
@assessment.course.course_users.staff.without_phantom_users
127-
when COURSE_USERS[:staff_w_phantom]
128-
@assessment.course.course_users.staff
129-
else
130-
@assessment.course.course_users.students.without_phantom_users
131-
end.order_phantom_user.order_alphabetically.includes(user: :emails)
109+
def course_users
110+
# We cannot use ORDER BY because it conflicts with the selection
111+
@course_users ||= @current_course_user.users_in_course_by_type(@course_users_type).
112+
includes(user: :emails).sort_by { |cu| [cu.phantom? ? 0 : 1, cu.name] }
132113
end
133114
end

‎app/services/course/assessment/submission/zip_download_service.rb‎

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,5 @@
11
# frozen_string_literal: true
22
class Course::Assessment::Submission::ZipDownloadService < Course::Assessment::Submission::BaseZipDownloadService
3-
COURSE_USERS = { my_students: 'my_students',
4-
my_students_w_phantom: 'my_students_w_phantom',
5-
students: 'students',
6-
students_w_phantom: 'students_w_phantom',
7-
staff: 'staff',
8-
staff_w_phantom: 'staff_w_phantom' }.freeze
9-
103
# @param [CourseUser] course_user The course user downloading the submissions.
114
# @param [Course::Assessment] assessment The assessments to download submissions from.
125
# @param [String|nil] course_users The subset of course users whose submissions to download.
@@ -44,21 +37,7 @@ def download_answers(submission, submission_dir)
4437
end
4538
end
4639

47-
def course_user_ids # rubocop:disable Metrics/AbcSize
48-
@course_user_ids ||=
49-
case @course_users
50-
when COURSE_USERS[:my_students]
51-
@course_user.my_students.without_phantom_users
52-
when COURSE_USERS[:my_students_w_phantom]
53-
@course_user.my_students
54-
when COURSE_USERS[:students_w_phantom]
55-
@assessment.course.course_users.students
56-
when COURSE_USERS[:staff]
57-
@assessment.course.course_users.staff.without_phantom_users
58-
when COURSE_USERS[:staff_w_phantom]
59-
@assessment.course.course_users.staff
60-
else
61-
@assessment.course.course_users.students.without_phantom_users
62-
end.select(:user_id)
40+
def course_user_ids
41+
@course_user_ids ||= @course_user.users_in_course_by_type(@course_users).select(:user_id)
6342
end
6443
end

‎app/views/course/statistics/aggregate/all_assessments.json.jbuilder‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22
json.numStudents @all_students.size
33

44
json.assessments @assessments do |assessment|
5-
grade_stats = @grades_hash[assessment.id] || [0, 0]
6-
duration_stats = @durations_hash[assessment.id] || [0, 0]
5+
grade_stats = @grades_hash.fetch(assessment.id, nil)
6+
duration_stats = @durations_hash.fetch(assessment.id, nil)
77

88
json.id assessment.id
99
json.title assessment.title
@@ -21,11 +21,15 @@ json.assessments @assessments do |assessment|
2121

2222
json.maximumGrade @max_grades_hash[assessment.id] || 0
2323

24-
json.averageGrade grade_stats ? grade_stats[0] : 0
25-
json.stdevGrade grade_stats ? grade_stats[1] : 0
24+
if grade_stats.present?
25+
json.averageGrade grade_stats[0]
26+
json.stdevGrade grade_stats[1]
27+
end
2628

27-
json.averageTimeTaken duration_stats[0]
28-
json.stdevTimeTaken duration_stats[1]
29+
if duration_stats.present?
30+
json.averageTimeTaken duration_stats[0]
31+
json.stdevTimeTaken duration_stats[1]
32+
end
2933

3034
json.numSubmitted @num_submitted_students_hash[assessment.id] || 0
3135
json.numAttempted @num_attempted_students_hash[assessment.id] || 0

‎client/app/bundles/course/statistics/pages/StatisticsIndex/assessments/AssessmentsStatisticsTable.tsx‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -196,12 +196,13 @@ const AssessmentsStatisticsTable: FC<Props> = (props) => {
196196
sortable: true,
197197
className: NUM_CELL_CLASS_NAME,
198198
cell: (assessment): JSX.Element => {
199-
const averageGrade = parseFloat(
200-
Number(assessment.averageGrade).toFixed(1),
201-
);
202199
const maximumGrade = parseFloat(
203200
Number(assessment.maximumGrade).toFixed(1),
204201
);
202+
const averageGrade =
203+
assessment.averageGrade === undefined
204+
? '--'
205+
: parseFloat(Number(assessment.averageGrade).toFixed(1));
205206
return (
206207
<div className={NUM_CELL_CLASS_NAME}>
207208
{`${averageGrade} / ${maximumGrade}`}
@@ -217,7 +218,9 @@ const AssessmentsStatisticsTable: FC<Props> = (props) => {
217218
className: NUM_CELL_CLASS_NAME,
218219
cell: (assessment) => (
219220
<div className={NUM_CELL_CLASS_NAME}>
220-
{parseFloat(Number(assessment.stdevGrade).toFixed(1))}
221+
{assessment.stdevGrade === undefined
222+
? '--'
223+
: parseFloat(Number(assessment.stdevGrade).toFixed(1))}
221224
</div>
222225
),
223226
csvDownloadable: true,

0 commit comments

Comments
 (0)