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

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)

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

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

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

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

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