From a9ff89f5095f72cd7c3d44dc85bd384ae628e010 Mon Sep 17 00:00:00 2001 From: Michael Brewer-Davis Date: Sun, 14 Mar 2021 20:50:28 -0500 Subject: [PATCH] make LearningOutcomeResult soft-deleteable refs OUT-4247 Test plan: - ensure results from aligned rubrics and from quizzes are visible in the learning mastery gradebook Change-Id: Ie9731f2c702f91b3da2ce4d8ae43f7045ee5509e Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/260613 Tested-by: Service Cloud Jenkins QA-Review: Brian Watson Product-Review: Michael Brewer-Davis Reviewed-by: Augusto Callejas --- .../outcome_groups_api_controller.rb | 2 +- app/controllers/outcomes_controller.rb | 8 +- app/graphql/types/learning_outcome_type.rb | 2 +- app/models/learning_outcome.rb | 11 +-- app/models/learning_outcome_result.rb | 4 +- app/models/live_assessments/submission.rb | 1 + .../quizzes/quiz_outcome_result_builder.rb | 2 + app/models/rubric_assessment.rb | 1 + app/models/user.rb | 2 +- ...rkflow_state_to_learning_outcome_result.rb | 39 +++++++++ .../lib/account_reports/outcome_reports.rb | 4 +- lib/api/v1/outcome.rb | 4 +- lib/api/v1/outcome_results.rb | 2 +- lib/canvas/live_events.rb | 3 +- lib/outcomes/result_analytics.rb | 2 +- spec/apis/v1/outcome_groups_api_spec.rb | 87 +++++++++++++------ spec/controllers/courses_controller_spec.rb | 4 +- spec/controllers/outcomes_controller_spec.rb | 43 +++++++++ .../rubric_assessments_controller_spec.rb | 2 +- .../types/learning_outcome_type_spec.rb | 6 ++ spec/lib/canvas/live_events_spec.rb | 6 +- spec/lib/outcomes/result_analytics_spec.rb | 6 ++ spec/models/learning_outcome_result_spec.rb | 16 ++-- spec/models/learning_outcome_spec.rb | 15 +++- .../live_assessments/submission_spec.rb | 10 +++ .../quiz_outcome_result_builder_spec.rb | 38 ++++---- spec/models/rubric_assessment_spec.rb | 30 +++++++ 27 files changed, 273 insertions(+), 77 deletions(-) create mode 100644 db/migrate/20210311224137_add_workflow_state_to_learning_outcome_result.rb diff --git a/app/controllers/outcome_groups_api_controller.rb b/app/controllers/outcome_groups_api_controller.rb index 2923addb536..db5e2b474c3 100644 --- a/app/controllers/outcome_groups_api_controller.rb +++ b/app/controllers/outcome_groups_api_controller.rb @@ -192,7 +192,7 @@ class OutcomeGroupsApiController < ApplicationController unless params["outcome_style"] == "abbrev" outcome_ids = links.map(&:content_id) - ret = LearningOutcomeResult.distinct.where(learning_outcome_id: outcome_ids).pluck(:learning_outcome_id) + ret = LearningOutcomeResult.active.distinct.where(learning_outcome_id: outcome_ids).pluck(:learning_outcome_id) # ret is now a list of outcomes that have been assessed outcome_params[:assessed_outcomes] = ret end diff --git a/app/controllers/outcomes_controller.rb b/app/controllers/outcomes_controller.rb index 115a476f186..8270d4fd491 100644 --- a/app/controllers/outcomes_controller.rb +++ b/app/controllers/outcomes_controller.rb @@ -73,7 +73,7 @@ class OutcomesController < ApplicationController end @alignments = @outcome.alignments.active.for_context(@context) add_crumb(@outcome.short_description, named_context_url(@context, :context_outcome_url, @outcome.id)) - @results = @outcome.learning_outcome_results.for_context_codes(codes).custom_ordering(params[:sort]).paginate(:page => params[:page], :per_page => 10) + @results = @outcome.learning_outcome_results.active.for_context_codes(codes).custom_ordering(params[:sort]).paginate(:page => params[:page], :per_page => 10) js_env({ :PERMISSIONS => { @@ -104,7 +104,7 @@ class OutcomesController < ApplicationController codes = @context.all_courses.pluck(:id).map{|id| "course_#{id}"} end end - @results = @outcome.learning_outcome_results.for_context_codes(codes).custom_ordering(params[:sort]) + @results = @outcome.learning_outcome_results.active.for_context_codes(codes).custom_ordering(params[:sort]) render :json => Api.paginate(@results, self, polymorphic_url([@context, :outcome_results])) end @@ -125,7 +125,7 @@ class OutcomesController < ApplicationController else @outcomes = @context.available_outcomes end - @results = LearningOutcomeResult.for_user(@user).for_outcome_ids(@outcomes.map(&:id)) #.for_context_codes(@codes) + @results = LearningOutcomeResult.active.for_user(@user).for_outcome_ids(@outcomes.map(&:id)) #.for_context_codes(@codes) @results_for_outcome = @results.group_by(&:learning_outcome_id) @google_analytics_page_title = t("Outcomes for Student") @@ -199,7 +199,7 @@ class OutcomesController < ApplicationController return unless authorized_action(@context, @current_user, :manage_outcomes) @outcome = @context.linked_learning_outcomes.find(params[:outcome_id]) - @result = @outcome.learning_outcome_results.find(params[:id]) + @result = @outcome.learning_outcome_results.active.find(params[:id]) return unless authorized_action(@result.context, @current_user, :manage_outcomes) diff --git a/app/graphql/types/learning_outcome_type.rb b/app/graphql/types/learning_outcome_type.rb index b9b2b812256..874fc329910 100644 --- a/app/graphql/types/learning_outcome_type.rb +++ b/app/graphql/types/learning_outcome_type.rb @@ -22,7 +22,7 @@ module Types class LearningOutcomeType < ApplicationObjectType class AssessedLoader < GraphQL::Batch::Loader def perform(outcomes) - assessed_ids = LearningOutcomeResult.where(learning_outcome_id: outcomes).distinct.pluck(:learning_outcome_id) + assessed_ids = LearningOutcomeResult.active.where(learning_outcome_id: outcomes).distinct.pluck(:learning_outcome_id) outcomes.each do |outcome| fulfill(outcome, assessed_ids.include?(outcome.id)) end diff --git a/app/models/learning_outcome.rb b/app/models/learning_outcome.rb index 10f2eb6c27c..0b8df1ff9fa 100644 --- a/app/models/learning_outcome.rb +++ b/app/models/learning_outcome.rb @@ -317,12 +317,12 @@ class LearningOutcome < ActiveRecord::Base def assessed?(course = nil) if course - self.learning_outcome_results.where(context_id: course, context_type: "Course").exists? + self.learning_outcome_results.active.where(context_id: course, context_type: "Course").exists? else - if learning_outcome_results.loaded? - learning_outcome_results.any? + if learning_outcome_results.active.loaded? + learning_outcome_results.active.any? else - learning_outcome_results.exists? + learning_outcome_results.active.exists? end end end @@ -353,7 +353,7 @@ class LearningOutcome < ActiveRecord::Base codes = @tied_context.all_courses.select(:id).map(&:asset_string) end end - self.learning_outcome_results.for_context_codes(codes).count + self.learning_outcome_results.active.for_context_codes(codes).count end def self.delete_if_unused(ids) @@ -380,6 +380,7 @@ class LearningOutcome < ActiveRecord::Base lambda do |user| joins(:learning_outcome_results) .where("learning_outcomes.id=learning_outcome_results.learning_outcome_id " \ + "AND learning_outcome_results.workflow_state <> 'deleted' " \ "AND learning_outcome_results.user_id=?", user) .order(best_unicode_collation_key('short_description')) end diff --git a/app/models/learning_outcome_result.rb b/app/models/learning_outcome_result.rb index 0c4f3d1ff1b..612a2b6e531 100644 --- a/app/models/learning_outcome_result.rb +++ b/app/models/learning_outcome_result.rb @@ -19,6 +19,8 @@ # class LearningOutcomeResult < ActiveRecord::Base + include Canvas::SoftDeletable + belongs_to :user belongs_to :learning_outcome belongs_to :alignment, :class_name => 'ContentTag', :foreign_key => :content_tag_id @@ -135,7 +137,7 @@ class LearningOutcomeResult < ActiveRecord::Base scope :for_outcome_ids, lambda { |ids| where(:learning_outcome_id => ids) } scope :for_association, lambda { |association| where(:association_type => association.class.to_s, :association_id => association.id) } scope :for_associated_asset, lambda { |associated_asset| where(:associated_asset_type => associated_asset.class.to_s, :associated_asset_id => associated_asset.id) } - scope :active, lambda { where("content_tags.workflow_state <> 'deleted'").joins(:alignment) } + scope :with_active_link, lambda { where("content_tags.workflow_state <> 'deleted'").joins(:alignment) } # rubocop:disable Metrics/LineLength scope :exclude_muted_associations, -> { joins("LEFT JOIN #{RubricAssociation.quoted_table_name} rassoc ON rassoc.id = learning_outcome_results.association_id AND learning_outcome_results.association_type = 'RubricAssociation'"). diff --git a/app/models/live_assessments/submission.rb b/app/models/live_assessments/submission.rb index 575cbb7066f..09e7f4da1c5 100644 --- a/app/models/live_assessments/submission.rb +++ b/app/models/live_assessments/submission.rb @@ -31,6 +31,7 @@ module LiveAssessments return if possible == 0 outcome_result = alignment.learning_outcome_results.where(user_id: user.id).first_or_initialize + outcome_result.workflow_state = :active outcome_result.title = "#{user.name}, #{assessment.title}" outcome_result.context = assessment.context outcome_result.associated_asset = assessment diff --git a/app/models/quizzes/quiz_outcome_result_builder.rb b/app/models/quizzes/quiz_outcome_result_builder.rb index b314c4447a9..bc6f981164f 100644 --- a/app/models/quizzes/quiz_outcome_result_builder.rb +++ b/app/models/quizzes/quiz_outcome_result_builder.rb @@ -45,6 +45,7 @@ module Quizzes where(user_id: @qs.user.id). first_or_initialize + quiz_result.workflow_state = :active quiz_result.user_uuid = @qs.user.uuid # get data from quiz submission's question result to ensure result should be generated @@ -96,6 +97,7 @@ module Quizzes where(user_id: @qs.user.id). first_or_initialize + result.workflow_state = :active result.user_uuid = @qs.user.uuid result.artifact = @qs diff --git a/app/models/rubric_assessment.rb b/app/models/rubric_assessment.rb index 2a7764aedd8..c7821bf562c 100644 --- a/app/models/rubric_assessment.rb +++ b/app/models/rubric_assessment.rb @@ -83,6 +83,7 @@ class RubricAssessment < ActiveRecord::Base where(user_id: user.id). first_or_initialize + result.workflow_state = :active result.user_uuid = user.uuid # force the context and artifact diff --git a/app/models/user.rb b/app/models/user.rb index 3d2a635f0b2..de1524e6d29 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2450,7 +2450,7 @@ class User < ActiveRecord::Base end def last_mastered_assignment - self.learning_outcome_results.sort_by{|r| r.assessed_at || r.created_at }.select{|r| r.mastery? }.map{|r| r.assignment }.last + self.learning_outcome_results.active.sort_by{|r| r.assessed_at || r.created_at }.select{|r| r.mastery? }.map{|r| r.assignment }.last end def profile_pics_folder diff --git a/db/migrate/20210311224137_add_workflow_state_to_learning_outcome_result.rb b/db/migrate/20210311224137_add_workflow_state_to_learning_outcome_result.rb new file mode 100644 index 00000000000..ddd6b14397a --- /dev/null +++ b/db/migrate/20210311224137_add_workflow_state_to_learning_outcome_result.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +# +# Copyright (C) 2021 - present Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . + +class AddWorkflowStateToLearningOutcomeResult < ActiveRecord::Migration[6.0] + tag :predeploy + + disable_ddl_transaction! + + def up + if (connection.postgresql_version >= 110000) + add_column :learning_outcome_results, :workflow_state, :string, default: 'active', null: false, if_not_exists: true + else + add_column :learning_outcome_results, :workflow_state, :string, if_not_exists: true + change_column_default :learning_outcome_results, :workflow_state, 'active' + DataFixup::BackfillNulls.run(LearningOutcomeResult, :workflow_state, default_value: 'active') + change_column_null :learning_outcome_results, :workflow_state, false + end + end + + def down + remove_column :learning_outcome_results, :workflow_state + end +end diff --git a/gems/plugins/account_reports/lib/account_reports/outcome_reports.rb b/gems/plugins/account_reports/lib/account_reports/outcome_reports.rb index 4d07eb07834..2fd3679cd25 100644 --- a/gems/plugins/account_reports/lib/account_reports/outcome_reports.rb +++ b/gems/plugins/account_reports/lib/account_reports/outcome_reports.rb @@ -177,7 +177,7 @@ module AccountReports AND sub.user_id = pseudonyms.user_id AND sub.workflow_state <> 'deleted' AND sub.workflow_state <> 'unsubmitted'", parameters])). where("ct.tag_type = 'learning_outcome' AND ct.workflow_state <> 'deleted' - AND (r.id IS NULL OR (r.artifact_type IS NOT NULL AND r.artifact_type <> 'Submission'))") + AND (r.id IS NULL OR (r.workflow_state <> 'deleted' AND r.artifact_type IS NOT NULL AND r.artifact_type <> 'Submission'))") unless @include_deleted students = students.where("pseudonyms.workflow_state<>'deleted' AND c.workflow_state IN ('available', 'completed')") @@ -249,7 +249,7 @@ module AccountReports LEFT OUTER JOIN #{AssessmentQuestion.quoted_table_name} aq ON aq.id = qr.associated_asset_id AND qr.associated_asset_type = 'AssessmentQuestion' JOINS - where("ct.workflow_state <> 'deleted' AND r.artifact_type <> 'Submission'") + where("ct.workflow_state <> 'deleted' AND r.workflow_state <> 'deleted' AND r.artifact_type <> 'Submission'") unless @include_deleted students = students.where("p.workflow_state<>'deleted' AND c.workflow_state IN ('available', 'completed')") diff --git a/lib/api/v1/outcome.rb b/lib/api/v1/outcome.rb index 47151c1dd2e..379093728d3 100644 --- a/lib/api/v1/outcome.rb +++ b/lib/api/v1/outcome.rb @@ -27,7 +27,7 @@ module Api::V1::Outcome # context id and type, and description. def outcomes_json(outcomes, user, session, opts = {}) outcome_ids = outcomes.map(&:id) - opts[:assessed_outcomes] = LearningOutcomeResult.distinct.where(learning_outcome_id: outcome_ids).pluck(:learning_outcome_id) + opts[:assessed_outcomes] = LearningOutcomeResult.active.distinct.where(learning_outcome_id: outcome_ids).pluck(:learning_outcome_id) outcomes.map { |o| outcome_json(o, user, session, opts) } end @@ -128,7 +128,7 @@ module Api::V1::Outcome # # Assumption: All of the outcome links have the same context. # - opts[:assessed_outcomes] = LearningOutcomeResult.distinct.where( + opts[:assessed_outcomes] = LearningOutcomeResult.active.distinct.where( context_type: outcome_links.first.context_type, context_id: outcome_links.map(&:context_id), learning_outcome_id: outcome_links.map(&:content_id) diff --git a/lib/api/v1/outcome_results.rb b/lib/api/v1/outcome_results.rb index 614693542ad..4a3c354a2c7 100644 --- a/lib/api/v1/outcome_results.rb +++ b/lib/api/v1/outcome_results.rb @@ -73,7 +73,7 @@ module Api::V1::OutcomeResults end assessed_outcomes = [] outcomes.map(&:id).each_slice(100) do |outcome_ids| - assessed_outcomes += LearningOutcomeResult.distinct.where(learning_outcome_id: outcome_ids).pluck(:learning_outcome_id) + assessed_outcomes += LearningOutcomeResult.active.distinct.where(learning_outcome_id: outcome_ids).pluck(:learning_outcome_id) end outcomes.map do |o| hash = outcome_json( diff --git a/lib/canvas/live_events.rb b/lib/canvas/live_events.rb index 2c9c7ee7291..3afe58514e0 100644 --- a/lib/canvas/live_events.rb +++ b/lib/canvas/live_events.rb @@ -768,7 +768,8 @@ module Canvas::LiveEvents original_mastery: result.original_mastery, assessed_at: result.assessed_at, title: result.title, - percent: result.percent + percent: result.percent, + workflow_state: result.workflow_state } end diff --git a/lib/outcomes/result_analytics.rb b/lib/outcomes/result_analytics.rb index 3b1d3e714dc..8c82d7c7cc1 100644 --- a/lib/outcomes/result_analytics.rb +++ b/lib/outcomes/result_analytics.rb @@ -38,7 +38,7 @@ module Outcomes required_opts = [:users, :context, :outcomes] required_opts.each { |p| raise "#{p} option is required" unless opts[p] } users, context, outcomes = opts.values_at(*required_opts) - results = LearningOutcomeResult.active.where( + results = LearningOutcomeResult.active.with_active_link.where( context_code: context.asset_string, user_id: users.map(&:id), learning_outcome_id: outcomes.map(&:id), diff --git a/spec/apis/v1/outcome_groups_api_spec.rb b/spec/apis/v1/outcome_groups_api_spec.rb index ece1198424d..0fa5d1dbd77 100644 --- a/spec/apis/v1/outcome_groups_api_spec.rb +++ b/spec/apis/v1/outcome_groups_api_spec.rb @@ -346,35 +346,72 @@ describe "Outcome Groups API", type: :request do ) end - it "outcome is assessed" do - course_with_teacher(active_all: true) - student_in_course(context: @course) - @course.root_outcome_group.add_outcome(@outcome) - expect(@outcome).not_to be_assessed(@course) + context "outcome is assessed" do + before do + course_with_teacher(active_all: true) + student_in_course(context: @course) + @course.root_outcome_group.add_outcome(@outcome) - course_with_teacher(active_all: true) - student_in_course(context: @course) - @course.root_outcome_group.add_outcome(@outcome) - assess_with(@outcome, @course, @student) - expect(@outcome).to be_assessed(@course) + course_with_teacher(active_all: true) + student_in_course(context: @course) + @course.root_outcome_group.add_outcome(@outcome) + assess_with(@outcome, @course, @student) + end - json = api_call(:get, "/api/v1/accounts/#{@account.id}/outcome_group_links", - controller: 'outcome_groups_api', - action: 'link_index', - account_id: @account.id, - format: 'json') + it 'shows outcome assessed' do + json = api_call(:get, "/api/v1/courses/#{@course.id}/outcome_group_links", + controller: 'outcome_groups_api', + action: 'link_index', + course_id: @course.id, + format: 'json') - check_outcome.call(json.last["outcome"], false) + check_outcome.call(json.last["outcome"], false) + check_outcome_link.call( + json.last.tap{|j| j.delete("outcome")}, + @course, + @course.root_outcome_group, + true, # assessed + false, + false + ) + end - # Account context should never be assessed - check_outcome_link.call( - json.last.tap{ |j| j.delete("outcome") }, - @account, - @account.root_outcome_group, - false, - false, - false - ) + it 'shows outcome unassessed when assessment deleted' do + @outcome.learning_outcome_results.where(user: @student).last.destroy! + json = api_call(:get, "/api/v1/courses/#{@course.id}/outcome_group_links", + controller: 'outcome_groups_api', + action: 'link_index', + course_id: @course.id, + format: 'json') + + check_outcome.call(json.last["outcome"], false) + check_outcome_link.call( + json.last.tap{|j| j.delete("outcome")}, + @course, + @course.root_outcome_group, + false, # assessed + false, + false + ) + end + + it 'shows outcome unassessed at account level' do + # Account context should never be assessed + json = api_call(:get, "/api/v1/accounts/#{@account.id}/outcome_group_links", + controller: 'outcome_groups_api', + action: 'link_index', + account_id: @account.id, + format: 'json') + + check_outcome_link.call( + json.last.tap{ |j| j.delete("outcome") }, + @account, + @account.root_outcome_group, + false, # assessed + false, + false + ) + end end end diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index cc234468545..4ca9154d126 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -2823,13 +2823,13 @@ describe CoursesController do test_student = @course.student_view_student session[:become_user_id] = test_student.id rubric_assessment_model(rubric_association: @rubric_association, user: test_student) - expect(test_student.learning_outcome_results.size).not_to be_zero + expect(test_student.learning_outcome_results.active.size).not_to be_zero expect(@outcome.assessed?).to be_truthy delete 'reset_test_student', params: {course_id: @course.id} test_student.reload - expect(test_student.learning_outcome_results.size).to be_zero + expect(test_student.learning_outcome_results.active.size).to be_zero expect(@outcome.assessed?).to be_falsey end end diff --git a/spec/controllers/outcomes_controller_spec.rb b/spec/controllers/outcomes_controller_spec.rb index ad90289ca36..7b04eca5c54 100644 --- a/spec/controllers/outcomes_controller_spec.rb +++ b/spec/controllers/outcomes_controller_spec.rb @@ -207,6 +207,39 @@ describe OutcomesController do expect(response).to be_successful expect(response).to render_template('user_outcome_results') end + + context 'deleted results' do + before(:once) do + account_outcome + assessment_question_bank_with_questions + @outcome.align(@bank, @bank.context, :mastery_score => 0.7) + + @quiz = @course.quizzes.create!(:title => "a quiz") + @quiz.add_assessment_questions [ @q1, @q2 ] + + @submission = @quiz.generate_submission @student + @submission.quiz_data = @quiz.generate_quiz_data + @submission.mark_completed + Quizzes::SubmissionGrader.new(@submission).grade_submission + end + + it 'should return existing results' do + user_session(@admin) + + get 'user_outcome_results', params: {account_id: @account.id, user_id: @student.id} + expect(response).to be_successful + expect(assigns[:results]).not_to be_empty + end + + it 'should not return deleted results' do + user_session(@admin) + LearningOutcomeResult.find_by!(artifact: @submission, user: @student).destroy + + get 'user_outcome_results', params: {account_id: @account.id, user_id: @student.id} + expect(response).to be_successful + expect(assigns[:results]).to be_empty + end + end end describe "GET 'list'" do @@ -386,6 +419,16 @@ describe OutcomesController do assert_unauthorized end + it "should not return deleted results" do + @outcome.learning_outcome_results.last.destroy + user_session(@teacher) + get 'outcome_result', + params: {:course_id => @course.id, + :outcome_id => @outcome.id, + :id => @outcome.learning_outcome_results.last} + expect(response).to be_not_found + end + it "should redirect to show quiz when result is a quiz" do user_session(@teacher) get 'outcome_result', diff --git a/spec/controllers/rubric_assessments_controller_spec.rb b/spec/controllers/rubric_assessments_controller_spec.rb index 310be242fbf..9ee9d016a49 100644 --- a/spec/controllers/rubric_assessments_controller_spec.rb +++ b/spec/controllers/rubric_assessments_controller_spec.rb @@ -365,7 +365,7 @@ describe RubricAssessmentsController do :id => @rubric_assessment.id } expect(response).to be_successful - expect(LearningOutcomeResult.find_by(id: result.id)).to be_nil + expect(LearningOutcomeResult.active.find_by(id: result.id)).to be_nil end end diff --git a/spec/graphql/types/learning_outcome_type_spec.rb b/spec/graphql/types/learning_outcome_type_spec.rb index e76438430d9..a15b323448e 100644 --- a/spec/graphql/types/learning_outcome_type_spec.rb +++ b/spec/graphql/types/learning_outcome_type_spec.rb @@ -78,5 +78,11 @@ describe Types::LearningOutcomeType do rubric_assessment_model(rubric: @rubric, user: @student) expect(outcome_type.resolve("assessed")).to eq true end + + it "returns false when assessment deleted" do + assessment = rubric_assessment_model(rubric: @rubric, user: @student) + assessment.learning_outcome_results.destroy_all + expect(outcome_type.resolve("assessed")).to eq false + end end end diff --git a/spec/lib/canvas/live_events_spec.rb b/spec/lib/canvas/live_events_spec.rb index ab25cb6fcd6..f83df05b7ff 100644 --- a/spec/lib/canvas/live_events_spec.rb +++ b/spec/lib/canvas/live_events_spec.rb @@ -1612,7 +1612,8 @@ describe Canvas::LiveEvents do original_mastery: result.original_mastery, assessed_at: result.assessed_at, title: result.title, - percent: result.percent + percent: result.percent, + workflow_state: result.workflow_state }.compact!).once Canvas::LiveEvents.learning_outcome_result_created(result) @@ -1635,7 +1636,8 @@ describe Canvas::LiveEvents do original_mastery: result.original_mastery, assessed_at: result.assessed_at, title: result.title, - percent: result.percent + percent: result.percent, + workflow_state: result.workflow_state }.compact!).once Canvas::LiveEvents.learning_outcome_result_updated(result) diff --git a/spec/lib/outcomes/result_analytics_spec.rb b/spec/lib/outcomes/result_analytics_spec.rb index c0e2b668897..a58eea3c6d8 100644 --- a/spec/lib/outcomes/result_analytics_spec.rb +++ b/spec/lib/outcomes/result_analytics_spec.rb @@ -108,6 +108,12 @@ describe Outcomes::ResultAnalytics do expect(results.length).to eq 2 end + it 'does return deleted results' do + LearningOutcomeResult.last.destroy + results = ra.find_outcome_results(@student, users: [@student], context: @course, outcomes: [@outcome]) + expect(results.length).to eq 1 + end + it 'does return muted assignment results with auto post policy' do @assignment.mute! @assignment.ensure_post_policy(post_manually: false) diff --git a/spec/models/learning_outcome_result_spec.rb b/spec/models/learning_outcome_result_spec.rb index 67de83ee0d3..cdeb17a4328 100644 --- a/spec/models/learning_outcome_result_spec.rb +++ b/spec/models/learning_outcome_result_spec.rb @@ -59,6 +59,12 @@ describe LearningOutcomeResult do end end + it_behaves_like 'soft deletion' do + let(:first) { learning_outcome_result } + let(:second) { create_and_associate_lor(course.assignments.create!) } + subject { LearningOutcomeResult.all } + end + describe '#submitted_or_assessed_at' do let_once(:submitted_at) { 1.month.ago } let_once(:assessed_at) { 2.weeks.ago } @@ -95,10 +101,10 @@ describe LearningOutcomeResult do context 'assignment with unposted submissions with default posting policy' do it 'includes assignment result' do - # By default, an automatic post policy (post_manually: false) is associated to + # By default, an automatic post policy (post_manually: false) is associated to # an assignment. Now that post policy is included in exclude_muted_associations # the outcome result will appear in LMGB/SLMGB. It will not appear for manual - # post policy assignment until the submission is posted. See "manual posting + # post policy assignment until the submission is posted. See "manual posting # policy" test cases below. expect(outcome_result_scope.exclude_muted_associations.count).to eq 1 end @@ -161,12 +167,12 @@ describe LearningOutcomeResult do end end - describe "active scope" do + describe "with_active_link scope" do it "doesn't return deleted outcomes" do - expect(LearningOutcomeResult.active.count).to eq(1), "precondition" + expect(LearningOutcomeResult.with_active_link.count).to eq(1), "precondition" learning_outcome_result.alignment.workflow_state = 'deleted' learning_outcome_result.alignment.save! - expect(LearningOutcomeResult.active.count).to eq 0 + expect(LearningOutcomeResult.with_active_link.count).to eq 0 end end diff --git a/spec/models/learning_outcome_spec.rb b/spec/models/learning_outcome_spec.rb index 7604a8cbf08..02232807d71 100644 --- a/spec/models/learning_outcome_spec.rb +++ b/spec/models/learning_outcome_spec.rb @@ -1052,21 +1052,30 @@ describe LearningOutcome do }) result.reload rubric.reload - { assignment: assignment, assessment: assessment, rubric: rubric } + { assignment: assignment, assessment: assessment, rubric: rubric, result: result } end end context "learning outcome results" do - it "properly reports whether assessed in a course" do + before do add_student.call(c1, c2) add_or_get_rubric(outcome) [c1, c2].each { |c| outcome.align(nil, c, :mastery_type => "points") } - assess_with.call(outcome, c1) + @result = assess_with.call(outcome, c1)[:result] + end + it "properly reports whether assessed in a course" do expect(outcome).to be_assessed expect(outcome).to be_assessed(c1) expect(outcome).not_to be_assessed(c2) end + + it 'does not include deleted results when testing assessed' do + @result.destroy + expect(outcome).not_to be_assessed + expect(outcome).not_to be_assessed(c1) + expect(outcome).not_to be_assessed(c2) + end end describe '#ensure_presence_in_context' do diff --git a/spec/models/live_assessments/submission_spec.rb b/spec/models/live_assessments/submission_spec.rb index 7cc7303ca90..c78ec2db111 100644 --- a/spec/models/live_assessments/submission_spec.rb +++ b/spec/models/live_assessments/submission_spec.rb @@ -68,6 +68,16 @@ describe LiveAssessments::Submission do expect(result.reload.percent).to eq 0.8 end + it 'restores a deleted outcome result' do + submission.create_outcome_result(alignment) + result = alignment.learning_outcome_results.first + result.destroy + submission.score = 80 + submission.possible = 100 + submission.create_outcome_result(alignment) + expect(result.reload).to be_active + end + it "scales the score to the outcome rubric criterion if present" do outcome.data = {rubric_criterion: {mastery_points: 3, points_possible: 5}} outcome.save! diff --git a/spec/models/quizzes/quiz_outcome_result_builder_spec.rb b/spec/models/quizzes/quiz_outcome_result_builder_spec.rb index f2440272603..82150622183 100644 --- a/spec/models/quizzes/quiz_outcome_result_builder_spec.rb +++ b/spec/models/quizzes/quiz_outcome_result_builder_spec.rb @@ -64,7 +64,7 @@ describe Quizzes::QuizOutcomeResultBuilder do answer_a_question(@q2, @sub, correct: false) Quizzes::SubmissionGrader.new(@sub).grade_submission @outcome.reload - @quiz_results = @outcome.learning_outcome_results.where(user_id: @user).to_a + @quiz_results = @outcome.learning_outcome_results.active.where(user_id: @user).to_a @quiz_result = @quiz_results.first @question_results = @quiz_results.first.learning_outcome_question_results end @@ -181,7 +181,7 @@ describe Quizzes::QuizOutcomeResultBuilder do Quizzes::SubmissionGrader.new(@sub).grade_submission expect(@sub.score).to eql(1.0) @outcome.reload - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results expect(@results.length).to eql(2) @results = @results.sort_by(&:associated_asset_id) @@ -203,7 +203,7 @@ describe Quizzes::QuizOutcomeResultBuilder do Quizzes::SubmissionGrader.new(@sub).grade_submission expect(@sub.score).to eql(1.0) @outcome.reload - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results.sort_by(&:associated_asset_id) updated_at_times = @results.map(&:updated_at) expect(@results.length).to eql(2) @@ -219,7 +219,7 @@ describe Quizzes::QuizOutcomeResultBuilder do Quizzes::SubmissionGrader.new(@sub).grade_submission expect(@sub.score).to eql(1.0) @outcome.reload - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results.sort_by(&:associated_asset_id) expect(@results.length).to eql(2) expect(updated_at_times).not_to eql(@results.map(&:updated_at)) @@ -243,7 +243,7 @@ describe Quizzes::QuizOutcomeResultBuilder do Quizzes::SubmissionGrader.new(@sub).grade_submission expect(@sub.score).to eql(1.0) @outcome.reload - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results.sort_by(&:associated_asset_id) expect(@results.length).to eql(2) updated_at_times = @results.map(&:updated_at) @@ -259,7 +259,7 @@ describe Quizzes::QuizOutcomeResultBuilder do Quizzes::SubmissionGrader.new(@sub).grade_submission expect(@sub.score).to eql(1.0) @outcome.reload - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results.sort_by(&:associated_asset_id) expect(updated_at_times).to eql(@results.map(&:updated_at)) expect(@results.first.mastery).to eql(true) @@ -281,7 +281,7 @@ describe Quizzes::QuizOutcomeResultBuilder do end it "creates an outcome result even if the total score doesn't increase after grading an essay question" do - expect(@outcome.learning_outcome_results.where(user_id: @user).count).to equal 0 + expect(@outcome.learning_outcome_results.active.where(user_id: @user).count).to equal 0 @sub.update_scores({ 'context_id' => @course.id, 'override_scores' => true, @@ -289,7 +289,7 @@ describe Quizzes::QuizOutcomeResultBuilder do 'submission_version_number' => '1', "question_score_#{@q2.id}" => "0" }) - expect(@outcome.learning_outcome_results.where(user_id: @user).count).to equal 1 + expect(@outcome.learning_outcome_results.active.where(user_id: @user).count).to equal 1 end end @@ -314,7 +314,7 @@ describe Quizzes::QuizOutcomeResultBuilder do 'submission_version_number' => '1', "question_score_#{@q1.id}" => "1", }) - expect(@outcome.learning_outcome_results.where(user_id: @user).count).to equal 0 + expect(@outcome.learning_outcome_results.active.where(user_id: @user).count).to equal 0 @sub.update_scores({ 'context_id' => @course.id, 'override_scores' => true, @@ -322,7 +322,7 @@ describe Quizzes::QuizOutcomeResultBuilder do 'submission_version_number' => '1', "question_score_#{@q2.id}" => "1" }) - results = @outcome.learning_outcome_results.where(user_id: @user) + results = @outcome.learning_outcome_results.active.where(user_id: @user) expect(results.count).to equal 1 expect(results[0].score).to equal 2.0 @sub.update_scores({ @@ -351,21 +351,21 @@ describe Quizzes::QuizOutcomeResultBuilder do @quiz.update_attribute('quiz_type', 'survey') Quizzes::SubmissionGrader.new(@sub).grade_submission @outcome.reload - expect(@outcome.learning_outcome_results.where(user_id: @user).length).to eql(0) + expect(@outcome.learning_outcome_results.active.where(user_id: @user).length).to eql(0) end it "should not create learning outcome results for a graded survey" do @quiz.update_attribute('quiz_type', 'graded_survey') Quizzes::SubmissionGrader.new(@sub).grade_submission @outcome.reload - expect(@outcome.learning_outcome_results.where(user_id: @user).length).to eql(0) + expect(@outcome.learning_outcome_results.active.where(user_id: @user).length).to eql(0) end it "should not create learning outcome results for a practice quiz" do @quiz.update_attribute('quiz_type', 'practice_quiz') Quizzes::SubmissionGrader.new(@sub).grade_submission @outcome.reload - expect(@outcome.learning_outcome_results.where(user_id: @user).length).to eql(0) + expect(@outcome.learning_outcome_results.active.where(user_id: @user).length).to eql(0) end end @@ -386,7 +386,7 @@ describe Quizzes::QuizOutcomeResultBuilder do answer_a_question(@q1, @sub) answer_a_question(@q2, @sub) Quizzes::SubmissionGrader.new(@sub).grade_submission - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results expect(@results.length).to eql(1) @results = @results.sort_by(&:associated_asset_id) @@ -399,7 +399,7 @@ describe Quizzes::QuizOutcomeResultBuilder do answer_a_question(@q1, @sub) answer_a_question(@q2, @sub) Quizzes::SubmissionGrader.new(@sub).grade_submission - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results expect(@results.length).to eql(2) q2_data = @q2.question_data @@ -411,7 +411,7 @@ describe Quizzes::QuizOutcomeResultBuilder do answer_a_question(@q1, @sub) answer_a_question(@q2, @sub) Quizzes::SubmissionGrader.new(@sub).grade_submission - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first @results = @quiz_result.learning_outcome_question_results expect(@results.length).to eql(1) end @@ -437,7 +437,7 @@ describe Quizzes::QuizOutcomeResultBuilder do answer_a_question(@q2, @sub) Quizzes::SubmissionGrader.new(@sub).grade_submission @outcome.reload - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first expect(@quiz_result).to be_nil end @@ -448,7 +448,7 @@ describe Quizzes::QuizOutcomeResultBuilder do answer_a_question(@q1, @sub) answer_a_question(@q2, @sub) Quizzes::SubmissionGrader.new(@sub).grade_submission - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first expect(@quiz_result).to be_present q1_data = @q1.question_data q1_data[:points_possible] = 0.0 @@ -462,7 +462,7 @@ describe Quizzes::QuizOutcomeResultBuilder do answer_a_question(@q1, @sub) answer_a_question(@q2, @sub) Quizzes::SubmissionGrader.new(@sub).grade_submission - @quiz_result = @outcome.learning_outcome_results.where(user_id: @user).first + @quiz_result = @outcome.learning_outcome_results.active.where(user_id: @user).first expect(@quiz_result).to be_nil end end diff --git a/spec/models/rubric_assessment_spec.rb b/spec/models/rubric_assessment_spec.rb index 149b7c37ddf..c7c796b4543 100644 --- a/spec/models/rubric_assessment_spec.rb +++ b/spec/models/rubric_assessment_spec.rb @@ -373,6 +373,36 @@ describe RubricAssessment do expect(LearningOutcomeResult.last.hidden).to be true end + it "restores a deleted result" do + criterion_id = "criterion_#{@rubric.data[0][:id]}".to_sym + assessment = @association.assess({ + :user => @student, + :assessor => @teacher, + :artifact => @assignment.find_or_create_submission(@student), + :assessment => { + :assessment_type => 'grading', + criterion_id => { + :points => "3" + } + } + }) + result = LearningOutcomeResult.last + result.destroy + + assessment = @association.assess({ + :user => @student, + :assessor => @teacher, + :artifact => @assignment.find_or_create_submission(@student), + :assessment => { + :assessment_type => 'grading', + criterion_id => { + :points => "3" + } + } + }) + expect(result.reload).to be_active + end + it "does not update outcomes on a peer assessment" do criterion_id = "criterion_#{@rubric.data[0][:id]}".to_sym expect do