From 462242770691ffee4b0063c2b16cd8737ddb48c0 Mon Sep 17 00:00:00 2001 From: Chrystal Langston Date: Wed, 26 Jun 2024 14:28:49 -0400 Subject: [PATCH] Add post policy to update and create course API endpoints Please note that this PS also updates the permissions checked for setting a course's post policy through the Graphql mutation. This is to ensure REST and Graphql permissions are consistent to eliminate confusion. closes OUT-6401 flag=none test plan: - Test plan: - Follow instructions in https://instructure.atlassian.net/wiki/spaces/ENG/pages/40206418/Canvas+API to set up Postman OR log in to your local canvas as an admin - Testing Creating a Course (without post_manually) - Past the below after your canvas domain which will call the create a course API endpoint /api/v1/accounts/{account id}/courses?course[name]=PostPolicyCourseFalse - Take note of the course id returned in the json response - Navigate the course -> grades -> click on the gear icon -> choose "Grade posting policy" and assert the default setting of "Automatically" is set for the course - Create another course using the same API endpoint but add post_manually course parameter with a value of true /api/v1/accounts/{account id}/courses?course[name]=PostPolicyCourseTrue &course[post_manually]=true - Take note of the course id returned in the json response - Navigate the course -> grades -> click on the gear icon -> choose "Grade posting policy" and assert the default setting of "Manually" is set for the course - Testing Updating a Course - Use the course id from the first course created and update the course name to "PostPolicyCourseFalseUpdated" /api/v1/courses/{course id}?course[post_manually]=true - Navigate the course -> grades -> click on the gear icon -> choose "Grade posting policy" and assert the default setting of "Manually" is set for the course - Use the course id from the second course created and update the course post policy to false /api/v1/courses/{course id}?course[post_manually]=false - Navigate the course -> grades -> click on the gear icon -> choose "Grade posting policy" and assert the default setting of "Automatically" is set for the course - Use the course id from the second course created and update the name only to ensure update performs as expected without post_manually parameter /api/v1/courses/{course id}?course[name]=SomethingElse - Navigate to the course and notice the name has been updated - Testing permissions up on graphql endpoint - navigate to Graphql interface on your local instance http://{canvas domain}/graphiql - Run the below mutation on the first course created mutation MyMutation { __typename setCoursePostPolicy(input: {courseId: "{first course created id}", postManually: true}) { postPolicy { _id postManually } } } - Assert that the course post policy has been updated to manually - Log in as a student and perform the same mutation - Assert in the response you receive an error message saying "not found" - Log out as student and log in as an instructor that is not enrolled in the first created course - Perform the same mutation and assert in the response you receive an error message saying "not found" - Log in as the instructor of the course, perform the same mutation and assert the mutation runs successfully Change-Id: I46d35138aacc8d324c6c642550f5dac5f5f0a9b9 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/353158 Tested-by: Service Cloud Jenkins Reviewed-by: Dave Wenzlick QA-Review: Dave Wenzlick Product-Review: Kyle Rosenbaum --- app/controllers/courses_controller.rb | 33 +++++++++++- app/graphql/mutations/base_mutation.rb | 4 ++ .../mutations/set_course_post_policy.rb | 5 +- spec/controllers/courses_controller_spec.rb | 50 +++++++++++++++++++ .../mutations/set_course_post_policy_spec.rb | 39 +++++++++++---- 5 files changed, 118 insertions(+), 13 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 3870c98c93c..183b9992d4a 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -865,6 +865,11 @@ class CoursesController < ApplicationController # @argument course[course_format] [String] # Optional. Specifies the format of the course. (Should be 'on_campus', 'online', or 'blended') # + # @argument course[post_manually] [Boolean] + # Default is false. + # When true, all grades in the course must be posted manually, and will not be automatically posted. + # When false, all grades in the course will be automatically posted. + # # @argument enable_sis_reactivation [Boolean] # When true, will first try to re-activate a deleted course with matching sis_course_id if possible. # @@ -894,6 +899,12 @@ class CoursesController < ApplicationController sis_course_id = params[:course].delete(:sis_course_id) apply_assignment_group_weights = params[:course].delete(:apply_assignment_group_weights) + # params_for_create is used to build the course object + # since post_manually is not an actual attribute of the course object, + # we need to remove it from the params. We will use it later to + # apply the post policy to the course after the course is saved. + post_manually = params_for_create.delete(:post_manually) if params_for_create.key?(:post_manually) + # accept end_at as an alias for conclude_at. continue to accept # conclude_at for legacy support, and return conclude_at only if # the user uses that name. @@ -931,6 +942,7 @@ class CoursesController < ApplicationController @course.account = @sub_account if @sub_account end end + @course ||= (@sub_account || @account).courses.build(params_for_create) if can_manage_sis @@ -971,6 +983,11 @@ class CoursesController < ApplicationController # force the user to refresh the page after the job finishes to see the changes @course.sync_homeroom_participation end + + # Cannot set the course PostPolicy until the course has saved a PostPolicy + # is a polymorphic association + @course.apply_post_policy!(post_manually: value_to_boolean(post_manually)) unless post_manually.nil? + format.html { redirect_to @course } format.json do render json: course_json( @@ -3026,6 +3043,11 @@ class CoursesController < ApplicationController # @argument course[conditional_release] [Boolean] # Enable or disable individual learning paths for students based on assessment # + # @argument course[post_manually] [Boolean] + # When true, all grades in the course will be posted manually. + # When false, all grades in the course will be automatically posted. + # Use with caution as this setting will override any assignment level post policy. + # # @argument override_sis_stickiness [boolean] # Default is true. If false, any fields containing “sticky” changes will not be updated. # See SIS CSV Format documentation for information on which fields can have SIS stickiness @@ -3150,6 +3172,14 @@ class CoursesController < ApplicationController update_grade_passback_setting(grade_passback_setting) end + if params_for_update.key?(:post_manually) + @course.apply_post_policy!(post_manually: value_to_boolean(params_for_update[:post_manually])) + + # attributes in params_for_update will be applied to the course + # since post_manually is not an attribute on the Course model, it needs to be removed + params_for_update.delete :post_manually + end + unless @course.account.grants_right? @current_user, session, :manage_storage_quotas params_for_update.delete :storage_quota params_for_update.delete :storage_quota_mb @@ -4237,7 +4267,8 @@ class CoursesController < ApplicationController :friendly_name, :enable_course_paces, :default_due_time, - :conditional_release + :conditional_release, + :post_manually ) end diff --git a/app/graphql/mutations/base_mutation.rb b/app/graphql/mutations/base_mutation.rb index 9e8f83e5f1e..133d53d97b1 100644 --- a/app/graphql/mutations/base_mutation.rb +++ b/app/graphql/mutations/base_mutation.rb @@ -55,6 +55,10 @@ class Mutations::BaseMutation < GraphQL::Schema::Mutation raise GraphQL::ExecutionError, "not found" unless obj.grants_right?(current_user, session, perm) end + def verify_any_authorized_actions!(obj, perms) + raise GraphQL::ExecutionError, "not found" unless obj.grants_any_right?(current_user, session, *Array(perms)) + end + # TODO: replace this with model validation where applicable def validation_error(message, attribute: "message") { diff --git a/app/graphql/mutations/set_course_post_policy.rb b/app/graphql/mutations/set_course_post_policy.rb index a8b7d12343f..158d4255cf2 100644 --- a/app/graphql/mutations/set_course_post_policy.rb +++ b/app/graphql/mutations/set_course_post_policy.rb @@ -33,7 +33,10 @@ class Mutations::SetCoursePostPolicy < Mutations::BaseMutation raise GraphQL::ExecutionError, "A course with that id does not exist" end - verify_authorized_action!(course, :manage_grades) + # checking if the current user has manage_content or + # manage_course_content_edit permission in the current course + perms = %i[manage_content manage_course_content_edit] + verify_any_authorized_actions!(course, perms) course.apply_post_policy!(post_manually: input[:post_manually]) { post_policy: course.default_post_policy } diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index 5d10bdd2a35..f9f84c875cc 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -2519,6 +2519,38 @@ describe CoursesController do expect(Course.find(json["id"]).grade_passback_setting).to eq "nightly_sync" end + describe "post policy" do + it "sets to true" do + post "create", + params: { + account_id: @account.id, + course: { + name: "new course with post policy set to true", + post_manually: true + } + }, + format: :json + + json = response.parsed_body + expect(Course.find(json["id"]).post_manually?).to be true + end + + it "sets to false" do + post "create", + params: { + account_id: @account.id, + course: { + name: "new course with post policy set to false", + post_manually: false + } + }, + format: :json + + json = response.parsed_body + expect(Course.find(json["id"]).post_manually?).to be false + end + end + it "does not allow visibility to be set when we don't have permission" do @visperm.enabled = false @visperm.save @@ -3017,6 +3049,24 @@ describe CoursesController do expect(@course.account_id).to eq account2.id end + describe "post policy" do + before do + user_session(@teacher) + end + + it "updates to true" do + put "update", params: { id: @course.id, course: { post_manually: true } } + @course.reload + expect(@course.post_manually?).to be true + end + + it "updates to false" do + put "update", params: { id: @course.id, course: { post_manually: false } } + @course.reload + expect(@course.post_manually?).to be false + end + end + describe "touching content when public visibility changes" do before do user_session(@teacher) diff --git a/spec/graphql/mutations/set_course_post_policy_spec.rb b/spec/graphql/mutations/set_course_post_policy_spec.rb index de6652b9d2f..2341135eb1a 100644 --- a/spec/graphql/mutations/set_course_post_policy_spec.rb +++ b/spec/graphql/mutations/set_course_post_policy_spec.rb @@ -23,7 +23,7 @@ require_relative "../graphql_spec_helper" describe Mutations::SetCoursePostPolicy do let(:assignment) { course.assignments.create! } - let(:course) { Course.create!(workflow_state: "available") } + let(:course) { Course.create!(workflow_state: "available", root_account: Account.default) } let(:student) { course.enroll_user(User.create!, "StudentEnrollment", enrollment_state: "active").user } let(:teacher) { course.enroll_user(User.create!, "TeacherEnrollment", enrollment_state: "active").user } @@ -56,7 +56,7 @@ describe Mutations::SetCoursePostPolicy do CanvasSchema.execute(mutation_str, context:) end - context "when user has manage_grades permission" do + context "when user has permission (aka teacher role)" do let(:context) { { current_user: teacher } } it "requires that courseId be passed in the query" do @@ -79,7 +79,7 @@ describe Mutations::SetCoursePostPolicy do end it "returns the related post policy" do - result = execute_query(mutation_str(course_id: course.id, post_manually: true), context) + result = execute_query(mutation_str(course_id: course.id, post_manually: true), { current_user: teacher }) policy = PostPolicy.find_by(course:, assignment: nil) expect(result.dig("data", "setCoursePostPolicy", "postPolicy", "_id").to_i).to be policy.id end @@ -139,17 +139,34 @@ describe Mutations::SetCoursePostPolicy do end end - context "when user does not have manage_grades permission" do - let(:context) { { current_user: student } } + context "when user does not have proper permissions" do + shared_examples "user cannot interact with endpoint" do + it "returns an error" do + result = execute_query(mutation_str(course_id: course.id, post_manually: true), context) + expect(result.dig("errors", 0, "message")).to eql "not found" + end - it "returns an error" do - result = execute_query(mutation_str(course_id: course.id, post_manually: true), context) - expect(result.dig("errors", 0, "message")).to eql "not found" + it "does not return data for the related post policy" do + result = execute_query(mutation_str(course_id: course.id, post_manually: true), context) + expect(result.dig("data", "setCoursePostPolicy")).to be_nil + end end - it "does not return data for the related post policy" do - result = execute_query(mutation_str(course_id: course.id, post_manually: true), context) - expect(result.dig("data", "setCoursePostPolicy")).to be_nil + describe "when user is a student" do + let(:context) { { current_user: student } } + + it_behaves_like "user cannot interact with endpoint" + end + + describe "when user is a teacher" do + let(:context) { { current_user: teacher } } + + before do + Account.default.role_overrides.create!(role: Role.find_by(name: "TeacherEnrollment"), permission: "manage_content", enabled: false) + Account.default.role_overrides.create!(role: Role.find_by(name: "TeacherEnrollment"), permission: "manage_course_content_edit", enabled: false) + end + + it_behaves_like "user cannot interact with endpoint" end end end