Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions app/controllers/api/feedback_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +33 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to 404 it if the feedback id is invalid?


private

def project
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 2 additions & 1 deletion app/views/api/feedback/_feedback.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ json.call(
:user_id,
:content,
:created_at,
:updated_at
:updated_at,
:read_at
)
4 changes: 3 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20251106170425_add_read_at_to_feedback.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddReadAtToFeedback < ActiveRecord::Migration[7.2]
def change
add_column :feedback, :read_at, :datetime
end
end
3 changes: 2 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions lib/concepts/feedback/set_read.rb
Original file line number Diff line number Diff line change
@@ -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
31 changes: 31 additions & 0 deletions spec/concepts/feedback/set_read_at_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +15 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused what this is testing to be honest. Might be simpler to test it is an instance of Feedback and then check the read at was set below like you're already doing


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
71 changes: 71 additions & 0 deletions spec/features/feedback/setting_read_at_spec.rb
Original file line number Diff line number Diff line change
@@ -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