Index route for feedback

app/controllers/api/feedback_controller.rb

@@ -5,6 +5,20 @@ class FeedbackController < ApiController
     before_action :authorize_user
     load_and_authorize_resource :feedback
 
+    def index
+      if project.blank? || project.school_project.blank?
+        render json: { error: 'School project not found' }, status: :not_found
+        return
+      end
+
+      # Checks that the user is authorised to read the feedback so that if not we can return a 403 rather than an empty array
+      project_feedback.each do |feedback|
+        authorize! :read, feedback
+      end
+      @feedback = project_feedback.accessible_by(current_ability)
+      render :index, formats: [:json], status: :ok
+    end
+
     def create
       result = Feedback::Create.call(feedback_params: feedback_create_params)
 
@@ -16,11 +30,31 @@ def create
       end
     end
 
+    private
+
+    def project
+      return @project if defined?(@project)
+
+      @project = Project.find_by(identifier: url_params[:identifier])
+    end
+
+    def project_feedback
+      return @project_feedback if defined?(@project_feedback)
+
+      @project_feedback = if project.blank? || project.school_project.blank?
+                            Feedback.none
+                          else
+                            Feedback.where(school_project_id: project.school_project.id)
+                          end
+
+      @project_feedback
+    end
+
     # These params are used to authorize the resource with CanCanCan. The project identifier is sent in the URL,
     # but these params need to match the shape of the feedback object whiich is attached to the SchoolProject,
     # not the Project.
     def feedback_params
-      school_project = Project.find_by(identifier: base_params[:identifier])&.school_project
+      school_project = project&.school_project
       feedback_create_params.except(:identifier).merge(
         school_project_id: school_project&.id
       )

app/models/ability.rb

@@ -96,16 +96,23 @@ def define_school_teacher_abilities(user:, school:)
       )
     ).pluck(:id)
     can(%i[read], Project, remixed_from_id: teacher_project_ids)
-    can(%i[create], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } })
+    can(%i[read create], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } })
   end
 
   def define_school_student_abilities(user:, school:)
+    visible_lesson_project_ids = Project.where(
+      school_id: school.id,
+      lesson_id: Lesson.where(
+        visibility: 'students'
+      ).select(:id)
+    ).pluck(:id)
     can(%i[read], School, id: school.id)
     can(%i[read], SchoolClass, school: { id: school.id }, students: { student_id: user.id })
     # Ensure no access to ClassMember resources, relationships otherwise allow access in some circumstances.
     can(%i[read], Lesson, school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } })
-    can(%i[read create update], Project, school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: Project.where(school_id: school.id, lesson_id: Lesson.where(visibility: 'students').select(:id)).pluck(:id))
+    can(%i[read create update], Project, school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids)
     can(%i[read show_context], Project, lesson: { school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } } })
+    can(%i[read], Feedback, school_project: { project: { school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids } })
     can(%i[show_finished set_finished], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id)
   end
 

app/views/api/feedback/index.json.jbuilder

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+json.array!(@feedback) do |feedback|
+  json.partial! 'feedback', feedback:
+end

config/routes.rb

@@ -41,7 +41,7 @@
       resource :remix, only: %i[show create], controller: 'projects/remixes'
       resources :remixes, only: %i[index], controller: 'projects/remixes'
       resource :images, only: %i[show create], controller: 'projects/images'
-      resource :feedback, only: %i[create], controller: 'feedback'
+      resources :feedback, only: %i[index create], controller: 'feedback'
     end
 
     resource :project_errors, only: %i[create]

spec/features/feedback/listing_feedback_spec.rb

@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'List 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 listing feedback for student work' do
+      before do
+        get("/api/projects/#{student_project.identifier}/feedback", headers:)
+      end
+
+      it 'returns ok response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'returns a list of the feedback' do
+        data = JSON.parse(response.body, symbolize_names: true)
+        expect(data.count).to eq(1)
+      end
+
+      it 'returns the feedback json containing feedback content' do
+        data = JSON.parse(response.body, symbolize_names: true)
+        expect(data[0][:content]).to eq(feedback.content)
+      end
+    end
+
+    context 'when listing feedback for a project that is not student work' do
+      before do
+        get("/api/projects/#{teacher_project.identifier}/feedback", headers:)
+      end
+
+      it 'returns ok response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'returns an empty list' do
+        data = JSON.parse(response.body, symbolize_names: true)
+        expect(data).to be_empty
+      end
+    end
+
+    context 'when the project does not exist' do
+      before do
+        get('/api/projects/does-not-exist/feedback', headers:)
+      end
+
+      it 'returns not found response' do
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+  end
+
+  context 'when logged in as another teacher' do
+    let(:other_teacher) { create(:teacher, school:) }
+
+    before do
+      authenticated_in_hydra_as(other_teacher)
+      get("/api/projects/#{student_project.identifier}/feedback", headers:)
+    end
+
+    it 'returns forbidden response' do
+      expect(response).to have_http_status(:forbidden)
+    end
+  end
+
+  context 'when logged in as the student' do
+    before do
+      authenticated_in_hydra_as(student)
+      get("/api/projects/#{student_project.identifier}/feedback", headers:)
+    end
+
+    it 'returns ok response' do
+      expect(response).to have_http_status(:ok)
+    end
+
+    it 'returns the feedback json containing feedback content' do
+      data = JSON.parse(response.body, symbolize_names: true)
+      expect(data[0][:content]).to eq(feedback.content)
+    end
+  end
+end

spec/models/ability_spec.rb

@@ -308,6 +308,7 @@
 
         it { is_expected.to be_able_to(:read, remixed_project) }
         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(:create, remixed_project) }
         it { is_expected.to be_able_to(:update, remixed_project) }
         it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -332,6 +333,7 @@
 
         it { is_expected.not_to be_able_to(:read, remixed_project) }
         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(: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) }
@@ -343,6 +345,7 @@
 
         it { is_expected.to be_able_to(:read, remixed_project) }
         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(: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) }
@@ -354,6 +357,7 @@
 
         it { is_expected.to be_able_to(:read, original_project) }
         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(:create, original_project) }
         it { is_expected.to be_able_to(:update, original_project) }