From 6c6f18930f70385961ab1b5e6e51747d64456ddb Mon Sep 17 00:00:00 2001 From: Keith Garner Date: Mon, 4 Dec 2017 13:46:11 -0600 Subject: [PATCH] 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 Reviewed-by: Derek Bender QA-Review: Anju Reddy Product-Review: Keith T. Garner --- app/models/course.rb | 16 +++ ...index_on_submissions_late_policy_status.rb | 26 +++++ lib/feature.rb | 11 ++- spec/lib/feature_spec.rb | 98 ++++++++++++++++--- spec/models/course_spec.rb | 65 ++++++++++++ 5 files changed, 202 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20171204191806_add_index_on_submissions_late_policy_status.rb diff --git a/app/models/course.rb b/app/models/course.rb index 58c3cfc5ae6..659203d7eb5 100644 --- a/app/models/course.rb +++ b/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 diff --git a/db/migrate/20171204191806_add_index_on_submissions_late_policy_status.rb b/db/migrate/20171204191806_add_index_on_submissions_late_policy_status.rb new file mode 100644 index 00000000000..bf309140458 --- /dev/null +++ b/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 . + +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 diff --git a/lib/feature.rb b/lib/feature.rb index dc9307c5ecc..4a35d5c68bc 100644 --- a/lib/feature.rb +++ b/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 diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 3fb63f07b8b..6293027d0c4 100644 --- a/spec/lib/feature_spec.rb +++ b/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 diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index ca4425f2065..5cd9e14e50f 100644 --- a/spec/models/course_spec.rb +++ b/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