new gradebook: fake student ids always sort last

closes CNVS-37752

Test Plan:
1. Create a course that has some active students, inactive students,
   concluded students, and a fake student. Enable New Gradebook and go
   to the gradebook.
2. Click on the student header and select 'Inactive enrollments' and
   'Concluded enrollments'. Then select 'Sort by -> A-Z'.
3. As a teacher in the course, hit the user_ids endpoint and verify you
   get a list of user IDs back. The user IDs should be ordered such
   that their corresponding users' names are in alpha ascending order.
   The list of user IDs should also include inactive and concluded
   students' IDs. The fake student's ID should always be last.
4. Go to the gradebook and click on the student header. Select 'Sort by
   -> Z-A'.
5. Hit the user_ids endpoint and verify the returned user IDs are now
   ordered such that their corresponding users' names are in alpha
   descending order. The list of user IDs should also include inactive
   and concluded students' IDs. The fake student's ID should be last.
6. Go to the gradebook and click on the student column header. Deselect
   the 'Inactive enrollments' and 'Concluded enrollments' options.
7. Hit the user_ids endpoint and verify the returned user IDs no longer
   include IDs for inactive or concluded students. The IDs should still
   be ordered such that their corresponding users' names are in alpha
   descending order. The fake student's ID should be last.
8. Go to the gradebook and click on the total grade header. Select
   'Sort by Grade -> Low to High'
9. Hit the user_ids endpoint and verify the returned user IDs are
   ordered such that their corresponding users' current total grades
   are in ascending order. The fake student's ID should be last.
10. Go to the gradebook and click on the total grade header. Select
   'Sort by Grade -> High to Low'
11. Hit the user_ids endpoint and verify the returned user IDs are
    ordered such that their corresponding users' current total grades
    are in descending order. The fake student's ID should be last.
12. In the same manner as the steps above, make sure changing the
    following settings in the gradebook causes the user_ids
    endpoint to return an array of IDs ordered according to the
    gradebook sort preferences, with the fake student's ID sorted last:
    a) Assignment column header -> Sort by Grade -> Low to High
    b) Assignment column header -> Sort by Grade -> High to Low
    c) Assignment column header -> Sort by -> Missing (the user_ids
       endpoint should return all users with a missing submission for
       that assignment first)
    d) Assignment column header -> Sort by -> Late (the user_ids
       endpoint shoudl return all users with a late submission for that
       assignment first)
    e) Select a grading period and sort by total grade. The user_ids
       endpoint should return user IDs that are ordered such that their
       corresponding users' grades are in ascending or descending order
       for the selected grading period.
13. Conclude the course. Hit the user_ids endpoint as the teacher and
    verify you do not get a 401 response.
* Note: Sorting by assignment group totals is not currently supported.

Change-Id: I6c4b16b72e051d46cc7b7521949d5c82b92960b9
Reviewed-on: https://gerrit.instructure.com/116698
Reviewed-by: Keith T. Garner <kgarner@instructure.com>
Reviewed-by: Jeremy Neander <jneander@instructure.com>
Tested-by: Jenkins
QA-Review: KC Naegle <knaegle@instructure.com>
Product-Review: Keith T. Garner <kgarner@instructure.com>

app/controllers/gradebooks_controller.rb

@@ -687,7 +687,7 @@ class GradebooksController < ApplicationController
   end
 
   def user_ids
-    return unless authorized_action(@context, @current_user, :manage_grades)
+    return unless authorized_action(@context, @current_user, [:manage_grades, :view_all_grades])
 
     gradebook_user_ids = GradebookUserIds.new(@context, @current_user)
     render json: { user_ids: gradebook_user_ids.user_ids }

lib/gradebook_user_ids.rb

@@ -44,7 +44,11 @@ class GradebookUserIds
   private
 
   def sort_by_student_name
-    students.order_by_sortable_name(direction: @direction.to_sym).pluck(:id)
+    students.joins(:enrollments).
+      order("enrollments.type = 'StudentViewEnrollment'").
+      order_by_sortable_name(direction: @direction.to_sym).
+      pluck(:id).
+      uniq
   end
 
   def sort_by_assignment_grade(assignment_id)
@@ -54,21 +58,27 @@ class GradebookUserIds
           ON submissions.user_id = enrollments.user_id
           AND submissions.assignment_id = #{assignment_id}
       ").
-      order("submissions.score #{sort_direction} NULLS LAST").
+      order("enrollments.type = 'StudentViewEnrollment'", "submissions.score #{sort_direction} NULLS LAST").
       pluck(:user_id).
       uniq
   end
 
   def sort_by_assignment_missing(assignment_id)
     all_user_ids = student_enrollments_scope.pluck(:user_id)
-    user_ids_for_missing = Submission.missing.where(assignment_id: assignment_id, user_id: all_user_ids).pluck(:user_id)
-    user_ids_for_missing.concat(all_user_ids).uniq
+    fake_user_ids = student_enrollments_scope.where(type: "StudentViewEnrollment").pluck(:user_id)
+    real_user_ids = all_user_ids - fake_user_ids
+    user_ids_for_missing = Submission.missing.
+      where(assignment_id: assignment_id, user_id: real_user_ids).
+      pluck(:user_id)
+    user_ids_for_missing.concat(real_user_ids).concat(fake_user_ids).uniq
   end
 
   def sort_by_assignment_late(assignment_id)
     all_user_ids = student_enrollments_scope.pluck(:user_id)
-    user_ids_for_late = Submission.late.where(assignment_id: assignment_id, user_id: all_user_ids).pluck(:user_id)
-    user_ids_for_late.concat(all_user_ids).uniq
+    fake_user_ids = student_enrollments_scope.where(type: "StudentViewEnrollment").pluck(:user_id)
+    real_user_ids = all_user_ids - fake_user_ids
+    user_ids_for_late = Submission.late.where(assignment_id: assignment_id, user_id: real_user_ids).pluck(:user_id)
+    user_ids_for_late.concat(real_user_ids).concat(fake_user_ids).uniq
   end
 
   def sort_by_total_grade
@@ -78,7 +88,7 @@ class GradebookUserIds
           ON enrollments.id = scores.enrollment_id
           AND scores.grading_period_id #{grading_period_id ? "= #{grading_period_id}" : 'IS NULL'}
       ").
-      order("scores.current_score #{sort_direction} NULLS LAST").
+      order("enrollments.type = 'StudentViewEnrollment'", "scores.current_score #{sort_direction} NULLS LAST").
       pluck(:user_id).
       uniq
   end

spec/controllers/gradebooks_controller_spec.rb

@@ -788,6 +788,19 @@ describe GradebooksController do
       assert_status(401)
     end
 
+    it "grants authorization to teachers in active courses" do
+      user_session(@teacher)
+      get :user_ids, course_id: @course.id, format: :json
+      expect(response).to be_ok
+    end
+
+    it "grants authorization to teachers in concluded courses" do
+      @course.complete!
+      user_session(@teacher)
+      get :user_ids, course_id: @course.id, format: :json
+      expect(response).to be_ok
+    end
+
     it "returns an array of user ids sorted according to the user's preferences" do
       student1 = @student
       student1.update!(name: "Jon")

spec/lib/gradebook_user_ids_spec.rb

@@ -61,18 +61,20 @@ describe GradebookUserIds do
     concluded_enrollment.conclude
     @concluded_student = concluded_enrollment.user
     @concluded_student.update!(sortable_name: "Concluded Student")
+    @fake_student_enrollment = course_with_user('StudentViewEnrollment', course: @course, active_all: true)
+    @fake_student = @fake_student_enrollment.user
   end
 
   let(:gradebook_user_ids) { GradebookUserIds.new(@course, @teacher) }
 
   it "sorts by sortable name ascending if the user does not have any saved sort preferences" do
     @teacher.preferences[:gradebook_settings] = {}
-    expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id])
+    expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id, @fake_student.id])
   end
 
   it "sorts by sortable name ascending if the user's sort preferences are not supported" do
     @teacher.preferences[:gradebook_settings][@course.id][:sort_rows_by_column_id] = "some_new_column"
-    expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id])
+    expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id, @fake_student.id])
   end
 
   it "does not return duplicate user ids for students with multiple enrollments" do
@@ -84,7 +86,7 @@ describe GradebookUserIds do
       active_all: true,
       allow_multiple_enrollments: true
     )
-    expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id])
+    expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id, @fake_student.id])
   end
 
   it "only returns users belonging to the selected section" do
@@ -101,13 +103,18 @@ describe GradebookUserIds do
   end
 
   describe "student sortable name sorting" do
-    it "sorts by student sortbale name ascending" do
-      expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id])
+    it "sorts by student sortable name ascending" do
+      expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id, @fake_student.id])
     end
 
-    it "sorts by student sortbale name descending" do
+    it "excludes fake students if they are deactivated" do
+      @fake_student_enrollment.deactivate
+      expect(gradebook_user_ids.user_ids).not_to include @fake_student.id
+    end
+
+    it "sorts by student sortable name descending" do
       @teacher.preferences[:gradebook_settings][@course.id][:sort_rows_by_direction] = "descending"
-      expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id])
+      expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id, @fake_student.id])
     end
 
     it "includes inactive student ids if the user preferences include show_inactive_enrollments" do
@@ -140,6 +147,11 @@ describe GradebookUserIds do
         expect(gradebook_user_ids.user_ids.first).to eq(@student3.id)
       end
 
+      it "excludes fake students if they are deactivated" do
+        @fake_student_enrollment.deactivate
+        expect(gradebook_user_ids.user_ids).not_to include @fake_student.id
+      end
+
       it "includes inactive student ids if the user preferences include show_inactive_enrollments" do
         @teacher.preferences[:gradebook_settings][@course.id][:show_inactive_enrollments] = "true"
         expect(gradebook_user_ids.user_ids).to include @inactive_student.id
@@ -165,6 +177,11 @@ describe GradebookUserIds do
         expect(gradebook_user_ids.user_ids.first).to eq(@student3.id)
       end
 
+      it "excludes fake students if they are deactivated" do
+        @fake_student_enrollment.deactivate
+        expect(gradebook_user_ids.user_ids).not_to include @fake_student.id
+      end
+
       it "includes inactive student ids if the user preferences include show_inactive_enrollments" do
         @teacher.preferences[:gradebook_settings][@course.id][:show_inactive_enrollments] = "true"
         expect(gradebook_user_ids.user_ids).to include @inactive_student.id
@@ -190,24 +207,33 @@ describe GradebookUserIds do
         end
 
         it "returns user ids sorted by grade on the assignment" do
-          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student1.id, @student3.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student1.id, @student3.id, @fake_student.id])
         end
 
-        it "places students without submissions at the end" do
-          student3 = student_in_course(course: @course, active_all: true).user
-          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student1.id, @student3.id, student3.id])
+        it "excludes fake students if they are deactivated" do
+          @fake_student_enrollment.deactivate
+          expect(gradebook_user_ids.user_ids).not_to include @fake_student.id
         end
 
-        it "places students that have been graded with nil grades at the end" do
+        it "places students without submissions at the end, but before fake students" do
+          student4 = student_in_course(course: @course, active_all: true).user
+          expect(gradebook_user_ids.user_ids).to eq(
+            [@student2.id, @student1.id, @student3.id, student4.id, @fake_student.id]
+          )
+        end
+
+        it "places students that have been graded with nil grades at the end, but before fake students" do
           @assignment.grade_student(@student1, grade: nil, grader: @teacher)
-          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id, @fake_student.id])
         end
 
-        it "places students that are not assigned at the end" do
+        it "places students that are not assigned at the end, but before fake students" do
           @assignment.update!(only_visible_to_overrides: true)
           create_adhoc_override_for_assignment(@assignment, [@student1, @student3, @student2], due_at: nil)
-          student3 = student_in_course(course: @course, active_all: true).user
-          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student1.id, @student3.id, student3.id])
+          student4 = student_in_course(course: @course, active_all: true).user
+          expect(gradebook_user_ids.user_ids).to eq(
+            [@student2.id, @student1.id, @student3.id, student4.id, @fake_student.id]
+          )
         end
       end
 
@@ -219,25 +245,36 @@ describe GradebookUserIds do
         end
 
         it "returns user ids sorted by grade on the assignment" do
-          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student1.id, @student2.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student1.id, @student2.id, @fake_student.id])
         end
 
-        it "places students without submissions at the end" do
-          student3 = student_in_course(course: @course, active_all: true).user
-          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student1.id, @student2.id, student3.id])
+        it "excludes fake students if they are deactivated" do
+          @fake_student_enrollment.deactivate
+          expect(gradebook_user_ids.user_ids).not_to include @fake_student.id
         end
 
-        it "places students that have been graded with a nil grade at the end" do
+        it "places students without submissions at the end, but before fake students" do
+          student4 = student_in_course(course: @course, active_all: true).user
+          expect(gradebook_user_ids.user_ids).to eq(
+            [@student3.id, @student1.id, @student2.id, student4.id, @fake_student.id]
+          )
+        end
+
+        it "places students that have been graded with a nil grade at the end, but before fake students" do
           student3 = student_in_course(course: @course, active_all: true).user
           @assignment.grade_student(student3, grade: nil, grader: @teacher)
-          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student1.id, @student2.id, student3.id])
+          expect(gradebook_user_ids.user_ids).to eq(
+            [@student3.id, @student1.id, @student2.id, student3.id, @fake_student.id]
+          )
         end
 
-        it "places students that are not assigned at the end" do
+        it "places students that are not assigned at the end, but before fake students" do
           @assignment.update!(only_visible_to_overrides: true)
           create_adhoc_override_for_assignment(@assignment, [@student1, @student3, @student2], due_at: nil)
           student3 = student_in_course(course: @course, active_all: true).user
-          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student1.id, @student2.id, student3.id])
+          expect(gradebook_user_ids.user_ids).to eq(
+            [@student3.id, @student1.id, @student2.id, student3.id, @fake_student.id]
+          )
         end
       end
 
@@ -272,12 +309,17 @@ describe GradebookUserIds do
     end
 
     it "sorts by total grade ascending" do
-      expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student2.id, @student3.id])
+      expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student2.id, @student3.id, @fake_student.id])
+    end
+
+    it "excludes fake students if they are deactivated" do
+      @fake_student_enrollment.deactivate
+      expect(gradebook_user_ids.user_ids).not_to include @fake_student.id
     end
 
     it "sorts by total grade descending" do
       @teacher.preferences[:gradebook_settings][@course.id][:sort_rows_by_direction] = "descending"
-      expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student2.id, @student1.id])
+      expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student2.id, @student1.id, @fake_student.id])
     end
 
     it "includes inactive student ids if the user preferences include show_inactive_enrollments" do
@@ -310,25 +352,26 @@ describe GradebookUserIds do
       context "ascending" do
         it "sorts by the current grading period totals if no selected grading period is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
-          expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id, @fake_student.id])
         end
 
         it "sorts by the current grading period totals if a grading period ID of 'null' is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
           @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] = "null"
-          expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student3.id, @student2.id, @fake_student.id])
         end
 
         it "sorts by the selected grading period totals if a selected grading period is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
-          @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] = @future_period.id.to_s
-          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student1.id, @student3.id])
+          @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] =
+            @future_period.id.to_s
+          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student1.id, @student3.id, @fake_student.id])
         end
 
         it "sorts by 'All Grading Periods' if a grading period ID of '0' is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
           @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] = "0"
-          expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student2.id, @student3.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student1.id, @student2.id, @student3.id, @fake_student.id])
         end
       end
 
@@ -339,25 +382,26 @@ describe GradebookUserIds do
 
         it "sorts by the current grading period totals if no selected grading period is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
-          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id, @fake_student.id])
         end
 
         it "sorts by the current grading period totals if a grading period ID of 'null' is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
           @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] = "null"
-          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student2.id, @student3.id, @student1.id, @fake_student.id])
         end
 
         it "sorts by the selected grading period totals if a selected grading period is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
-          @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] = @future_period.id.to_s
-          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student1.id, @student2.id])
+          @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] =
+            @future_period.id.to_s
+          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student1.id, @student2.id, @fake_student.id])
         end
 
         it "sorts by 'All Grading Periods' if a grading period ID of '0' is in user preferences" do
           @course.stubs(:grading_periods?).returns(true)
           @teacher.preferences[:gradebook_settings][@course.id][:filter_columns_by][:grading_period_id] = "0"
-          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student2.id, @student1.id])
+          expect(gradebook_user_ids.user_ids).to eq([@student3.id, @student2.id, @student1.id, @fake_student.id])
         end
       end
     end