Skip to content

Issue 6225: Allow multiple periods in Assignments API routes #8128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
david-yz-liu merged 7 commits into from
Sep 9, 2026
Merged

Issue 6225: Allow multiple periods in Assignments API routes #8128

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc5b042
Updating assignment create and update API routes to allow for multipl…
sophia-huynh Aug 10, 2026
45e9dc4
Merge branch 'master' into issue-6225-api-nested-parameters
sophia-huynh Aug 10, 2026
ea770d6
Updating Changelog
sophia-huynh Aug 10, 2026
fd493e1
Revising to use Rails nested attributes for Submission Rule creation
sophia-huynh Sep 2, 2026
333b977
Merge branch 'master' into issue-6225-api-nested-parameters
sophia-huynh Sep 2, 2026
e1d298b
Syntax changes in response to review
sophia-huynh Sep 8, 2026
98da991
Merge branch 'master' into issue-6225-api-nested-parameters
sophia-huynh Sep 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
86 changes: 66 additions & 20 deletions app/controllers/api/assignments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -138,15 +136,19 @@ def update
# Update the submission rule if provided
unless params[:submission_rule_type].nil?
submission_rule = get_submission_rule(params)

Comment thread
sophia-huynh marked this conversation as resolved.
Outdated
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)
Comment thread
sophia-huynh marked this conversation as resolved.
render 'shared/http_status', locals: { code: '500', message:
HttpStatusHelper::ERROR_CODE['message']['500'] }, status: :internal_server_error
return
end

end

unless assignment.save
Expand Down Expand Up @@ -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? ||

@david-yz-liu david-yz-liu Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So overall we can make use of Rails support for nested attributes to have a more nested structure for these params:

{
  submission_rule_attributes: {
    type: ...,
    period_attributes: [
      { hours: ..., ... },
      ...
    ]
  }
}

This should allow you to create a new SubmissionRule object directly from the submission_rule_attributes, and if the creation fails due to a validation error, an error should be reported automatically, eliminating the need to check for particular keys manually,

@sophia-huynh sophia-huynh Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised to use nested attributes. However, error checking for missing attributes within periods was still required - or I can't find a way around it.

Without it, the creation doesn't fail properly, and checking .errors.any? seems to always give an error even for correctly created rules (probably for a similar reason as valid?'s failures.) It does raise an error when update is called*, but the error isn't handled elegantly and a traceback gets sent in the response.
* can't modify frozen attributes is the error that gets raised... for some reason I haven't been able to completely figure out. If it was just from the validation failing, then update should have returned false instead.

!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
Expand Down
12 changes: 12 additions & 0 deletions docs/docs/technical-guides/restful-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down
119 changes: 115 additions & 4 deletions spec/controllers/api/assignments_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down