From 4e90438d25c7e6c9c5a012c09331445d97ca3513 Mon Sep 17 00:00:00 2001 From: Rob Orton Date: Mon, 31 Mar 2014 15:35:55 -0600 Subject: [PATCH] ensure assignments API uses correct date time format part2 fixes CNVS-10087 test plan - assignments PUT and CREATE request will only accept is08601 formatted dates - will return 400 bad_request - still allow clearing the dates Change-Id: Ie5e25eb1a5e85e0aec0b934f7a256b7a8b0dc473 Reviewed-on: https://gerrit.instructure.com/32634 Reviewed-by: Cody Cutrer Tested-by: Jenkins QA-Review: August Thornton Product-Review: Rob Orton --- lib/api/v1/assignment.rb | 48 ++++++------------ spec/apis/v1/assignments_api_spec.rb | 76 ++++++++++++++++++++++------ 2 files changed, 76 insertions(+), 48 deletions(-) diff --git a/lib/api/v1/assignment.rb b/lib/api/v1/assignment.rb index ce90b7801c6..e8873114ad8 100644 --- a/lib/api/v1/assignment.rb +++ b/lib/api/v1/assignment.rb @@ -262,6 +262,7 @@ module Api::V1::Assignment return if overrides && !overrides.is_a?(Array) return false unless valid_assignment_group_id?(assignment, assignment_params) + return false unless valid_assignment_dates?(assignment, assignment_params) assignment = update_from_params(assignment, assignment_params) @@ -280,6 +281,20 @@ module Api::V1::Assignment return false end + # validate that date and times are iso8601 + def valid_assignment_dates?(assignment, assignment_params) + errors = ['due_at', 'lock_at', 'unlock_at', 'peer_reviews_assign_at'].map do |v| + if assignment_params[v].present? && assignment_params[v] !~ Api::ISO8601_REGEX + assignment.errors.add("assignment[#{v}]", + I18n.t("assignments_api.invalid_date_time", + 'Invalid datetime for %{attribute}', + attribute: v)) + end + end + + errors.compact.empty? + end + def valid_assignment_group_id?(assignment, assignment_params) ag_id = assignment_params["assignment_group_id"].presence # if ag_id is a non-numeric string, ag_id.to_i will == 0 @@ -334,42 +349,11 @@ module Api::V1::Assignment assignment.muted = value_to_boolean(assignment_params.delete("muted")) end - exception_message = ["invalid due_at", - "assignment_params: #{assignment_params}", - "user: #{@current_user.attributes}", - "account: #{assignment.context.root_account.attributes}", - "course: #{assignment.context.attributes}", - "assignment: #{assignment.attributes}"].join(",\n") - # do some fiddling with due_at for fancy midnight and add to update_params - # validate that date and times are iso8601 otherwise ignore them, but still - # allow clearing them when set to nil - if update_params['due_at'].present? && update_params['due_at'] !~ Api::ISO8601_REGEX - Api.invalid_time_stamp_error('due_at', exception_message) - # todo stop logging and delete invalid dates - # update_params.delete(:due_at) - elsif update_params.has_key?('due_at') + if update_params['due_at'].present? && update_params['due_at'] =~ Api::ISO8601_REGEX update_params['time_zone_edited'] = Time.zone.name end - if update_params['lock_at'].present? && update_params['lock_at'] !~ Api::ISO8601_REGEX - Api.invalid_time_stamp_error('lock_at', exception_message) - # todo stop logging and delete invalid dates - # update_params.delete(:lock_at) - end - - if update_params['unlock_at'].present? && update_params['unlock_at'] !~ Api::ISO8601_REGEX - Api.invalid_time_stamp_error('unlock_at', exception_message) - # todo stop logging and delete invalid dates - # update_params.delete(:unlock_at) - end - - if update_params['peer_reviews_due_at'].present? && update_params['peer_reviews_due_at'] !~ Api::ISO8601_REGEX - Api.invalid_time_stamp_error('peer_reviews_due_at', exception_message) - # todo stop logging and delete invalid dates - # update_params.delete(:peer_reviews_due_at) - end - if !assignment.context.try(:turnitin_enabled?) update_params.delete("turnitin_enabled") update_params.delete("turnitin_settings") diff --git a/spec/apis/v1/assignments_api_spec.rb b/spec/apis/v1/assignments_api_spec.rb index bb91d941e84..6fe4a80ea90 100644 --- a/spec/apis/v1/assignments_api_spec.rb +++ b/spec/apis/v1/assignments_api_spec.rb @@ -1,5 +1,5 @@ # -# Copyright (C) 2011 Instructure, Inc. +# Copyright (C) 2011 - 2014 Instructure, Inc. # # This file is part of Canvas. # @@ -401,8 +401,12 @@ describe AssignmentsApiController, type: :request do describe "POST /courses/:course_id/assignments (#create)" do - it "allows authenticated users to create assignments" do + + before do course_with_teacher(:active_all => true) + end + + it "allows authenticated users to create assignments" do @course.assignment_groups.create!({:name => "first group"}) @group = @course.assignment_groups.create!({:name => "some group"}) @course.assignment_groups.create!({:name => "last group", @@ -414,9 +418,9 @@ describe AssignmentsApiController, type: :request do { 'name' => 'some assignment', 'position' => '1', 'points_possible' => '12', - 'due_at' => '2011-01-01', - 'lock_at' => '2011-01-03', - 'unlock_at' => '2011-12-31', + 'due_at' => '2011-01-01T00:00:00Z', + 'lock_at' => '2011-01-03T00:00:00Z', + 'unlock_at' => '2011-12-31T00:00:00Z', 'description' => 'assignment description', 'assignment_group_id' => @group.id, 'submission_types' => [ @@ -429,7 +433,7 @@ describe AssignmentsApiController, type: :request do 'grade_group_students_individually' => true, 'automatic_peer_reviews' => true, 'peer_reviews' => true, - 'peer_reviews_assign_at' => '2011-01-01', + 'peer_reviews_assign_at' => '2011-01-02T00:00:00Z', 'peer_review_count' => 2, 'group_category_id' => @group_category.id, 'turnitin_enabled' => true, @@ -489,7 +493,6 @@ describe AssignmentsApiController, type: :request do it "should not allow assignment titles longer than 255 characters" do name_too_long = greater_than_255 - course_with_teacher(:active_all => true) #not create an assignment with a name too long lambda{ raw_api_call(:post, @@ -508,7 +511,6 @@ describe AssignmentsApiController, type: :request do it "should not allow updating an assignment title to longer than 255 characters" do name_too_long = greater_than_255 - course_with_teacher(:active_all => true) #create an assignment @json = api_create_assignment_in_course(@course, {'name' => 'some name'}) @assignment = Assignment.find @json['id'] @@ -533,7 +535,6 @@ describe AssignmentsApiController, type: :request do end it "does not allow modifying turnitin_enabled when not enabled on the context" do - course_with_teacher(:active_all => true) Course.any_instance.expects(:turnitin_enabled?).at_least_once.returns false response = api_create_assignment_in_course(@course, { 'name' => 'some assignment', @@ -546,8 +547,6 @@ describe AssignmentsApiController, type: :request do end it "should process html content in description on create" do - course_with_teacher(:active_all => true) - should_process_incoming_user_content(@course) do |content| api_create_assignment_in_course(@course, { 'description' => content }) @@ -558,7 +557,6 @@ describe AssignmentsApiController, type: :request do end it "allows creating an assignment with overrides via the API" do - course_with_teacher(:active_all => true) student_in_course(:course => @course, :active_enrollment => true) @adhoc_due_at = 5.days.from_now @@ -602,7 +600,6 @@ describe AssignmentsApiController, type: :request do it "takes overrides into account in the assignment-created notification " + "for assignments created with overrides" do - course_with_teacher(:active_all => true) student_in_course(:course => @course, :active_enrollment => true) course_with_ta(:course => @course, :active_enrollment => true) @course.course_sections.create! @@ -648,7 +645,6 @@ describe AssignmentsApiController, type: :request do end it "should not allow an assignment_group_id that is not a number" do - course_with_teacher(:active_all => true) student_in_course(:course => @course, :active_enrollment => true) @user = @teacher @@ -671,7 +667,6 @@ describe AssignmentsApiController, type: :request do end - describe "PUT /courses/:course_id/assignments/:id (#update)" do it "should update published/unpublished" do @@ -716,6 +711,55 @@ describe AssignmentsApiController, type: :request do should == "Can't unpublish if there are student submissions" end + it "should 400 with invalid date times" do + course_with_teacher(:active_all => true) + the_date = 1.day.ago + @assignment = @course.assignments.create({ + :name => "some assignment", + :points_possible => 15 + }) + @assignment.due_at = the_date + @assignment.lock_at = the_date + @assignment.unlock_at = the_date + @assignment.peer_reviews_assign_at = the_date + @assignment.save! + + raw_api_update_assignment(@course, @assignment, + {'peer_reviews_assign_at' => '1/1/2013' }) + response.should_not be_success + response.code.should eql '400' + json = JSON.parse response.body + json['errors']['assignment[peer_reviews_assign_at]'].first['message']. + should == 'Invalid datetime for peer_reviews_assign_at' + end + + it "should allow clearing dates" do + course_with_teacher(:active_all => true) + the_date = 1.day.ago + @assignment = @course.assignments.create({ + :name => "some assignment", + :points_possible => 15 + }) + @assignment.due_at = the_date + @assignment.lock_at = the_date + @assignment.unlock_at = the_date + @assignment.peer_reviews_assign_at = the_date + @assignment.save! + + api_update_assignment_call(@course, @assignment, + {'due_at' => nil, + 'lock_at' => '', + 'unlock_at' => nil, + 'peer_reviews_assign_at' => nil }) + response.should be_success + @assignment.reload + + @assignment.due_at.should be_nil + @assignment.lock_at.should be_nil + @assignment.unlock_at.should be_nil + @assignment.peer_reviews_assign_at.should be_nil + end + context "without overrides or frozen attributes" do before do course_with_teacher(:active_all => true) @@ -746,7 +790,7 @@ describe AssignmentsApiController, type: :request do 'group_category_id' => nil, 'description' => 'assignment description', 'grading_type' => 'letter_grade', - 'due_at' => '2011-01-01', + 'due_at' => '2011-01-01T00:00:00Z', 'position' => 1, 'muted' => true })