Skip to content

Commit 2df8738

Browse files
fix(submissions): fix 500 on course user type requests for admins
- Moved CourseUser::users_in_course_by_type -> Course::course_users_by_type - Standardized variable names for CsvDownloadService and ZipDownloadService, handle case where current_course_user is nil
1 parent 2b34c60 commit 2df8738

12 files changed

Lines changed: 195 additions & 110 deletions

File tree

app/controllers/course/assessment/assessments_controller.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ def authenticate
193193

194194
def remind
195195
authorize!(:manage, @assessment)
196-
return head :bad_request unless CourseUser.valid_course_user_type?(params[:course_users])
196+
return head :bad_request unless Course.valid_course_user_type?(params[:course_users])
197197

198198
Course::Assessment::ReminderService.
199199
send_closing_reminder(@assessment, student_course_users.pluck(:id), include_unsubscribed: true)
@@ -441,7 +441,7 @@ def ordered_assessments_by_tab
441441
end
442442

443443
def student_course_users
444-
current_course_user.users_in_course_by_type(params[:course_users])
444+
current_course.course_users_by_type(params[:course_users], current_course_user)
445445
end
446446

447447
def can_access_assessment?

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,8 @@ def check_zombie_jobs # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComple
394394
end
395395

396396
def course_user_ids
397-
@course_user_ids ||= current_course_user.users_in_course_by_type(params[:course_users]).select(:user_id)
397+
@course_user_ids ||=
398+
current_course.course_users_by_type(params[:course_users], current_course_user).select(:user_id)
398399
end
399400

400401
def user_ids_without_submission

app/controllers/course/survey/surveys_controller.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ def results
4545

4646
def remind
4747
authorize!(:manage, @survey)
48-
return head :bad_request unless CourseUser.valid_course_user_type?(params[:course_users])
48+
return head :bad_request unless Course.valid_course_user_type?(params[:course_users])
4949

5050
Course::Survey::ReminderService.
5151
send_closing_reminder(
@@ -68,7 +68,7 @@ def download
6868
private
6969

7070
def student_course_users
71-
current_course_user.users_in_course_by_type(params[:course_users])
71+
current_course.course_users_by_type(params[:course_users], current_course_user)
7272
end
7373

7474
def render_survey_with_questions_json
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# frozen_string_literal: true
2+
module Course::CourseUserTypeConcern
3+
extend ActiveSupport::Concern
4+
5+
COURSE_USER_TYPES = {
6+
my_students: 'my_students',
7+
my_students_w_phantom: 'my_students_w_phantom',
8+
students: 'students',
9+
students_w_phantom: 'students_w_phantom',
10+
staff: 'staff',
11+
staff_w_phantom: 'staff_w_phantom'
12+
}.freeze
13+
14+
module ClassMethods
15+
def valid_course_user_type?(type)
16+
COURSE_USER_TYPES.value?(type)
17+
end
18+
end
19+
20+
# rubocop:disable Metrics/CyclomaticComplexity
21+
def course_users_by_type(type, user)
22+
case type
23+
when COURSE_USER_TYPES[:my_students]
24+
user&.my_students&.without_phantom_users || CourseUser.none
25+
when COURSE_USER_TYPES[:my_students_w_phantom]
26+
user&.my_students || CourseUser.none
27+
when COURSE_USER_TYPES[:students_w_phantom]
28+
students
29+
when COURSE_USER_TYPES[:staff]
30+
staff.without_phantom_users
31+
when COURSE_USER_TYPES[:staff_w_phantom]
32+
staff
33+
else
34+
students.without_phantom_users # :students is the default type
35+
end
36+
end
37+
# rubocop:enable Metrics/CyclomaticComplexity
38+
end

app/models/course.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ class Course < ApplicationRecord
55
include Course::CourseComponentsConcern
66
include TimeZoneConcern
77
include Generic::CollectionConcern
8+
include Course::CourseUserTypeConcern
89

910
acts_as_tenant :instance, inverse_of: :courses
1011
has_settings_on :settings do |s|

app/models/course_user.rb

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -170,15 +170,6 @@ 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-
182173
# Test whether the current scope includes the current user.
183174
#
184175
# @param [User] user The user to check
@@ -255,27 +246,6 @@ def latest_learning_rate_record
255246
learning_rate_records.limit(1).first
256247
end
257248

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-
279249
private
280250

281251
def set_defaults

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,14 @@
33
class Course::Assessment::Submission::CsvDownloadService
44
include TmpCleanupHelper
55

6-
# @param [CourseUser] current_course_user The course user downloading the submissions.
6+
# @param [CourseUser|nil] current_course_user The course user downloading the submissions.
77
# @param [Course::Assessment] assessment The assessments to download submissions from.
8-
# @param [String|nil] course_users_type The subset of course users whose submissions to download.
8+
# @param [String|nil] course_user_type The subset of course users whose submissions to download.
99
# Accepted values: 'my_students', 'my_students_w_phantom', 'students', 'students_w_phantom'
1010
# 'staff', 'staff_w_phantom'
11-
def initialize(current_course_user, assessment, course_users_type)
11+
def initialize(current_course_user, assessment, course_user_type)
1212
@current_course_user = current_course_user
13-
@course_users_type = course_users_type
13+
@course_user_type = course_user_type
1414
@assessment = assessment
1515

1616
@question_assessments = Course::QuestionAssessment.where(assessment_id: assessment.id).
@@ -108,7 +108,8 @@ def generate_answer_row(question, answer)
108108

109109
def course_users
110110
# 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).
111+
source_course = @current_course_user&.course || @assessment.course
112+
@course_users ||= source_course.course_users_by_type(@course_user_type, @current_course_user).
112113
includes(user: :emails).sort_by { |cu| [cu.phantom? ? 0 : 1, cu.name] }
113114
end
114115
end

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
# frozen_string_literal: true
22
class Course::Assessment::Submission::ZipDownloadService < Course::Assessment::Submission::BaseZipDownloadService
3-
# @param [CourseUser] course_user The course user downloading the submissions.
3+
# @param [CourseUser|nil] current_course_user The course user downloading the submissions.
44
# @param [Course::Assessment] assessment The assessments to download submissions from.
5-
# @param [String|nil] course_users The subset of course users whose submissions to download.
5+
# @param [String|nil] course_user_type The subset of course users whose submissions to download.
66
# Accepted values: 'my_students', 'my_students_w_phantom', 'students', 'students_w_phantom'
77
# 'staff', 'staff_w_phantom'
8-
def initialize(course_user, assessment, course_users)
8+
def initialize(current_course_user, assessment, course_user_type)
99
super()
10-
@course_user = course_user
10+
@current_course_user = current_course_user
1111
@assessment = assessment
1212
@questions = assessment.questions.to_h { |q| [q.id, q] }
13-
@course_users = course_users
13+
@course_user_type = course_user_type
1414
end
1515

1616
private
@@ -38,6 +38,7 @@ def download_answers(submission, submission_dir)
3838
end
3939

4040
def course_user_ids
41-
@course_user_ids ||= @course_user.users_in_course_by_type(@course_users).select(:user_id)
41+
source_course = @current_course_user&.course || @assessment.course
42+
@course_user_ids ||= source_course.course_users_by_type(@course_user_type, @current_course_user).select(:user_id)
4243
end
4344
end

spec/controllers/course/assessment/assessments_controller_spec.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@
233233
get :auto_feedback_count, as: :json, params: {
234234
course_id: course,
235235
id: assessment,
236-
course_users: CourseUser::COURSE_USER_TYPES[:students]
236+
course_users: Course::COURSE_USER_TYPES[:students]
237237
}
238238
end
239239

@@ -269,7 +269,7 @@
269269
patch :publish_auto_feedback, as: :json, params: {
270270
course_id: course,
271271
id: assessment,
272-
course_users: CourseUser::COURSE_USER_TYPES[:students],
272+
course_users: Course::COURSE_USER_TYPES[:students],
273273
rating: 4
274274
}
275275
end

spec/models/course_spec.rb

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,135 @@
201201
end
202202
end
203203

204+
describe Course::CourseUserTypeConcern do
205+
let(:course) { create(:course) }
206+
let!(:group_manager) { create(:course_manager, course: course) }
207+
let!(:student) { create(:course_student, course: course) }
208+
let!(:phantom_student) { create(:course_student, :phantom, course: course) }
209+
let!(:ta) { create(:course_teaching_assistant, course: course) }
210+
let!(:phantom_ta) { create(:course_teaching_assistant, :phantom, course: course) }
211+
let!(:group) do
212+
grp = create(:course_group, course: course)
213+
create(:course_group_manager, course: course, group: grp, course_user: group_manager)
214+
create(:course_group_student, course: course, group: grp, course_user: student)
215+
create(:course_group_student, course: course, group: grp, course_user: phantom_student)
216+
grp
217+
end
218+
219+
describe '.valid_course_user_type?' do
220+
it 'returns true for all valid type strings' do
221+
%w[students students_w_phantom my_students my_students_w_phantom staff staff_w_phantom].each do |type|
222+
expect(Course.valid_course_user_type?(type)).to be true
223+
end
224+
end
225+
226+
it 'returns false for nil' do
227+
expect(Course.valid_course_user_type?(nil)).to be false
228+
end
229+
230+
it 'returns false for unknown strings' do
231+
expect(Course.valid_course_user_type?('invalid')).to be false
232+
end
233+
234+
it 'returns false for symbol versions of valid types' do
235+
expect(Course.valid_course_user_type?(:students)).to be false
236+
end
237+
end
238+
239+
describe '#course_users_by_type' do
240+
context 'when user is a course user' do
241+
it "returns non-phantom students for 'students'" do
242+
result = course.course_users_by_type('students', group_manager)
243+
expect(result).to include(student)
244+
expect(result).not_to include(phantom_student, ta)
245+
end
246+
247+
it "returns all students including phantoms for 'students_w_phantom'" do
248+
result = course.course_users_by_type('students_w_phantom', group_manager)
249+
expect(result).to include(student, phantom_student)
250+
expect(result).not_to include(ta)
251+
end
252+
253+
it "returns non-phantom group students for 'my_students'" do
254+
result = course.course_users_by_type('my_students', group_manager)
255+
expect(result).to include(student)
256+
expect(result).not_to include(phantom_student, ta)
257+
end
258+
259+
it "returns all group students including phantoms for 'my_students_w_phantom'" do
260+
result = course.course_users_by_type('my_students_w_phantom', group_manager)
261+
expect(result).to include(student, phantom_student)
262+
expect(result).not_to include(ta)
263+
end
264+
265+
it "returns non-phantom staff for 'staff'" do
266+
result = course.course_users_by_type('staff', group_manager)
267+
expect(result).to include(ta)
268+
expect(result).not_to include(phantom_ta, student)
269+
end
270+
271+
it "returns all staff including phantoms for 'staff_w_phantom'" do
272+
result = course.course_users_by_type('staff_w_phantom', group_manager)
273+
expect(result).to include(ta, phantom_ta)
274+
expect(result).not_to include(student)
275+
end
276+
277+
it 'defaults to non-phantom students for unknown types' do
278+
result = course.course_users_by_type(nil, group_manager)
279+
expect(result).to include(student)
280+
expect(result).not_to include(phantom_student, ta)
281+
end
282+
end
283+
284+
# Regression: admins not enrolled in a course have nil as current_course_user,
285+
# which previously caused a 500 when the old CourseUser#users_in_course_by_type
286+
# was called on nil.
287+
context 'when user is nil (e.g. a system administrator not enrolled in the course)' do
288+
it "returns non-phantom students for 'students'" do
289+
result = course.course_users_by_type('students', nil)
290+
expect(result).to include(student)
291+
expect(result).not_to include(phantom_student, ta)
292+
end
293+
294+
it "returns all students including phantoms for 'students_w_phantom'" do
295+
result = course.course_users_by_type('students_w_phantom', nil)
296+
expect(result).to include(student, phantom_student)
297+
expect(result).not_to include(ta)
298+
end
299+
300+
it "returns an empty result for 'my_students'" do
301+
result = course.course_users_by_type('my_students', nil)
302+
expect(result).to be_a(ActiveRecord::Relation)
303+
expect(result).to be_empty
304+
end
305+
306+
it "returns an empty result for 'my_students_w_phantom'" do
307+
result = course.course_users_by_type('my_students_w_phantom', nil)
308+
expect(result).to be_a(ActiveRecord::Relation)
309+
expect(result).to be_empty
310+
end
311+
312+
it "returns non-phantom staff for 'staff'" do
313+
result = course.course_users_by_type('staff', nil)
314+
expect(result).to include(ta)
315+
expect(result).not_to include(phantom_ta, student)
316+
end
317+
318+
it "returns all staff including phantoms for 'staff_w_phantom'" do
319+
result = course.course_users_by_type('staff_w_phantom', nil)
320+
expect(result).to include(ta, phantom_ta)
321+
expect(result).not_to include(student)
322+
end
323+
324+
it 'defaults to non-phantom students for unknown types' do
325+
result = course.course_users_by_type(nil, nil)
326+
expect(result).to include(student)
327+
expect(result).not_to include(phantom_student, ta)
328+
end
329+
end
330+
end
331+
end
332+
204333
describe 'calculated attributes' do
205334
let(:course) { create(:course) }
206335
let!(:course_users) { create_list(:course_user, 2, course: course) }

0 commit comments

Comments
 (0)