update new gradebook feature flag

This patchset updates the new gradebook feature flag to 1) allow
teachers to enable the new gradebook for a course (if the admin has
set new gradebook to allow) and 2) disable the ability to turn off the
new gradebook for a course if backwards incompatible features are
used. For now, the features that would stop a move backwards are
enabled late or missing policies, or any submission that had a
late_policy_status manually set.

closes GRADE-691

test plan:
 - As an admin, in the account settings set new gradebook to allow
 - Create a course with a teacher, a ta, a student, and an assignment
 - As a ta, go the course settings and confirm you can't configure new
   gradebook
 - Perform the following actions as a teacher unless otherwise
   specified
    - go to the course settings and enable the new gradebook
    - in the gradebook, configure a late policy
    - go the course settings and confirm you can't turn off new
      gradebook
    - as an admin, go to the course settings and confirm you can't
      turn off new gradebook
    - in the gradebook, turn off the late policy and configure a
      missing policy
    - go the course settings and confirm you can't turn off new
      gradebook
    - as an admin, go the course settings and confirm you can't turn
      off new gradebook
    - in the gradebook, turn off the missing policy
    - go the course settings and confirm you can turn off new
      gradebook (but leave it enabled)
    - as an admin, go the course settings and confirm you can turn off
      new gradebook (but leave it enabled)
    - go into the gradebook, set a missing or late status on the
      assignment for the student
    - go the course settings and confirm you can't turn off new
      gradebook
    - as an admin, go the course settings and confirm you can't turn
      off new gradebook

Change-Id: Ibfde1398c60254de1fd2b9697e8155fabbb1b781
Reviewed-on: https://gerrit.instructure.com/134521
Tested-by: Jenkins
Reviewed-by: Shahbaz Javeed <sjaveed@instructure.com>
Reviewed-by: Derek Bender <djbender@instructure.com>
QA-Review: Anju Reddy <areddy@instructure.com>
Product-Review: Keith T. Garner <kgarner@instructure.com>

app/models/course.rb

@@ -3090,6 +3090,22 @@ class Course < ActiveRecord::Base
       !relevant_grading_period_group&.weighted?
   end
 
+  # This method will be around while we still have two
+  # gradebooks. This method should be used in situations where we want
+  # to identify the user can't move backwards, such as feature flags
+  def gradebook_backwards_incompatible_features_enabled?
+    # The old gradebook can't deal with late policies at all
+    return true if late_policy&.missing_submission_deduction_enabled? || late_policy&.late_submission_deduction_enabled?
+
+    # If you've used the grade tray status changes at all, you can't
+    # go back. Even if set to none, it'll break "Message Students
+    # Who..." for unsubmitted.
+    expire_time = Setting.get('late_policy_tainted_submissions', 1.hour).to_i
+    Rails.cache.fetch(['late_policy_tainted_submissions', self].cache_key, expires_in: expire_time) do
+      submissions.except(:order).where(late_policy_status: ['missing', 'late', 'none']).exists?
+    end
+  end
+
   private
 
   def effective_due_dates

db/migrate/20171204191806_add_index_on_submissions_late_policy_status.rb

@@ -0,0 +1,26 @@
+# Copyright (C) 2017 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+
+class AddIndexOnSubmissionsLatePolicyStatus < ActiveRecord::Migration[5.0]
+  tag :predeploy
+  disable_ddl_transaction!
+
+  def change
+    add_index :submissions, :late_policy_status,
+              where: "workflow_state<>'deleted' and late_policy_status IS NOT NULL",
+              algorithm: :concurrently
+  end
+end

lib/feature.rb

@@ -223,9 +223,14 @@ END
 
       custom_transition_proc: ->(user, context, _from_state, transitions) do
         if context.is_a?(Course)
-          user_may_change_flag = context.account.grants_right?(user, :manage_account_settings)
-          transitions['on']['locked'] = !user_may_change_flag if transitions&.dig('on')
-          transitions['off']['locked'] = !user_may_change_flag if transitions&.dig('off')
+          if !context.grants_right?(user, :change_course_state)
+            transitions['on']['locked'] = true if transitions&.dig('on')
+            transitions['off']['locked'] = true if transitions&.dig('off')
+          else
+            should_lock = context.gradebook_backwards_incompatible_features_enabled?
+            transitions['on']['locked'] = should_lock if transitions&.dig('on')
+            transitions['off']['locked'] = should_lock if transitions&.dig('off')
+          end
         elsif context.is_a?(Account)
           transitions['on']['locked'] = true if transitions&.dig('on')
         end

spec/lib/feature_spec.rb

@@ -232,21 +232,97 @@ end
 describe "new_gradebook" do
   let(:ngb_trans_proc) { Feature.definitions["new_gradebook"].custom_transition_proc }
   let(:root_account) { account_model }
-  let(:trans_empty) { { "on" => {}, "allowed" => {}, "off" => {} } }
-  let(:locked) { { "locked" => true } }
+  let(:transitions) { { "on" => {}, "allowed" => {}, "off" => {} } }
+  let(:course) { course_factory(account: root_account, active_all: true) }
+  let(:teacher) { teacher_in_course(course: course).user }
+  let(:ta) { ta_in_course(course: course).user }
+  let(:admin) { account_admin_user(account: root_account) }
 
-  it "only allows admins to enable the new gradebook" do
-    test_course = course_factory(account: root_account, active_all: true)
-    teacher = teacher_in_course(course: test_course)
-    transitions = trans_empty
-    ngb_trans_proc.call(teacher, test_course, nil, transitions)
-    expect(transitions).to include({ "on" => locked, "off" => locked })
+  LOCKED = { "locked" => true }.freeze
+  UNLOCKED = { "locked" => false }.freeze
+
+  it "allows admins to enable the new gradebook" do
+    ngb_trans_proc.call(admin, course, nil, transitions)
+    expect(transitions).to include({ "on" => UNLOCKED, "off" => UNLOCKED })
+  end
+
+  it "allows teachers to enable the new gradebook" do
+    ngb_trans_proc.call(teacher, course, nil, transitions)
+    expect(transitions).to include({ "on" => UNLOCKED, "off" => UNLOCKED })
+  end
+
+  it "doesn't allow tas to enable the new gradebook" do
+    ngb_trans_proc.call(ta, course, nil, transitions)
+    expect(transitions).to include({ "on" => LOCKED, "off" => LOCKED })
   end
 
   it "does not allow enabling new gradebook on an entire account" do
-    admin = user_factory(account: root_account)
-    transitions = trans_empty
     ngb_trans_proc.call(admin, root_account, nil, transitions)
-    expect(transitions).to include({ "on" => locked })
+    expect(transitions).to include({ "on" => LOCKED })
+  end
+
+  context "backwards compatibility" do
+    let(:student) { student_in_course(course: course).user }
+    let!(:assignment) { course.assignments.create!(title: 'assignment', points_possible: 10) }
+    let(:submission) { assignment.submissions.find_by(user: student) }
+
+    it "blocks disabling new gradebook on a course if there are any submissions with a late_policy_status of none" do
+      submission.late_policy_status = 'none'
+      submission.save!
+
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => LOCKED, "off" => LOCKED })
+    end
+
+    it "blocks disabling new gradebook on a course if there are any submissions with a late_policy_status of missing" do
+      submission.late_policy_status = 'missing'
+      submission.save!
+
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => LOCKED, "off" => LOCKED })
+    end
+
+    it "blocks disabling new gradebook on a course if there are any submissions with a late_policy_status of late" do
+      submission.late_policy_status = 'late'
+      submission.save!
+
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => LOCKED, "off" => LOCKED })
+    end
+
+    it "allows disabling new gradebook on a course if there are no submissions with a late_policy_status" do
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => UNLOCKED, "off" => UNLOCKED })
+    end
+
+    it "blocks disabling new gradebook on a course if a late policy is configured" do
+      course.late_policy = LatePolicy.new(late_submission_deduction_enabled: true)
+
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => LOCKED, "off" => LOCKED })
+    end
+
+    it "blocks disabling new gradebook on a course if a missing policy is configured" do
+      course.late_policy = LatePolicy.new(missing_submission_deduction_enabled: true)
+
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => LOCKED, "off" => LOCKED })
+    end
+
+    it "blocks disabling new gradebook on a course if both a late and missing policy is configured" do
+      course.late_policy =
+        LatePolicy.new(late_submission_deduction_enabled: true, missing_submission_deduction_enabled: true)
+
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => LOCKED, "off" => LOCKED })
+    end
+
+    it "allows disabling new gradebook on a course if both policies are disabled" do
+      course.late_policy =
+        LatePolicy.new(late_submission_deduction_enabled: false, missing_submission_deduction_enabled: false)
+
+      ngb_trans_proc.call(admin, course, nil, transitions)
+      expect(transitions).to include({ "on" => UNLOCKED, "off" => UNLOCKED })
+    end
   end
 end

spec/models/course_spec.rb

@@ -4996,4 +4996,69 @@ describe Course, "#show_total_grade_as_points?" do
       end
     end
   end
+
+  describe Course, "#gradebook_backwards_incompatible_features_enabled?" do
+    let(:course) { Course.create! }
+
+    it "returns true if a late policy is enabled" do
+      course.late_policy = LatePolicy.new(late_submission_deduction_enabled: true)
+
+      expect(course.gradebook_backwards_incompatible_features_enabled?).to be true
+    end
+
+    it "returns true if a missing policy is enabled" do
+      course.late_policy = LatePolicy.new(missing_submission_deduction_enabled: true)
+
+      expect(course.gradebook_backwards_incompatible_features_enabled?).to be true
+    end
+
+    it "returns true if both a late and missing policy are enabled" do
+      course.late_policy =
+        LatePolicy.new(late_submission_deduction_enabled: true, missing_submission_deduction_enabled: true)
+
+      expect(course.gradebook_backwards_incompatible_features_enabled?).to be true
+    end
+
+    it "returns false if there are no policies" do
+      expect(course.gradebook_backwards_incompatible_features_enabled?).to be false
+    end
+
+    it "returns false if both policies are disabled" do
+      course.late_policy =
+        LatePolicy.new(late_submission_deduction_enabled: false, missing_submission_deduction_enabled: false)
+
+      expect(course.gradebook_backwards_incompatible_features_enabled?).to be false
+    end
+
+    context "With submissions" do
+      let(:student) { student_in_course(course: course).user }
+      let!(:assignment) { course.assignments.create!(title: 'assignment', points_possible: 10) }
+      let(:submission) { assignment.submissions.find_by(user: student) }
+
+      it "returns true if they are any submissions with a late_policy_status of none" do
+        submission.late_policy_status = 'none'
+        submission.save!
+
+        expect(course.gradebook_backwards_incompatible_features_enabled?).to be true
+      end
+
+      it "returns true if they are any submissions with a late_policy_status of missing" do
+        submission.late_policy_status = 'missing'
+        submission.save!
+
+        expect(course.gradebook_backwards_incompatible_features_enabled?).to be true
+      end
+
+      it "returns true if they are any submissions with a late_policy_status of late" do
+        submission.late_policy_status = 'late'
+        submission.save!
+
+        expect(course.gradebook_backwards_incompatible_features_enabled?).to be true
+      end
+
+      it "returns false if there are no policies and no submissions with late_policy_status" do
+        expect(course.gradebook_backwards_incompatible_features_enabled?).to be false
+      end
+    end
+  end
 end