ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

implement conditional release assignment releasing on grading 

test plan:
* enable the native conditional editor (see plan for g/239985)
* set up a course some assignments assigned to "Mastery Paths"
 so they're only visible to students with overrides
* use the editor to create a trigger assignment that can
 release the hidden assignments
* as a student, submit to the trigger assignment
* as a teacher, grade the trigger assignment with a
score that will release a hidden assignment
* as the student, confirm that the newly released assignment
 is visible

closes #LS-1062

Change-Id: Id17a06807c8c668ec95c86acc6f95846c5394efc
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/240338
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Robin Kuss <rkuss@instructure.com>
Product-Review: James Williams <jamesw@instructure.com>
This commit is contained in:
James Williams 2020-06-15 12:52:04 -06:00
parent 01d434fab0
commit 56effa3991
9 changed files with 250 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/models/assignment_override.rb
View File

@ -31,7 +31,7 @@ class AssignmentOverride < ActiveRecord::Base
belongs_to :assignment
belongs_to :quiz, class_name: 'Quizzes::Quiz'
belongs_to :set, polymorphic: [:group, :course_section], exhaustive: false
has_many :assignment_override_students, -> { where(workflow_state: 'active') }, :dependent => :destroy, :validate => false
has_many :assignment_override_students, -> { where(workflow_state: 'active') }, :inverse_of => :assignment_override, :dependent => :destroy, :validate => false
validates_presence_of :assignment_version, :if => :assignment
validates_presence_of :title, :workflow_state
validates :set_type, inclusion: ['CourseSection', 'Group', 'ADHOC', SET_TYPE_NOOP]

2
app/models/assignment_override_student.rb
View File

@ -98,6 +98,8 @@ class AssignmentOverrideStudent < ActiveRecord::Base
.each {|aos| aos.assignment_override.skip_broadcasts = true; aos.destroy}
end
attr_writer :no_enrollment
private
def clean_up_assignment_if_override_student_orphaned

93
app/models/conditional_release/override_handler.rb Normal file
View File

@ -0,0 +1,93 @@
#
# Copyright (C) 2020 - 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/>.
module ConditionalRelease
module OverrideHandler
class << self
# handle the parts of the service that was making API calls back to canvas to create/remove assignment overrides
def handle_grade_change(submission)
return unless submission.graded? && submission.posted? # sanity check
sets_to_assign, sets_to_unassign = find_assignment_sets(submission)
set_assignment_overrides(submission.user_id, sets_to_assign, sets_to_unassign)
ConditionalRelease::AssignmentSetAction.create_from_sets(sets_to_assign, sets_to_unassign,
student_id: submission.user_id, actor_id: submission.grader_id, source: 'grade_change')
end
def find_assignment_sets(submission)
rules = submission.course.conditional_release_rules.active.where(:trigger_assignment_id => submission.assignment).preload(:assignment_sets).to_a
relative_score = ConditionalRelease::Stats.percent_from_points(submission.score, submission.assignment.points_possible)
sets_to_assign = []
excluded_sets = []
rules.each do |rule|
new_sets = relative_score ? rule.assignment_sets_for_score(relative_score).to_a : []
if new_sets.length == 1 # otherwise they have to choose between sets
sets_to_assign += new_sets
end
excluded_sets += rule.assignment_sets.to_a - new_sets
end
# see if there are any they were previously assigned to
sets_to_unassign = ConditionalRelease::AssignmentSetAction.current_assignments(submission.user_id, excluded_sets).preload(:assignment_set).map(&:assignment_set)
[sets_to_assign, sets_to_unassign]
end
def set_assignment_overrides(student_id, sets_to_assign, sets_to_unassign)
assignments_to_assign = assignments_for_sets(sets_to_assign)
assignments_to_unassign = assignments_for_sets(sets_to_unassign)
existing_overrides = AssignmentOverride.active.
where(:assignment_id => assignments_to_assign + assignments_to_unassign, :set_type => 'ADHOC').to_a
ActiveRecord::Associations::Preloader.new.preload(existing_overrides, :assignment_override_students,
AssignmentOverrideStudent.where(:user_id => student_id)) # only care about records for this student
existing_overrides_map = existing_overrides.group_by(&:assignment_id)
assignments_to_assign.each do |to_assign|
overrides = existing_overrides_map[to_assign.id]
if overrides
unless overrides.any?{|o| o.assignment_override_students.map(&:user_id).include?(student_id)}
override = overrides.sort_by(&:id).first # kind of arbitrary but may as well be consistent and always pick the earliest
# we can pass in :no_enrollment to skip some queries - i assume they have an enrollment since they have a submission
override.assignment_override_students.create!(:user_id => student_id, :no_enrollment => false)
end
else
# have to create an override
new_override = to_assign.assignment_overrides.create!(
:set_type => 'ADHOC',
:assignment_override_students => [
AssignmentOverrideStudent.new(:assignment => to_assign, :user_id => student_id, :no_enrollment => false)
])
existing_overrides_map[to_assign.id] = [new_override]
end
end
assignments_to_unassign.each do |to_unassign|
overrides = existing_overrides_map[to_unassign.id] || []
overrides.each do |o|
aos = o.assignment_override_students.detect{|aos| aos.user_id == student_id}
aos.destroy! if aos
end
end
end
def assignments_for_sets(sets)
sets.any? ? Assignment.active.where(:id => ConditionalRelease::AssignmentSetAssociation.active.where(:assignment_set_id => sets).select(:assignment_id)).to_a : []
end
end
end
end

17
app/models/conditional_release/rule.rb
View File

@ -31,6 +31,8 @@ module ConditionalRelease
has_many :assignment_set_associations, -> { active.order(position: :asc) }, through: :scoring_ranges
accepts_nested_attributes_for :scoring_ranges, allow_destroy: true
after_save :clear_trigger_assignment_cache
before_create :set_root_account_id
def set_root_account_id
self.root_account_id ||= course.root_account_id
@ -52,7 +54,20 @@ module ConditionalRelease
end
def assignment_sets_for_score(score)
AssignmentSet.where(scoring_range: scoring_ranges.for_score(score)).preload(:assignment_set_associations)
AssignmentSet.active.where(scoring_range: scoring_ranges.for_score(score))
end
def clear_trigger_assignment_cache
self.trigger_assignment.clear_cache_key(:conditional_release)
end
def self.is_trigger_assignment?(assignment)
# i'm only using the cache key currently for this one case but i figure it can be extended to handle caching around all rule data fetching
RequestCache.cache('conditional_release_is_trigger', assignment) do
Rails.cache.fetch_with_batched_keys('conditional_release_is_trigger', batch_object: assignment, batched_keys: :conditional_release) do
assignment.shard.activate { self.active.where(:trigger_assignment_id => assignment).exists? }
end
end
end
end
end

10
app/models/conditional_release/stats.rb
View File

@ -111,6 +111,11 @@ module ConditionalRelease
}
end
def percent_from_points(points, points_possible)
return points.to_f / points_possible.to_f if points.present? && points_possible.to_f.nonzero?
return points.to_f / 100 if points.present? # mirror Canvas rule
end
private
def assignment_detail(assignment, submission, trend_score: nil)
score = submission ? percent_from_points(submission.score, assignment.points_possible) : nil
@ -141,11 +146,6 @@ module ConditionalRelease
end
compute_trend(score, new_scores)
end
def percent_from_points(points, points_possible)
return points.to_f / points_possible.to_f if points.present? && points_possible.to_f.nonzero?
return points.to_f / 100 if points.present? # mirror Canvas rule
end
end
end
end

21
app/models/submission.rb
View File

@ -1889,6 +1889,27 @@ class Submission < ActiveRecord::Base
end
self.class.connection.after_transaction_commit do
Auditors::GradeChange.record(skip_insert: skip_insert, submission: self)
queue_conditional_release_grade_change_handler if newly_graded || grade_changed
end
end
def queue_conditional_release_grade_change_handler
self.shard.activate do
return unless self.graded? && self.posted?
# use request caches to handle n+1's when updating a lot of submissions in the same course in one request
return unless RequestCache.cache('conditional_release_native', self.root_account_id) do
ConditionalRelease::Service.natively_enabled_for_account?(self.root_account)
end
return unless RequestCache.cache('conditional_release_feature_enabled', self.course_id) do
self.course.feature_enabled?(:conditional_release)
end
if ConditionalRelease::Rule.is_trigger_assignment?(self.assignment)
ConditionalRelease::OverrideHandler.send_later_if_production_enqueue_args(
:handle_grade_change,
{:priority => Delayed::LOW_PRIORITY, :strand => "conditional_release_grade_change:#{self.global_assignment_id}"},
self
)
end
end
end

2
lib/canvas/cache_register.rb
View File

@ -30,7 +30,7 @@ module Canvas
'Account' => %w{account_chain role_overrides global_navigation feature_flags},
'Course' => %w{account_associations},
'User' => %w{enrollments groups account_users todo_list submissions user_services},
'Assignment' => %w{availability},
'Assignment' => %w{availability conditional_release},
'Quizzes::Quiz' => %w{availability}
}.freeze

2
lib/feature_flags/hooks.rb
View File

@ -64,7 +64,7 @@ module FeatureFlags
def self.conditional_release_after_state_change_hook(user, context, _old_state, new_state)
if %w(on allowed).include?(new_state) && context.is_a?(Account)
if ConditionalRelease::Service.prefer_native?
if ConditionalRelease::Service.prefer_native? || ConditionalRelease::Service.natively_enabled_for_account?(context.root_account)
context.root_account.tap do |ra|
ra.settings[:use_native_conditional_release] = true
ra.save!

110
spec/models/conditional_release/override_handler_spec.rb Normal file
View File

@ -0,0 +1,110 @@
#
# Copyright (C) 2020 - 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/>.
#
require_relative '../../conditional_release_spec_helper'
require_dependency "conditional_release/override_handler"
module ConditionalRelease
describe OverrideHandler do
context 'handle_grade_change' do
before :once do
# set up a trigger assignment with rules and whatnot
course_with_student(:active_all => true)
@trigger_assmt = @course.assignments.create!(:points_possible => 10, submission_types: "online_text_entry")
@sub = @trigger_assmt.submit_homework(@student, body: "hi")
@set1_assmt1 = @course.assignments.create!(:only_visible_to_overrides => true) # one in one set
@set2_assmt1 = @course.assignments.create!(:only_visible_to_overrides => true)
@set2_assmt2 = @course.assignments.create!(:only_visible_to_overrides => true) # two in one set
@set3a_assmt = @course.assignments.create!(:only_visible_to_overrides => true) # two sets in one range - will have to choose
@set3b_assmt = @course.assignments.create!(:only_visible_to_overrides => true)
ranges = [
ScoringRange.new(:lower_bound => 0.7, :upper_bound => 1.0, :assignment_sets => [
AssignmentSet.new(:assignment_set_associations => [AssignmentSetAssociation.new(:assignment_id => @set1_assmt1.id)])
]),
ScoringRange.new(:lower_bound => 0.4, :upper_bound => 0.7, :assignment_sets => [
AssignmentSet.new(:assignment_set_associations => [
AssignmentSetAssociation.new(:assignment_id => @set2_assmt1.id),
AssignmentSetAssociation.new(:assignment_id => @set2_assmt2.id)
])
]),
ScoringRange.new(:lower_bound => 0, :upper_bound => 0.4, :assignment_sets => [
AssignmentSet.new(:assignment_set_associations => [AssignmentSetAssociation.new(:assignment_id => @set3a_assmt.id)]),
AssignmentSet.new(:assignment_set_associations => [AssignmentSetAssociation.new(:assignment_id => @set3b_assmt.id)])
])
]
@rule = @course.conditional_release_rules.create!(:trigger_assignment => @trigger_assmt, :scoring_ranges => ranges)
Account.default.tap do |ra|
ra.settings[:use_native_conditional_release] = true
ra.save!
end
@course.enable_feature!(:conditional_release)
end
it "should require native conditional release" do
expect(ConditionalRelease::Service).to receive(:natively_enabled_for_account?).and_return(false).once
expect(ConditionalRelease::OverrideHandler).to_not receive(:handle_grade_change)
@trigger_assmt.grade_student(@student, grade: 9, grader: @teacher)
end
it "should check that the assignment is actually a trigger assignment" do
@rule.destroy!
expect(ConditionalRelease::OverrideHandler).to_not receive(:handle_grade_change)
@trigger_assmt.grade_student(@student, grade: 9, grader: @teacher)
end
it "should automatically assign to the proper assignment set when graded" do
@trigger_assmt.grade_student(@student, grade: 9, grader: @teacher) # should automatically assign to top set
visible_assmts = DifferentiableAssignment.scope_filter(@course.assignments, @student, @course).to_a
expect(visible_assmts).to include(@set1_assmt1)
expect(visible_assmts).to_not include(@set2_assmt1) # and only the top set
end
it "should automatically unassign if the grade changes" do
@trigger_assmt.grade_student(@student, grade: 9, grader: @teacher) # should automatically assign to top set
@trigger_assmt.grade_student(@student, grade: 5, grader: @teacher) # actually nvm should automatically assign to middle set
visible_assmts = DifferentiableAssignment.scope_filter(@course.assignments, @student, @course).to_a
expect(visible_assmts).to_not include(@set1_assmt1)
expect(visible_assmts).to include(@set2_assmt1)
expect(visible_assmts).to include(@set2_assmt2) # assign to both
end
it "should reuse an existing override when assigning (and leave it be when unassigning)" do
old_student = @student
student_in_course(:course => @course, :active_all => true)
@trigger_assmt.grade_student(old_student, grade: 9, grader: @teacher)
@trigger_assmt.grade_student(@student, grade: 9, grader: @teacher)
expect(@set1_assmt1.assignment_overrides.count).to eq 1
expect(@set1_assmt1.assignment_overrides.first.assignment_override_students.count).to eq 2
@trigger_assmt.grade_student(@student, grade: 5, grader: @teacher) # now unassign
expect(DifferentiableAssignment.scope_filter(@course.assignments, @student, @course).to_a).to_not include(@set1_assmt1)
expect(DifferentiableAssignment.scope_filter(@course.assignments, old_student, @course).to_a).to include(@set1_assmt1)
end
it "should not automatically assign when there are multiple applicable sets for the student to choose from" do
@trigger_assmt.grade_student(@student, grade: 2, grader: @teacher) # should automatically assign to top set
visible_assmts = DifferentiableAssignment.scope_filter(@course.assignments, @student, @course).to_a
expect(visible_assmts).to_not include(@set3a_assmt)
expect(visible_assmts).to_not include(@set3b_assmt)
end
end
end
end