Implement GET and PATCH /overall_comment API routes, closes #7708

[philipkukulak]

Proposed Changes

Implements GET and PATCH /overall_comment, allowing for API retrieval and updating of the overall_comment field in the results table. Closes #7708.

• GET /api/courses/:course_id/assignments/:assignment_id/groups/:id/overall_comment — returns the current overall comment for a group's result
• PATCH /api/courses/:course_id/assignments/:assignment_id/groups/:id/overall_comment — sets or clears the overall comment

Both endpoints return JSON or XML depending on the Accept header, consistent with the rest of the API.

Screenshots of your changes (if applicable)

n/a

Associated documentation repository pull request (if applicable)

https://github.com/MarkUsProject/Wiki/pull/263

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	x
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

• I have updated the project Changelog (this is required for all changes).
• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

Detailed description of smoke tests:

Smoke Test: GET/PATCH .../groups/:id/overall_comment

Setup

Prerequisites

• Dev server running: docker compose up -d rails
• App is mounted under a course prefix — all API routes live at http://localhost:3000/csc108/api/...
• An instructor API key for course 1. Reset it if needed:

docker compose run --rm rails bundle exec rails runner "
u = Instructor.where(course_id: 1).first.user
u.reset_api_key
puts u.api_key
"

• A group with a submission (group_id=1, used below)

Variables used in all commands below

KEY="<instructor api key>"
URL="http://localhost:3000/csc108/api/courses/1/assignments/1/groups/1/overall_comment"

Reset to a known state before running:

curl -s -X PATCH -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  -d "overall_comment=baseline" "$URL"

---

Test Cases

Auth

# No auth header → 403
curl -s -o /dev/null -w "%{http_code}" -H "Accept: application/json" "$URL"

# Garbage token → 403
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth BADTOKEN" -H "Accept: application/json" "$URL"

# Wrong scheme (Bearer) → 403
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: Bearer $KEY" -H "Accept: application/json" "$URL"

Format

# No Accept header → 200 (defaults to XML)
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth $KEY" "$URL"

# JSON Accept → 200
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"

# XML Accept → 200
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/xml" "$URL"

# Unsupported Accept (text/html) → 406
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth $KEY" -H "Accept: text/html" "$URL"

# JSON body shape contains 'overall_comment' key
curl -s -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"
# Expected: {"overall_comment":"..."}

# XML body shape contains <result><overall-comment> structure
curl -s -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/xml" "$URL"
# Expected: <result><overall-comment>...</overall-comment></result>

Routing

# Non-existent course_id → 404
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  "http://localhost:3000/csc108/api/courses/9999/assignments/1/groups/1/overall_comment"

# Non-existent assignment_id → 404
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  "http://localhost:3000/csc108/api/courses/1/assignments/9999/groups/1/overall_comment"

# Non-existent group_id → 404
curl -s -o /dev/null -w "%{http_code}" -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  "http://localhost:3000/csc108/api/courses/1/assignments/1/groups/9999/overall_comment"

GET

# Returns current value
curl -s -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"
# Expected: {"overall_comment":"baseline"}

PATCH

# Plain string — set and verify
curl -s -o /dev/null -w "%{http_code}" -X PATCH \
  -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  -d "overall_comment=hello" "$URL"
curl -s -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"
# Expected: 200, {"overall_comment":"hello"}

# Empty string — clears to ""
curl -s -o /dev/null -w "%{http_code}" -X PATCH \
  -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  -d "overall_comment=" "$URL"
curl -s -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"
# Expected: 200, {"overall_comment":""}

# No body param — clears to null
curl -s -o /dev/null -w "%{http_code}" -X PATCH \
  -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"
curl -s -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"
# Expected: 200, {"overall_comment":null}

# PATCH twice — second write wins
curl -s -o /dev/null -w "%{http_code}" -X PATCH \
  -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  -d "overall_comment=first" "$URL"
curl -s -o /dev/null -w "%{http_code}" -X PATCH \
  -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" \
  -d "overall_comment=final" "$URL"
curl -s -H "Authorization: MarkUsAuth $KEY" -H "Accept: application/json" "$URL"
# Expected: 200, {"overall_comment":"final"}

---

Results (2026-05-22)

#	Category	Test	Expected	Result
1	Auth	No auth header	403	✅ 403
2	Auth	Garbage token	403	✅ 403
3	Auth	Wrong scheme (Bearer)	403	✅ 403
4	Format	No Accept header	200 XML	✅ 200
5	Format	JSON Accept	200 JSON	✅ 200
6	Format	XML Accept	200 XML	✅ 200
7	Format	Unsupported Accept	406	✅ 406
8	Format	JSON body shape	{"overall_comment":...}	✅
9	Format	XML body shape	<result><overall-comment>	✅
10	Routing	Bad course_id	404	✅ 404
11	Routing	Bad assignment_id	404	✅ 404
12	Routing	Bad group_id	404	✅ 404
13	GET	Returns current value	200 + value	✅
14	PATCH	Plain string	200 + persisted	✅
15	PATCH	Empty string	200 + ""	✅
16	PATCH	No body param → null	200 + null	✅
17	PATCH	Second write wins	200 + final value	✅

[philipkukulak]

Noting that I will update the API documentation and open a PR for that on Monday.

[coveralls]

Coverage Report for CI Build 26410002258

Coverage increased (+0.02%) to 90.277%

Details

• Coverage increased (+0.02%) from the base build.
• Patch coverage: 102 of 102 lines across 5 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	49901
Covered Lines:	46020
Line Coverage:	92.22%
Relevant Branches:	2151
Covered Branches:	971
Branch Coverage:	45.14%
Branches in Coverage %:	Yes
Coverage Strength:	121.84 hits per line

---

💛 - Coveralls

[Naragod]

LGTM

[david-yz-liu]

@philipkukulak nice work, I left a few comments, and also please make sure to update the Changelog.

[david-yz-liu]

Here we should check whether :overall_comment is present in the params first. If it isn't, render the 422 error response (similar to how we do this in update_marking_state).

Separately, when reviewing this I realized that we should not allow the overall_comment field to be updated if the result is released. This should actually be controlled by a model validation (i.e., within the Result class). There's already a validation but it's only checking a change to the marking_state status, so please add to that validation to detect a change to the overall_comment field.

[philipkukulak]

Thank you David. I've updated the overall_comment method to validate the existence of :overall_comment when a PATCH is received since no parameter is sent for GET requests.

I've also added a case to the check_for_released method in the Result model.

Tests updated accordingly, and Changelog has been updated as well. I'll work on documentation shortly.

[david-yz-liu]

Nice work, @philipkukulak!