Skip to content

Commit f64fe1d

Browse files
Add ability to set read at on feedback (#610)
## Status - Closes RaspberryPiFoundation/digital-editor-issues#992 ## What's changed? - Added read_at datetime to the feedback table - Created endpoint to set feedback as read - Added functionality to set a datetime on the read_at field on feedback - Added tests
1 parent 6a222df commit f64fe1d

File tree

10 files changed

+190
-6
lines changed

10 files changed

+190
-6
lines changed

app/controllers/api/feedback_controller.rb

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,18 @@ def create
3030
end
3131
end
3232

33+
def set_read
34+
feedback = Feedback.find(params[:id])
35+
result = Feedback::SetRead.call(feedback: feedback)
36+
37+
if result.success?
38+
@feedback = result[:feedback]
39+
render :show, formats: [:json], status: :ok
40+
else
41+
render json: { error: result[:error] }, status: :unprocessable_entity
42+
end
43+
end
44+
3345
private
3446

3547
def project
@@ -68,8 +80,8 @@ def feedback_create_params
6880
end
6981

7082
def url_params
71-
permitted_params = params.permit(:project_id)
72-
{ identifier: permitted_params[:project_id] }
83+
permitted_params = params.permit(:project_id, :id)
84+
{ identifier: permitted_params[:project_id], id: permitted_params[:id] }
7385
end
7486

7587
def base_params

app/models/ability.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ def define_school_student_abilities(user:, school:)
113113
can(%i[read], Lesson, school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } })
114114
can(%i[read create update], Project, school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids)
115115
can(%i[read show_context], Project, lesson: { school_id: school.id, visibility: 'students', school_class: { students: { student_id: user.id } } })
116-
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 } })
116+
can(%i[read set_read], Feedback, school_project: { project: { school_id: school.id, user_id: user.id, lesson_id: nil, remixed_from_id: visible_lesson_project_ids } })
117117
can(%i[show_finished set_finished show_status unsubmit submit], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id)
118118
end
119119

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,6 @@ json.call(
77
:user_id,
88
:content,
99
:created_at,
10-
:updated_at
10+
:updated_at,
11+
:read_at
1112
)

config/routes.rb

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,9 @@
4646
resource :remix, only: %i[show create], controller: 'projects/remixes'
4747
resources :remixes, only: %i[index], controller: 'projects/remixes'
4848
resource :images, only: %i[show create], controller: 'projects/images'
49-
resources :feedback, only: %i[index create], controller: 'feedback'
49+
resources :feedback, only: %i[index create], controller: 'feedback' do
50+
put :read, on: :member, to: 'feedback#set_read'
51+
end
5052
end
5153

5254
resource :project_errors, only: %i[create]
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
class AddReadAtToFeedback < ActiveRecord::Migration[7.2]
2+
def change
3+
add_column :feedback, :read_at, :datetime
4+
end
5+
end

db/schema.rb

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

lib/concepts/feedback/set_read.rb

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# frozen_string_literal: true
2+
3+
class Feedback
4+
class SetRead
5+
class << self
6+
def call(feedback:)
7+
response = OperationResponse.new
8+
response[:feedback] = feedback
9+
response[:feedback].read_at = Time.current
10+
response[:feedback].save!
11+
response
12+
rescue StandardError => e
13+
Sentry.capture_exception(e)
14+
response[:error] = e.message
15+
response
16+
end
17+
end
18+
end
19+
end
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
# frozen_string_literal: true
2+
3+
require 'rails_helper'
4+
5+
RSpec.describe Feedback::SetRead, type: :unit do
6+
let(:feedback) { create(:feedback) }
7+
8+
describe '.call' do
9+
context 'when set_read is successful' do
10+
it 'returns a successful operation response' do
11+
response = described_class.call(feedback: feedback)
12+
expect(response.success?).to be(true)
13+
end
14+
15+
it 'returns the updated feedback' do
16+
response = described_class.call(feedback: feedback)
17+
expect(response[:feedback]).to be_a(Feedback)
18+
end
19+
20+
it 'returns read_at' do
21+
response = described_class.call(feedback: feedback)
22+
expect(response[:feedback].read_at).to be_present
23+
end
24+
25+
it 'returns read_at as a timestamp' do
26+
response = described_class.call(feedback: feedback)
27+
expect(response[:feedback].read_at).to be_a(ActiveSupport::TimeWithZone)
28+
end
29+
end
30+
31+
context 'when set_read fails' do
32+
before do
33+
allow(feedback).to receive(:save!).and_raise(StandardError, 'Some API error')
34+
end
35+
36+
it 'returns a failed operation response' do
37+
response = described_class.call(feedback: feedback)
38+
expect(response.success?).to be(false)
39+
end
40+
41+
it 'does not persist read_at' do
42+
described_class.call(feedback: feedback)
43+
feedback.reload
44+
expect(feedback.read_at).to be_nil
45+
end
46+
47+
it 'includes the correct error response' do
48+
response = described_class.call(feedback: feedback)
49+
expect(response[:error]).to eq('Some API error')
50+
end
51+
end
52+
end
53+
end
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
# frozen_string_literal: true
2+
3+
require 'rails_helper'
4+
5+
RSpec.describe 'Set read_at on 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+
let(:feedback_json) do
17+
{
18+
id: feedback.id,
19+
school_project_id: feedback.school_project_id,
20+
user_id: feedback.user_id,
21+
content: feedback.content,
22+
created_at: feedback.created_at,
23+
updated_at: feedback.updated_at,
24+
read_at: feedback.read_at
25+
}.to_json
26+
end
27+
28+
context 'when logged in as the class teacher' do
29+
before do
30+
authenticated_in_hydra_as(teacher)
31+
put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers)
32+
feedback.reload
33+
end
34+
35+
it 'returns forbidden response' do
36+
expect(response).to have_http_status(:forbidden)
37+
end
38+
39+
it 'does not set the read_at on feedback' do
40+
expect(feedback.read_at).to be_nil
41+
end
42+
end
43+
44+
context 'when logged in as the student' do
45+
before do
46+
authenticated_in_hydra_as(student)
47+
end
48+
49+
context 'when feedback exists' do
50+
before do
51+
put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers)
52+
feedback.reload
53+
end
54+
55+
it 'returns ok response' do
56+
expect(response).to have_http_status(:ok)
57+
end
58+
59+
it 'returns the feedback json' do
60+
expect(response.body).to eq(feedback_json)
61+
end
62+
63+
it 'does set the read_at on feedback' do
64+
expect(feedback.read_at).to be_present
65+
end
66+
67+
it 'sets read_at to be a time' do
68+
expect(feedback.read_at).to be_a(ActiveSupport::TimeWithZone)
69+
end
70+
71+
it 'sets read_at to a recent time' do
72+
expect(feedback.read_at).to be_within(5.seconds).of(Time.current)
73+
end
74+
end
75+
76+
context 'when feedback does not exist' do
77+
before do
78+
put("/api/projects/#{student_project.identifier}/feedback/does-not-exist/read", headers: headers)
79+
feedback.reload
80+
end
81+
82+
it 'returns not found response' do
83+
expect(response).to have_http_status(:not_found)
84+
end
85+
end
86+
end
87+
end

spec/models/ability_spec.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,7 @@
309309
it { is_expected.to be_able_to(:read, remixed_project) }
310310
it { is_expected.not_to be_able_to(:create, feedback) }
311311
it { is_expected.to be_able_to(:read, feedback) }
312+
it { is_expected.to be_able_to(:set_read, feedback) }
312313
it { is_expected.to be_able_to(:create, remixed_project) }
313314
it { is_expected.to be_able_to(:update, remixed_project) }
314315
it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -338,6 +339,7 @@
338339
it { is_expected.not_to be_able_to(:read, remixed_project) }
339340
it { is_expected.not_to be_able_to(:create, feedback) }
340341
it { is_expected.not_to be_able_to(:read, feedback) }
342+
it { is_expected.not_to be_able_to(:set_read, feedback) }
341343
it { is_expected.not_to be_able_to(:create, remixed_project) }
342344
it { is_expected.not_to be_able_to(:update, remixed_project) }
343345
it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -354,6 +356,7 @@
354356
it { is_expected.to be_able_to(:read, remixed_project) }
355357
it { is_expected.to be_able_to(:create, feedback) }
356358
it { is_expected.to be_able_to(:read, feedback) }
359+
it { is_expected.not_to be_able_to(:set_read, feedback) }
357360
it { is_expected.not_to be_able_to(:create, remixed_project) }
358361
it { is_expected.not_to be_able_to(:update, remixed_project) }
359362
it { is_expected.not_to be_able_to(:destroy, remixed_project) }
@@ -370,6 +373,7 @@
370373
it { is_expected.to be_able_to(:read, original_project) }
371374
it { is_expected.to be_able_to(:create, feedback) }
372375
it { is_expected.to be_able_to(:read, feedback) }
376+
it { is_expected.not_to be_able_to(:set_read, feedback) }
373377
it { is_expected.not_to be_able_to(:create, original_project) }
374378
it { is_expected.to be_able_to(:update, original_project) }
375379

0 commit comments

Comments
 (0)