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/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 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 diff --git a/lib/concepts/feedback/set_read.rb b/lib/concepts/feedback/set_read.rb new file mode 100644 index 00000000..503aa448 --- /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] = e.message + response + end + end + end +end 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..615782b9 --- /dev/null +++ b/spec/concepts/feedback/set_read_at_spec.rb @@ -0,0 +1,53 @@ +# 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 be_a(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 + + context 'when set_read fails' do + before do + allow(feedback).to receive(:save!).and_raise(StandardError, 'Some API error') + end + + it 'returns a failed operation response' do + response = described_class.call(feedback: feedback) + expect(response.success?).to be(false) + 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 the correct error response' do + response = described_class.call(feedback: feedback) + expect(response[:error]).to eq('Some API error') + end + end + end +end 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..8065d57e --- /dev/null +++ b/spec/features/feedback/setting_read_at_spec.rb @@ -0,0 +1,87 @@ +# 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) + 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 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 + + 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 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) }