Skip to content

Commit d2ebba4

Browse files
authored
Index route for feedback (#604)
## Status - closes RaspberryPiFoundation/digital-editor-issues#979 ## What's changed? - Added `index` route for project feedback
1 parent ec87a3a commit d2ebba4

File tree

6 files changed

+150
-4
lines changed

6 files changed

+150
-4
lines changed

app/controllers/api/feedback_controller.rb

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,20 @@ class FeedbackController < ApiController
55
before_action :authorize_user
66
load_and_authorize_resource :feedback
77

8+
def index
9+
if project.blank? || project.school_project.blank?
10+
render json: { error: 'School project not found' }, status: :not_found
11+
return
12+
end
13+
14+
# Checks that the user is authorised to read the feedback so that if not we can return a 403 rather than an empty array
15+
project_feedback.each do |feedback|
16+
authorize! :read, feedback
17+
end
18+
@feedback = project_feedback.accessible_by(current_ability)
19+
render :index, formats: [:json], status: :ok
20+
end
21+
822
def create
923
result = Feedback::Create.call(feedback_params: feedback_create_params)
1024

@@ -16,11 +30,31 @@ def create
1630
end
1731
end
1832

33+
private
34+
35+
def project
36+
return @project if defined?(@project)
37+
38+
@project = Project.find_by(identifier: url_params[:identifier])
39+
end
40+
41+
def project_feedback
42+
return @project_feedback if defined?(@project_feedback)
43+
44+
@project_feedback = if project.blank? || project.school_project.blank?
45+
Feedback.none
46+
else
47+
Feedback.where(school_project_id: project.school_project.id)
48+
end
49+
50+
@project_feedback
51+
end
52+
1953
# These params are used to authorize the resource with CanCanCan. The project identifier is sent in the URL,
2054
# but these params need to match the shape of the feedback object whiich is attached to the SchoolProject,
2155
# not the Project.
2256
def feedback_params
23-
school_project = Project.find_by(identifier: base_params[:identifier])&.school_project
57+
school_project = project&.school_project
2458
feedback_create_params.except(:identifier).merge(
2559
school_project_id: school_project&.id
2660
)

app/models/ability.rb

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,16 +96,23 @@ def define_school_teacher_abilities(user:, school:)
9696
)
9797
).pluck(:id)
9898
can(%i[read], Project, remixed_from_id: teacher_project_ids)
99-
can(%i[create], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } })
99+
can(%i[read create], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } })
100100
end
101101

102102
def define_school_student_abilities(user:, school:)
103+
visible_lesson_project_ids = Project.where(
104+
school_id: school.id,
105+
lesson_id: Lesson.where(
106+
visibility: 'students'
107+
).select(:id)
108+
).pluck(:id)
103109
can(%i[read], School, id: school.id)
104110
can(%i[read], SchoolClass, school: { id: school.id }, students: { student_id: user.id })
105111
# Ensure no access to ClassMember resources, relationships otherwise allow access in some circumstances.
106112
can(%i[read], Lesson, school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } })
107-
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))
113+
can(%i[read create update], Project, school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids)
108114
can(%i[read show_context], Project, lesson: { school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } } })
115+
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 } })
109116
can(%i[show_finished set_finished], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id)
110117
end
111118

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# frozen_string_literal: true
2+
3+
json.array!(@feedback) do |feedback|
4+
json.partial! 'feedback', feedback:
5+
end

config/routes.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
resource :remix, only: %i[show create], controller: 'projects/remixes'
4242
resources :remixes, only: %i[index], controller: 'projects/remixes'
4343
resource :images, only: %i[show create], controller: 'projects/images'
44-
resource :feedback, only: %i[create], controller: 'feedback'
44+
resources :feedback, only: %i[index create], controller: 'feedback'
4545
end
4646

4747
resource :project_errors, only: %i[create]
Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
# frozen_string_literal: true
2+
3+
require 'rails_helper'
4+
5+
RSpec.describe 'List feedback requests', type: :request do
6+
let(:headers) { { Authorization: UserProfileMock::TOKEN } }
7+
let(:school) { create(:school) }
8+
let(:student) { create(:student, school:) }
9+
let(:teacher) { create(:teacher, school:) }
10+
let(:school_class) { create(:school_class, teacher_ids: [teacher.id], school:) }
11+
let(:class_student) { create(:class_student, school_class:, student_id: student.id) }
12+
let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') }
13+
let(:teacher_project) { create(:project, user_id: teacher.id, school:, lesson:) }
14+
let(:student_project) { create(:project, user_id: class_student.student_id, school:, parent: teacher_project) }
15+
let!(:feedback) { create(:feedback, school_project: student_project.school_project, user_id: teacher.id, content: 'Excellent work!') }
16+
17+
context 'when logged in as the class teacher' do
18+
before do
19+
authenticated_in_hydra_as(teacher)
20+
end
21+
22+
context 'when listing feedback for student work' do
23+
before do
24+
get("/api/projects/#{student_project.identifier}/feedback", headers:)
25+
end
26+
27+
it 'returns ok response' do
28+
expect(response).to have_http_status(:ok)
29+
end
30+
31+
it 'returns a list of the feedback' do
32+
data = JSON.parse(response.body, symbolize_names: true)
33+
expect(data.count).to eq(1)
34+
end
35+
36+
it 'returns the feedback json containing feedback content' do
37+
data = JSON.parse(response.body, symbolize_names: true)
38+
expect(data[0][:content]).to eq(feedback.content)
39+
end
40+
end
41+
42+
context 'when listing feedback for a project that is not student work' do
43+
before do
44+
get("/api/projects/#{teacher_project.identifier}/feedback", headers:)
45+
end
46+
47+
it 'returns ok response' do
48+
expect(response).to have_http_status(:ok)
49+
end
50+
51+
it 'returns an empty list' do
52+
data = JSON.parse(response.body, symbolize_names: true)
53+
expect(data).to be_empty
54+
end
55+
end
56+
57+
context 'when the project does not exist' do
58+
before do
59+
get('/api/projects/does-not-exist/feedback', headers:)
60+
end
61+
62+
it 'returns not found response' do
63+
expect(response).to have_http_status(:not_found)
64+
end
65+
end
66+
end
67+
68+
context 'when logged in as another teacher' do
69+
let(:other_teacher) { create(:teacher, school:) }
70+
71+
before do
72+
authenticated_in_hydra_as(other_teacher)
73+
get("/api/projects/#{student_project.identifier}/feedback", headers:)
74+
end
75+
76+
it 'returns forbidden response' do
77+
expect(response).to have_http_status(:forbidden)
78+
end
79+
end
80+
81+
context 'when logged in as the student' do
82+
before do
83+
authenticated_in_hydra_as(student)
84+
get("/api/projects/#{student_project.identifier}/feedback", headers:)
85+
end
86+
87+
it 'returns ok response' do
88+
expect(response).to have_http_status(:ok)
89+
end
90+
91+
it 'returns the feedback json containing feedback content' do
92+
data = JSON.parse(response.body, symbolize_names: true)
93+
expect(data[0][:content]).to eq(feedback.content)
94+
end
95+
end
96+
end

spec/models/ability_spec.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,7 @@
308308

309309
it { is_expected.to be_able_to(:read, remixed_project) }
310310
it { is_expected.not_to be_able_to(:create, feedback) }
311+
it { is_expected.to be_able_to(:read, feedback) }
311312
it { is_expected.to be_able_to(:create, remixed_project) }
312313
it { is_expected.to be_able_to(:update, remixed_project) }
313314
it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -332,6 +333,7 @@
332333

333334
it { is_expected.not_to be_able_to(:read, remixed_project) }
334335
it { is_expected.not_to be_able_to(:create, feedback) }
336+
it { is_expected.not_to be_able_to(:read, feedback) }
335337
it { is_expected.not_to be_able_to(:create, remixed_project) }
336338
it { is_expected.not_to be_able_to(:update, remixed_project) }
337339
it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -343,6 +345,7 @@
343345

344346
it { is_expected.to be_able_to(:read, remixed_project) }
345347
it { is_expected.to be_able_to(:create, feedback) }
348+
it { is_expected.to be_able_to(:read, feedback) }
346349
it { is_expected.not_to be_able_to(:create, remixed_project) }
347350
it { is_expected.not_to be_able_to(:update, remixed_project) }
348351
it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -354,6 +357,7 @@
354357

355358
it { is_expected.to be_able_to(:read, original_project) }
356359
it { is_expected.to be_able_to(:create, feedback) }
360+
it { is_expected.to be_able_to(:read, feedback) }
357361
it { is_expected.not_to be_able_to(:create, original_project) }
358362
it { is_expected.to be_able_to(:update, original_project) }
359363

0 commit comments

Comments
 (0)