From 3218e969bc960c5221c7264d46492e61ef22c770 Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Thu, 6 Nov 2025 17:05:57 +0000 Subject: [PATCH 1/8] Create migration --- db/migrate/20251106170425_add_read_at_to_feedback.rb | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 db/migrate/20251106170425_add_read_at_to_feedback.rb diff --git a/db/migrate/20251106170425_add_read_at_to_feedback.rb b/db/migrate/20251106170425_add_read_at_to_feedback.rb new file mode 100644 index 00000000..ae53467c --- /dev/null +++ b/db/migrate/20251106170425_add_read_at_to_feedback.rb @@ -0,0 +1,5 @@ +class AddReadAtToFeedback < ActiveRecord::Migration[7.2] + def change + add_column :feedback, :read_at, :datetime + end +end From d50910e9eb03397927b0e9804923ad23315ef34c Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Thu, 6 Nov 2025 17:06:10 +0000 Subject: [PATCH 2/8] Run migration to add read_at to feedback --- db/schema.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index b78deede..6a8bf9d8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_10_31_122812) do +ActiveRecord::Schema[7.2].define(version: 2025_11_06_170425) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -80,6 +80,7 @@ t.uuid "user_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "read_at" t.index ["school_project_id"], name: "index_feedback_on_school_project_id" end From 25114a3601a84f3ecec0cec47cdb5c13cee7ea12 Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Fri, 7 Nov 2025 14:37:37 +0000 Subject: [PATCH 3/8] Create read endpoint, add functionality --- app/controllers/api/feedback_controller.rb | 16 ++++++++++++++-- app/models/ability.rb | 2 +- .../api/feedback/_feedback.json.jbuilder | 3 ++- config/routes.rb | 4 +++- lib/concepts/feedback/set_read.rb | 19 +++++++++++++++++++ 5 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 lib/concepts/feedback/set_read.rb diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb index d96d362e..280cfafd 100644 --- a/app/controllers/api/feedback_controller.rb +++ b/app/controllers/api/feedback_controller.rb @@ -30,6 +30,18 @@ def create end end + def set_read + feedback = Feedback.find(params[:id]) + result = Feedback::SetRead.call(feedback: feedback) + + if result.success? + @feedback = result[:feedback] + render :show, formats: [:json], status: :ok + else + render json: { error: result[:error] }, status: :unprocessable_entity + end + end + private def project @@ -68,8 +80,8 @@ def feedback_create_params end def url_params - permitted_params = params.permit(:project_id) - { identifier: permitted_params[:project_id] } + permitted_params = params.permit(:project_id, :id) + { identifier: permitted_params[:project_id], id: permitted_params[:id] } end def base_params diff --git a/app/models/ability.rb b/app/models/ability.rb index 5640518a..29bd7073 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -113,7 +113,7 @@ def define_school_student_abilities(user:, school:) 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: 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[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 } }) can(%i[show_finished set_finished show_status unsubmit submit], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id) end diff --git a/app/views/api/feedback/_feedback.json.jbuilder b/app/views/api/feedback/_feedback.json.jbuilder index 8eccd576..e0c59d88 100644 --- a/app/views/api/feedback/_feedback.json.jbuilder +++ b/app/views/api/feedback/_feedback.json.jbuilder @@ -7,5 +7,6 @@ json.call( :user_id, :content, :created_at, - :updated_at + :updated_at, + :read_at ) diff --git a/config/routes.rb b/config/routes.rb index affbe044..37318e0f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -46,7 +46,9 @@ 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' - resources :feedback, only: %i[index create], controller: 'feedback' + resources :feedback, only: %i[index create], controller: 'feedback' do + put :read, on: :member, to: 'feedback#set_read' + end end resource :project_errors, only: %i[create] diff --git a/lib/concepts/feedback/set_read.rb b/lib/concepts/feedback/set_read.rb new file mode 100644 index 00000000..59efb79b --- /dev/null +++ b/lib/concepts/feedback/set_read.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class Feedback + class SetRead + class << self + def call(feedback:) + response = OperationResponse.new + response[:feedback] = feedback + response[:feedback].read_at = Time.current + response[:feedback].save! + response + rescue StandardError => e + Sentry.capture_exception(e) + response[:error] = response[:feedback]&.errors + response + end + end + end +end From 3b059e97aff16da20ea8e69d13574030d5b1e21b Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Fri, 7 Nov 2025 14:37:45 +0000 Subject: [PATCH 4/8] Add api route tests --- .../features/feedback/setting_read_at_spec.rb | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 spec/features/feedback/setting_read_at_spec.rb diff --git a/spec/features/feedback/setting_read_at_spec.rb b/spec/features/feedback/setting_read_at_spec.rb new file mode 100644 index 00000000..64ed4dd5 --- /dev/null +++ b/spec/features/feedback/setting_read_at_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Set read_at on 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!') } + let(:feedback_json) do + { + id: feedback.id, + school_project_id: feedback.school_project_id, + user_id: feedback.user_id, + content: feedback.content, + created_at: feedback.created_at, + updated_at: feedback.updated_at, + read_at: feedback.read_at + }.to_json + end + + context 'when logged in as the class teacher' do + before do + authenticated_in_hydra_as(teacher) + put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers) + feedback.reload + end + + it 'returns forbidden response' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not set the read_at on feedback' do + expect(feedback.read_at).to be_nil + end + end + + context 'when logged in as the student' do + before do + authenticated_in_hydra_as(student) + put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers) + feedback.reload + end + + it 'returns ok response' do + expect(response).to have_http_status(:ok) + end + + it 'returns the feedback json' do + expect(response.body).to eq(feedback_json) + end + + it 'does set the read_at on feedback' do + expect(feedback.read_at).to be_present + end + + it 'sets read_at to be a time' do + expect(feedback.read_at).to be_a(ActiveSupport::TimeWithZone) + end + + it 'sets read_at to a recent time' do + expect(feedback.read_at).to be_within(5.seconds).of(Time.current) + end + end +end From 74e82ebbe9e079928914ba36b3fc380093d6fe1e Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Fri, 7 Nov 2025 15:06:26 +0000 Subject: [PATCH 5/8] Add test file for set_read --- spec/concepts/feedback/set_read_at_spec.rb | 31 ++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 spec/concepts/feedback/set_read_at_spec.rb diff --git a/spec/concepts/feedback/set_read_at_spec.rb b/spec/concepts/feedback/set_read_at_spec.rb new file mode 100644 index 00000000..a1a26f43 --- /dev/null +++ b/spec/concepts/feedback/set_read_at_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Feedback::SetRead, type: :unit do + let(:feedback) { create(:feedback) } + + describe '.call' do + context 'when set_read is successful' do + it 'returns a successful operation response' do + response = described_class.call(feedback: feedback) + expect(response.success?).to be(true) + end + + it 'returns the updated feedback' do + response = described_class.call(feedback: feedback) + expect(response[:feedback]).to eq(feedback) + end + + it 'returns read_at' do + response = described_class.call(feedback: feedback) + expect(response[:feedback].read_at).to be_present + end + + it 'returns read_at as a timestamp' do + response = described_class.call(feedback: feedback) + expect(response[:feedback].read_at).to be_a(ActiveSupport::TimeWithZone) + end + end + end +end From 27bd784c94b084bac7b0b99631cedc11d695971c Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Fri, 7 Nov 2025 15:27:29 +0000 Subject: [PATCH 6/8] Add tests around when set_read fails --- spec/concepts/feedback/set_read_at_spec.rb | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/spec/concepts/feedback/set_read_at_spec.rb b/spec/concepts/feedback/set_read_at_spec.rb index a1a26f43..59c0161a 100644 --- a/spec/concepts/feedback/set_read_at_spec.rb +++ b/spec/concepts/feedback/set_read_at_spec.rb @@ -27,5 +27,32 @@ expect(response[:feedback].read_at).to be_a(ActiveSupport::TimeWithZone) end end + + context 'when set_read fails' do + before do + allow(feedback).to receive(:save!).and_raise(StandardError, 'Transition failed') + end + + it 'returns a failed operation response' do + response = described_class.call(feedback: feedback) + expect(response.success?).to be(false) + end + + it 'returns the original feedback' do + response = described_class.call(feedback: feedback) + expect(response[:feedback]).to eq(feedback) + end + + it 'does not persist read_at' do + described_class.call(feedback: feedback) + feedback.reload + expect(feedback.read_at).to be_nil + end + + it 'includes an error object in the response' do + response = described_class.call(feedback: feedback) + expect(response[:error]).to be_a(ActiveModel::Errors) + end + end end end From 27a1438f717eb4bf7224362ae2a3efe5028c82d9 Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Fri, 7 Nov 2025 16:07:16 +0000 Subject: [PATCH 7/8] Add set_read tests to ability_spec --- spec/models/ability_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index adc35c3d..d7f94254 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -309,6 +309,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(:set_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) } @@ -338,6 +339,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(:set_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 +356,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(:set_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) } @@ -370,6 +373,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(:set_read, feedback) } it { is_expected.not_to be_able_to(:create, original_project) } it { is_expected.to be_able_to(:update, original_project) } From 1c1b1770b2f22b13a33cdb9a3544895862687d28 Mon Sep 17 00:00:00 2001 From: Jamie Benstead Date: Mon, 10 Nov 2025 12:43:34 +0000 Subject: [PATCH 8/8] Update error response and tests --- lib/concepts/feedback/set_read.rb | 2 +- spec/concepts/feedback/set_read_at_spec.rb | 13 ++---- .../features/feedback/setting_read_at_spec.rb | 46 +++++++++++++------ 3 files changed, 36 insertions(+), 25 deletions(-) diff --git a/lib/concepts/feedback/set_read.rb b/lib/concepts/feedback/set_read.rb index 59efb79b..503aa448 100644 --- a/lib/concepts/feedback/set_read.rb +++ b/lib/concepts/feedback/set_read.rb @@ -11,7 +11,7 @@ def call(feedback:) response rescue StandardError => e Sentry.capture_exception(e) - response[:error] = response[:feedback]&.errors + response[:error] = e.message response end end diff --git a/spec/concepts/feedback/set_read_at_spec.rb b/spec/concepts/feedback/set_read_at_spec.rb index 59c0161a..615782b9 100644 --- a/spec/concepts/feedback/set_read_at_spec.rb +++ b/spec/concepts/feedback/set_read_at_spec.rb @@ -14,7 +14,7 @@ it 'returns the updated feedback' do response = described_class.call(feedback: feedback) - expect(response[:feedback]).to eq(feedback) + expect(response[:feedback]).to be_a(Feedback) end it 'returns read_at' do @@ -30,7 +30,7 @@ context 'when set_read fails' do before do - allow(feedback).to receive(:save!).and_raise(StandardError, 'Transition failed') + allow(feedback).to receive(:save!).and_raise(StandardError, 'Some API error') end it 'returns a failed operation response' do @@ -38,20 +38,15 @@ expect(response.success?).to be(false) end - it 'returns the original feedback' do - response = described_class.call(feedback: feedback) - expect(response[:feedback]).to eq(feedback) - end - it 'does not persist read_at' do described_class.call(feedback: feedback) feedback.reload expect(feedback.read_at).to be_nil end - it 'includes an error object in the response' do + it 'includes the correct error response' do response = described_class.call(feedback: feedback) - expect(response[:error]).to be_a(ActiveModel::Errors) + expect(response[:error]).to eq('Some API error') end end end diff --git a/spec/features/feedback/setting_read_at_spec.rb b/spec/features/feedback/setting_read_at_spec.rb index 64ed4dd5..8065d57e 100644 --- a/spec/features/feedback/setting_read_at_spec.rb +++ b/spec/features/feedback/setting_read_at_spec.rb @@ -44,28 +44,44 @@ context 'when logged in as the student' do before do authenticated_in_hydra_as(student) - put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers) - feedback.reload end - it 'returns ok response' do - expect(response).to have_http_status(:ok) - end + context 'when feedback exists' do + before do + put("/api/projects/#{student_project.identifier}/feedback/#{feedback.id}/read", headers: headers) + feedback.reload + end - it 'returns the feedback json' do - expect(response.body).to eq(feedback_json) - end + it 'returns ok response' do + expect(response).to have_http_status(:ok) + end - it 'does set the read_at on feedback' do - expect(feedback.read_at).to be_present - end + it 'returns the feedback json' do + expect(response.body).to eq(feedback_json) + end + + it 'does set the read_at on feedback' do + expect(feedback.read_at).to be_present + end - it 'sets read_at to be a time' do - expect(feedback.read_at).to be_a(ActiveSupport::TimeWithZone) + it 'sets read_at to be a time' do + expect(feedback.read_at).to be_a(ActiveSupport::TimeWithZone) + end + + it 'sets read_at to a recent time' do + expect(feedback.read_at).to be_within(5.seconds).of(Time.current) + end end - it 'sets read_at to a recent time' do - expect(feedback.read_at).to be_within(5.seconds).of(Time.current) + context 'when feedback does not exist' do + before do + put("/api/projects/#{student_project.identifier}/feedback/does-not-exist/read", headers: headers) + feedback.reload + end + + it 'returns not found response' do + expect(response).to have_http_status(:not_found) + end end end end