diff --git a/Changelog.md b/Changelog.md index a2041dddd3..ee0955ff48 100644 --- a/Changelog.md +++ b/Changelog.md @@ -26,6 +26,7 @@ - Added a submission scope filter to the grading view so TAs with manage submissions permission can navigate either all submissions or only their assigned submissions (#8046) - 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 - Ensured random grader assignment excludes ineligible roles and recalculates weights using eligible graders (#8073) diff --git a/app/controllers/api/assignments_controller.rb b/app/controllers/api/assignments_controller.rb index dd720ed728..6246d62f9c 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. @@ -138,15 +136,19 @@ def update # Update the submission rule if provided unless params[:submission_rule_type].nil? submission_rule = get_submission_rule(params) + if submission_rule.nil? 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 + + 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 + end unless assignment.save @@ -230,22 +232,66 @@ def update_test_specs # Defaults to NoLateSubmissionRule def get_submission_rule(params) if params[:submission_rule_type] == 'GracePeriod' + if params[:submission_rule_periods].nil? || + !params[:submission_rule_periods].is_a?(Array) || + params[:submission_rule_periods].empty? + return + end + + rule_periods = params[:submission_rule_periods] + + # If hours is missing from any of the periods, return + if rule_periods.any? { |period| !period.respond_to?(:to_h) || !period.key?('hours') } + return + end + submission_rule = GracePeriodSubmissionRule.new - period = Period.new(hours: params[:submission_rule_hours]) - submission_rule.periods << period + submission_rule.periods = rule_periods.map do |period| + Period.new(hours: period[:hours]) + end 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 + if params[:submission_rule_periods].nil? || + !params[:submission_rule_periods].is_a?(Array) || + params[:submission_rule_periods].empty? + return + end + rule_periods = params[:submission_rule_periods] + + # If any of the required keys are missing, return + if rule_periods.any? do |period| + !period.respond_to?(:to_h) || !period.key?('hours') || + !period.key?('deduction') || !period.key?('interval') + end + return + end + submission_rule = PenaltyDecayPeriodSubmissionRule.new + submission_rule.periods = rule_periods.map do |period| + Period.new(hours: period[:hours], + deduction: period[:deduction], + interval: period[:interval]) + end elsif params[:submission_rule_type] == 'PenaltyPeriod' + if params[:submission_rule_periods].nil? || + !params[:submission_rule_periods].is_a?(Array) || + params[:submission_rule_periods].empty? + return + end + + rule_periods = params[:submission_rule_periods] + # If any of the required keys are missing, return + if rule_periods.any? do |period| + !period.respond_to?(:to_h) || !period.key?('hours') || !period.key?('deduction') + end + return + end + submission_rule = PenaltyPeriodSubmissionRule.new - period = Period.new(hours: params[:submission_rule_hours], - deduction: params[:submission_rule_deduction]) - submission_rule.periods << period + submission_rule.periods = rule_periods.map do |period| + Period.new(hours: period[:hours], + deduction: period[:deduction]) + end else submission_rule = NoLateSubmissionRule.new 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..91bf99247a 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,106 @@ 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 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 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 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