diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb index dc445317..d96d362e 100644 --- a/app/controllers/api/feedback_controller.rb +++ b/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 ) diff --git a/app/models/ability.rb b/app/models/ability.rb index 7509cb4b..62addc81 100644 --- a/app/models/ability.rb +++ b/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 diff --git a/app/views/api/feedback/index.json.jbuilder b/app/views/api/feedback/index.json.jbuilder new file mode 100644 index 00000000..88dc6a58 --- /dev/null +++ b/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 diff --git a/config/routes.rb b/config/routes.rb index 35e45706..41fb0fc3 100644 --- a/config/routes.rb +++ b/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] diff --git a/spec/features/feedback/listing_feedback_spec.rb b/spec/features/feedback/listing_feedback_spec.rb new file mode 100644 index 00000000..a362c308 --- /dev/null +++ b/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 diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index a470a53d..f7fae799 100644 --- a/spec/models/ability_spec.rb +++ b/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) }