diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb new file mode 100644 index 00000000..dc445317 --- /dev/null +++ b/app/controllers/api/feedback_controller.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Api + class FeedbackController < ApiController + before_action :authorize_user + load_and_authorize_resource :feedback + + def create + result = Feedback::Create.call(feedback_params: feedback_create_params) + + if result.success? + @feedback = result[:feedback] + render :show, formats: [:json], status: :created + else + render json: { error: result[:error] }, status: :unprocessable_entity + end + end + + # These params are used to authorize the resource with CanCanCan. The project identifier is sent in the URL, + # but these params need to match the shape of the feedback object whiich is attached to the SchoolProject, + # not the Project. + def feedback_params + school_project = Project.find_by(identifier: base_params[:identifier])&.school_project + feedback_create_params.except(:identifier).merge( + school_project_id: school_project&.id + ) + end + + # These params are used to create the feedback in the Feedback::Create operation. The project_id parameter, + # which is automatically named by Rails based on the route structure, is renamed to identifier for readability, + # as it is actually the human-readable project_identifier, not the project_id. + def feedback_create_params + base_params.merge(user_id: current_user.id) + end + + def url_params + permitted_params = params.permit(:project_id) + { identifier: permitted_params[:project_id] } + end + + def base_params + params.fetch(:feedback, {}).permit( + :content + ).merge(url_params) + end + end +end diff --git a/app/models/ability.rb b/app/models/ability.rb index 422b8f4f..7509cb4b 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -88,8 +88,15 @@ def define_school_teacher_abilities(user:, school:) school_teacher_can_manage_project?(user:, school:, project:) end can(%i[read update show_context], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) - can(%i[read], Project, - remixed_from_id: Project.where(school_id: school.id, remixed_from_id: nil, lesson_id: Lesson.where(school_class_id: ClassTeacher.where(teacher_id: user.id).select(:school_class_id))).pluck(:id)) + teacher_project_ids = Project.where( + school_id: school.id, + remixed_from_id: nil, + lesson_id: Lesson.where( + school_class_id: ClassTeacher.where(teacher_id: user.id).select(:school_class_id) + ) + ).pluck(:id) + can(%i[read], Project, remixed_from_id: teacher_project_ids) + can(%i[create], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } }) end def define_school_student_abilities(user:, school:) diff --git a/app/models/feedback.rb b/app/models/feedback.rb new file mode 100644 index 00000000..45ca2d21 --- /dev/null +++ b/app/models/feedback.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +class Feedback < ApplicationRecord + belongs_to :school_project + validates :content, presence: true + validates :user_id, presence: true + validate :user_has_the_school_owner_or_school_teacher_role_for_the_school + validate :parent_project_belongs_to_lesson + validate :parent_project_belongs_to_school_class + validate :user_is_the_class_teacher_for_the_school_project + + has_paper_trail( + meta: { + meta_school_project_id: ->(f) { f.school_project&.id }, + meta_school_id: ->(f) { f.school_project&.school_id } + } + ) + + private + + def user_has_the_school_owner_or_school_teacher_role_for_the_school + school = school_project&.school + return unless user_id_changed? && errors.blank? && school + + user = User.new(id: user_id) + return if user.school_owner?(school) + return if user.school_teacher?(school) + + msg = "'#{user_id}' does not have the 'school-owner' or 'school-teacher' role for organisation '#{school.id}'" + errors.add(:user, msg) + end + + def parent_project_belongs_to_lesson + parent_project = school_project&.project&.parent + return if parent_project&.lesson_id.present? + + msg = "Parent project '#{parent_project&.id}' does not belong to a 'lesson'" + errors.add(:user, msg) + end + + def parent_project_belongs_to_school_class + parent_project = school_project&.project&.parent + return if parent_project&.lesson&.school_class_id.present? + + msg = "Parent project '#{parent_project&.id}' does not belong to a 'school-class'" + errors.add(:user, msg) + end + + def user_is_the_class_teacher_for_the_school_project + return if !school_project || school_class&.teacher_ids&.include?(user_id) + + errors.add(:user, "'#{user_id}' is not the 'school-teacher' for school_project '#{school_project.id}'") + end + + def school_class + school_project&.project&.parent&.lesson&.school_class + end +end diff --git a/app/models/school_project.rb b/app/models/school_project.rb index 1ce8df1f..cc19ad28 100644 --- a/app/models/school_project.rb +++ b/app/models/school_project.rb @@ -3,4 +3,5 @@ class SchoolProject < ApplicationRecord belongs_to :school belongs_to :project + has_many :feedback, dependent: :destroy end diff --git a/app/views/api/feedback/_feedback.json.jbuilder b/app/views/api/feedback/_feedback.json.jbuilder new file mode 100644 index 00000000..8eccd576 --- /dev/null +++ b/app/views/api/feedback/_feedback.json.jbuilder @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +json.call( + feedback, + :id, + :school_project_id, + :user_id, + :content, + :created_at, + :updated_at +) diff --git a/app/views/api/feedback/show.json.jbuilder b/app/views/api/feedback/show.json.jbuilder new file mode 100644 index 00000000..8e369213 --- /dev/null +++ b/app/views/api/feedback/show.json.jbuilder @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +json.partial! 'feedback', feedback: @feedback diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb index b8c144e3..4af99737 100644 --- a/config/initializers/inflections.rb +++ b/config/initializers/inflections.rb @@ -12,6 +12,10 @@ # inflect.uncountable %w( fish sheep ) # end +ActiveSupport::Inflector.inflections(:en) do |inflect| + inflect.uncountable %w[feedback] +end + # These inflection rules are supported but not enabled by default: ActiveSupport::Inflector.inflections(:en) do |inflect| inflect.acronym 'SSO' diff --git a/config/routes.rb b/config/routes.rb index 4b576fc3..35e45706 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -41,6 +41,7 @@ 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' + resource :feedback, only: %i[create], controller: 'feedback' end resource :project_errors, only: %i[create] diff --git a/db/migrate/20251008161139_create_feedback.rb b/db/migrate/20251008161139_create_feedback.rb new file mode 100644 index 00000000..30578888 --- /dev/null +++ b/db/migrate/20251008161139_create_feedback.rb @@ -0,0 +1,10 @@ +class CreateFeedback < ActiveRecord::Migration[7.2] + def change + create_table :feedback, id: :uuid do |t| + t.references :school_project, foreign_key: true, type: :uuid + t.text :content + t.uuid :user_id, null: false + t.timestamps + end + end +end diff --git a/db/migrate/20251021162845_add_meta_school_project_id_to_versions.rb b/db/migrate/20251021162845_add_meta_school_project_id_to_versions.rb new file mode 100644 index 00000000..c04d0523 --- /dev/null +++ b/db/migrate/20251021162845_add_meta_school_project_id_to_versions.rb @@ -0,0 +1,5 @@ +class AddMetaSchoolProjectIdToVersions < ActiveRecord::Migration[7.2] + def change + add_column :versions, :meta_school_project_id, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 0012b362..c355edcb 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_15_113652) do +ActiveRecord::Schema[7.2].define(version: 2025_10_21_162845) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -74,6 +74,15 @@ t.index ["project_id"], name: "index_components_on_project_id" end + create_table "feedback", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "school_project_id" + t.text "content" + t.uuid "user_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["school_project_id"], name: "index_feedback_on_school_project_id" + end + create_table "good_job_batches", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false @@ -306,6 +315,7 @@ t.uuid "meta_project_id" t.uuid "meta_school_id" t.uuid "meta_remixed_from_id" + t.string "meta_school_project_id" t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id" end @@ -314,6 +324,7 @@ add_foreign_key "class_students", "school_classes" add_foreign_key "class_teachers", "school_classes" add_foreign_key "components", "projects" + add_foreign_key "feedback", "school_projects" add_foreign_key "lessons", "lessons", column: "copied_from_id" add_foreign_key "lessons", "school_classes" add_foreign_key "lessons", "schools" diff --git a/lib/concepts/feedback/create.rb b/lib/concepts/feedback/create.rb new file mode 100644 index 00000000..5aa962e7 --- /dev/null +++ b/lib/concepts/feedback/create.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class Feedback + class Create + class << self + def call(feedback_params:) + response = OperationResponse.new + response[:feedback] = build_feedback(feedback_params) + response[:feedback].save! + response + rescue StandardError => e + Sentry.capture_exception(e) + if response[:feedback].present? && response[:feedback].errors.any? + errors = response[:feedback]&.errors&.full_messages&.join(',') + response[:error] = "Error creating feedback: #{errors}" + else + response[:error] = "Error creating feedback: #{e.message}" + end + response + end + + private + + def build_feedback(feedback_hash) + project = Project.find_by(identifier: feedback_hash[:identifier]) + school_project = project&.school_project + + # replace identifier with school_project_id + feedback_hash[:school_project_id] = school_project&.id + feedback_hash.delete(:identifier) + Feedback.new(feedback_hash) + end + end + end +end diff --git a/spec/concepts/feedback/create_spec.rb b/spec/concepts/feedback/create_spec.rb new file mode 100644 index 00000000..d70a66d3 --- /dev/null +++ b/spec/concepts/feedback/create_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Feedback::Create, type: :unit do + 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) } + let(:teacher_project) { create(:project, user_id: teacher.id, school:, lesson:) } + let(:student_project) { create(:project, user_id: class_student.student_id, school:, lesson:, parent: teacher_project) } + + let(:feedback_params) do + { + content: 'Great job!', + user_id: teacher.id, + identifier: student_project.identifier + } + end + + context 'when a teacher' do + before do + allow(User).to receive(:from_userinfo).with(ids: teacher.id).and_return([teacher]) + end + + it 'returns a successful operation response' do + response = described_class.call(feedback_params:) + expect(response.success?).to be(true) + end + + it 'creates a piece of feedback' do + expect { described_class.call(feedback_params:) }.to change(Feedback, :count).by(1) + end + + it 'returns the feedback in the operation response' do + response = described_class.call(feedback_params:) + expect(response[:feedback]).to be_a(Feedback) + end + + it 'assigns the content' do + response = described_class.call(feedback_params:) + expect(response[:feedback].content).to eq('Great job!') + end + + it 'assigns the user_id' do + response = described_class.call(feedback_params:) + expect(response[:feedback].user_id).to eq(teacher.id) + end + + it 'assigns the school_project_id' do + response = described_class.call(feedback_params:) + expect(response[:feedback].school_project_id).to eq(student_project.school_project.id) + end + end + + context 'when feedback creation fails' do + let(:rogue_project) { create(:project, user_id: student.id) } + let(:feedback_params) do + { + content: nil, + user_id: teacher.id, + identifier: rogue_project.identifier + } + end + + before do + allow(Sentry).to receive(:capture_exception) + end + + it 'does not create feedback' do + expect { described_class.call(feedback_params:) }.not_to change(Feedback, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(feedback_params:) + expect(response.failure?).to be(true) + end + + it 'returns the error message in the operation response' do + response = described_class.call(feedback_params:) + expect(response[:error]).to match(/Error creating feedback/) + end + + it 'raises school project not found error when no school project' do + response = described_class.call(feedback_params:) + expect(response[:error]).to match(/School project must exist/) + end + + it 'sent the exception to Sentry' do + described_class.call(feedback_params:) + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end + end +end diff --git a/spec/factories/feedback.rb b/spec/factories/feedback.rb new file mode 100644 index 00000000..1c76e714 --- /dev/null +++ b/spec/factories/feedback.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :feedback do + content { Faker::Lorem.paragraph } + user_id { nil } + school_project { nil } + + after(:build) do |feedback, _evaluator| + school = create(:school) + teacher = create(:teacher, school: school) + student = create(:student, school: school) + school_class = create(:school_class, school: school, teacher_ids: [teacher.id]) + create(:class_student, school_class: school_class, student_id: student.id) + parent_project = create(:project, user_id: teacher.id, school: school, lesson: create(:lesson, school_class: school_class, user_id: teacher.id)) + student_project = create(:project, parent: parent_project, user_id: student.id) + + feedback.user_id ||= teacher.id + feedback.school_project ||= create(:school_project, school: school, project: student_project) + end + end +end diff --git a/spec/features/feedback/creating_feedback_spec.rb b/spec/features/feedback/creating_feedback_spec.rb new file mode 100644 index 00000000..51d7b868 --- /dev/null +++ b/spec/features/feedback/creating_feedback_spec.rb @@ -0,0 +1,125 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Create 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) } + let(:teacher_project) { create(:project, user_id: teacher.id, school:, lesson:) } + let(:student_project) { create(:project, user_id: class_student.student_id, school:, lesson:, parent: teacher_project) } + + context 'when logged in as the class teacher' do + before do + authenticated_in_hydra_as(teacher) + end + + context 'when leaving feedback on student work' do + before do + post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } }) + student_project.reload + end + + it 'returns created response' do + expect(response).to have_http_status(:created) + end + + it 'returns the feedback json containing feedback content' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:content]).to eq('Nice one!') + end + + it 'returns the feedback json containing school project id' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:school_project_id]).to eq(student_project.school_project.id) + end + + it 'returns the feedback json containing user id' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:user_id]).to eq(teacher.id) + end + + it 'adds the feedback to the school project' do + expect(student_project.school_project.feedback.count).to eq(1) + end + end + + context 'when leaving feedback on a project that is not student work' do + before do + post("/api/projects/#{teacher_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } }) + teacher_project.reload + end + + it 'returns forbidden response' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not add the feedback to the school project' do + expect(teacher_project.school_project.feedback.count).to eq(0) + end + end + + context 'when leaving empty feedback' do + before do + post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: '' } }) + student_project.reload + end + + it 'returns unprocessable entity response' do + expect(response).to have_http_status(:unprocessable_entity) + end + + it 'does not add the feedback to the school project' do + expect(student_project.school_project.feedback.count).to eq(0) + end + end + + context 'when the project does not exist' do + before do + post('/api/projects/does-not-exist/feedback', headers:, params: { feedback: { content: 'Nice one!' } }) + end + + it 'returns forbidden response' do + expect(response).to have_http_status(:forbidden) + end + end + end + + context 'when logged in as another teacher' do + let(:other_teacher) { create(:teacher, school:) } + + before do + authenticated_in_hydra_as(other_teacher) + post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } }) + student_project.reload + end + + it 'returns forbidden response' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not add the feedback to the school project' do + expect(student_project.school_project.feedback.count).to eq(0) + end + end + + context 'when logged in as the student' do + before do + authenticated_in_hydra_as(student) + post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } }) + student_project.reload + end + + it 'returns forbidden response' do + expect(response).to have_http_status(:forbidden) + end + + it 'does not add the feedback to the school project' do + expect(student_project.school_project.feedback.count).to eq(0) + end + end +end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index cfa09e54..a470a53d 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -301,11 +301,13 @@ let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') } let(:original_project) { create(:project, school:, lesson:, user_id: teacher.id) } let!(:remixed_project) { create(:project, school:, user_id: student.id, remixed_from_id: original_project.id) } + let(:feedback) { build(:feedback, user_id:, school_project: remixed_project&.school_project) } context 'when user is the student' do let(:user) { student } 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(:create, remixed_project) } it { is_expected.to be_able_to(:update, remixed_project) } it { is_expected.not_to be_able_to(:destroy, remixed_project) } @@ -329,6 +331,7 @@ let(:user) { create(:teacher, school:) } 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(: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) } @@ -339,6 +342,7 @@ let(:user) { teacher } it { is_expected.to be_able_to(:read, remixed_project) } + it { is_expected.to be_able_to(:create, 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) } @@ -349,6 +353,7 @@ let(:user) { another_teacher } it { is_expected.to be_able_to(:read, original_project) } + it { is_expected.to be_able_to(:create, feedback) } it { is_expected.not_to be_able_to(:create, original_project) } it { is_expected.to be_able_to(:update, original_project) } diff --git a/spec/models/feedback_spec.rb b/spec/models/feedback_spec.rb new file mode 100644 index 00000000..9c3c7250 --- /dev/null +++ b/spec/models/feedback_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Feedback do + before do + stub_user_info_api_for(teacher) + end + + let(:teacher) { create(:teacher, school:, name: 'School Teacher') } + let(:school) { create(:school) } + + describe 'associations' do + it { is_expected.to belong_to(:school_project) } + end + + describe 'validations', :versioning do + it { is_expected.to validate_presence_of(:content) } + it { is_expected.to validate_presence_of(:user_id) } + + it 'validates that the user has the school-owner or school-teacher role for the school' do + feedback = build(:feedback, user_id: SecureRandom.uuid) + expect(feedback).not_to be_valid + expect(feedback.errors[:user]).to include(/does not have the 'school-owner' or 'school-teacher' role/) + end + + it 'validates that the parent project belongs to a lesson' do + school_project = create(:school_project, school:, project: create(:project)) + feedback = build(:feedback, school_project:, user_id: teacher.id) + expect(feedback).not_to be_valid + expect(feedback.errors[:user]).to include(/Parent project '.*' does not belong to a 'lesson'/) + end + + it 'validates that the parent project belongs to a school class' do + parent_project = create(:project, user_id: teacher.id, school:, lesson: create(:lesson, school:, user_id: teacher.id)) + school_project = create(:school_project, school:, project: create(:project, parent: parent_project)) + feedback = build(:feedback, school_project:, user_id: teacher.id) + expect(feedback).not_to be_valid + expect(feedback.errors[:user]).to include(/Parent project '.*' does not belong to a 'school-class'/) + end + + it 'validates that the user is the class teacher for the school project' do + school_class = create(:school_class, teacher_ids: [teacher.id], school:) + parent_project = create(:project, user_id: teacher.id, school:, lesson: create(:lesson, school:, school_class:, user_id: teacher.id)) + school_project = create(:school_project, school:, project: create(:project, parent: parent_project)) + other_teacher = create(:teacher, school:) + feedback = build(:feedback, school_project:, user_id: other_teacher.id) + expect(feedback).not_to be_valid + expect(feedback.errors[:user]).to include(/is not the 'school-teacher' for school_project/) + end + + it 'has a valid default factory' do + feedback = build(:feedback) + expect(feedback).to be_valid + end + end +end