From e3dadb5f861910404497a88b6017d3eedf24852f Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 9 Oct 2025 17:03:24 +0100
Subject: [PATCH 01/25] WIP: create feedback route

---
 app/controllers/api/feedback_controller.rb   | 33 ++++++++++++++
 app/models/feedback.rb                       | 28 ++++++++++++
 app/views/api/feedback/show.json.jbuilder    | 11 +++++
 config/initializers/inflections.rb           |  4 ++
 config/routes.rb                             |  1 +
 db/migrate/20251008161139_create_feedback.rb | 10 +++++
 db/schema.rb                                 | 12 ++++-
 lib/concepts/feedback/operations/create.rb   | 46 ++++++++++++++++++++
 8 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 app/controllers/api/feedback_controller.rb
 create mode 100644 app/models/feedback.rb
 create mode 100644 app/views/api/feedback/show.json.jbuilder
 create mode 100644 db/migrate/20251008161139_create_feedback.rb
 create mode 100644 lib/concepts/feedback/operations/create.rb

diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb
new file mode 100644
index 000000000..3ff8bc6ff
--- /dev/null
+++ b/app/controllers/api/feedback_controller.rb
@@ -0,0 +1,33 @@
+module Api
+  class FeedbackController < ApiController
+    before_action :authorize_user
+    # load_and_authorize_resource :feedback
+
+    def create
+      puts "create feedback called with params: #{params.inspect}"
+      puts "params before being passed in are: #{feedback_params.inspect}"
+      result = Feedback::Create.call(feedback_params:)
+
+      if result.success?
+        @feedback = result[:feedback]
+        render :show, formats: [:json], status: :created
+      else
+        render json: { error: result[:error] }, status: :unprocessable_entity
+      end
+    end
+
+    def feedback_params
+      base_params.merge(user_id: current_user.id)
+    end
+
+    def url_params
+      params.permit(:project_id)
+    end
+
+    def base_params
+      params.fetch(:feedback, {}).permit(
+        :content,
+      ).merge(url_params)
+    end
+  end
+end
diff --git a/app/models/feedback.rb b/app/models/feedback.rb
new file mode 100644
index 000000000..5bc17bc71
--- /dev/null
+++ b/app/models/feedback.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+class Feedback < ApplicationRecord
+  belongs_to :school_project
+  validates :content, presence: true
+  validates :user_id, presence: true
+  validates :school_project, presence: true
+  validate :user_has_the_school_owner_or_school_teacher_role_for_the_school
+  validate :user_is_the_class_teacher_for_the_school_project
+
+  def user_has_the_school_owner_or_school_teacher_role_for_the_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 user_is_the_school_teacher_for_the_school_project
+    school_class = school_project&.project&.parent&.lesson&.school_class
+    return if !school_class || school_class.teacher_ids.include?(user_id)
+
+    errors.add(:user, "'#{user_id}' is not the 'school-teacher' for school_project '#{school_project.id}'")
+  end
+end
\ No newline at end of file
diff --git a/app/views/api/feedback/show.json.jbuilder b/app/views/api/feedback/show.json.jbuilder
new file mode 100644
index 000000000..5d4bc8a9a
--- /dev/null
+++ b/app/views/api/feedback/show.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/config/initializers/inflections.rb b/config/initializers/inflections.rb
index b8c144e3d..4af99737b 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 4b576fc3c..35e457062 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 000000000..305788889
--- /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/schema.rb b/db/schema.rb
index be56697f3..010383991 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.1].define(version: 2025_08_25_102042) do
+ActiveRecord::Schema[7.2].define(version: 2025_10_08_161139) 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
@@ -313,6 +322,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/operations/create.rb b/lib/concepts/feedback/operations/create.rb
new file mode 100644
index 000000000..c44072a15
--- /dev/null
+++ b/lib/concepts/feedback/operations/create.rb
@@ -0,0 +1,46 @@
+# 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
+        pp 'there was an error'
+        pp 'the error was:'
+        pp e
+        Sentry.capture_exception(e)
+        if response[:feedback].nil?
+          response[:error] = "Error creating feedback #{e}"
+        else
+          errors = response[:feedback].errors.full_messages.join(',')
+          response[:error] = "Error creating feedback: #{errors}"
+        end
+        response
+      end
+
+      private
+
+      def build_feedback(feedback_hash)
+        puts 'building feedback from hash:'
+        puts feedback_hash.inspect
+        project = Project.find_by(identifier: feedback_hash[:project_id])
+        school_project = project&.school_project
+        if school_project.nil?
+          raise "School project not found for identifier: #{feedback_hash[:project_id]}"
+        end
+        # replace identifier with school_project_id
+        feedback_hash[:school_project_id] = school_project.id
+        feedback_hash.delete(:project_id)
+        puts 'the feedback hash is now:'
+        puts feedback_hash.inspect
+        puts 'building feedback object'
+        new_feedback = Feedback.new(feedback_hash)
+        new_feedback
+      end
+    end
+  end
+end
\ No newline at end of file

From 195eaafdc9ee1ac87965c58916774cc4ee8f6c60 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 9 Oct 2025 17:14:58 +0100
Subject: [PATCH 02/25] fixing

---
 app/models/feedback.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index 5bc17bc71..b1503b1a1 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -9,6 +9,7 @@ class Feedback < ApplicationRecord
   validate :user_is_the_class_teacher_for_the_school_project
 
   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)
@@ -19,7 +20,7 @@ def user_has_the_school_owner_or_school_teacher_role_for_the_school
     errors.add(:user, msg)
   end
 
-  def user_is_the_school_teacher_for_the_school_project
+  def user_is_the_class_teacher_for_the_school_project
     school_class = school_project&.project&.parent&.lesson&.school_class
     return if !school_class || school_class.teacher_ids.include?(user_id)
 

From 7e78390050c2b2fef46f5743a589528cf897f2bf Mon Sep 17 00:00:00 2001
From: Lois Wells <88904316+loiswells97@users.noreply.github.com>
Date: Mon, 13 Oct 2025 10:14:14 +0100
Subject: [PATCH 03/25] Update feedback.rb

---
 app/models/feedback.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index b1503b1a1..936a50525 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -26,4 +26,4 @@ def user_is_the_class_teacher_for_the_school_project
 
     errors.add(:user, "'#{user_id}' is not the 'school-teacher' for school_project '#{school_project.id}'")
   end
-end
\ No newline at end of file
+end

From 1dc0f11cfe8023ce5136d2fc5cb633771f070977 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Tue, 14 Oct 2025 11:57:06 +0100
Subject: [PATCH 04/25] improving feedback validations and adding factory and
 model tests

---
 app/models/feedback.rb       | 19 +++++++++++-
 spec/factories/feedback.rb   | 19 ++++++++++++
 spec/models/feedback_spec.rb | 58 ++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 spec/factories/feedback.rb
 create mode 100644 spec/models/feedback_spec.rb

diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index 936a50525..26db1db46 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -6,6 +6,8 @@ class Feedback < ApplicationRecord
   validates :user_id, presence: true
   validates :school_project, 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
 
   def user_has_the_school_owner_or_school_teacher_role_for_the_school
@@ -20,9 +22,24 @@ def user_has_the_school_owner_or_school_teacher_role_for_the_school
     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
     school_class = school_project&.project&.parent&.lesson&.school_class
-    return if !school_class || school_class.teacher_ids.include?(user_id)
+    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
diff --git a/spec/factories/feedback.rb b/spec/factories/feedback.rb
new file mode 100644
index 000000000..487c61987
--- /dev/null
+++ b/spec/factories/feedback.rb
@@ -0,0 +1,19 @@
+# # frozen_string_literal: true
+
+FactoryBot.define do
+  factory :feedback do
+    content { Faker::Lorem.sentence }
+    user_id { teacher.id }
+    school_project { create(:school_project, school: school, project: student_project) }
+
+    transient do
+      school { create(:school) }
+      teacher { create(:teacher, school: school) }
+      student { create(:student, school: school) }
+      school_class { create(:school_class, school: school, teacher_ids: [teacher.id]) }
+      class_student { 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) }
+    end
+  end
+end
\ No newline at end of file
diff --git a/spec/models/feedback_spec.rb b/spec/models/feedback_spec.rb
new file mode 100644
index 000000000..5229669a2
--- /dev/null
+++ b/spec/models/feedback_spec.rb
@@ -0,0 +1,58 @@
+# 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' do
+    it { is_expected.to validate_presence_of(:school_project) }
+    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
\ No newline at end of file

From 8fa479103009a9f2e9722667e9e29f7f5c25d837 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Tue, 14 Oct 2025 12:24:51 +0100
Subject: [PATCH 05/25] adding abilities

---
 app/models/ability.rb       | 2 ++
 spec/models/ability_spec.rb | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index 422b8f4f5..d560cd21b 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -90,6 +90,8 @@ def define_school_teacher_abilities(user:, school:)
     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))
+    can(%i[create], Feedback,
+        school_project: { 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) } })
   end
 
   def define_school_student_abilities(user:, school:)
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
index cfa09e546..a470a53d2 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) }
 

From a560c7327d7a97a3b99927b1e560e44bf75e2e93 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Tue, 14 Oct 2025 14:45:37 +0100
Subject: [PATCH 06/25] testing feedback create operation

---
 lib/concepts/feedback/operations/create.rb |  8 --
 spec/concepts/feedback/create_spec.rb      | 96 ++++++++++++++++++++++
 2 files changed, 96 insertions(+), 8 deletions(-)
 create mode 100644 spec/concepts/feedback/create_spec.rb

diff --git a/lib/concepts/feedback/operations/create.rb b/lib/concepts/feedback/operations/create.rb
index c44072a15..66912eb1c 100644
--- a/lib/concepts/feedback/operations/create.rb
+++ b/lib/concepts/feedback/operations/create.rb
@@ -9,9 +9,6 @@ def call(feedback_params:)
         response[:feedback].save!
         response
       rescue StandardError => e
-        pp 'there was an error'
-        pp 'the error was:'
-        pp e
         Sentry.capture_exception(e)
         if response[:feedback].nil?
           response[:error] = "Error creating feedback #{e}"
@@ -25,8 +22,6 @@ def call(feedback_params:)
       private
 
       def build_feedback(feedback_hash)
-        puts 'building feedback from hash:'
-        puts feedback_hash.inspect
         project = Project.find_by(identifier: feedback_hash[:project_id])
         school_project = project&.school_project
         if school_project.nil?
@@ -35,9 +30,6 @@ def build_feedback(feedback_hash)
         # replace identifier with school_project_id
         feedback_hash[:school_project_id] = school_project.id
         feedback_hash.delete(:project_id)
-        puts 'the feedback hash is now:'
-        puts feedback_hash.inspect
-        puts 'building feedback object'
         new_feedback = Feedback.new(feedback_hash)
         new_feedback
       end
diff --git a/spec/concepts/feedback/create_spec.rb b/spec/concepts/feedback/create_spec.rb
new file mode 100644
index 000000000..d40d62ed2
--- /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: student.id, school:, lesson:, parent: teacher_project) }
+
+  let(:feedback_params) do
+    {
+      content: 'Great job!',
+      user_id: teacher.id,
+      project_id: 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 lesson creation fails' do
+    let(:rogue_project) { create(:project, user_id: student.id) }
+    let(:feedback_params) do
+      {
+        content: nil,
+        user_id: teacher.id,
+        project_id: 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 not found/)
+    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

From 56f32dabb17795e94a3f2739b568370004fb2bd8 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Tue, 14 Oct 2025 17:26:44 +0100
Subject: [PATCH 07/25] tidying

---
 app/controllers/api/feedback_controller.rb | 11 ++++++++---
 lib/concepts/feedback/operations/create.rb |  6 +++---
 spec/concepts/feedback/create_spec.rb      |  4 ++--
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb
index 3ff8bc6ff..9772d3f73 100644
--- a/app/controllers/api/feedback_controller.rb
+++ b/app/controllers/api/feedback_controller.rb
@@ -4,8 +4,6 @@ class FeedbackController < ApiController
     # load_and_authorize_resource :feedback
 
     def create
-      puts "create feedback called with params: #{params.inspect}"
-      puts "params before being passed in are: #{feedback_params.inspect}"
       result = Feedback::Create.call(feedback_params:)
 
       if result.success?
@@ -21,9 +19,16 @@ def feedback_params
     end
 
     def url_params
-      params.permit(:project_id)
+      permitted_params = params.permit(:project_id)
+      { identifier: permitted_params[:project_id] }
     end
 
+    # def school_project_params
+    #   project_params = params.permit(:project_id)
+    #   school_project = Project.find_by(identifier: project_params[:project_id])&.school_project
+    #   {school_project_id: school_project&.id}
+    # end
+
     def base_params
       params.fetch(:feedback, {}).permit(
         :content,
diff --git a/lib/concepts/feedback/operations/create.rb b/lib/concepts/feedback/operations/create.rb
index 66912eb1c..c910a5943 100644
--- a/lib/concepts/feedback/operations/create.rb
+++ b/lib/concepts/feedback/operations/create.rb
@@ -22,14 +22,14 @@ def call(feedback_params:)
       private
 
       def build_feedback(feedback_hash)
-        project = Project.find_by(identifier: feedback_hash[:project_id])
+        project = Project.find_by(identifier: feedback_hash[:identifier])
         school_project = project&.school_project
         if school_project.nil?
-          raise "School project not found for identifier: #{feedback_hash[:project_id]}"
+          raise "School project not found for identifier: #{feedback_hash[:identifier]}"
         end
         # replace identifier with school_project_id
         feedback_hash[:school_project_id] = school_project.id
-        feedback_hash.delete(:project_id)
+        feedback_hash.delete(:identifier)
         new_feedback = Feedback.new(feedback_hash)
         new_feedback
       end
diff --git a/spec/concepts/feedback/create_spec.rb b/spec/concepts/feedback/create_spec.rb
index d40d62ed2..758ca5be2 100644
--- a/spec/concepts/feedback/create_spec.rb
+++ b/spec/concepts/feedback/create_spec.rb
@@ -16,7 +16,7 @@
     {
       content: 'Great job!',
       user_id: teacher.id,
-      project_id: student_project.identifier
+      identifier: student_project.identifier
     }
   end
 
@@ -61,7 +61,7 @@
       {
         content: nil,
         user_id: teacher.id,
-        project_id: rogue_project.identifier
+        identifier: rogue_project.identifier
       }
     end
 

From 1ec8287414a358b9636506076513cc30710b07f7 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Tue, 14 Oct 2025 17:36:28 +0100
Subject: [PATCH 08/25] initial rubocop fixes

---
 app/controllers/api/feedback_controller.rb |  4 +++-
 app/models/feedback.rb                     |  2 +-
 lib/concepts/feedback/operations/create.rb | 10 ++++------
 spec/factories/feedback.rb                 |  4 +++-
 spec/models/feedback_spec.rb               |  4 ++--
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb
index 9772d3f73..c17c89385 100644
--- a/app/controllers/api/feedback_controller.rb
+++ b/app/controllers/api/feedback_controller.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Api
   class FeedbackController < ApiController
     before_action :authorize_user
@@ -31,7 +33,7 @@ def url_params
 
     def base_params
       params.fetch(:feedback, {}).permit(
-        :content,
+        :content
       ).merge(url_params)
     end
   end
diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index 26db1db46..0915346eb 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -4,7 +4,6 @@ class Feedback < ApplicationRecord
   belongs_to :school_project
   validates :content, presence: true
   validates :user_id, presence: true
-  validates :school_project, 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
@@ -25,6 +24,7 @@ def user_has_the_school_owner_or_school_teacher_role_for_the_school
   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
diff --git a/lib/concepts/feedback/operations/create.rb b/lib/concepts/feedback/operations/create.rb
index c910a5943..33ec32da5 100644
--- a/lib/concepts/feedback/operations/create.rb
+++ b/lib/concepts/feedback/operations/create.rb
@@ -24,15 +24,13 @@ def call(feedback_params:)
       def build_feedback(feedback_hash)
         project = Project.find_by(identifier: feedback_hash[:identifier])
         school_project = project&.school_project
-        if school_project.nil?
-          raise "School project not found for identifier: #{feedback_hash[:identifier]}"
-        end
+        raise "School project not found for identifier: #{feedback_hash[:identifier]}" if school_project.nil?
+
         # replace identifier with school_project_id
         feedback_hash[:school_project_id] = school_project.id
         feedback_hash.delete(:identifier)
-        new_feedback = Feedback.new(feedback_hash)
-        new_feedback
+        Feedback.new(feedback_hash)
       end
     end
   end
-end
\ No newline at end of file
+end
diff --git a/spec/factories/feedback.rb b/spec/factories/feedback.rb
index 487c61987..3044256fb 100644
--- a/spec/factories/feedback.rb
+++ b/spec/factories/feedback.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # # frozen_string_literal: true
 
 FactoryBot.define do
@@ -16,4 +18,4 @@
       student_project { create(:project, parent: parent_project, user_id: student.id) }
     end
   end
-end
\ No newline at end of file
+end
diff --git a/spec/models/feedback_spec.rb b/spec/models/feedback_spec.rb
index 5229669a2..eb4b861bd 100644
--- a/spec/models/feedback_spec.rb
+++ b/spec/models/feedback_spec.rb
@@ -43,7 +43,7 @@
     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))      
+      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
@@ -55,4 +55,4 @@
       expect(feedback).to be_valid
     end
   end
-end
\ No newline at end of file
+end

From 826e1a402a2d93761780ddc193e371fd9943053f Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 16 Oct 2025 10:51:37 +0100
Subject: [PATCH 09/25] updating schema version

---
 db/schema.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/db/schema.rb b/db/schema.rb
index 6f382d7ab..bf355cfe7 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_08_161139) do
+ActiveRecord::Schema[7.2].define(version: 2025_10_15_113652) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"

From 788b4471f7d37bc3c06dd098aec91bcecc8e769a Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 16 Oct 2025 10:53:47 +0100
Subject: [PATCH 10/25] remove unneeded test

---
 spec/models/feedback_spec.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spec/models/feedback_spec.rb b/spec/models/feedback_spec.rb
index eb4b861bd..a81f7aa52 100644
--- a/spec/models/feedback_spec.rb
+++ b/spec/models/feedback_spec.rb
@@ -15,7 +15,6 @@
   end
 
   describe 'validations' do
-    it { is_expected.to validate_presence_of(:school_project) }
     it { is_expected.to validate_presence_of(:content) }
     it { is_expected.to validate_presence_of(:user_id) }
 

From 8a1d61cf52daed7ef9481d8250d322605099a567 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 16 Oct 2025 11:11:15 +0100
Subject: [PATCH 11/25] some rubocop fixes

---
 app/models/feedback.rb                | 7 ++++++-
 spec/concepts/feedback/create_spec.rb | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index 0915346eb..ecb3200a9 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -38,9 +38,14 @@ def parent_project_belongs_to_school_class
   end
 
   def user_is_the_class_teacher_for_the_school_project
-    school_class = school_project&.project&.parent&.lesson&.school_class
     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
+
+  private
+
+  def school_class
+    school_project&.project&.parent&.lesson&.school_class
+  end
 end
diff --git a/spec/concepts/feedback/create_spec.rb b/spec/concepts/feedback/create_spec.rb
index 758ca5be2..7f7f6b7ce 100644
--- a/spec/concepts/feedback/create_spec.rb
+++ b/spec/concepts/feedback/create_spec.rb
@@ -7,10 +7,10 @@
   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(: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: student.id, school:, lesson:, parent: teacher_project) }
+  let(:student_project) { create(:project, user_id: class_student.student_id, school:, lesson:, parent: teacher_project) }
 
   let(:feedback_params) do
     {

From 054be3f2e9e56b755574782c4b1ce9ab8dcf7651 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 16 Oct 2025 12:51:32 +0100
Subject: [PATCH 12/25] refactoring feedback factory to please rubocop

---
 spec/factories/feedback.rb | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/spec/factories/feedback.rb b/spec/factories/feedback.rb
index 3044256fb..1c76e714d 100644
--- a/spec/factories/feedback.rb
+++ b/spec/factories/feedback.rb
@@ -1,21 +1,22 @@
 # frozen_string_literal: true
 
-# # frozen_string_literal: true
-
 FactoryBot.define do
   factory :feedback do
-    content { Faker::Lorem.sentence }
-    user_id { teacher.id }
-    school_project { create(:school_project, school: school, project: student_project) }
+    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)
 
-    transient do
-      school { create(:school) }
-      teacher { create(:teacher, school: school) }
-      student { create(:student, school: school) }
-      school_class { create(:school_class, school: school, teacher_ids: [teacher.id]) }
-      class_student { 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

From c57741daf04ddceda88f596ac83255dbbfd34bb7 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 16 Oct 2025 16:39:33 +0100
Subject: [PATCH 13/25] adding authorisation for feedback creation

---
 app/controllers/api/feedback_controller.rb | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb
index c17c89385..5e79ca3ed 100644
--- a/app/controllers/api/feedback_controller.rb
+++ b/app/controllers/api/feedback_controller.rb
@@ -3,10 +3,10 @@
 module Api
   class FeedbackController < ApiController
     before_action :authorize_user
-    # load_and_authorize_resource :feedback
+    load_and_authorize_resource :feedback
 
     def create
-      result = Feedback::Create.call(feedback_params:)
+      result = Feedback::Create.call(feedback_params: feedback_create_params)
 
       if result.success?
         @feedback = result[:feedback]
@@ -17,6 +17,13 @@ def create
     end
 
     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
+
+    def feedback_create_params
       base_params.merge(user_id: current_user.id)
     end
 
@@ -25,12 +32,6 @@ def url_params
       { identifier: permitted_params[:project_id] }
     end
 
-    # def school_project_params
-    #   project_params = params.permit(:project_id)
-    #   school_project = Project.find_by(identifier: project_params[:project_id])&.school_project
-    #   {school_project_id: school_project&.id}
-    # end
-
     def base_params
       params.fetch(:feedback, {}).permit(
         :content

From 40ab7c52ca334498a09fda3e91c323579bee146f Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 16 Oct 2025 16:56:51 +0100
Subject: [PATCH 14/25] add feedback auditing

---
 app/models/feedback.rb       | 7 +++++++
 spec/models/feedback_spec.rb | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index ecb3200a9..6f0c31d96 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -9,6 +9,13 @@ class Feedback < ApplicationRecord
   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: ->(c) { c.school_project&.school_id }
+    }
+  )
+
   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
diff --git a/spec/models/feedback_spec.rb b/spec/models/feedback_spec.rb
index a81f7aa52..3df2bceb2 100644
--- a/spec/models/feedback_spec.rb
+++ b/spec/models/feedback_spec.rb
@@ -14,7 +14,7 @@
     it { is_expected.to belong_to(:school_project) }
   end
 
-  describe 'validations' do
+  describe 'validations', versioning: true do
     it { is_expected.to validate_presence_of(:content) }
     it { is_expected.to validate_presence_of(:user_id) }
 

From 7e1ea4a58d8d45fb966dcfda08d344bda0baeb50 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Thu, 16 Oct 2025 16:57:24 +0100
Subject: [PATCH 15/25] tidying

---
 app/models/feedback.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index 6f0c31d96..2bdbc6d01 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -12,7 +12,7 @@ class Feedback < ApplicationRecord
   has_paper_trail(
     meta: {
       meta_school_project_id: ->(f) { f.school_project&.id },
-      meta_school_id: ->(c) { c.school_project&.school_id }
+      meta_school_id: ->(f) { f.school_project&.school_id }
     }
   )
 

From 3ad056b96a1a5ac136f3c48a2c117ac1615efa7a Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Fri, 17 Oct 2025 10:45:13 +0100
Subject: [PATCH 16/25] tidying and request specs

---
 app/models/feedback.rb                        |   4 +-
 app/models/school_project.rb                  |   1 +
 .../feedback/creating_feedback_spec.rb        | 171 ++++++++++++++++++
 3 files changed, 174 insertions(+), 2 deletions(-)
 create mode 100644 spec/features/feedback/creating_feedback_spec.rb

diff --git a/app/models/feedback.rb b/app/models/feedback.rb
index 2bdbc6d01..45ca2d21d 100644
--- a/app/models/feedback.rb
+++ b/app/models/feedback.rb
@@ -16,6 +16,8 @@ class Feedback < ApplicationRecord
     }
   )
 
+  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
@@ -50,8 +52,6 @@ def user_is_the_class_teacher_for_the_school_project
     errors.add(:user, "'#{user_id}' is not the 'school-teacher' for school_project '#{school_project.id}'")
   end
 
-  private
-
   def school_class
     school_project&.project&.parent&.lesson&.school_class
   end
diff --git a/app/models/school_project.rb b/app/models/school_project.rb
index 1ce8df1ff..cc19ad288 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/spec/features/feedback/creating_feedback_spec.rb b/spec/features/feedback/creating_feedback_spec.rb
new file mode 100644
index 000000000..971a72d3f
--- /dev/null
+++ b/spec/features/feedback/creating_feedback_spec.rb
@@ -0,0 +1,171 @@
+# 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
+
+  # context 'when the finished flag is initially true' do
+  #   let!(:student_project) { create(:project, school_id: school.id, lesson_id: nil, user_id: student.id, remixed_from_id: teacher_project.id, locale: nil, finished: true) }
+
+  #   before do
+  #     put("/api/projects/#{student_project.identifier}/finished", headers:, params: { finished: false })
+  #     student_project.reload
+  #   end
+
+  #   it 'returns success response' do
+  #     expect(response).to have_http_status(:ok)
+  #   end
+
+  #   it 'returns the school project json' do
+  #     expect(response.body).to eq(school_project_json)
+  #   end
+
+  #   it 'sets the completed flag to false' do
+  #     expect(student_project.school_project.finished).to be_falsey
+  #   end
+  # end
+
+  # context 'when the user does not own the project' do
+  #   before do
+  #     put("/api/projects/#{teacher_project.identifier}/finished", headers:, params: { finished: true })
+  #     teacher_project.reload
+  #   end
+
+  #   it 'returns forbidden response' do
+  #     expect(response).to have_http_status(:forbidden)
+  #   end
+
+  #   it 'does not change the finished flag' do
+  #     expect(teacher_project.school_project.finished).to be_falsey
+  #   end
+  # end
+
+  # context 'when project does not exist' do
+  #   before do
+  #     put('/api/projects/does-not-exist/finished', headers:, params: { finished: false })
+  #   end
+
+  #   it 'returns not found response' do
+  #     expect(response).to have_http_status(:not_found)
+  #   end
+  # end
+end

From 581e3aa5faec3f79ef3b555d67eb9759b064e8dd Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Fri, 17 Oct 2025 10:47:40 +0100
Subject: [PATCH 17/25] rubocop fixes

---
 spec/features/feedback/creating_feedback_spec.rb | 14 +++++++-------
 spec/models/feedback_spec.rb                     |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/spec/features/feedback/creating_feedback_spec.rb b/spec/features/feedback/creating_feedback_spec.rb
index 971a72d3f..8f0664599 100644
--- a/spec/features/feedback/creating_feedback_spec.rb
+++ b/spec/features/feedback/creating_feedback_spec.rb
@@ -20,7 +20,7 @@
 
     context 'when leaving feedback on student work' do
       before do
-        post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: {content: "Nice one!"} })
+        post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } })
         student_project.reload
       end
 
@@ -50,7 +50,7 @@
 
     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!"} })
+        post("/api/projects/#{teacher_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } })
         teacher_project.reload
       end
 
@@ -65,7 +65,7 @@
 
     context 'when leaving empty feedback' do
       before do
-        post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: {content: ""} })
+        post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: '' } })
         student_project.reload
       end
 
@@ -80,7 +80,7 @@
 
     context 'when the project does not exist' do
       before do
-        post('/api/projects/does-not-exist/feedback', headers:, params: { feedback: {content: "Nice one!"} })
+        post('/api/projects/does-not-exist/feedback', headers:, params: { feedback: { content: 'Nice one!' } })
       end
 
       it 'returns forbidden response' do
@@ -91,10 +91,10 @@
 
   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!"} })
+      post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } })
       student_project.reload
     end
 
@@ -110,7 +110,7 @@
   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!"} })
+      post("/api/projects/#{student_project.identifier}/feedback", headers:, params: { feedback: { content: 'Nice one!' } })
       student_project.reload
     end
 
diff --git a/spec/models/feedback_spec.rb b/spec/models/feedback_spec.rb
index 3df2bceb2..9c3c72502 100644
--- a/spec/models/feedback_spec.rb
+++ b/spec/models/feedback_spec.rb
@@ -14,7 +14,7 @@
     it { is_expected.to belong_to(:school_project) }
   end
 
-  describe 'validations', versioning: true do
+  describe 'validations', :versioning do
     it { is_expected.to validate_presence_of(:content) }
     it { is_expected.to validate_presence_of(:user_id) }
 

From 59ecd17d918ec21748e4732eb23ae2229478181f Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Fri, 17 Oct 2025 10:59:59 +0100
Subject: [PATCH 18/25] adding comments to explain params

---
 app/controllers/api/feedback_controller.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/controllers/api/feedback_controller.rb b/app/controllers/api/feedback_controller.rb
index 5e79ca3ed..dc445317c 100644
--- a/app/controllers/api/feedback_controller.rb
+++ b/app/controllers/api/feedback_controller.rb
@@ -16,6 +16,9 @@ def create
       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(
@@ -23,6 +26,9 @@ def feedback_params
       )
     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

From 066ee401c3f53340ff02bde4a5b83cbb88097260 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Fri, 17 Oct 2025 11:03:42 +0100
Subject: [PATCH 19/25] removing commented code

---
 .../feedback/creating_feedback_spec.rb        | 46 -------------------
 1 file changed, 46 deletions(-)

diff --git a/spec/features/feedback/creating_feedback_spec.rb b/spec/features/feedback/creating_feedback_spec.rb
index 8f0664599..51d7b8689 100644
--- a/spec/features/feedback/creating_feedback_spec.rb
+++ b/spec/features/feedback/creating_feedback_spec.rb
@@ -122,50 +122,4 @@
       expect(student_project.school_project.feedback.count).to eq(0)
     end
   end
-
-  # context 'when the finished flag is initially true' do
-  #   let!(:student_project) { create(:project, school_id: school.id, lesson_id: nil, user_id: student.id, remixed_from_id: teacher_project.id, locale: nil, finished: true) }
-
-  #   before do
-  #     put("/api/projects/#{student_project.identifier}/finished", headers:, params: { finished: false })
-  #     student_project.reload
-  #   end
-
-  #   it 'returns success response' do
-  #     expect(response).to have_http_status(:ok)
-  #   end
-
-  #   it 'returns the school project json' do
-  #     expect(response.body).to eq(school_project_json)
-  #   end
-
-  #   it 'sets the completed flag to false' do
-  #     expect(student_project.school_project.finished).to be_falsey
-  #   end
-  # end
-
-  # context 'when the user does not own the project' do
-  #   before do
-  #     put("/api/projects/#{teacher_project.identifier}/finished", headers:, params: { finished: true })
-  #     teacher_project.reload
-  #   end
-
-  #   it 'returns forbidden response' do
-  #     expect(response).to have_http_status(:forbidden)
-  #   end
-
-  #   it 'does not change the finished flag' do
-  #     expect(teacher_project.school_project.finished).to be_falsey
-  #   end
-  # end
-
-  # context 'when project does not exist' do
-  #   before do
-  #     put('/api/projects/does-not-exist/finished', headers:, params: { finished: false })
-  #   end
-
-  #   it 'returns not found response' do
-  #     expect(response).to have_http_status(:not_found)
-  #   end
-  # end
 end

From 5678604e786910bfeb64db12a3f325d1e752f8b9 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Tue, 21 Oct 2025 17:30:50 +0100
Subject: [PATCH 20/25] fixing papertrail by adding meta migration to versions
 table

---
 .../20251021162845_add_meta_school_project_id_to_versions.rb | 5 +++++
 db/schema.rb                                                 | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20251021162845_add_meta_school_project_id_to_versions.rb

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 000000000..c04d0523b
--- /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 bf355cfe7..c355edcbb 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"
@@ -315,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
 

From 96c4b92c294763ca35b2838631ef8748b9c5280a Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Tue, 21 Oct 2025 17:32:28 +0100
Subject: [PATCH 21/25] remove redundant operations directory

---
 lib/concepts/feedback/{operations => }/create.rb | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename lib/concepts/feedback/{operations => }/create.rb (100%)

diff --git a/lib/concepts/feedback/operations/create.rb b/lib/concepts/feedback/create.rb
similarity index 100%
rename from lib/concepts/feedback/operations/create.rb
rename to lib/concepts/feedback/create.rb

From a3c612b4967e9dd14108450afd6ca82507dc70f6 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Wed, 22 Oct 2025 12:42:00 +0100
Subject: [PATCH 22/25] factoring out teacher project ids

---
 app/models/ability.rb | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/app/models/ability.rb b/app/models/ability.rb
index d560cd21b..7509cb4bb 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -88,10 +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))
-    can(%i[create], Feedback,
-        school_project: { 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:)

From 88752918a0b841fdf9b8a78693e0fe6457f363f1 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Wed, 22 Oct 2025 12:50:38 +0100
Subject: [PATCH 23/25] factoring feedback template out into a partial

---
 app/views/api/feedback/_feedback.json.jbuilder | 11 +++++++++++
 app/views/api/feedback/show.json.jbuilder      | 10 +---------
 2 files changed, 12 insertions(+), 9 deletions(-)
 create mode 100644 app/views/api/feedback/_feedback.json.jbuilder

diff --git a/app/views/api/feedback/_feedback.json.jbuilder b/app/views/api/feedback/_feedback.json.jbuilder
new file mode 100644
index 000000000..8eccd5761
--- /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
index 5d4bc8a9a..8e369213c 100644
--- a/app/views/api/feedback/show.json.jbuilder
+++ b/app/views/api/feedback/show.json.jbuilder
@@ -1,11 +1,3 @@
 # frozen_string_literal: true
 
-json.call(
-  @feedback,
-  :id,
-  :school_project_id,
-  :user_id,
-  :content,
-  :created_at,
-  :updated_at
-)
+json.partial! 'feedback', feedback: @feedback

From 2adbcef347f832bec78de76c9878168e182bbb74 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Wed, 22 Oct 2025 14:21:43 +0100
Subject: [PATCH 24/25] simplify feedback creation error handling

---
 lib/concepts/feedback/create.rb       | 11 +++--------
 spec/concepts/feedback/create_spec.rb |  4 ++--
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/lib/concepts/feedback/create.rb b/lib/concepts/feedback/create.rb
index 33ec32da5..3c4296bad 100644
--- a/lib/concepts/feedback/create.rb
+++ b/lib/concepts/feedback/create.rb
@@ -10,12 +10,8 @@ def call(feedback_params:)
         response
       rescue StandardError => e
         Sentry.capture_exception(e)
-        if response[:feedback].nil?
-          response[:error] = "Error creating feedback #{e}"
-        else
-          errors = response[:feedback].errors.full_messages.join(',')
-          response[:error] = "Error creating feedback: #{errors}"
-        end
+        errors = response[:feedback]&.errors&.full_messages&.join(',')
+        response[:error] = "Error creating feedback: #{errors}"
         response
       end
 
@@ -24,10 +20,9 @@ def call(feedback_params:)
       def build_feedback(feedback_hash)
         project = Project.find_by(identifier: feedback_hash[:identifier])
         school_project = project&.school_project
-        raise "School project not found for identifier: #{feedback_hash[:identifier]}" if school_project.nil?
 
         # replace identifier with school_project_id
-        feedback_hash[:school_project_id] = school_project.id
+        feedback_hash[:school_project_id] = school_project&.id
         feedback_hash.delete(:identifier)
         Feedback.new(feedback_hash)
       end
diff --git a/spec/concepts/feedback/create_spec.rb b/spec/concepts/feedback/create_spec.rb
index 7f7f6b7ce..d70a66d3a 100644
--- a/spec/concepts/feedback/create_spec.rb
+++ b/spec/concepts/feedback/create_spec.rb
@@ -55,7 +55,7 @@
     end
   end
 
-  context 'when lesson creation fails' do
+  context 'when feedback creation fails' do
     let(:rogue_project) { create(:project, user_id: student.id) }
     let(:feedback_params) do
       {
@@ -85,7 +85,7 @@
 
     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 not found/)
+      expect(response[:error]).to match(/School project must exist/)
     end
 
     it 'sent the exception to Sentry' do

From 1c801ae06f0a2852976b7aed700a83e00242e982 Mon Sep 17 00:00:00 2001
From: Lois Wells <lois.wells@raspberrypi.org>
Date: Wed, 22 Oct 2025 16:07:15 +0100
Subject: [PATCH 25/25] refactor error handling slightly to catch
 non-validation errors

---
 lib/concepts/feedback/create.rb | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/concepts/feedback/create.rb b/lib/concepts/feedback/create.rb
index 3c4296bad..5aa962e7e 100644
--- a/lib/concepts/feedback/create.rb
+++ b/lib/concepts/feedback/create.rb
@@ -10,8 +10,12 @@ def call(feedback_params:)
         response
       rescue StandardError => e
         Sentry.capture_exception(e)
-        errors = response[:feedback]&.errors&.full_messages&.join(',')
-        response[:error] = "Error creating feedback: #{errors}"
+        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