From a848924b571a17dd6cd1e3c26279a632f60d463c Mon Sep 17 00:00:00 2001 From: James Williams Date: Thu, 18 Jun 2020 11:53:16 -0600 Subject: [PATCH] retrieve conditional release rule information natively also add native support for choosing from multiple assignment sets test plan: * enable native conditional release (see g/239985) * set up a course with 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, setting up a scoring range that has an "or" option (by clicking the "&" between multiple assignments) that won't automatically assign students but rather present a choice * as a student, submit to the trigger assignment * as a teacher, grade the trigger assignment with a score that will trigger the "or" path * as the student, confirm that the modules page reveals a link to a page that allows the student to choose one assignment set or the other * confirm that choosing an assignment set will reveal its assignments to the student * confirm that as a teacher, viewing the trigger assignment page shows a "Mastery Paths Breakdown" that can be interacted with to show which students fell into each scoring range * all other native conditional release functionality not listed here should be in-line with current functionality via the full service (apart from course copy support) closes #LS-1065 Change-Id: Iede1689b3896380d4da155fa7ae325796d85f934 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/240607 Tested-by: Service Cloud Jenkins Reviewed-by: Jeremy Stanley QA-Review: Jeremy Stanley Product-Review: Jeremy Stanley --- .../conditional_release/rules_controller.rb | 19 +- .../context_module_items_api_controller.rb | 27 +-- app/controllers/context_modules_controller.rb | 6 +- app/helpers/context_modules_helper.rb | 11 +- app/helpers/cyoe_helper.rb | 22 ++- .../shared/conditional_release/CyoeHelper.js | 6 +- .../shared/helpers/assignment-categories.js | 2 +- app/models/assignment.rb | 3 + .../assignment_set_association.rb | 11 ++ .../conditional_release/override_handler.rb | 18 ++ app/models/conditional_release/rule.rb | 29 ++- app/models/conditional_release/service.rb | 116 ++++++++---- app/models/conditional_release/stats.rb | 8 +- lib/canvas/cache_register.rb | 2 +- spec/apis/v1/context_module_items_api_spec.rb | 10 ++ spec/conditional_release_spec_helper.rb | 43 +++++ spec/helpers/context_modules_helper_spec.rb | 18 +- .../conditional_release_service_spec.rb | 167 +++++++++++++----- .../override_handler_spec.rb | 100 +++++++---- 19 files changed, 439 insertions(+), 179 deletions(-) diff --git a/app/controllers/conditional_release/rules_controller.rb b/app/controllers/conditional_release/rules_controller.rb index 5a3cc592bd2..82142cc184a 100644 --- a/app/controllers/conditional_release/rules_controller.rb +++ b/app/controllers/conditional_release/rules_controller.rb @@ -27,7 +27,7 @@ module ConditionalRelease # GET /api/rules def index rules = get_rules - rules = rules.preload(Rule.all_includes) if include_param.include?('all') + rules = rules.preload(Rule.preload_associations) if include_param.include?('all') rules = rules.with_assignments if value_to_boolean(params[:active]) render json: rules.as_json(include: json_includes, include_root: false, except: [:root_account_id, :deleted_at]) @@ -52,7 +52,7 @@ module ConditionalRelease rule = Rule.new(create_params) if rule.save - render json: rule.as_json(include: all_includes, include_root: false, except: [:root_account_id, :deleted_at]) + render json: rule.as_json(include: Rule.includes_for_json, include_root: false, except: [:root_account_id, :deleted_at]) else render json: rule.errors, status: :bad_request end @@ -70,7 +70,7 @@ module ConditionalRelease :assignment_set_associations ) if rule.update(update_params) - render json: rule.as_json(include: all_includes, include_root: false, except: [:root_account_id, :deleted_at]) + render json: rule.as_json(include: Rule.includes_for_json, include_root: false, except: [:root_account_id, :deleted_at]) else render json: rule.errors, status: :bad_request end @@ -99,19 +99,8 @@ module ConditionalRelease Array.wrap(params[:include]) end - def all_includes - { scoring_ranges: { - include: { - assignment_sets: { - include: {assignment_set_associations: {except: [:root_account_id, :deleted_at]}}, - except: [:root_account_id, :deleted_at] - } }, - except: [:root_account_id, :deleted_at] - } } - end - def json_includes - return all_includes if include_param.include? 'all' + return Rule.includes_for_json if include_param.include? 'all' end def add_ordering_to(attrs) diff --git a/app/controllers/context_module_items_api_controller.rb b/app/controllers/context_module_items_api_controller.rb index 56b84b03dde..7b5306311cc 100644 --- a/app/controllers/context_module_items_api_controller.rb +++ b/app/controllers/context_module_items_api_controller.rb @@ -276,7 +276,7 @@ class ContextModuleItemsApiController < ApplicationController # Get conditionally released objects if requested # TODO: Document in API when out of beta if includes.include?("mastery_paths") - opts[:conditional_release_rules] = ConditionalRelease::Service.rules_for(@context, @student, items, session) + opts[:conditional_release_rules] = ConditionalRelease::Service.rules_for(@context, @student, session) end render :json => items.map { |item| module_item_json(item, @student || @current_user, session, mod, prog, includes, opts) } end @@ -542,18 +542,25 @@ class ContextModuleItemsApiController < ApplicationController assignment = @item.assignment return render json: { message: 'requested item is not an assignment' }, status: :bad_request unless assignment - response = ConditionalRelease::Service.select_mastery_path( - @context, - @current_user, - @student, - assignment, - params[:assignment_set_id], - session) + if ConditionalRelease::Service.natively_enabled_for_account?(@context.root_account) + assignment_ids = ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, assignment, params[:assignment_set_id]) + request_failed = false + else + response = ConditionalRelease::Service.select_mastery_path( + @context, + @current_user, + @student, + assignment, + params[:assignment_set_id], + session) + request_failed = response[:code] != '200' + assignment_ids = response[:body]['assignments'].map {|a| a['assignment_id'].try(&:to_i) } unless request_failed + end - if response[:code] != '200' + if request_failed render json: response[:body], status: response[:code] else - assignment_ids = response[:body]['assignments'].map {|a| a['assignment_id'].try(&:to_i) } + # assignment occurs in delayed job, may not be fully visible to user until job completes assignments = @context.assignments.published.where(id: assignment_ids). preload(Api::V1::Assignment::PRELOADS) diff --git a/app/controllers/context_modules_controller.rb b/app/controllers/context_modules_controller.rb index c85484e4064..a86862ebcb2 100644 --- a/app/controllers/context_modules_controller.rb +++ b/app/controllers/context_modules_controller.rb @@ -47,7 +47,7 @@ class ContextModulesController < ApplicationController collection_cache_key(@modules), Time.zone, Digest::MD5.hexdigest([visible_assignments, @section_visibility].join("/"))] cache_key = cache_key_items.join('/') cache_key = add_menu_tools_to_cache_key(cache_key) - cache_key = add_mastery_paths_to_cache_key(cache_key, @context, @modules, @current_user) + cache_key = add_mastery_paths_to_cache_key(cache_key, @context, @current_user) end end @@ -139,7 +139,7 @@ class ContextModulesController < ApplicationController item = @context.context_module_tags.not_deleted.find(params[:id]) if item.present? && item.published? && item.context_module.published? - rules = ConditionalRelease::Service.rules_for(@context, @current_user, item, session) + rules = ConditionalRelease::Service.rules_for(@context, @current_user, session) rule = conditional_release_rule_for_module_item(item, conditional_release_rules: rules) # locked assignments always have 0 sets, so this check makes it not return 404 if locked @@ -151,7 +151,7 @@ class ContextModulesController < ApplicationController setId: set[:id] } - option[:assignments] = set[:assignments].map { |a| + option[:assignments] = (set[:assignments] || set[:assignment_set_associations]).map { |a| assg = assignment_json(a[:model], @current_user, session) assg[:assignmentId] = a[:assignment_id] assg diff --git a/app/helpers/context_modules_helper.rb b/app/helpers/context_modules_helper.rb index f91de813ef9..183cbea87a6 100644 --- a/app/helpers/context_modules_helper.rb +++ b/app/helpers/context_modules_helper.rb @@ -40,7 +40,7 @@ module ContextModulesHelper true, Time.zone, Digest::MD5.hexdigest([visible_assignments, @section_visibility].join("/"))] cache_key = cache_key_items.join('/') cache_key = add_menu_tools_to_cache_key(cache_key) - cache_key = add_mastery_paths_to_cache_key(cache_key, context, context_module, user) + cache_key = add_mastery_paths_to_cache_key(cache_key, context, user) cache(cache_key, {}, &block) else yield @@ -55,13 +55,10 @@ module ContextModulesHelper cache_key end - def add_mastery_paths_to_cache_key(cache_key, context, module_or_modules, user) + def add_mastery_paths_to_cache_key(cache_key, context, user) if user && cyoe_enabled?(context) if context.user_is_student?(user) - items = Rails.cache.fetch("visible_content_tags_for/#{cache_key}") do - Array.wrap(module_or_modules).map{ |m| m.content_tags_visible_to(user, :is_teacher => false) }.flatten - end - rules = cyoe_rules(context, user, items, @session) + rules = cyoe_rules(context, user, @session) else rules = ConditionalRelease::Service.active_rules(context, user, @session) end @@ -120,7 +117,7 @@ module ContextModulesHelper } if cyoe_enabled?(@context) - rules = cyoe_rules(@context, current_user, module_data[:items], session) || [] + rules = cyoe_rules(@context, current_user, session) || [] end items_data = {} diff --git a/app/helpers/cyoe_helper.rb b/app/helpers/cyoe_helper.rb index 4d5b4e80db6..4416aa68df7 100644 --- a/app/helpers/cyoe_helper.rb +++ b/app/helpers/cyoe_helper.rb @@ -31,12 +31,12 @@ module CyoeHelper ConditionalRelease::Service.enabled_in_context?(context) end - def cyoe_rules(context, current_user, items, session) - ConditionalRelease::Service.rules_for(context, current_user, items, session) + def cyoe_rules(context, current_user, session) + ConditionalRelease::Service.rules_for(context, current_user, session) end def conditional_release_rule_for_module_item(content_tag, opts = {}) - rules = opts[:conditional_release_rules] || cyoe_rules(opts[:context], opts[:user], [], opts[:session]) + rules = opts[:conditional_release_rules] || cyoe_rules(opts[:context], opts[:user], opts[:session]) assignment_id = content_tag.assignment.try(:id) path_data = conditional_release_assignment_set(rules, assignment_id) if rules.present? && assignment_id.present? if path_data.present? && opts[:is_student] @@ -48,7 +48,7 @@ module CyoeHelper end def conditional_release_assignment_set(rules, id) - result = rules.find { |rule| rule[:trigger_assignment].to_s == id.to_s } + result = rules.find { |rule| rule[:trigger_assignment].to_s == id.to_s || rule[:trigger_assignment_id] == id } return if result.blank? result.slice(:locked, :assignment_sets, :selected_set_id) end @@ -57,8 +57,9 @@ module CyoeHelper result = conditional_release_rule_for_module_item(content_tag, opts) return if result.blank? result[:assignment_sets].each do |as| - next if as[:assignments].blank? - as[:assignments].each do |a| + associations = as[:assignments] || as[:assignment_set_associations] + next if associations.blank? + associations.each do |a| a[:model] = assignment_json(a[:model], user, nil) if a[:model] end end @@ -76,8 +77,11 @@ module CyoeHelper def check_if_processing(data) if !data[:awaiting_choice] && data[:assignment_sets].length == 1 - vis_assignments = AssignmentStudentVisibility.visible_assignment_ids_for_user(@current_user.id, @context.id) || [] - selected_set_assignment_ids = data[:assignment_sets][0][:assignments].map{ |a| a[:assignment_id].to_i } || [] + vis_assignments = RequestCache.cache('visible_assignment_ids_in_course', @current_user, @context) do + AssignmentStudentVisibility.visible_assignment_ids_for_user(@current_user.id, @context.id) || [] + end + set = data[:assignment_sets][0] + selected_set_assignment_ids = (set[:assignments] || set[:assignment_set_associations]).map{ |a| a[:assignment_id].to_i } || [] data[:still_processing] = (selected_set_assignment_ids - vis_assignments).present? end end @@ -94,4 +98,4 @@ module CyoeHelper modules_tab_disabled: modules_disabled }) end -end \ No newline at end of file +end diff --git a/app/jsx/shared/conditional_release/CyoeHelper.js b/app/jsx/shared/conditional_release/CyoeHelper.js index 9987fc0c8da..6e6cdb6937f 100644 --- a/app/jsx/shared/conditional_release/CyoeHelper.js +++ b/app/jsx/shared/conditional_release/CyoeHelper.js @@ -25,17 +25,17 @@ const parseEnvData = () => { (ENV.CONDITIONAL_RELEASE_ENV && ENV.CONDITIONAL_RELEASE_ENV.active_rules) || [] return { triggerAssignments: activeRules.reduce((triggers, rule) => { - triggers[rule.trigger_assignment] = rule + triggers[rule.trigger_assignment || rule.trigger_assignment_id] = rule return triggers }, {}), releasedAssignments: activeRules.reduce((released, rule) => { rule.scoring_ranges.forEach(range => { range.assignment_sets.forEach(set => { - set.assignments.forEach(asg => { + ;(set.assignments || set.assignment_set_associations).forEach(asg => { const id = asg.assignment_id released[id] = released[id] || [] released[id].push({ - assignment_id: rule.trigger_assignment, + assignment_id: rule.trigger_assignment || rule.trigger_assignment_id, assignment: rule.trigger_assignment_model, upper_bound: range.upper_bound, lower_bound: range.lower_bound diff --git a/app/jsx/shared/helpers/assignment-categories.js b/app/jsx/shared/helpers/assignment-categories.js index 52c74ba6842..abd2f2d18b2 100644 --- a/app/jsx/shared/helpers/assignment-categories.js +++ b/app/jsx/shared/helpers/assignment-categories.js @@ -71,7 +71,7 @@ Categories.getCategory = assg => { _.find(assg.submission_types, sub => cat.submissionTypes.indexOf(sub) !== -1) ) }) - return category || Categories.OTHER + return category || OTHER } export default Categories diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 76aae5b1937..039bf6f462e 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -108,6 +108,7 @@ class Assignment < ActiveRecord::Base dependent: :destroy, inverse_of: :assignment + has_many :conditional_release_rules, class_name: "ConditionalRelease::Rule", dependent: :destroy, foreign_key: 'trigger_assignment_id', inverse_of: :trigger_assignment has_many :conditional_release_associations, class_name: "ConditionalRelease::AssignmentSetAssociation", dependent: :destroy, inverse_of: :assignment scope :anonymous, -> { where(anonymous_grading: true) } @@ -1213,6 +1214,8 @@ class Assignment < ActiveRecord::Base self.save! each_submission_type { |submission| submission.destroy if submission && !submission.deleted? } + self.conditional_release_rules.destroy_all + self.conditional_release_associations.destroy_all refresh_course_content_participation_counts ScheduledSmartAlert.where(context_type: 'Assignment', context_id: self.id).destroy_all diff --git a/app/models/conditional_release/assignment_set_association.rb b/app/models/conditional_release/assignment_set_association.rb index 447e7906966..dbe42d5b999 100644 --- a/app/models/conditional_release/assignment_set_association.rb +++ b/app/models/conditional_release/assignment_set_association.rb @@ -32,11 +32,22 @@ module ConditionalRelease has_one :rule, through: :assignment_set belongs_to :root_account, :class_name => "Account" + after_save :clear_caches + before_create :set_root_account_id def set_root_account_id self.root_account_id ||= assignment_set.root_account_id end + def clear_caches + if self.saved_change_to_deleted_at? && self.assignment.deleted? + # normally this will be cleared by the rule, but not after assignment deletion + self.class.connection.after_transaction_commit do + self.assignment.context.clear_cache_key(:conditional_release) + end + end + end + delegate :course_id, to: :rule, allow_nil: true private diff --git a/app/models/conditional_release/override_handler.rb b/app/models/conditional_release/override_handler.rb index a321d2dea39..75dd6a83a53 100644 --- a/app/models/conditional_release/override_handler.rb +++ b/app/models/conditional_release/override_handler.rb @@ -29,6 +29,24 @@ module ConditionalRelease student_id: submission.user_id, actor_id: submission.grader_id, source: 'grade_change') end + def handle_assignment_set_selection(student, trigger_assignment, assignment_set_id) + # just going to raise a 404 if the choice is invalid because i'm too lazy to return more specific errors + rule_ids = trigger_assignment.conditional_release_rules.active.pluck(:id) + submission = trigger_assignment.submissions.for_user(student).in_workflow_state(:graded).posted.take! + relative_score = ConditionalRelease::Stats.percent_from_points(submission.score, trigger_assignment.points_possible) + assignment_set = AssignmentSet.active.where(id: assignment_set_id, + scoring_range: ScoringRange.active.where(:rule_id => rule_ids).for_score(relative_score)).take! + + other_assignment_sets = assignment_set.scoring_range.assignment_sets - [assignment_set] + previous_set_ids = AssignmentSetAction.current_assignments(student.id, other_assignment_sets).pluck(:assignment_set_id) + sets_to_unassign = other_assignment_sets.select{|set| previous_set_ids.include?(set.id)} + + set_assignment_overrides(submission.user_id, [assignment_set], sets_to_unassign) + ConditionalRelease::AssignmentSetAction.create_from_sets([assignment_set], sets_to_unassign, + student_id: submission.user_id, source: 'select_assignment_set') + assignment_set.assignment_set_associations.map(&:assignment_id) + 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) diff --git a/app/models/conditional_release/rule.rb b/app/models/conditional_release/rule.rb index 82028e975bf..1d562241473 100644 --- a/app/models/conditional_release/rule.rb +++ b/app/models/conditional_release/rule.rb @@ -31,7 +31,7 @@ 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 + after_save :clear_caches before_create :set_root_account_id def set_root_account_id @@ -45,20 +45,37 @@ module ConditionalRelease end scope :with_assignments, -> do - having_assignments = joins(all_includes).group(Arel.sql("conditional_release_rules.id")) - preload(all_includes).where(id: having_assignments.pluck(:id)) + having_assignments = joins(preload_associations).group(Arel.sql("conditional_release_rules.id")) + preload(preload_associations).where(id: having_assignments.pluck(:id)) end - def self.all_includes + def self.preload_associations { scoring_ranges: { assignment_sets: :assignment_set_associations } } end + def self.includes_for_json + { + scoring_ranges: { + include: { + assignment_sets: { + include: {assignment_set_associations: {except: [:root_account_id, :deleted_at]}}, + except: [:root_account_id, :deleted_at] + } + }, + except: [:root_account_id, :deleted_at] + } + } + end + def assignment_sets_for_score(score) AssignmentSet.active.where(scoring_range: scoring_ranges.for_score(score)) end - def clear_trigger_assignment_cache - self.trigger_assignment.clear_cache_key(:conditional_release) + def clear_caches + self.class.connection.after_transaction_commit do + self.trigger_assignment.clear_cache_key(:conditional_release) + self.course.clear_cache_key(:conditional_release) + end end def self.is_trigger_assignment?(assignment) diff --git a/app/models/conditional_release/service.rb b/app/models/conditional_release/service.rb index aa9c91f61d8..bff694acc21 100644 --- a/app/models/conditional_release/service.rb +++ b/app/models/conditional_release/service.rb @@ -56,7 +56,6 @@ module ConditionalRelease cyoe_env[:course_id] = context.id cyoe_env[:stats_url] = "/api/v1/courses/#{context.id}/mastery_paths/stats" end - # TODO: add rules and whatnot else cyoe_env = { jwt: jwt_for(context, user, domain, session: session, real_user: real_user), @@ -67,10 +66,10 @@ module ConditionalRelease base_url: base_url, context_id: context.id } - - cyoe_env[:rule] = rule_triggered_by(assignment, user, session) if includes.include? :rule - cyoe_env[:active_rules] = active_rules(context, user, session) if includes.include? :active_rules end + + cyoe_env[:rule] = rule_triggered_by(assignment, user, session) if includes.include? :rule + cyoe_env[:active_rules] = active_rules(context, user, session) if includes.include? :active_rules env.merge(CONDITIONAL_RELEASE_ENV: cyoe_env) end @@ -89,9 +88,9 @@ module ConditionalRelease ) end - def self.rules_for(context, student, content_tags, session) + def self.rules_for(context, student, session) return unless enabled_in_context?(context) - rules_data(context, student, Array.wrap(content_tags), session) + rules_data(context, student, session) end def self.clear_active_rules_cache(course) @@ -201,36 +200,13 @@ module ConditionalRelease rules = active_rules(assignment.context, current_user, session) return nil unless rules - rules.find {|r| r['trigger_assignment'] == assignment.id.to_s} - end - - def self.rules_assigning(assignment, current_user, session = nil) - reverse_lookup = Rails.cache.fetch(active_rules_reverse_cache_key(assignment.context)) do - all_rules = active_rules(assignment.context, current_user, session) - return nil unless all_rules - - lookup = {} - all_rules.each do |rule| - (rule['scoring_ranges'] || []).each do |sr| - (sr['assignment_sets'] || []).each do |as| - (as['assignments'] || []).each do |a| - if a['assignment_id'].present? - lookup[a['assignment_id']] ||= [] - lookup[a['assignment_id']] << rule - end - end - end - end - end - lookup.each {|_id, rules| rules.uniq!} - lookup - end - reverse_lookup[assignment.id.to_s] + rules.find {|r| r['trigger_assignment'] == assignment.id.to_s || r['trigger_assignment_id'] == assignment.id} end def self.active_rules(course, current_user, session) return unless enabled_in_context?(course) return unless course.grants_any_right?(current_user, session, :read, :manage_assignments) + return native_active_rules(course) if natively_enabled_for_account?(course.root_account) Rails.cache.fetch(active_rules_cache_key(course)) do headers = headers_for(course, current_user, domain_for(course), session) @@ -263,6 +239,25 @@ module ConditionalRelease [] end + def self.native_active_rules(course) + rules_data = Rails.cache.fetch_with_batched_keys('conditional_release_active_rules', batch_object: course, batched_keys: :conditional_release) do + rules = course.conditional_release_rules.active.with_assignments.to_a + rules.as_json(include: Rule.includes_for_json, include_root: false, except: [:root_account_id, :deleted_at]) + end + trigger_ids = rules_data.map { |rule| rule['trigger_assignment_id'] } + trigger_assgs = course.assignments.preload(:grading_standard).where(id: trigger_ids).each_with_object({}) do |a, assgs| + assgs[a.id] = { + points_possible: a.points_possible, + grading_type: a.grading_type, + grading_scheme: a.uses_grading_standard ? a.grading_scheme : nil, + } + end + rules_data.each do |rule| + rule['trigger_assignment_model'] = trigger_assgs[rule['trigger_assignment_id']] + end + rules_data + end + class << self private def config_file @@ -328,8 +323,12 @@ module ConditionalRelease end end - def rules_data(context, student, content_tags = [], session = {}) + def rules_data(context, student, session = {}) return [] if context.blank? || student.blank? + if natively_enabled_for_account?(context.root_account) + return native_rules_data_for_student(context, student) + end + cached = rules_cache(context, student) assignments = assignments_for(cached[:rules]) if cached force_cache = rules_cache_expired?(context, cached) @@ -349,6 +348,59 @@ module ConditionalRelease [] end + def native_rules_data_for_student(course, student) + rules_data = + ::Rails.cache.fetch(['conditional_release_rules_for_student', student.cache_key(:submissions), course.cache_key(:conditional_release)].cache_key) do + rules = course.conditional_release_rules.active.preload(Rule.preload_associations).to_a + + trigger_assignments = course.assignments.where(:id => rules.map(&:trigger_assignment_id)).to_a.index_by(&:id) + trigger_submissions = course.submissions.where(:assignment_id => trigger_assignments.keys). + for_user(student).in_workflow_state(:graded).posted.to_a.index_by(&:assignment_id) + + assigned_set_ids = ConditionalRelease::AssignmentSetAction.current_assignments( + student, rules.flat_map(&:scoring_ranges).flat_map(&:assignment_sets)).pluck(:assignment_set_id) + rules.map do |rule| + trigger_assignment = trigger_assignments[rule.trigger_assignment_id] + trigger_sub = trigger_submissions[trigger_assignment.id] + if trigger_sub&.score + relative_score = ConditionalRelease::Stats.percent_from_points(trigger_sub.score, trigger_assignment.points_possible) + assignment_sets = rule.scoring_ranges.select{|sr| sr.contains_score(relative_score)}.flat_map(&:assignment_sets) + selected_set_id = + if assignment_sets.length == 1 + assignment_sets.first.id + else + (assignment_sets.map(&:id) & assigned_set_ids).first + end + end + assignment_sets_data = (assignment_sets || []).as_json( + include_root: false, except: [:root_account_id, :deleted_at], + include: {assignment_set_associations: {except: [:root_account_id, :deleted_at]}} + ).map(&:deep_symbolize_keys) + rule.as_json(include_root: false, except: [:root_account_id, :deleted_at]).merge( + locked: relative_score.blank?, + selected_set_id: selected_set_id, + assignment_sets: assignment_sets_data + ) + end + end + # TODO: do something less weird than mixing AR records into json + # to get the assignment data in when we're not maintaining back compat + referenced_assignment_ids = rules_data.map do |rule_hash| + rule_hash[:assignment_sets].map do |set_hash| + set_hash[:assignment_set_associations].map{|assoc_hash| assoc_hash[:assignment_id]} + end + end.flatten + referenced_assignments = course.assignments.where(:id => referenced_assignment_ids).to_a.index_by(&:id) + rules_data.each do |rule_hash| + rule_hash[:assignment_sets].each do |set_hash| + set_hash[:assignment_set_associations].each do |assoc_hash| + assoc_hash[:model] = referenced_assignments[assoc_hash[:assignment_id]] + end + end + end + rules_data + end + def rules_cache(context, student, force: false, &block) Rails.cache.fetch(rules_cache_key(context, student), force: force, &block) end diff --git a/app/models/conditional_release/stats.rb b/app/models/conditional_release/stats.rb index 9e4865ac21d..d0d75454174 100644 --- a/app/models/conditional_release/stats.rb +++ b/app/models/conditional_release/stats.rb @@ -15,6 +15,8 @@ # You should have received a copy of the GNU Affero General Public License along # with this program. If not, see . +require_dependency 'conditional_release/assignment_set_action' + module ConditionalRelease module Stats class << self @@ -78,7 +80,9 @@ module ConditionalRelease end end end - + ranges.each do |r| + r[:scoring_range] = r[:scoring_range].as_json(include_root: false, except: [:root_account_id, :deleted_at]) # can't rely on normal json serialization + end { rule: rule, ranges: ranges, enrolled: users_by_id.count } end @@ -120,7 +124,7 @@ module ConditionalRelease def assignment_detail(assignment, submission, trend_score: nil) score = submission ? percent_from_points(submission.score, assignment.points_possible) : nil detail = { - assignment: {id: assignment.id, name: assignment.title, submission_types: assignment.submission_types}, + assignment: {id: assignment.id, name: assignment.title, submission_types: assignment.submission_types_array, grading_type: assignment.grading_type}, submission: {id: submission.id, score: submission.score, grade: submission.grade, submitted_at: submission.submitted_at}, score: score } diff --git a/lib/canvas/cache_register.rb b/lib/canvas/cache_register.rb index d6a03ae7321..162a720b99d 100644 --- a/lib/canvas/cache_register.rb +++ b/lib/canvas/cache_register.rb @@ -28,7 +28,7 @@ module Canvas ALLOWED_TYPES = { 'Account' => %w{account_chain role_overrides global_navigation feature_flags}, - 'Course' => %w{account_associations}, + 'Course' => %w{account_associations conditional_release}, 'User' => %w{enrollments groups account_users todo_list submissions user_services}, 'Assignment' => %w{availability conditional_release}, 'Quizzes::Quiz' => %w{availability} diff --git a/spec/apis/v1/context_module_items_api_spec.rb b/spec/apis/v1/context_module_items_api_spec.rb index 46342904d65..6b1ffb570f3 100644 --- a/spec/apis/v1/context_module_items_api_spec.rb +++ b/spec/apis/v1/context_module_items_api_spec.rb @@ -1036,6 +1036,16 @@ describe "Module Items API", type: :request do call_select_mastery_path @assignment_tag, 100, @student.id, expected_status: 404 end + it 'should call the native selection path if configured' do + expect(ConditionalRelease::Service).to receive(:natively_enabled_for_account?).and_return(true) + + assignment_ids = create_assignments([@course.id], 2) + expect(ConditionalRelease::OverrideHandler).to receive(:handle_assignment_set_selection). + with(@student, @assignment, "100").and_return(assignment_ids) + json = call_select_mastery_path @assignment_tag, "100", @student.id, expected_status: 200 + expect(json['assignments'].map {|a| a['id']}).to eq assignment_ids + end + context 'successful' do def cyoe_returns(assignment_ids) cyoe_ids = assignment_ids.map {|id| { 'assignment_id' => "#{id}" }} # cyoe ids in strings diff --git a/spec/conditional_release_spec_helper.rb b/spec/conditional_release_spec_helper.rb index 9cab8dc8e4a..03e98a00bf5 100644 --- a/spec/conditional_release_spec_helper.rb +++ b/spec/conditional_release_spec_helper.rb @@ -43,3 +43,46 @@ RSpec.shared_examples 'a soft-deletable model' do expect { copy.save! }.to_not raise_error end end + +module ConditionalRelease + module SpecHelper + def setup_course_with_native_conditional_release + Account.default.tap do |ra| + ra.settings[:use_native_conditional_release] = true + ra.save! + end + # 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) + + @course.enable_feature!(:conditional_release) + end + end +end +RSpec.configure do |config| + config.include ConditionalRelease::SpecHelper +end diff --git a/spec/helpers/context_modules_helper_spec.rb b/spec/helpers/context_modules_helper_spec.rb index 04dfb34e6a9..78c400ecdaa 100644 --- a/spec/helpers/context_modules_helper_spec.rb +++ b/spec/helpers/context_modules_helper_spec.rb @@ -222,15 +222,15 @@ describe ContextModulesHelper do it "does not affect cache keys unless mastery paths enabled" do allow(ConditionalRelease::Service).to receive(:enabled_in_context?).and_return(false) student_in_course(course: t_course, active_all: true) - cache = add_mastery_paths_to_cache_key('foo', t_course, t_module, @student) + cache = add_mastery_paths_to_cache_key('foo', t_course, @student) expect(cache).to eq 'foo' end it "creates the same key for the same mastery paths rules for a student" do s1 = student_in_course(course: t_course, active_all: true) s2 = student_in_course(course: t_course, active_all: true) - cache1 = add_mastery_paths_to_cache_key('foo', t_course, t_module, s1.user) - cache2 = add_mastery_paths_to_cache_key('foo', t_course, t_module, s2.user) + cache1 = add_mastery_paths_to_cache_key('foo', t_course, s1.user) + cache2 = add_mastery_paths_to_cache_key('foo', t_course, s2.user) expect(cache1).not_to eq 'foo' expect(cache1).to eq cache2 end @@ -238,17 +238,17 @@ describe ContextModulesHelper do it "creates different keys for different mastery paths rules for a student" do s1 = student_in_course(course: t_course, active_all: true) s2 = student_in_course(course: t_course, active_all: true) - cache1 = add_mastery_paths_to_cache_key('foo', t_course, t_module, s1.user) + cache1 = add_mastery_paths_to_cache_key('foo', t_course, s1.user) allow(ConditionalRelease::Service).to receive(:rules_for).and_return([3, 2, 1]) - cache2 = add_mastery_paths_to_cache_key('foo', t_course, t_module, s2.user) + cache2 = add_mastery_paths_to_cache_key('foo', t_course, s2.user) expect(cache1).not_to eq cache2 end it "creates the same key for the same mastery paths rules for a teacher" do t1 = teacher_in_course(course: t_course) t2 = teacher_in_course(course: t_course) - cache1 = add_mastery_paths_to_cache_key('foo', t_course, t_module, t1.user) - cache2 = add_mastery_paths_to_cache_key('foo', t_course, t_module, t2.user) + cache1 = add_mastery_paths_to_cache_key('foo', t_course, t1.user) + cache2 = add_mastery_paths_to_cache_key('foo', t_course, t2.user) expect(cache1).not_to eq 'foo' expect(cache1).to eq cache2 end @@ -256,9 +256,9 @@ describe ContextModulesHelper do it "creates different keys for different mastery paths rules for a teacher" do t1 = teacher_in_course(course: t_course) t2 = teacher_in_course(course: t_course) - cache1 = add_mastery_paths_to_cache_key('foo', t_course, t_module, t1.user) + cache1 = add_mastery_paths_to_cache_key('foo', t_course, t1.user) allow(ConditionalRelease::Service).to receive(:active_rules).and_return([3, 2, 1]) - cache2 = add_mastery_paths_to_cache_key('foo', t_course, t_module, t2.user) + cache2 = add_mastery_paths_to_cache_key('foo', t_course, t2.user) expect(cache1).not_to eq cache2 end end diff --git a/spec/models/conditional_release/conditional_release_service_spec.rb b/spec/models/conditional_release/conditional_release_service_spec.rb index b9702052a09..8568d9761aa 100644 --- a/spec/models/conditional_release/conditional_release_service_spec.rb +++ b/spec/models/conditional_release/conditional_release_service_spec.rb @@ -16,7 +16,7 @@ # with this program. If not, see . # -require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb') +require_relative '../../conditional_release_spec_helper' require File.expand_path(File.dirname(__FILE__) + '/../../sharding_spec_helper') describe ConditionalRelease::Service do @@ -493,32 +493,6 @@ describe ConditionalRelease::Service do Service.rule_triggered_by(@a1, @teacher, nil) end end - - describe 'rules_assigning' do - before(:each) do - allow(Service).to receive(:active_rules).and_return(default_rules) - end - - it 'caches the calculation of the reverse index' do - enable_cache do - expect do - Service.rules_assigning(@a1, @teacher, nil) - allow(Service).to receive(:active_rules).and_raise 'should not refetch rules' - Service.rules_assigning(@a2, @teacher, nil) - end.to_not raise_error - end - end - - it 'returns all rules which matched assignments' do - expect(Service.rules_assigning(@a2, @teacher, nil).map{|r| r['id']}).to eq [1] - expect(Service.rules_assigning(@a3, @teacher, nil).map{|r| r['id']}).to eq [1, 2] - end - - it 'returns nil if no rules matched assignments' do - expect(Service.rules_assigning(@a1, @teacher, nil)).to eq nil - expect(Service.rules_assigning(@a4, @teacher, nil)).to eq nil - end - end end describe 'rules_for' do @@ -550,7 +524,7 @@ describe ConditionalRelease::Service do expect(CanvasHttp).to receive(:post).once.and_return(response) end - let(:rules) { Service.rules_for(@course, @student, [], nil) } + let(:rules) { Service.rules_for(@course, @student, nil) } let(:assignments0) { rules[0][:assignment_sets][0][:assignments] } let(:models0) { assignments0.map{|a| a[:model]} } @@ -601,55 +575,55 @@ describe ConditionalRelease::Service do it 'uses the cache' do enable_cache do expect_cyoe_request '200', @a1 - Service.rules_for(@course, @student, [], nil) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) + Service.rules_for(@course, @student, nil) end end it 'does not use the cache if cache cleared manually' do enable_cache do expect_cyoe_request '200', @a1 - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) Service.clear_rules_cache_for(@course, @student) expect_cyoe_request '200', @a1 - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end end it 'does not use the cache if assignments updated' do enable_cache do expect_cyoe_request '200', @a1 - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) @a1.title = 'updated' @a1.save! expect_cyoe_request '200', @a1 - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end end it 'does not use the cache if assignments are saved' do enable_cache do expect_cyoe_request '200', @a1 - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) @a1.save! expect_cyoe_request '200', @a1 - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end end it 'does not store an error response in the cache' do enable_cache do expect_cyoe_request '404' - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) expect_cyoe_request '404' - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end end end @@ -686,7 +660,7 @@ describe ConditionalRelease::Service do submission_model(course: @course, user: @student) @submission.assignment.grade_student(@student, grade: 10, grader: @teacher) expect_request_rules(@submission.reload) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end end end @@ -699,7 +673,7 @@ describe ConditionalRelease::Service do s2 = graded_submission_model(course: @course, user: @student) s2.assignment.grade_student(@student, grade: 10, grader: @teacher) expect_request_rules(s2.reload) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end it 'includes only completely graded submissions' do @@ -707,7 +681,7 @@ describe ConditionalRelease::Service do s1.assignment.grade_student(@student, grade: 10, grader: @teacher) _s2 = submission_model(course: @course, user: @student) expect_request_rules(s1.reload) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end it 'includes only non-muted assignments' do @@ -715,11 +689,11 @@ describe ConditionalRelease::Service do enable_cache do @submission.assignment.mute! expect_request_rules([]) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) @submission.assignment.unmute! expect_request_rules(@submission) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end end @@ -735,7 +709,7 @@ describe ConditionalRelease::Service do enable_cache do @submission.assignment.post_submissions(submission_ids: [@submission.id]) expect_request_rules(@submission.reload) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) end end @@ -752,7 +726,110 @@ describe ConditionalRelease::Service do @submission.assignment.post_submissions @submission.assignment.hide_submissions(submission_ids: [@submission.id]) expect_request_rules([]) - Service.rules_for(@course, @student, [], nil) + Service.rules_for(@course, @student, nil) + end + end + end + end + + context "native conditional release" do + before :once do + setup_course_with_native_conditional_release + end + + context "active_rules" do + it "should show all the rules in the course to teachers" do + data = Service.active_rules(@course, @teacher, nil) + # basically it's just the same thing as returned by the api + expect(data.first["scoring_ranges"].first["assignment_sets"].first["assignment_set_associations"].first["assignment_id"]).to eq @set1_assmt1.id + expect(data.first["trigger_assignment_model"]["points_possible"]).to eq @trigger_assmt.points_possible + end + + context "caching" do + specs_require_cache(:redis_cache_store) + + it "should cache across admins" do + old_teacher = @teacher + teacher_in_course(:course => @course) + data = Service.active_rules(@course, old_teacher, nil) + @course.conditional_release_rules.update_all(:deleted_at => Time.now.utc) # skip callbacks + expect(Service.active_rules(@course, @teacher, nil)).to eq data # doesn't matter who accesses it if they have rights + end + + it "should invalidate cache when a rule is saved" do + Service.active_rules(@course, @teacher, nil) + @rule.update_attribute(:deleted_at, Time.now.utc) + expect(Service.active_rules(@course, @teacher, nil)).to eq [] + end + + it "should invalidate cache when a trigger assignment is deleted" do + Service.active_rules(@course, @teacher, nil) + @trigger_assmt.destroy + expect(Service.active_rules(@course, @teacher, nil)).to eq [] + end + + it "should invalidate cache when a releasable assignment is deleted" do + old_data = Service.active_rules(@course, @teacher, nil) + @set1_assmt1.destroy + data = Service.active_rules(@course, @teacher, nil) + expect(data).to_not eq old_data + expect(data.first["scoring_ranges"].first["assignment_sets"].first["assignment_set_associations"]).to eq [] + end + end + end + + context "rules_for" do + it "should return no assignment set data for unreleased rules" do + data = Service.rules_for(@course, @student, nil) + expect(data.count).to eq 1 + rule_hash = data.first + expect(rule_hash['trigger_assignment_id']).to eq @trigger_assmt.id + expect(rule_hash['locked']).to eq true + expect(rule_hash['selected_set_id']).to eq nil + expect(rule_hash['assignment_sets']).to eq [] + end + + it "should return data about released assignment sets" do + @trigger_assmt.grade_student(@student, grade: 9, grader: @teacher) + rule_hash = Service.rules_for(@course, @student, nil).first + expect(rule_hash['trigger_assignment_id']).to eq @trigger_assmt.id + expect(rule_hash['locked']).to eq false + released_set = @set1_assmt1.conditional_release_associations.first.assignment_set + expect(rule_hash['selected_set_id']).to eq released_set.id + expect(rule_hash['assignment_sets'].count).to eq 1 + set_hash = rule_hash['assignment_sets'].first + expect(set_hash['assignment_set_associations'].first['model']).to eq @set1_assmt1 + end + + it "should return data about multiple assignment set choices" do + @trigger_assmt.grade_student(@student, grade: 2, grader: @teacher) # has two choices now + rule_hash = Service.rules_for(@course, @student, nil).first + expect(rule_hash['trigger_assignment_id']).to eq @trigger_assmt.id + expect(rule_hash['locked']).to eq false + expect(rule_hash['selected_set_id']).to eq nil # neither one was picked yet + expect(rule_hash['assignment_sets'].count).to eq 2 + expect(rule_hash['assignment_sets'].map{|s| s['assignment_set_associations'].first['model']}).to match_array([@set3a_assmt, @set3b_assmt]) + end + + context "caching" do + specs_require_cache(:redis_cache_store) + + it "should cache" do + data = Service.rules_for(@course, @student, nil) + @course.conditional_release_rules.update_all(:deleted_at => Time.now.utc) # skip callbacks + expect(Service.rules_for(@course, @student, nil)).to eq data + end + + it "should invalidate cache on rule change" do + data = Service.rules_for(@course, @student, nil) + @rule.update_attribute(:deleted_at, Time.now.utc) + expect(Service.rules_for(@course, @student, nil)).to eq [] + end + + it "should invalidate cache on submission change" do + data = Service.rules_for(@course, @student, nil) + @trigger_assmt.grade_student(@student, grade: 8, grader: @teacher) + expect(Service.rules_for(@course, @student, nil)).to_not eq data end end end diff --git a/spec/models/conditional_release/override_handler_spec.rb b/spec/models/conditional_release/override_handler_spec.rb index b17ae5bc8bf..ca8a3099616 100644 --- a/spec/models/conditional_release/override_handler_spec.rb +++ b/spec/models/conditional_release/override_handler_spec.rb @@ -21,43 +21,11 @@ require_dependency "conditional_release/override_handler" module ConditionalRelease describe OverrideHandler do + before :once do + setup_course_with_native_conditional_release + end + 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) @@ -106,5 +74,65 @@ module ConditionalRelease expect(visible_assmts).to_not include(@set3b_assmt) end end + + context 'handle_assignment_set_selection' do + before :once do + @trigger_assmt.grade_student(@student, grade: 2, grader: @teacher) # set up the choice + @set_a = @set3a_assmt.conditional_release_associations.first.assignment_set + @set_b = @set3b_assmt.conditional_release_associations.first.assignment_set + @invalid_set = @set1_assmt1.conditional_release_associations.first.assignment_set + end + + it "should check that a rule exists for the assignment" do + @rule.destroy! + expect { + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @set_a.id) + }.to raise_error(ActiveRecord::RecordNotFound) + end + + it "should check that the submission is actually graded" do + Submission.where(:id => @sub).update_all(:posted_at => nil) + expect { + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @set_a.id) + }.to raise_error(ActiveRecord::RecordNotFound) + end + + it "should check that the assignment set is valid for the submissions core" do + expect { + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @invalid_set.id) + }.to raise_error(ActiveRecord::RecordNotFound) + end + + it "should create the assignment override" do + assignment_ids = ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @set_a.id) + expect(assignment_ids).to eq [@set3a_assmt.id] + visible_assmts = DifferentiableAssignment.scope_filter(@course.assignments, @student, @course).to_a + expect(visible_assmts).to include(@set3a_assmt) + expect(visible_assmts).to_not include(@set3b_assmt) + end + + it "should be able to switch" do + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @set_a.id) + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @set_b.id) + visible_assmts = DifferentiableAssignment.scope_filter(@course.assignments, @student, @course).to_a + expect(visible_assmts).to include(@set3b_assmt) + expect(visible_assmts).to_not include(@set3a_assmt) + end + + it "should reuse an existing override when assigning (and leave it be when unassigning)" do + old_student = @student + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(old_student, @trigger_assmt, @set_a.id) + student_in_course(:course => @course, :active_all => true) + @trigger_assmt.grade_student(@student, grade: 3, grader: @teacher) + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @set_a.id) + + expect(@set3a_assmt.assignment_overrides.count).to eq 1 + expect(@set3a_assmt.assignment_overrides.first.assignment_override_students.count).to eq 2 + + ConditionalRelease::OverrideHandler.handle_assignment_set_selection(@student, @trigger_assmt, @set_b.id) # now unassign + expect(DifferentiableAssignment.scope_filter(@course.assignments, @student, @course).to_a).to_not include(@set3a_assmt) + expect(DifferentiableAssignment.scope_filter(@course.assignments, old_student, @course).to_a).to include(@set3a_assmt) + end + end end end