From a01bac97cbb0fd5c981c12d54a4f78360c062adf Mon Sep 17 00:00:00 2001 From: Spencer Olson Date: Mon, 31 Jul 2023 15:31:05 -0500 Subject: [PATCH] allow setting a custom status on a submission closes EVAL-3274 flag=custom_gradebook_statuses Test Plan: 1. Enable the "Custom Status Labels for Submissions and Final Grades" feature flag. 2. Create a few custom statuses for a root account. 3. As a teacher, go to the Gradebook, click on a submission cell, and open the Gradebook tray. Verify you can mark the student with a standard or a custom status, and that the status is still present after a page reload. Make sure to test that you can change the status from standard -> custom (e.g. "Missing" -> "My Custom Status") and from custom -> standard (e.g. "My Custom Status" -> "Missing"). Change-Id: Id648ee7d1511428b36d9aaafd2d8d04819e69fe7 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/323983 Tested-by: Service Cloud Jenkins Reviewed-by: Christopher Soto Reviewed-by: Rohan Chugh QA-Review: Rohan Chugh Product-Review: Sam Garza --- app/controllers/submissions_api_controller.rb | 27 ++++-- app/models/submission.rb | 14 +-- spec/apis/v1/submissions_api_spec.rb | 87 +++++++++++++++++++ spec/models/submission_spec.rb | 60 ++++++++++++- 4 files changed, 176 insertions(+), 12 deletions(-) diff --git a/app/controllers/submissions_api_controller.rb b/app/controllers/submissions_api_controller.rb index 56a6793e127..0cb7cce04ba 100644 --- a/app/controllers/submissions_api_controller.rb +++ b/app/controllers/submissions_api_controller.rb @@ -854,9 +854,12 @@ class SubmissionsApiController < ApplicationController if params[:submission].is_a?(ActionController::Parameters) submission[:grade] = params[:submission].delete(:posted_grade) submission[:excuse] = params[:submission].delete(:excuse) - if params[:submission].key?(:late_policy_status) - submission[:late_policy_status] = params[:submission].delete(:late_policy_status) + [:late_policy_status, :custom_grade_status_id].each do |status_attr| + if params[:submission].key?(status_attr) + submission[status_attr] = params[:submission].delete(status_attr) + end end + if params[:submission].key?(:seconds_late_override) submission[:seconds_late_override] = params[:submission].delete(:seconds_late_override) end @@ -888,8 +891,11 @@ class SubmissionsApiController < ApplicationController @submissions ||= [@submission] end - late_attrs_changed = submission.key?(:late_policy_status) || submission.key?(:seconds_late_override) - if late_attrs_changed || submission.key?(:sticker) + submission_status_changed = + %i[late_policy_status seconds_late_override custom_grade_status_id] + .any? { |status_attr| submission.key?(status_attr) } + + if submission_status_changed || submission.key?(:sticker) excused = Canvas::Plugin.value_to_boolean(submission[:excuse]) grade_group_students = !(@assignment.grade_group_students_individually || excused) @@ -898,13 +904,22 @@ class SubmissionsApiController < ApplicationController @submissions = @assignment.find_or_create_submissions(students, Submission.preload(:grading_period, :stream_item)) end + if submission.key?(:custom_grade_status_id) + custom_status = @context.root_account.custom_grade_statuses.active.find(submission[:custom_grade_status_id]) + end + @submissions.each do |sub| - sub.late_policy_status = submission[:late_policy_status] if submission.key?(:late_policy_status) + if custom_status + sub.custom_grade_status = custom_status + elsif submission.key?(:late_policy_status) + sub.late_policy_status = submission[:late_policy_status] + end + if sub.late_policy_status == "late" && submission[:seconds_late_override].present? sub.seconds_late_override = submission[:seconds_late_override] end sub.sticker = submission[:sticker] if submission.key?(:sticker) - sub.grader = @current_user if late_attrs_changed + sub.grader = @current_user if submission_status_changed # If we've called Assignment#grade_student, it has already created a # new submission version on this request. previously_graded = graded_just_now && (sub.grade.present? || sub.excused?) diff --git a/app/models/submission.rb b/app/models/submission.rb index ca8e67ca6c1..e92b14e2e58 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -412,7 +412,7 @@ class Submission < ActiveRecord::Base # validation. Otherwise if we place it in any earlier (e.g. # before/after_initialize), every Submission.new will make database calls. before_validation :set_anonymous_id, if: :new_record? - before_save :set_late_policy_attributes + before_save :set_status_attributes before_save :apply_late_policy, if: :late_policy_relevant_changes? before_save :update_if_pending before_save :validate_single_submission, :infer_values @@ -3080,15 +3080,19 @@ class Submission < ActiveRecord::Base self.sticker = nil end - def set_late_policy_attributes - self.seconds_late_override = nil unless late_policy_status == "late" - + def set_status_attributes if will_save_change_to_excused?(to: true) self.late_policy_status = nil - self.seconds_late_override = nil + self.custom_grade_status_id = nil + elsif will_save_change_to_custom_grade_status_id? && custom_grade_status_id.present? + self.excused = false + self.late_policy_status = nil elsif will_save_change_to_late_policy_status? && late_policy_status.present? self.excused = false + self.custom_grade_status_id = nil end + + self.seconds_late_override = nil unless late_policy_status == "late" end def reset_redo_request diff --git a/spec/apis/v1/submissions_api_spec.rb b/spec/apis/v1/submissions_api_spec.rb index e69006e6dcc..246581ce215 100644 --- a/spec/apis/v1/submissions_api_spec.rb +++ b/spec/apis/v1/submissions_api_spec.rb @@ -3739,6 +3739,12 @@ describe "Submissions API", type: :request do context "group assignments" do before do + @admin = account_admin_user(account: @course.root_account) + @custom_status = @course.root_account.custom_grade_statuses.create!( + color: "#BBB", + created_by: @admin, + name: "yolo" + ) @student2 = @course.enroll_student(User.create!, enrollment_state: :active).user group_category = @course.group_categories.create!(name: "Category") group = @course.groups.create!(name: "Group", group_category:) @@ -3769,6 +3775,55 @@ describe "Submissions API", type: :request do expect(submission.late_policy_status).to eq "missing" end + it "can set a custom status on a group member's submission" do + api_call( + :put, + "/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}/submissions/#{@student.id}.json", + { + controller: "submissions_api", + action: "update", + format: "json", + course_id: @course.id.to_s, + assignment_id: @assignment.id.to_s, + user_id: @student.id.to_s + }, + { + submission: { + custom_grade_status_id: @custom_status.id.to_s + } + } + ) + + submission = @assignment.submission_for_student(@student2) + expect(submission.custom_grade_status_id).to eq @custom_status.id + end + + it "does not set a custom status for the whole group when assignment is graded individually" do + @assignment.update!(grade_group_students_individually: true) + api_call( + :put, + "/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}/submissions/#{@student.id}.json", + { + controller: "submissions_api", + action: "update", + format: "json", + course_id: @course.id.to_s, + assignment_id: @assignment.id.to_s, + user_id: @student.id.to_s + }, + { + submission: { + custom_grade_status_id: @custom_status.id.to_s + } + } + ) + + first_submission = @assignment.submission_for_student(@student) + expect(first_submission.custom_grade_status_id).to eq @custom_status.id + second_submission = @assignment.submission_for_student(@student2) + expect(second_submission.custom_grade_status_id).to be_nil + end + it "can set seconds_late_override on a group member's submission" do seconds_late_override = 3.days api_call( @@ -3867,6 +3922,38 @@ describe "Submissions API", type: :request do expect(json["late_policy_status"]).to eq "missing" end + it "can set a custom status on a submission" do + admin = account_admin_user(account: @course.root_account) + custom_status = @course.root_account.custom_grade_statuses.create!( + color: "#BBB", + created_by: admin, + name: "yolo" + ) + submission = @assignment.submission_for_student(@student) + submission.update!(late_policy_status: "missing") + api_call( + :put, + "/api/v1/courses/#{@course.id}/assignments/#{@assignment.id}/submissions/#{@student.id}.json", + { + controller: "submissions_api", + action: "update", + format: "json", + course_id: @course.id.to_s, + assignment_id: @assignment.id.to_s, + user_id: @student.id.to_s + }, + { + submission: { + custom_grade_status_id: custom_status.id.to_s + } + } + ) + + submission.reload + expect(submission.late_policy_status).to be_nil + expect(submission.custom_grade_status_id).to eq custom_status.id + end + it "can set seconds_late_override on a submission along with the late_policy_status of late" do seconds_late_override = 3.days json = api_call( diff --git a/spec/models/submission_spec.rb b/spec/models/submission_spec.rb index b419c29fa9d..3a5549fcc24 100644 --- a/spec/models/submission_spec.rb +++ b/spec/models/submission_spec.rb @@ -766,12 +766,35 @@ describe Submission do expect(submission).not_to be_excused end - it "does not set excused to false if the late_policy_status ie being changed to a nil value" do + it "does not set excused to false if the late_policy_status is being changed to a nil value" do # need to skip callbacks so excused does not get set to false submission.update_column(:late_policy_status, "missing") submission.update!(late_policy_status: nil) expect(submission).to be_excused end + + context "custom statuses" do + before do + admin = account_admin_user(account: @assignment.root_account) + @custom_grade_status = @assignment.root_account.custom_grade_statuses.create!( + color: "#ABC", + name: "yolo", + created_by: admin + ) + end + + it "sets excused to false if the custom_grade_status_id is being changed to a not-nil value" do + submission.update!(custom_grade_status: @custom_grade_status) + expect(submission).not_to be_excused + end + + it "does not set excused to false if the custom_grade_status_id is being changed to a nil value" do + # need to skip callbacks so excused does not get set to false + submission.update_column(:custom_grade_status_id, @custom_grade_status.id) + submission.update!(custom_grade_status_id: nil) + expect(submission).to be_excused + end + end end describe "#late_policy_status" do @@ -804,6 +827,41 @@ describe Submission do submission.update!(excused: false) expect(submission.seconds_late_override).to be 60 end + + context "custom statuses" do + before do + admin = account_admin_user(account: @assignment.root_account) + @custom_grade_status = @assignment.root_account.custom_grade_statuses.create!( + color: "#ABC", + name: "yolo", + created_by: admin + ) + end + + it "sets late_policy_status to nil if the custom_grade_status_id is being changed to a not-nil value" do + submission.update!(custom_grade_status: @custom_grade_status) + expect(submission.late_policy_status).to be_nil + end + + it "sets seconds_late_override to nil if the submission is updated to have a custom status" do + submission.update!(custom_grade_status: @custom_grade_status) + expect(submission.seconds_late_override).to be_nil + end + + it "does not set late_policy_status to nil if the custom_grade_status_id is being changed to a nil value" do + # need to skip callbacks so excused does not get set to false + submission.update_column(:custom_grade_status_id, @custom_grade_status.id) + submission.update!(custom_grade_status_id: nil) + expect(submission.late_policy_status).to eql "late" + end + + it "does not set seconds_late_override to nil if the submission is updated to not have a custom status" do + # need to skip callbacks so seconds_late_override does not get set to nil + submission.update_column(:custom_grade_status_id, @custom_grade_status.id) + submission.update!(custom_grade_status_id: nil) + expect(submission.seconds_late_override).to be 60 + end + end end describe "seconds_late_override" do