From 147d171aa1edf884e65b213a54962191b5796acf Mon Sep 17 00:00:00 2001 From: Simon Williams Date: Tue, 13 Aug 2013 17:15:15 -0600 Subject: [PATCH] correctly maintain assignment/rubric <-> outcome links There are a few old LearningOutcomeResults that have a nil artifact, and many that have a Submission as an artifact. We want to get rid of these because they come from bad data, and step 1 toward that goal is to stop creating them and hide them in the UI. The LORs with a nil artifact are very old and I believe came from a very early incarnation of outcomes. The LORs with a Submission as an artifact came from a combination of two code problems. The first was old code that allowed an assignment that was aligned with an outcome but did not have a rubric to create LORs directly based on it's submission. The second was a bug that prevented the assignment <-> outcome link from being destroyed when a rubric with an outcome was removed from an assignment. fixes CNVS-7495 fixes CNVS-7498 test plan: - try different combinations of adding a rubric with an outcome to an assignment. - when you grade the assignment, the grade create a learning outcome result (which can be seen on the outcome show page, or in the account outcome report) if the rubric+outcome are currently attached to the assignment. - so for example, add a rubric with an outcome, check, remove just the outcome row, check, add a new outcome row, check, remove the whole rubric, check. - be sure to check both the show page and the outcome report TODO: - datafix migration Change-Id: I37700e3e5c08fc6cfb8fcf1cac42ea6693fcaba3 Reviewed-on: https://gerrit.instructure.com/23303 Tested-by: Jenkins Reviewed-by: Cameron Matheson QA-Review: Amber Taniuchi Product-Review: Simon Williams --- app/models/assignment.rb | 8 +- app/models/learning_outcome.rb | 10 +- app/models/rubric.rb | 24 +-- app/models/rubric_association.rb | 12 +- app/models/submission.rb | 33 --- app/views/outcomes/show.html.erb | 19 +- spec/apis/v1/assignments_api_spec.rb | 1 + .../rubric_associations_controller_spec.rb | 15 ++ spec/controllers/rubrics_controller_spec.rb | 197 ++++++++++++++++++ spec/models/content_migration_spec.rb | 2 - spec/models/learning_outcome_spec.rb | 38 +--- spec/models/rubric_association_spec.rb | 123 ++++++++--- spec/models/rubric_spec.rb | 6 +- spec/selenium/assignments_rubrics_spec.rb | 1 - spec/selenium/teacher_rubrics_spec.rb | 1 - spec/selenium/teacher_speed_grader_spec.rb | 1 - spec/spec_helper.rb | 92 ++++---- .../canvas/account_reports/outcome_reports.rb | 6 +- .../spec_canvas/outcome_reports_spec.rb | 16 +- 19 files changed, 415 insertions(+), 190 deletions(-) diff --git a/app/models/assignment.rb b/app/models/assignment.rb index eb1935103ae..a53378d1023 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -897,7 +897,7 @@ class Assignment < ActiveRecord::Base students.each do |student| submission_updated = false - submission = self.find_or_create_submission(student) #submissions.find_or_create_by_user_id(student.id) #(:first, :conditions => {:assignment_id => self.id, :user_id => student.id}) + submission = self.find_or_create_submission(student) if student == original_student || !grade_group_students_individually previously_graded = submission.grade.present? submission.attributes = opts @@ -923,12 +923,6 @@ class Assignment < ActiveRecord::Base submission.group = group submission.graded_at = Time.zone.now if did_grade previously_graded ? submission.with_versioning(:explicit => true) { submission.save! } : submission.save! - - unless self.rubric_association - self.learning_outcome_alignments.each do |alignment| - submission.create_outcome_result(alignment) - end - end end submission.add_comment(comment) if comment && (group_comment == "1" || student == original_student) submissions << submission if group_comment == "1" || student == original_student || submission_updated diff --git a/app/models/learning_outcome.rb b/app/models/learning_outcome.rb index 037e931b5aa..63772abc9ca 100644 --- a/app/models/learning_outcome.rb +++ b/app/models/learning_outcome.rb @@ -91,12 +91,16 @@ class LearningOutcome < ActiveRecord::Base end def self.update_alignments(asset, context, new_outcome_ids) - old_alignments = asset.learning_outcome_alignments - old_outcome_ids = old_alignments.map(&:learning_outcome_id).compact.uniq + old_outcome_ids = asset.learning_outcome_alignments. + where("learning_outcome_id IS NOT NULL"). + pluck(:learning_outcome_id). + uniq defunct_outcome_ids = old_outcome_ids - new_outcome_ids unless defunct_outcome_ids.empty? - asset.learning_outcome_alignments.where(:learning_outcome_id => defunct_outcome_ids).update_all(:workflow_state => 'deleted') + asset.learning_outcome_alignments. + where(:learning_outcome_id => defunct_outcome_ids). + update_all(:workflow_state => 'deleted') end missing_outcome_ids = new_outcome_ids - old_outcome_ids diff --git a/app/models/rubric.rb b/app/models/rubric.rb index 462fbf42b0f..b199e6da0b5 100644 --- a/app/models/rubric.rb +++ b/app/models/rubric.rb @@ -113,11 +113,14 @@ class Rubric < ActiveRecord::Base end end - attr_accessor :alignments_changed def update_alignments - return unless @alignments_changed - outcome_ids = (self.data || []).map{|c| c[:learning_outcome_id] }.compact.map(&:to_i).uniq - LearningOutcome.update_alignments(self, context, outcome_ids) + if data_changed? || workflow_state_changed? + outcome_ids = [] + unless deleted? + outcome_ids = (data || []).map{|c| c[:learning_outcome_id] }.compact.map(&:to_i).uniq + end + LearningOutcome.update_alignments(self, context, outcome_ids) + end true end @@ -205,7 +208,6 @@ class Rubric < ActiveRecord::Base if criterion_data[:learning_outcome_id].present? outcome = LearningOutcome.find_by_id(criterion_data[:learning_outcome_id]) if outcome - @alignments_changed = true criterion[:learning_outcome_id] = outcome.id criterion[:mastery_points] = ((criterion_data[:mastery_points] || outcome.data[:rubric_criterion][:mastery_points]).to_f rescue nil) criterion[:ignore_for_scoring] = criterion_data[:ignore_for_scoring] == '1' @@ -282,7 +284,6 @@ class Rubric < ActiveRecord::Base item.data = hash[:data] item.data.each do |crit| if crit[:learning_outcome_migration_id] - item.alignments_changed = true if migration.respond_to?(:outcome_to_id_map) && id = migration.outcome_to_id_map[crit[:learning_outcome_migration_id]] crit[:learning_outcome_id] = id elsif lo = context.created_learning_outcomes.find_by_migration_id(crit[:learning_outcome_migration_id]) @@ -302,15 +303,4 @@ class Rubric < ActiveRecord::Base item end - - def self.generate(opts={}) - context = opts[:context] - raise "Context required for rubrics" unless context - rubric = context.rubrics.build(:user => opts[:user]) - user = opts[:user] - params = opts[:data] - rubric.update_criteria(params) - rubric - end - end diff --git a/app/models/rubric_association.rb b/app/models/rubric_association.rb index 058fe23bdde..5bc3571d231 100644 --- a/app/models/rubric_association.rb +++ b/app/models/rubric_association.rb @@ -27,7 +27,7 @@ class RubricAssociation < ActiveRecord::Base belongs_to :association, :polymorphic => true belongs_to :context, :polymorphic => true - has_many :rubric_assessments, :dependent => :destroy + has_many :rubric_assessments has_many :assessment_requests, :dependent => :destroy has_a_broadcast_policy @@ -41,8 +41,9 @@ class RubricAssociation < ActiveRecord::Base after_create :link_to_assessments before_save :update_old_rubric after_destroy :update_rubric + after_destroy :update_alignments after_save :assert_uniqueness - after_save :update_outcome_relations + after_save :update_alignments serialize :summary_data ValidAssociationModels = { @@ -96,9 +97,12 @@ class RubricAssociation < ActiveRecord::Base end end - def update_outcome_relations + def update_alignments return unless assignment - outcome_ids = rubric.learning_outcome_alignments.map(&:learning_outcome_id) + outcome_ids = [] + unless self.destroyed? + outcome_ids = rubric.learning_outcome_alignments.map(&:learning_outcome_id) + end LearningOutcome.update_alignments(assignment, context, outcome_ids) true end diff --git a/app/models/submission.rb b/app/models/submission.rb index 86611be1944..53a9bc3939c 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -1090,39 +1090,6 @@ class Submission < ActiveRecord::Base hash end - def create_outcome_result(alignment, explicit_mastery=false) - # find or create the user's unique LearningOutcomeResult for this alignment - # of the submission's assignment. - result = alignment.learning_outcome_results. - for_association(assignment). - find_or_initialize_by_user_id(user.id) - - # force the context and artifact - result.artifact = self - result.context = alignment.context - - # mastery - result.possible = assignment.points_possible - result.score = score - if alignment.tag == "points_mastery" - result.mastery = result.score && assignment.mastery_score && result.score >= assignment.mastery_score - elsif alignment.tag == "explicit_mastery" - result.mastery = explicit_mastery - else - result.mastery = nil - end - - # attempt - result.attempt = attempt - - # title - result.title = "#{user.name}, #{assignment.title}" - - result.assessed_at = Time.now - result.save_to_version(result.attempt) - result - end - def update_participation # TODO: can we do this in bulk? return if assignment.deleted? || assignment.muted? diff --git a/app/views/outcomes/show.html.erb b/app/views/outcomes/show.html.erb index a0eac09044f..d9eed49bcf8 100644 --- a/app/views/outcomes/show.html.erb +++ b/app/views/outcomes/show.html.erb @@ -27,7 +27,24 @@
    <% @results.each do |result| %> - <%= render :partial => "outcome_result", :object => result %> + <%# + There are a few old LearningOutcomeResults that have a nil artifact, and + many that have a Submission as an artifact. We want to get rid of these + because they come from bad data, and step 1 toward that goal is to stop + creating them and hide them in the UI. + + The LORs with a nil artifact are very old and I believe came from a very + early incarnation of outcomes. The LORs with a Submission as an artifact + came from a combination of two code problems. The first was old code + that allowed an assignment that was aligned with an outcome but did not + have a rubric to create LORs directly based on it's submission. The + second was a bug that prevented the assignment <-> outcome link from + being destroyed when a rubric with an outcome was removed from an + assignment. + %> + <% if result.artifact_type.present? && result.artifact_type != 'Submission' %> + <%= render :partial => "outcome_result", :object => result %> + <% end %> <% end %>
<%= will_paginate(@results) %> diff --git a/spec/apis/v1/assignments_api_spec.rb b/spec/apis/v1/assignments_api_spec.rb index 94e5cfb7821..83b0ef4c65d 100644 --- a/spec/apis/v1/assignments_api_spec.rb +++ b/spec/apis/v1/assignments_api_spec.rb @@ -208,6 +208,7 @@ describe AssignmentsApiController, :type => :integration do :free_form_criterion_comments => true) @assignment.create_rubric_association(:rubric => @rubric, + :context => @course, :purpose => 'grading', :use_for_grading => true) json = api_get_assignments_index_from_course(@course) diff --git a/spec/controllers/rubric_associations_controller_spec.rb b/spec/controllers/rubric_associations_controller_spec.rb index 0748900623e..681cd970637 100644 --- a/spec/controllers/rubric_associations_controller_spec.rb +++ b/spec/controllers/rubric_associations_controller_spec.rb @@ -125,5 +125,20 @@ describe RubricAssociationsController do assigns[:association].should_not be_nil assigns[:association].should be_frozen end + + it "should remove aligments links" do + course_with_teacher_logged_in(:active_all => true) + outcome_with_rubric + rubric_association_model(:user => @user, :context => @course, :rubric => @rubric) + + @rubric_association_object.reload.learning_outcome_alignments.count.should == 1 + @rubric.reload.learning_outcome_alignments.count.should == 1 + + delete 'destroy', :course_id => @course.id, :id => @rubric_association.id + + @rubric.reload.deleted?.should be_true + @rubric_association_object.reload.learning_outcome_alignments.count.should == 0 + @rubric.reload.learning_outcome_alignments.count.should == 0 + end end end diff --git a/spec/controllers/rubrics_controller_spec.rb b/spec/controllers/rubrics_controller_spec.rb index e8806f7e94f..3dec498bf44 100644 --- a/spec/controllers/rubrics_controller_spec.rb +++ b/spec/controllers/rubrics_controller_spec.rb @@ -47,6 +47,7 @@ describe RubricsController do post 'create', :course_id => @course.id assert_unauthorized end + it "should assign variables" do course_with_teacher_logged_in(:active_all => true) post 'create', :course_id => @course.id, :rubric => {} @@ -55,6 +56,7 @@ describe RubricsController do response.should be_success end + it "should create an association if specified" do course_with_teacher_logged_in(:active_all => true) association = @course.assignments.create!(assignment_valid_attributes) @@ -64,6 +66,72 @@ describe RubricsController do assigns[:rubric].rubric_associations.length.should eql(1) response.should be_success end + + it "should associate outcomes correctly" do + course_with_teacher_logged_in(:active_all => true) + assignment = @course.assignments.create!(assignment_valid_attributes) + outcome_group = @course.root_outcome_group + outcome = @course.created_learning_outcomes.create!( + :description => 'hi', + :short_description => 'hi' + ) + outcome_group.add_outcome(outcome) + outcome_group.save! + + create_params = { + "course_id" => @course.id, + "points_possible" => "5", + "rubric" => { + "criteria" => { + "0" => { + "description" => "hi", + "id" => "", + "learning_outcome_id" => outcome.id, + "long_description" => "", + "mastery_points" => "3", + "points" => "5", + "ratings" => { + "0" => { + "description" => "Exceeds Expectations", + "id" => "blank", + "points" => "5" + }, + "1" => { + "description" => "Meets Expectations", + "id" => "blank", + "points" => "3" + }, + "2" => { + "description" => "Does Not Meet Expectations", + "id" => "blank_2", + "points" => "0" + } + } + } + }, + "free_form_criterion_comments" => "0", + "points_possible" => "5", + "title" => "Some Rubric" + }, + "rubric_association" => { + "association_id" => assignment.id, + "association_type" => "Assignment", + "hide_score_total" => "0", + "id" => "", + "purpose" => "grading", + "use_for_grading" => "1" + }, + "rubric_association_id" => "", + "rubric_id" => "new", + "skip_updating_points_possible" => "false", + "title" => "Some Rubric" + } + + post 'create', create_params + + assignment.reload.learning_outcome_alignments.count.should == 1 + Rubric.last.learning_outcome_alignments.count.should == 1 + end end describe "PUT 'update'" do @@ -203,6 +271,135 @@ describe RubricsController do assigns[:rubric].rubric_associations.find_by_id(@rubric_association.id).title.should eql("some title") response.should be_success end + + it "should add an outcome association if one is linked" do + course_with_teacher_logged_in(:active_all => true) + assignment = @course.assignments.create!(assignment_valid_attributes) + rubric_association_model(:user => @user, :context => @course) + outcome_group = @course.root_outcome_group + outcome = @course.created_learning_outcomes.create!( + :description => 'hi', + :short_description => 'hi' + ) + outcome_group.add_outcome(outcome) + outcome_group.save! + + update_params = { + "course_id" => @course.id, + "id" => @rubric.id, + "points_possible" => "5", + "rubric" => { + "criteria" => { + "0" => { + "description" => "hi", + "id" => "", + "learning_outcome_id" => outcome.id, + "long_description" => "", + "points" => "5", + "ratings" => { + "0" => { + "description" => "Exceeds Expectations", + "id" => "blank", + "points" => "5" + }, + "1" => { + "description" => "Meets Expectations", + "id" => "blank", + "points" => "3" + }, + "2" => { + "description" => "Does Not Meet Expectations", + "id" => "blank_2", + "points" => "0" + } + } + } + }, + "free_form_criterion_comments" => "0", + "points_possible" => "5", + "title" => "Some Rubric" + }, + "rubric_association" => { + "association_id" => assignment.id, + "association_type" => "Assignment", + "hide_score_total" => "0", + "id" => @rubric_association.id, + "purpose" => "grading", + "use_for_grading" => "1" + }, + "rubric_association_id" => @rubric_association.id, + "rubric_id" => @rubric.id, + "skip_updating_points_possible" => "false", + "title" => "Some Rubric" + } + + assignment.reload.learning_outcome_alignments.count.should == 0 + @rubric.reload.learning_outcome_alignments.count.should == 0 + + put 'update', update_params + + assignment.reload.learning_outcome_alignments.count.should == 1 + @rubric.reload.learning_outcome_alignments.count.should == 1 + end + + it "should remove an outcome association if one is removed" do + course_with_teacher_logged_in(:active_all => true) + outcome_with_rubric + assignment = @course.assignments.create!(assignment_valid_attributes) + association = @rubric.associate_with(assignment, @course, :purpose => 'grading') + + update_params = { + "course_id" => @course.id, + "id" => @rubric.id, + "points_possible" => "5", + "rubric" => { + "criteria" => { + "0" => { + "description" => "Description of criterion", + "id" => "", + "learning_outcome_id" => "", + "long_description" => "", + "points" => "5", + "ratings" => { + "0" => { + "description" => "Full Marks", + "id" => "blank", + "points" => "5" + }, + "1" => { + "description" => "No Marks", + "id" => "blank_2", + "points" => "0" + } + } + } + }, + "free_form_criterion_comments" => "0", + "points_possible" => "5", + "title" => "Some Rubric" + }, + "rubric_association" => { + "association_id" => assignment.id, + "association_type" => "Assignment", + "hide_score_total" => "0", + "id" => association.id, + "purpose" => "grading", + "use_for_grading" => "1" + }, + "rubric_association_id" => association.id, + "rubric_id" => @rubric.id, + "skip_updating_points_possible" => "false", + "title" => "Some Rubric" + } + + assignment.reload.learning_outcome_alignments.count.should == 1 + @rubric.reload.learning_outcome_alignments.count.should == 1 + + put 'update', update_params + + assignment.reload.learning_outcome_alignments.count.should == 0 + @rubric.reload.learning_outcome_alignments.count.should == 0 + end end describe "DELETE 'destroy'" do diff --git a/spec/models/content_migration_spec.rb b/spec/models/content_migration_spec.rb index 002d0daaed5..3b010741421 100644 --- a/spec/models/content_migration_spec.rb +++ b/spec/models/content_migration_spec.rb @@ -691,7 +691,6 @@ describe ContentMigration do :learning_outcome_id => lo2.id } ] - rub.alignments_changed = true rub.save! rub.associate_with(@copy_from, @copy_from) @@ -724,7 +723,6 @@ describe ContentMigration do :learning_outcome_id => lo.id } ] - rub.alignments_changed = true rub.save! from_assign = @copy_from.assignments.create!(:title => "some assignment") diff --git a/spec/models/learning_outcome_spec.rb b/spec/models/learning_outcome_spec.rb index 1856e82227f..632d2761a08 100644 --- a/spec/models/learning_outcome_spec.rb +++ b/spec/models/learning_outcome_spec.rb @@ -46,7 +46,6 @@ describe LearningOutcome do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.should_not be_new_record @rubric.reload @@ -80,7 +79,6 @@ describe LearningOutcome do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.should_not be_new_record @rubric.reload @@ -157,7 +155,6 @@ describe LearningOutcome do :learning_outcome_id => @outcome2.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.reload @rubric.should_not be_new_record @@ -191,7 +188,6 @@ describe LearningOutcome do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.reload @rubric.should_not be_new_record @@ -277,7 +273,6 @@ describe LearningOutcome do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.reload @rubric.should_not be_new_record @@ -321,6 +316,7 @@ describe LearningOutcome do @result.mastery.should eql(false) n = @result.version_number end + it "should not override rubric-based alignments with non-rubric-based alignments for the same assignment" do assignment_model @outcome = @course.created_learning_outcomes.create!(:title => 'outcome') @@ -347,7 +343,6 @@ describe LearningOutcome do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.reload @rubric.should_not be_new_record @@ -388,37 +383,6 @@ describe LearningOutcome do @result.mastery.should eql(false) n = @result.version_number end - it "should build an outcome result for a non-rubric alignment" do - assignment_model - @assignment.mastery_score = 2 - @assignment.save! - @outcome = @course.created_learning_outcomes.create!(:title => 'outcome') - @alignment = @outcome.align(@assignment, @course, :mastery_type => "points") - @alignment.should_not be_nil - @alignment.content.should eql(@assignment) - @alignment.context.should eql(@course) - - @user = user(:active_all => true) - @e = @course.enroll_student(@user) - @alignment.rubric_association_id.should eql(nil) - @assignment.reload - @assignment.learning_outcome_alignments.should_not be_empty - @assignment.learning_outcome_alignments.length.should eql(1) - @assignment.learning_outcome_alignments.map(&:learning_outcome_id).uniq.should eql([@outcome.id]) - - @submission = @assignment.grade_student(@user, :grade => "10").first - @outcome.learning_outcome_results.should_not be_empty - @outcome.learning_outcome_results.length.should eql(1) - - @result = @outcome.learning_outcome_results.select{|r| r.artifact_type == 'Submission'}.first - @result.should_not be_nil - @result.user_id.should eql(@user.id) - @result.possible.should eql(1.5) - @result.score.should eql(10.0) - @result.original_score.should eql(10.0) - @result.original_possible.should eql(1.5) - @result.mastery.should eql(true) - end end describe "permissions" do diff --git a/spec/models/rubric_association_spec.rb b/spec/models/rubric_association_spec.rb index 919a59529ac..e3ebaef57ed 100644 --- a/spec/models/rubric_association_spec.rb +++ b/spec/models/rubric_association_spec.rb @@ -20,34 +20,40 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb') describe RubricAssociation do - context "when course has multiple students enrolled" do + + def rubric_association_params_for_assignment(assign) + HashWithIndifferentAccess.new({ + hide_score_total: "0", + purpose: "grading", + skip_updating_points_possible: false, + update_if_existing: true, + use_for_grading: "1", + association: assign + }) + end + + context "course rubrics" do before :each do # Create a course, 2 students and enroll them - @test_course = course(:active_course => true) - @teacher = @test_course.teachers.first - @student_1 = user(:active_user => true) - @student_2 = user(:active_user => true) - @test_course.enroll_student(@student_1) - @test_course.enroll_student(@student_2) + course_with_teacher(:active_course => true, :active_user => true) + @student_1 = student_in_course(:active_user => true).user + @student_2 = student_in_course(:active_user => true).user end context "when a peer-review assignment has been completed AFTER rubric created" do before :each do # Create the assignment - @assignment = course.assignments.create!(:title => 'Test Assignment', :peer_reviews => true) - @assignment.workflow_state = 'published' - @assignment.submission_types = 'online_text_entry' - @assignment.context = @test_course + @assignment = @course.assignments.create!( + :title => 'Test Assignment', + :peer_reviews => true, + :submission_types => 'online_text_entry' + ) # Create the rubric - @rubric = @test_course.rubrics.build - @rubric.user = @teacher - @rubric.save! - rubric_association = HashWithIndifferentAccess.new({"hide_score_total"=>"0", "purpose"=>"grading", - "skip_updating_points_possible"=>false, "update_if_existing"=>true, - "use_for_grading"=>"1", "association"=>@assignment}) #, "id"=>3}) + @rubric = @course.rubrics.create! { |r| r.user = @teacher } - @rubric_assoc = RubricAssociation.generate(@teacher, @rubric, @test_course, rubric_association) + ra_params = rubric_association_params_for_assignment(@assignment) + @rubric_assoc = RubricAssociation.generate(@teacher, @rubric, @course, ra_params) # students complete it @assignment.submit_homework(@student_1, :submission_type => 'online_text_entry', :body => 'Finished first') @@ -70,10 +76,11 @@ describe RubricAssociation do context "when a peer-review assignment has been completed BEFORE rubric created" do before :each do # Create the assignment - @assignment = course.assignments.create!(:title => 'Test Assignment', :peer_reviews => true) - @assignment.workflow_state = 'published' - @assignment.submission_types = 'online_text_entry' - @assignment.context = @test_course + @assignment = @course.assignments.create!( + :title => 'Test Assignment', + :peer_reviews => true, + :submission_types => 'online_text_entry' + ) # students complete it @assignment.submit_homework(@student_1, :submission_type => 'online_text_entry', :body => 'Finished first') @@ -89,14 +96,9 @@ describe RubricAssociation do context "and a rubric is created" do before :each do - @rubric = @test_course.rubrics.build - @rubric.user = @teacher - @rubric.save! - rubric_association = HashWithIndifferentAccess.new({"hide_score_total"=>"0", "purpose"=>"grading", - "skip_updating_points_possible"=>false, "update_if_existing"=>true, - "use_for_grading"=>"1", "association"=>@assignment}) #, "id"=>3}) - - @rubric_assoc = RubricAssociation.generate(@teacher, @rubric, @test_course, rubric_association) + @rubric = @course.rubrics.create! { |r| r.user = @teacher } + ra_params = rubric_association_params_for_assignment(@assignment) + @rubric_assoc = RubricAssociation.generate(@teacher, @rubric, @course, ra_params) end it "should have 2 assessment_requests" do @@ -105,6 +107,67 @@ describe RubricAssociation do end end end + + context "#update_alignments" do + it "should do nothing if it is not associated to an assignment" do + rubric = @course.rubrics.create! + ra = RubricAssociation.create!( + :rubric => @rubric, + :association => @course, + :context => @course, + :purpose => 'bookmark' + ) + LearningOutcome.expects(:update_alignments).never + ra.update_alignments + end + + it "should align the outcome to the assignment when created and remove when destroyed" do + assignment = @course.assignments.create!( + :title => 'Test Assignment', + :peer_reviews => true, + :submission_types => 'online_text_entry' + ) + outcome_with_rubric + ra = @rubric.rubric_associations.create!( + :association => assignment, + :context => @course, + :purpose => 'grading' + ) + assignment.reload.learning_outcome_alignments.count.should == 1 + + ra.destroy + assignment.reload.learning_outcome_alignments.count.should == 0 + end + end + + it "should not delete assessments when an association is destroyed" do + assignment = @course.assignments.create!( + :title => 'Test Assignment', + :peer_reviews => true, + :submission_types => 'online_text_entry' + ) + outcome_with_rubric + ra = @rubric.rubric_associations.create!( + :association => assignment, + :context => @course, + :purpose => 'grading' + ) + assess = ra.assess({ + :user => @student_1, + :assessor => @teacher, + :artifact => assignment.find_or_create_submission(@student_1), + :assessment => { + :assessment_type => 'grading', + :criterion_crit1 => { + :points => 5 + } + } + }) + + assess.should_not be_nil + ra.destroy + assess.reload.should_not be_nil + end end context "when a rubric is associated with an account" do diff --git a/spec/models/rubric_spec.rb b/spec/models/rubric_spec.rb index b979c55268c..d32fb0ce389 100644 --- a/spec/models/rubric_spec.rb +++ b/spec/models/rubric_spec.rb @@ -47,7 +47,6 @@ describe Rubric do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.should_not be_new_record @rubric.learning_outcome_alignments(true).should_not be_empty @@ -80,7 +79,6 @@ describe Rubric do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.should_not be_new_record @rubric.learning_outcome_alignments(true).should_not be_empty @@ -107,6 +105,7 @@ describe Rubric do @rubric.save! @rubric.learning_outcome_alignments.active.should be_empty end + it "should create learning outcome associations for multiple outcome rows" do assignment_model @outcome = @course.created_learning_outcomes.create!(:title => 'outcome') @@ -154,12 +153,12 @@ describe Rubric do :learning_outcome_id => @outcome2.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.should_not be_new_record @rubric.learning_outcome_alignments(true).should_not be_empty @rubric.learning_outcome_alignments.map(&:learning_outcome_id).sort.should eql([@outcome.id, @outcome2.id].sort) end + it "should create outcome results when outcome-aligned rubrics are assessed" do assignment_model @outcome = @course.created_learning_outcomes.create!(:title => 'outcome') @@ -186,7 +185,6 @@ describe Rubric do :learning_outcome_id => @outcome.id } ] - @rubric.instance_variable_set('@alignments_changed', true) @rubric.save! @rubric.should_not be_new_record @rubric.learning_outcome_alignments(true).should_not be_empty diff --git a/spec/selenium/assignments_rubrics_spec.rb b/spec/selenium/assignments_rubrics_spec.rb index cd45ed42344..aafb0e3a963 100644 --- a/spec/selenium/assignments_rubrics_spec.rb +++ b/spec/selenium/assignments_rubrics_spec.rb @@ -182,7 +182,6 @@ describe "assignment rubrics" do @submission = @assignment.submit_homework(@student, {:url => "http://www.instructure.com/"}) @rubric.data[0][:ignore_for_scoring] = '1' @rubric.points_possible = 5 - @rubric.alignments_changed = true @rubric.save! @assignment.points_possible = 5 @assignment.save! diff --git a/spec/selenium/teacher_rubrics_spec.rb b/spec/selenium/teacher_rubrics_spec.rb index 87cea25dd92..6f28339ba2c 100644 --- a/spec/selenium/teacher_rubrics_spec.rb +++ b/spec/selenium/teacher_rubrics_spec.rb @@ -50,7 +50,6 @@ describe "course rubrics" do @association = @rubric.associate_with(@assignment, @course, :use_for_grading => true, :purpose => 'grading') @rubric.data[0][:ignore_for_scoring] = '1' @rubric.points_possible = 5 - @rubric.alignments_changed = true @rubric.save! get "/courses/#{@course.id}/rubrics/#{@rubric.id}" diff --git a/spec/selenium/teacher_speed_grader_spec.rb b/spec/selenium/teacher_speed_grader_spec.rb index e874a3bba5e..dfa4b4096aa 100644 --- a/spec/selenium/teacher_speed_grader_spec.rb +++ b/spec/selenium/teacher_speed_grader_spec.rb @@ -432,7 +432,6 @@ describe "speed grader" do :learning_outcome_id => @ignored.id, :ignore_for_scoring => '1', }] - @rubric.alignments_changed = true @rubric.save! get "/courses/#{@course.id}/gradebook/speed_grader?assignment_id=#{@assignment.id}" diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 8d3a75b37ee..a03dc3383c6 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -613,52 +613,52 @@ Spec::Runner.configure do |config| @outcome_group.add_outcome(@outcome) @outcome_group.save! - @rubric = Rubric.generate(:context => @course, - :data => { - :title => 'My Rubric', - :hide_score_total => false, - :criteria => { - "0" => { - :points => 3, - :mastery_points => 0, - :description => "Outcome row", - :long_description => @outcome.description, - :ratings => { - "0" => { - :points => 3, - :description => "Rockin'", - }, - "1" => { - :points => 0, - :description => "Lame", - } - }, - :learning_outcome_id => @outcome.id - }, - "1" => { - :points => 5, - :description => "no outcome row", - :long_description => 'non outcome criterion', - :ratings => { - "0" => { - :points => 5, - :description => "Amazing", - }, - "1" => { - :points => 3, - :description => "not too bad", - }, - "2" => { - :points => 0, - :description => "no bueno", - } - } - } - } - }) - @rubric.instance_variable_set('@alignments_changed', true) - @rubric.save! - @rubric.update_alignments + rubric_params = { + :title => 'My Rubric', + :hide_score_total => false, + :criteria => { + "0" => { + :points => 3, + :mastery_points => 0, + :description => "Outcome row", + :long_description => @outcome.description, + :ratings => { + "0" => { + :points => 3, + :description => "Rockin'", + }, + "1" => { + :points => 0, + :description => "Lame", + } + }, + :learning_outcome_id => @outcome.id + }, + "1" => { + :points => 5, + :description => "no outcome row", + :long_description => 'non outcome criterion', + :ratings => { + "0" => { + :points => 5, + :description => "Amazing", + }, + "1" => { + :points => 3, + :description => "not too bad", + }, + "2" => { + :points => 0, + :description => "no bueno", + } + } + } + } + } + + @rubric = @course.rubrics.build + @rubric.update_criteria(rubric_params) + @rubric.reload end def grading_standard_for(context, opts={}) diff --git a/vendor/plugins/account_reports/lib/canvas/account_reports/outcome_reports.rb b/vendor/plugins/account_reports/lib/canvas/account_reports/outcome_reports.rb index 3282bc18aaa..d68015661a6 100644 --- a/vendor/plugins/account_reports/lib/canvas/account_reports/outcome_reports.rb +++ b/vendor/plugins/account_reports/lib/canvas/account_reports/outcome_reports.rb @@ -104,7 +104,8 @@ module Canvas::AccountReports AND sub.user_id = pseudonyms.user_id", parameters])). where(" ct.tag_type = 'learning_outcome' - AND ct.workflow_state <> 'deleted'" + AND ct.workflow_state <> 'deleted' + AND (r.id IS NULL OR (r.artifact_type IS NOT NULL AND r.artifact_type <> 'Submission'))" ) unless @include_deleted @@ -205,7 +206,8 @@ module Canvas::AccountReports AND r.artifact_type = 'QuizSubmission' LEFT OUTER JOIN assessment_questions aq ON aq.id = r.associated_asset_id AND r.associated_asset_type = 'AssessmentQuestion'"). - where("ct.workflow_state <> 'deleted'") + where("ct.workflow_state <> 'deleted' + AND (r.id IS NULL OR (r.artifact_type IS NOT NULL AND r.artifact_type <> 'Submission'))") unless @include_deleted students = students.where("p.workflow_state<>'deleted' AND c.workflow_state='available'") diff --git a/vendor/plugins/account_reports/spec_canvas/outcome_reports_spec.rb b/vendor/plugins/account_reports/spec_canvas/outcome_reports_spec.rb index 0aa3b8c0414..733f630cbc2 100644 --- a/vendor/plugins/account_reports/spec_canvas/outcome_reports_spec.rb +++ b/vendor/plugins/account_reports/spec_canvas/outcome_reports_spec.rb @@ -215,6 +215,20 @@ describe "Outcom Reports" do "https://#{HostUrl.context_host(@course1)}/courses/#{@course1.id}/assignments/#{@assignment.id}"] end + + it "should not incluce invalid learning outcome results" do + ct = @assignment.learning_outcome_alignments.last + lor = ct.learning_outcome_results.for_association(@assignment).build + lor.user = @user1 + lor.artifact = @submission + lor.context = ct.context + lor.possible = @assignment.points_possible + lor.score = @submission.score + lor.save! + + parsed = ReportSpecHelper.run_report(@account, @type, {}, 1) + parsed.length.should == 2 + end end describe "outcome results report" do @@ -324,4 +338,4 @@ describe "Outcom Reports" do end end -end \ No newline at end of file +end