Delete feedback (#634)

## Status

- Related to
RaspberryPiFoundation/digital-editor-issues#948

## What's changed?

- Added route to allow users to delete feedback
- Owners can read and delete any feedback within the school
- Teachers can only delete feedback for projects within their class

Author: loiswells97

app/controllers/api/feedback_controller.rb

@@ -42,6 +42,16 @@ def set_read
       end
     end
 
+    def destroy
+      result = Feedback::Delete.call(feedback_id: params[:id])
+
+      if result.success?
+        head :no_content
+      else
+        render json: { error: result[:error] }, status: :unprocessable_entity
+      end
+    end
+
     private
 
     def project

app/models/ability.rb

@@ -69,6 +69,7 @@ def define_school_owner_abilities(school:)
     can(%i[read create create_batch update destroy], :school_student)
     can(%i[create create_copy], Lesson, school_id: school.id)
     can(%i[read update destroy], Lesson, school_id: school.id, visibility: %w[teachers students public])
+    can(%i[read destroy], Feedback, school_project: { school_id: school.id })
     can(%i[exchange_code], :google_auth)
   end
 
@@ -98,7 +99,7 @@ def define_school_teacher_abilities(user:, school:)
     ).pluck(:id)
     can(%i[read], Project, remixed_from_id: teacher_project_ids)
     can(%i[show_status unsubmit return complete], SchoolProject, project: { remixed_from_id: teacher_project_ids })
-    can(%i[read create], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } })
+    can(%i[read create destroy], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } })
     can(%i[exchange_code], :google_auth)
   end
 

config/routes.rb

@@ -48,7 +48,7 @@
       end
       resources :remixes, only: %i[index], controller: 'projects/remixes'
       resource :images, only: %i[show create], controller: 'projects/images'
-      resources :feedback, only: %i[index create], controller: 'feedback' do
+      resources :feedback, only: %i[index create destroy], controller: 'feedback' do
         put :read, on: :member, to: 'feedback#set_read'
       end
     end

lib/concepts/feedback/delete.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+class Feedback
+  class Delete
+    class << self
+      def call(feedback_id:)
+        response = OperationResponse.new
+        delete_feedback(feedback_id)
+        response
+      rescue StandardError => e
+        Sentry.capture_exception(e)
+        response[:error] = "Error deleting feedback: #{e}"
+        response
+      end
+
+      private
+
+      def delete_feedback(feedback_id)
+        feedback = Feedback.find(feedback_id)
+        feedback.destroy!
+      end
+    end
+  end
+end

spec/concepts/feedback/delete_spec.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Feedback::Delete, type: :unit do
+  let(:feedback) { create(:feedback) }
+
+  describe '.call' do
+    context 'when deletion is successful' do
+      let!(:feedback_id) { feedback.id }
+
+      it 'returns a successful operation response' do
+        response = described_class.call(feedback_id:)
+        expect(response.success?).to be(true)
+      end
+
+      it 'deletes the feedback' do
+        expect { described_class.call(feedback_id:) }.to change(Feedback, :count).by(-1)
+        expect(Feedback.where(id: feedback_id)).to be_empty
+      end
+    end
+
+    context 'when deletion fails' do
+      let(:feedback_id) { 'does-not-exist' }
+
+      before do
+        allow(Sentry).to receive(:capture_exception)
+      end
+
+      it 'returns a failed operation response' do
+        response = described_class.call(feedback_id:)
+        expect(response.success?).to be(false)
+      end
+
+      it 'returns the error message in the operation response' do
+        response = described_class.call(feedback_id:)
+        expect(response[:error]).to match(/does-not-exist/)
+      end
+
+      it 'sent the exception to Sentry' do
+        described_class.call(feedback_id:)
+        expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError))
+      end
+    end
+  end
+end

spec/features/feedback/deleting_feedback_spec.rb

@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Delete feedback requests', type: :request do
+  let(:headers) { { Authorization: UserProfileMock::TOKEN } }
+  let(:school) { create(:school) }
+  let(:student) { create(:student, school:) }
+  let(:teacher) { create(:teacher, school:) }
+  let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) }
+  let(:class_student) { create(:class_student, school_class:, student_id: student.id) }
+  let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') }
+  let(:teacher_project) { create(:project, user_id: teacher.id, school:, lesson:) }
+  let(:student_project) { create(:project, user_id: class_student.student_id, school:, parent: teacher_project) }
+  let!(:feedback) { create(:feedback, school_project: student_project.school_project, user_id: teacher.id, content: 'Excellent work!') }
+
+  context 'when logged in as the class teacher' do
+    before do
+      authenticated_in_hydra_as(teacher)
+    end
+
+    context 'when deleting feedback on student work' do
+      before do
+        delete("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}", headers:)
+        student_project.reload
+      end
+
+      it 'returns no content response' do
+        expect(response).to have_http_status(:no_content)
+      end
+
+      it 'removes the feedback from the school project' do
+        expect(student_project.school_project.feedback.count).to eq(0)
+      end
+    end
+
+    context 'when attempting to delete non-existent feedback' do
+      before do
+        delete("/api/projects/#{student_project.identifier}/feedback/invalid-id", headers:)
+      end
+
+      it 'returns not found response' do
+        expect(response).to have_http_status(:not_found)
+      end
+
+      it 'returns an error message' do
+        data = JSON.parse(response.body, symbolize_names: true)
+        expect(data[:error]).to match(/Couldn't find Feedback with 'id'="invalid-id"/)
+      end
+    end
+  end
+
+  context 'when logged in as the school owner' do
+    let(:owner) { create(:owner, school:) }
+
+    before do
+      authenticated_in_hydra_as(owner)
+      delete("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}", headers:)
+      student_project.reload
+    end
+
+    it 'returns no content response' do
+      expect(response).to have_http_status(:no_content)
+    end
+
+    it 'removes the feedback from the school project' do
+      expect(student_project.school_project.feedback.count).to eq(0)
+    end
+  end
+
+  context 'when logged in as a teacher not in the class' do
+    let(:other_teacher) { create(:teacher, school:) }
+
+    before do
+      authenticated_in_hydra_as(other_teacher)
+      delete("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}", headers:)
+      student_project.reload
+    end
+
+    it 'returns forbidden response' do
+      expect(response).to have_http_status(:forbidden)
+    end
+
+    it 'does not remove the feedback from the school project' do
+      expect(student_project.school_project.feedback.count).to eq(1)
+    end
+  end
+
+  context 'when logged in as a school student' do
+    before do
+      authenticated_in_hydra_as(student)
+      delete("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}", headers:)
+    end
+
+    it 'returns forbidden response' do
+      expect(response).to have_http_status(:forbidden)
+    end
+
+    it 'does not remove the feedback from the school project' do
+      expect(student_project.school_project.feedback.count).to eq(1)
+    end
+  end
+end

spec/models/ability_spec.rb

@@ -310,6 +310,7 @@
         it { is_expected.not_to be_able_to(:create, feedback) }
         it { is_expected.to be_able_to(:read, feedback) }
         it { is_expected.to be_able_to(:set_read, feedback) }
+        it { is_expected.not_to be_able_to(:destroy, feedback) }
         it { is_expected.to be_able_to(:create, remixed_project) }
         it { is_expected.to be_able_to(:update, remixed_project) }
         it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -340,6 +341,7 @@
         it { is_expected.not_to be_able_to(:create, feedback) }
         it { is_expected.not_to be_able_to(:read, feedback) }
         it { is_expected.not_to be_able_to(:set_read, feedback) }
+        it { is_expected.not_to be_able_to(:destroy, feedback) }
         it { is_expected.not_to be_able_to(:create, remixed_project) }
         it { is_expected.not_to be_able_to(:update, remixed_project) }
         it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -357,6 +359,7 @@
         it { is_expected.to be_able_to(:create, feedback) }
         it { is_expected.to be_able_to(:read, feedback) }
         it { is_expected.not_to be_able_to(:set_read, feedback) }
+        it { is_expected.to be_able_to(:destroy, feedback) }
         it { is_expected.not_to be_able_to(:create, remixed_project) }
         it { is_expected.not_to be_able_to(:update, remixed_project) }
         it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -374,6 +377,7 @@
         it { is_expected.to be_able_to(:create, feedback) }
         it { is_expected.to be_able_to(:read, feedback) }
         it { is_expected.not_to be_able_to(:set_read, feedback) }
+        it { is_expected.to be_able_to(:destroy, feedback) }
         it { is_expected.not_to be_able_to(:create, original_project) }
         it { is_expected.to be_able_to(:update, original_project) }