Issue 6225: Allow multiple periods in Assignments API routes

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)

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

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
 

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