From 977967e8dc887418d656f576c6aba9ae7e8b1f60 Mon Sep 17 00:00:00 2001 From: philipkukulak Date: Fri, 22 May 2026 16:42:01 -0400 Subject: [PATCH] Implement GET and PATCH /overall_comment API routes, closes #7708 --- Changelog.md | 1 + app/controllers/api/groups_controller.rb | 28 +++++ app/models/result.rb | 14 ++- config/routes.rb | 6 + .../controllers/api/groups_controller_spec.rb | 116 ++++++++++++++++++ spec/models/result_spec.rb | 20 +++ 6 files changed, 181 insertions(+), 4 deletions(-) diff --git a/Changelog.md b/Changelog.md index fae105630c..5a725700d4 100644 --- a/Changelog.md +++ b/Changelog.md @@ -18,6 +18,7 @@ - Added term-based suffixes to course names created via LTI to ensure uniqueness across academic years (#7881) - Added `db:populate_course_dates` rake task to backfill `start_at`/`end_at` for existing courses, and permit those fields when creating courses through the admin UI (#7925) - Sync due date when creating or updating LTI gradebook line items, and re-sync automatically when an assessment is edited (#7872) +- Added GET and PATCH /overall_comment API routes (#7963) ### 🐛 Bug fixes - Prevent "No rows found" message from displaying in tables when data is loading (#7790) diff --git a/app/controllers/api/groups_controller.rb b/app/controllers/api/groups_controller.rb index 538f5e444d..308ac4ecc1 100644 --- a/app/controllers/api/groups_controller.rb +++ b/app/controllers/api/groups_controller.rb @@ -466,6 +466,34 @@ def add_test_run end end + def overall_comment + result = grouping&.current_result + return page_not_found('No submission exists for that group') if result.nil? + + case request.method + when 'GET' + respond_to do |format| + format.xml do + render xml: { overall_comment: result.overall_comment }.to_xml(root: 'result', skip_types: 'true') + end + format.json { render json: { overall_comment: result.overall_comment } } + end + when 'PATCH' + if has_missing_params?([:overall_comment]) + render 'shared/http_status', locals: { code: '422', message: + HttpStatusHelper::ERROR_CODE['message']['422'] }, status: :unprocessable_content + return + end + if result.update(overall_comment: params[:overall_comment]) + head :ok + else + render 'shared/http_status', + locals: { code: '422', message: result.errors.full_messages.first }, + status: :unprocessable_content + end + end + end + private def assignment diff --git a/app/models/result.rb b/app/models/result.rb index 4466537e2a..b776464f79 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -444,11 +444,17 @@ def print_pdf_filename private - # Do not allow the marking state to be changed to incomplete if the result is released + # Do not allow certain fields to be changed when the result is released def check_for_released - if released_to_students && marking_state_changed?(to: Result::MARKING_STATES[:incomplete]) - errors.add(:base, :marks_released) - throw(:abort) + if released_to_students + if marking_state_changed?(to: Result::MARKING_STATES[:incomplete]) + errors.add(:base, :marks_released) + throw(:abort) + end + if overall_comment_changed? + errors.add(:overall_comment, :marks_released) + throw(:abort) + end end true end diff --git a/config/routes.rb b/config/routes.rb index 51c1de0ca1..2f922e2f43 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -50,6 +50,8 @@ # PATCH /api/courses/:course_id/assignments/:assignment_id/groups/:group_id/feedback_files/:id(.:format) api/feedback_files#update # PUT /api/courses/:course_id/assignments/:assignment_id/groups/:group_id/feedback_files/:id(.:format) api/feedback_files#update # DELETE /api/courses/:course_id/assignments/:assignment_id/groups/:group_id/feedback_files/:id(.:format) api/feedback_files#destroy +# overall_comment_api_course_assignment_group GET /api/courses/:course_id/assignments/:assignment_id/groups/:id/overall_comment(.:format) api/groups#overall_comment +# PATCH /api/courses/:course_id/assignments/:assignment_id/groups/:id/overall_comment(.:format) api/groups#overall_comment # annotations_api_course_assignment_group GET /api/courses/:course_id/assignments/:assignment_id/groups/:id/annotations(.:format) api/groups#annotations # add_annotations_api_course_assignment_group POST /api/courses/:course_id/assignments/:assignment_id/groups/:id/add_annotations(.:format) api/groups#add_annotations # add_members_api_course_assignment_group POST /api/courses/:course_id/assignments/:assignment_id/groups/:id/add_members(.:format) api/groups#add_members @@ -620,6 +622,10 @@ patch 'extension' delete 'extension' end + member do + get 'overall_comment' + patch 'overall_comment' + end end resources :starter_file_groups, only: [:index, :create] member do diff --git a/spec/controllers/api/groups_controller_spec.rb b/spec/controllers/api/groups_controller_spec.rb index 1a043a6389..d26c450894 100644 --- a/spec/controllers/api/groups_controller_spec.rb +++ b/spec/controllers/api/groups_controller_spec.rb @@ -52,6 +52,16 @@ params: { assignment_id: assignment.id, course_id: course.id, id: group.id, extension: extension_params } expect(response).to have_http_status(:forbidden) end + + it 'should fail to authenticate a GET overall_comment request' do + get :overall_comment, params: { assignment_id: assignment.id, id: group.id, course_id: course.id } + expect(response).to have_http_status(:forbidden) + end + + it 'should fail to authenticate a PATCH overall_comment request' do + patch :overall_comment, params: { assignment_id: assignment.id, id: group.id, course_id: course.id } + expect(response).to have_http_status(:forbidden) + end end context 'An authenticated request requesting' do @@ -1460,5 +1470,111 @@ end end end + + context 'Overall Comment' do + let!(:grouping) { create(:grouping, group: group, assignment: assignment) } + + before { request.env['HTTP_ACCEPT'] = 'application/json' } + + context 'when no submission/result exists' do + it 'GET returns 404' do + get :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + expect(response).to have_http_status(:not_found) + end + + it 'PATCH returns 404' do + patch :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, + id: group.id, overall_comment: 'hello' } + expect(response).to have_http_status(:not_found) + end + end + + context 'when a result exists' do + let(:submission) { create(:version_used_submission, grouping: grouping) } + + before { submission } + + context 'expecting a json response' do + it 'GET returns 200 with the overall_comment' do + grouping.current_result.update!(overall_comment: 'existing comment') + get :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + expect(response).to have_http_status(:ok) + expect(response.parsed_body['overall_comment']).to eq('existing comment') + end + + it 'GET returns 200 with null overall_comment when none set' do + get :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + expect(response).to have_http_status(:ok) + expect(response.parsed_body['overall_comment']).to be_nil + end + + it_behaves_like 'for a different course' do + before do + get :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + end + end + end + + context 'expecting an xml response' do + before { request.env['HTTP_ACCEPT'] = 'application/xml' } + + it 'GET returns 200' do + get :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + expect(response).to have_http_status(:ok) + end + + it 'GET returns overall_comment nested under a result root element' do + grouping.current_result.update!(overall_comment: 'xml comment') + get :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + parsed = Hash.from_xml(response.body) + expect(parsed.dig('result', 'overall_comment')).to eq('xml comment') + end + + it 'GET returns null overall_comment as empty element when none set' do + get :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + parsed = Hash.from_xml(response.body) + expect(parsed.dig('result', 'overall_comment')).to be_nil + end + end + + it 'PATCH updates overall_comment and returns 200' do + patch :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, + id: group.id, overall_comment: 'new comment' } + expect(response).to have_http_status(:ok) + expect(grouping.current_result.reload.overall_comment).to eq('new comment') + end + + it 'PATCH returns 422 when result is released' do + grouping.current_result.update!(released_to_students: true) + patch :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, + id: group.id, overall_comment: 'blocked' } + expect(response).to have_http_status(:unprocessable_content) + end + + it 'PATCH returns 422 when overall_comment param is missing' do + patch :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, id: group.id } + expect(response).to have_http_status(:unprocessable_content) + end + + it 'PATCH returns 422 when overall_comment param is nil' do + patch :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, + id: group.id, overall_comment: nil } + expect(response).to have_http_status(:unprocessable_content) + end + + it 'PATCH returns 422 when overall_comment param is empty string' do + patch :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, + id: group.id, overall_comment: '' } + expect(response).to have_http_status(:unprocessable_content) + end + + it_behaves_like 'for a different course' do + before do + patch :overall_comment, params: { course_id: course.id, assignment_id: assignment.id, + id: group.id, overall_comment: 'hello' } + end + end + end + end end end diff --git a/spec/models/result_spec.rb b/spec/models/result_spec.rb index de7f34a45f..8d08ca92f4 100644 --- a/spec/models/result_spec.rb +++ b/spec/models/result_spec.rb @@ -67,6 +67,26 @@ end end end + + context 'check_for_released' do + before { result.update!(released_to_students: true) } + + it 'does not allow marking_state to be changed to incomplete' do + result.marking_state = Result::MARKING_STATES[:incomplete] + expect(result.save).to be false + expect(result.errors[:base]).to be_present + end + + it 'does not allow overall_comment to be changed' do + result.overall_comment = 'new comment' + expect(result.save).to be false + expect(result.errors[:overall_comment]).to be_present + end + + it 'allows other fields to be updated' do + expect { result.regenerate_view_token }.not_to raise_error + end + end end shared_context 'get subtotal context' do