diff --git a/Changelog.md b/Changelog.md index 197a053eb6..1142ede046 100644 --- a/Changelog.md +++ b/Changelog.md @@ -54,6 +54,7 @@ - Forward the test batch id to the autotester so AI grading telemetry can attribute mass-grading runs (#7991) - Removed Graders Subcomponent and added a Graders column in the Assignment Grades tab (#7967) - Added GET /test_runs API route (#8055) +- Updated POST and PUT assignment API routes to allow for multiple submission rule periods (#8128) ### 🐛 Bug fixes - Fixed bulk grouping deletion assignment scoping (#8134) diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index dd720ed728..e1781d2bea 100644 --- a/app/controllers/api/assignments_controller.rb +++ b/app/controllers/api/assignments_controller.rb @@ -48,11 +48,10 @@ def show # Creates a new assignment # Requires: short_identifier, due_date, description # Optional: repository_folder, group_min, group_max, tokens_per_period, - # submission_rule_type, allow_web_submits, + # submission_rule_type, submission_rule_periods, allow_web_submits, # display_grader_names_to_students, enable_test, assign_graders_to_criteria, # message, allow_remarks, remark_due_date, remark_message, student_form_groups, - # group_name_autogenerated, submission_rule_deduction, submission_rule_hours, - # submission_rule_interval + # group_name_autogenerated def create if has_missing_params?([:short_identifier, :due_date, :description]) # incomplete/invalid HTTP params @@ -101,11 +100,10 @@ def create # Updates an existing assignment # Requires: id # Optional: short_identifier, due_date,repository_folder, group_min, group_max, - # tokens_per_period, submission_rule_type, allow_web_submits, + # tokens_per_period, submission_rule_type, submission_rule_periods, allow_web_submits, # display_grader_names_to_students, enable_test, assign_graders_to_criteria, # description, message, allow_remarks, remark_due_date, remark_message, - # student_form_groups, group_name_autogenerated, submission_rule_deduction, - # submission_rule_hours, submission_rule_interval, starter_file_type, + # student_form_groups, group_name_autogenerated, starter_file_type, # default_starter_file_group_id def update # If no assignment is found, render an error. @@ -142,11 +140,16 @@ def update render 'shared/http_status', locals: { code: '500', message: HttpStatusHelper::ERROR_CODE['message']['500'] }, status: :internal_server_error return - elsif submission_rule.valid? - # If it's a valid submission rule, replace the existing one - assignment.submission_rule.destroy - assignment.submission_rule = submission_rule end + + original_submission_rule = assignment.submission_rule + unless assignment.update(submission_rule: submission_rule) + render 'shared/http_status', locals: { code: '500', message: + HttpStatusHelper::ERROR_CODE['message']['500'] }, status: :internal_server_error + return + end + original_submission_rule.destroy + end unless assignment.save @@ -230,28 +233,35 @@ def update_test_specs # Defaults to NoLateSubmissionRule def get_submission_rule(params) if params[:submission_rule_type] == 'GracePeriod' - submission_rule = GracePeriodSubmissionRule.new - period = Period.new(hours: params[:submission_rule_hours]) - submission_rule.periods << period - + rule_type = 'GracePeriodSubmissionRule' + required = [:hours] elsif params[:submission_rule_type] == 'PenaltyDecayPeriod' - submission_rule = PenaltyDecayPeriodSubmissionRule.new - period = Period.new(hours: params[:submission_rule_hours], - deduction: params[:submission_rule_deduction], - interval: params[:submission_rule_interval]) - submission_rule.periods << period - + rule_type = 'PenaltyDecayPeriodSubmissionRule' + required = [:hours, :deduction, :interval] elsif params[:submission_rule_type] == 'PenaltyPeriod' - submission_rule = PenaltyPeriodSubmissionRule.new - period = Period.new(hours: params[:submission_rule_hours], - deduction: params[:submission_rule_deduction]) - submission_rule.periods << period - + rule_type = 'PenaltyPeriodSubmissionRule' + required = [:hours, :deduction] else - submission_rule = NoLateSubmissionRule.new + return NoLateSubmissionRule.new + end + + if params[:submission_rule_periods].nil? + return + end + + permitted_params = params.permit( + submission_rule_periods: [:hours, :deduction, :interval, :_destroy] + ) + permitted_params[:submission_rule_periods].each do |period| + if required.any? { |key| !period.key?(key) } + return + end end - submission_rule + SubmissionRule.new( + { type: rule_type, + periods_attributes: permitted_params[:submission_rule_periods] } + ) end def grades_summary diff --git a/docs/docs/technical-guides/restful-api.md b/docs/docs/technical-guides/restful-api.md index 225e336c37..eada4deb49 100644 --- a/docs/docs/technical-guides/restful-api.md +++ b/docs/docs/technical-guides/restful-api.md @@ -644,6 +644,12 @@ Returns the same JSON structure but with only the matching students in the `stud - token_start_date (string: that can be parsed into a Ruby DateTime object) - has_peer_review (boolean) - starter_file_type (one of "simple", "sections", "shuffle", "group") + - submission_rule_type (one of "NoLateSubmission", "PenaltyPeriod", "GracePeriod", "PenaltyDecayPeriod") + - "GracePeriod", "PenaltyPeriod", and "PenaltyDecayPeriod" require submission_rule_periods to be provided. + - submission_rule_periods (list of hashes: keys depend on the submission_rule_type): + - GracePeriod: requires key "hours" for each period + - PenaltyPeriod: requires keys "hours" and "deduction" for each period + - PenaltyDecayPeriod: requires keys "hours", "deduction", and "interval" for each period ### GET /api/courses/:course_id/assignments/:id @@ -714,6 +720,12 @@ Returns the same JSON structure but with only the matching students in the `stud - token_start_date (string: that can be parsed into a Ruby DateTime object) - has_peer_review (boolean) - starter_file_type (one of "simple", "sections", "shuffle", "group") + - submission_rule_type (one of "NoLateSubmission", "PenaltyPeriod", "GracePeriod", "PenaltyDecayPeriod") + - "GracePeriod", "PenaltyPeriod", and "PenaltyDecayPeriod" require submission_rule_periods to be provided. + - submission_rule_periods (list of hashes: keys depend on the submission_rule_type): + - GracePeriod: requires key "hours" for each period + - PenaltyPeriod: requires keys "hours" and "deduction" for each period + - PenaltyDecayPeriod: requires keys "hours", "deduction", and "interval" for each period ### DELETE /api/courses/:course_id/assignments/:id diff --git a/spec/controllers/api/assignments_controller_spec.rb b/spec/controllers/api/assignments_controller_spec.rb index 63e7689ff0..427caf75d0 100644 --- a/spec/controllers/api/assignments_controller_spec.rb +++ b/spec/controllers/api/assignments_controller_spec.rb @@ -300,9 +300,10 @@ allow_web_submits: false, display_grader_names_to_students: true, enable_test: true, assign_graders_to_criteria: true, student_form_groups: true, group_name_autogenerated: false, - submission_rule_deduction: 10, submission_rule_hours: 11, - submission_rule_interval: 12, remark_due_date: '2012-03-26 18:04:39', + remark_due_date: '2012-03-26 18:04:39', group_max: 3, submission_rule_type: 'PenaltyDecayPeriod', + submission_rule_periods: [{ hours: 1, deduction: 1, interval: 1 }, + { hours: 2, deduction: 2, interval: 2 }], group_min: 2, remark_message: 'Remark', allow_remarks: false, non_regenerating_tokens: false, unlimited_tokens: false, token_period: 1.0, @@ -396,8 +397,18 @@ end context 'where submission rule is invalid' do - it 'should respond with 500' do - post :create, params: { **full_params, submission_rule_interval: 'not a real interval' } + it 'should respond with 500 when submission_rule_periods is missing' do + post :create, params: { **full_params.except(:submission_rule_periods) } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is invalid' do + post :create, params: { **full_params, submission_rule_periods: ['not a valid interval'] } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is missing a parameter' do + post :create, params: { **full_params, submission_rule_periods: [{ hours: 2, interval: 1 }] } expect(response).to have_http_status(:internal_server_error) end end @@ -431,6 +442,127 @@ expect(response).to have_http_status(:forbidden) end end + + context 'updating submission_rule_type' do + context 'to NoLateSubmission' do + it 'should update an existing assignment' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'NoLateSubmission' } + expect(response).to have_http_status(:ok) + expect(Assignment.find_by(id: assignment.id).submission_rule).to be_a NoLateSubmissionRule + end + end + + context 'to GracePeriod' do + it 'should update an existing assignment' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'GracePeriod', + submission_rule_periods: [{ hours: 1 }, + { hours: 2 }] } + expect(response).to have_http_status(:ok) + expect(Assignment.find_by(id: assignment.id).submission_rule).to be_a GracePeriodSubmissionRule + expect(Assignment.find_by(id: assignment.id).submission_rule.periods.size).to eq(2) + expect(Assignment.find_by(id: assignment.id).submission_rule.periods.first.hours).to eq(1) + expect(Assignment.find_by(id: assignment.id).submission_rule.periods.last.hours).to eq(2) + end + + it 'should respond with 500 when submission_rule_periods is missing' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'GracePeriod' } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is an empty list' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'GracePeriod', + submission_rule_periods: [] } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is missing hours' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'GracePeriod', + submission_rule_periods: [{ interval: 1 }] } + expect(response).to have_http_status(:internal_server_error) + end + end + + context 'to PenaltyPeriod' do + it 'should update an existing assignment' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyPeriod', + submission_rule_periods: [{ hours: 1, deduction: 2 }, + { hours: 3, deduction: 4 }] } + expect(response).to have_http_status(:ok) + expect(Assignment.find_by(id: assignment.id).submission_rule).to be_a PenaltyPeriodSubmissionRule + submission_periods = Assignment.find_by(id: assignment.id).submission_rule.periods + expect(submission_periods.size).to eq(2) + expect(submission_periods.first.hours).to eq(1) + expect(submission_periods.first.deduction).to eq(2) + expect(submission_periods.last.hours).to eq(3) + expect(submission_periods.last.deduction).to eq(4) + end + + it 'should respond with 500 when submission_rule_periods is missing' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyPeriod' } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is an empty list' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyPeriod', + submission_rule_periods: [] } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is missing an argument' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyPeriod', + submission_rule_periods: [{ deduction: 1 }] } + expect(response).to have_http_status(:internal_server_error) + end + end + + context 'to PenaltyDecayPeriod' do + it 'should update an existing assignment' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyDecayPeriod', + submission_rule_periods: [{ hours: 1, deduction: 2, interval: 3 }, + { hours: 4, deduction: 5, interval: 6 }] } + expect(response).to have_http_status(:ok) + expect(Assignment.find_by(id: assignment.id).submission_rule).to be_a PenaltyDecayPeriodSubmissionRule + submission_periods = Assignment.find_by(id: assignment.id).submission_rule.periods + expect(submission_periods.size).to eq(2) + expect(submission_periods.first.hours).to eq(1) + expect(submission_periods.first.deduction).to eq(2) + expect(submission_periods.first.interval).to eq(3) + expect(submission_periods.last.hours).to eq(4) + expect(submission_periods.last.deduction).to eq(5) + expect(submission_periods.last.interval).to eq(6) + end + + it 'should respond with 500 when submission_rule_periods is missing' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyDecayPeriod' } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is an empty list' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyDecayPeriod', + submission_rule_periods: [] } + expect(response).to have_http_status(:internal_server_error) + end + + it 'should respond with 500 when submission_rule_periods is missing an argument' do + put :update, params: { id: assignment.id, course_id: course.id, + submission_rule_type: 'PenaltyDecayPeriod', + submission_rule_periods: [{ hours: 1, deduction: 2 }] } + expect(response).to have_http_status(:internal_server_error) + end + end + end end context 'GET test_files' do