From 05a49017ce5593381c6b353f9a519fba83b3db67 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 1 May 2026 15:38:48 +0100 Subject: [PATCH 1/2] Reduce N+1s by caching submitted project count There's currently N+1 queries on both the lessons and classes controller as they both try to show the number of submitted projects. Originally I tired to solve this by using 'include' to load the correct records, however I don't think this is a very scalable solutions as it means loading every submitted project object memory for classes or lessons (which there may be 1000s). Instead use the pattern of a counter cache. Each time a project transitions to/from submitted, we recalculate the number. This is the first part of two. I've broken up the work so we can deploy this and check that the counts are correct. If we deployed this in one go there would be a chance the counts are inaccurate if a project transition happened during the deploy. ### After deploy Run `rails lessons:backfill_submitted_projects_count` and verify the results by checking a few lessons and comparing it to `lesson#submitted_count` Co-authored-by: Copilot --- app/models/lesson.rb | 7 ++++ app/models/school_project.rb | 8 +++++ .../school_project_state_machine.rb | 3 ++ ...add_submitted_projects_count_to_lessons.rb | 7 ++++ db/schema.rb | 3 +- lib/tasks/lessons.rake | 25 +++++++++++++ spec/lib/tasks/lessons_spec.rb | 36 +++++++++++++++++++ spec/models/lesson_spec.rb | 28 +++++++++++++++ .../school_project_state_machine_spec.rb | 15 ++++++++ 9 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260429120000_add_submitted_projects_count_to_lessons.rb create mode 100644 lib/tasks/lessons.rake create mode 100644 spec/lib/tasks/lessons_spec.rb diff --git a/app/models/lesson.rb b/app/models/lesson.rb index e3b7a433d..bca7a1e17 100644 --- a/app/models/lesson.rb +++ b/app/models/lesson.rb @@ -8,6 +8,8 @@ class Lesson < ApplicationRecord belongs_to :parent, optional: true, class_name: :Lesson, foreign_key: :copied_from_id, inverse_of: :copies has_many :copies, dependent: :nullify, class_name: :Lesson, foreign_key: :copied_from_id, inverse_of: :parent has_one :project, dependent: :destroy + has_many :remixes, through: :project + has_many :school_projects, through: :remixes accepts_nested_attributes_for :project before_validation :assign_school_from_school_class @@ -38,6 +40,11 @@ def submitted_count project.remixes.count { |remix| remix.school_project&.submitted? } end + def recalculate_submitted_projects_count! + count = school_projects.in_state(:submitted).count + update!(submitted_projects_count: count) + end + private def assign_school_from_school_class diff --git a/app/models/school_project.rb b/app/models/school_project.rb index c8171a743..a848e1ae8 100644 --- a/app/models/school_project.rb +++ b/app/models/school_project.rb @@ -11,6 +11,10 @@ class SchoolProject < ApplicationRecord initial_state: :unsubmitted ] + def lesson + project.lesson || project.parent&.lesson + end + def status state_machine.current_state end @@ -23,6 +27,10 @@ def unread_feedback? feedback.exists?(read_at: nil) end + def recalculate_lesson_submitted_projects_count!(_transition = nil) + lesson&.recalculate_submitted_projects_count! + end + # Add convenience methods for each state def unsubmitted? state_machine.in_state?(:unsubmitted) diff --git a/app/state_machines/school_project_state_machine.rb b/app/state_machines/school_project_state_machine.rb index e021dec5b..f7284c801 100644 --- a/app/state_machines/school_project_state_machine.rb +++ b/app/state_machines/school_project_state_machine.rb @@ -14,4 +14,7 @@ class SchoolProjectStateMachine transition from: :submitted, to: %i[unsubmitted returned complete] transition from: :returned, to: %i[submitted complete] transition from: :complete, to: [:unsubmitted] + + after_transition(to: :submitted, &:recalculate_lesson_submitted_projects_count!) + after_transition(from: :submitted, &:recalculate_lesson_submitted_projects_count!) end diff --git a/db/migrate/20260429120000_add_submitted_projects_count_to_lessons.rb b/db/migrate/20260429120000_add_submitted_projects_count_to_lessons.rb new file mode 100644 index 000000000..390ab58c9 --- /dev/null +++ b/db/migrate/20260429120000_add_submitted_projects_count_to_lessons.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddSubmittedProjectsCountToLessons < ActiveRecord::Migration[7.2] + def change + add_column :lessons, :submitted_projects_count, :integer, null: false, default: 0 + end +end diff --git a/db/schema.rb b/db/schema.rb index eb22dba99..87ba019cd 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: 2026_04_10_110000) do +ActiveRecord::Schema[7.2].define(version: 2026_04_29_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -205,6 +205,7 @@ t.datetime "archived_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "submitted_projects_count", default: 0, null: false t.index ["archived_at"], name: "index_lessons_on_archived_at" t.index ["copied_from_id"], name: "index_lessons_on_copied_from_id" t.index ["name"], name: "index_lessons_on_name" diff --git a/lib/tasks/lessons.rake b/lib/tasks/lessons.rake new file mode 100644 index 000000000..23efe2cab --- /dev/null +++ b/lib/tasks/lessons.rake @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +namespace :lessons do + desc 'Backfill cached submitted project counts for lessons' + task backfill_submitted_projects_count: :environment do + Lesson.connection.execute <<~SQL.squish + WITH submitted_counts AS ( + SELECT lesson_projects.lesson_id, COUNT(*) AS submitted_projects_count + FROM projects lesson_projects + INNER JOIN projects remixes ON remixes.remixed_from_id = lesson_projects.id + INNER JOIN school_projects ON school_projects.project_id = remixes.id + INNER JOIN school_project_transitions ON school_project_transitions.school_project_id = school_projects.id + WHERE school_project_transitions.most_recent = TRUE + AND school_project_transitions.to_state = 'submitted' + AND lesson_projects.lesson_id IS NOT NULL + GROUP BY lesson_projects.lesson_id + ) + UPDATE lessons + SET submitted_projects_count = COALESCE(submitted_counts.submitted_projects_count, 0) + FROM lessons target_lessons + LEFT JOIN submitted_counts ON submitted_counts.lesson_id = target_lessons.id + WHERE lessons.id = target_lessons.id + SQL + end +end diff --git a/spec/lib/tasks/lessons_spec.rb b/spec/lib/tasks/lessons_spec.rb new file mode 100644 index 000000000..e35fc4aae --- /dev/null +++ b/spec/lib/tasks/lessons_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'rake' + +RSpec.describe 'lessons', type: :task do + describe ':backfill_submitted_projects_count' do + let(:task) { Rake::Task['lessons:backfill_submitted_projects_count'] } + let(:school) { create(:school) } + let(:teacher) { create(:teacher, school:) } + let(:student) { create(:student, school:) } + let(:lesson) { create(:lesson, school:, user_id: teacher.id) } + + before do + task.reenable + end + + it 'sets cached submitted project counts for all lessons' do + submitted_remix = create(:project, school:, remixed_from_id: lesson.project.id, user_id: student.id) + submitted_remix.school_project.transition_status_to!(:submitted, student.id) + + returned_remix = create(:project, school:, remixed_from_id: lesson.project.id, user_id: student.id) + returned_remix.school_project.transition_status_to!(:submitted, student.id) + returned_remix.school_project.transition_status_to!(:returned, teacher.id) + + other_lesson = create(:lesson, school:, user_id: teacher.id, submitted_projects_count: 7) + + lesson.update!(submitted_projects_count: 0) + + task.invoke + + expect(lesson.reload.submitted_projects_count).to eq(1) + expect(other_lesson.reload.submitted_projects_count).to eq(0) + end + end +end diff --git a/spec/models/lesson_spec.rb b/spec/models/lesson_spec.rb index c5e581e87..52640dc48 100644 --- a/spec/models/lesson_spec.rb +++ b/spec/models/lesson_spec.rb @@ -227,4 +227,32 @@ expect(lesson.submitted_count).to eq(2) end end + + describe '#recalculate_submitted_projects_count!' do + it 'sets the submitted projects count to 0 if there is no project' do + lesson = create(:lesson, project: nil, submitted_projects_count: 3) + + lesson.recalculate_submitted_projects_count! + + expect(lesson.reload.submitted_projects_count).to eq(0) + end + + it 'returns the count of submitted remixes of the lesson project' do + student = create(:student, school:) + lesson = create(:lesson, school:, user_id: teacher.id) + + remix_1 = create(:project, school:, remixed_from_id: lesson.project.id, user_id: student.id) + remix_1.school_project.transition_status_to!(:submitted, remix_1.user_id) + + remix_2 = create(:project, school:, remixed_from_id: lesson.project.id, user_id: student.id) + remix_2.school_project.transition_status_to!(:submitted, remix_2.user_id) + + create(:project, school:, remixed_from_id: lesson.project.id, user_id: student.id) # Not submitted + + lesson.update!(submitted_projects_count: 0) + lesson.recalculate_submitted_projects_count! + + expect(lesson.reload.submitted_projects_count).to eq(2) + end + end end diff --git a/spec/state_machines/school_project_state_machine_spec.rb b/spec/state_machines/school_project_state_machine_spec.rb index 696bc05b2..5eed84872 100644 --- a/spec/state_machines/school_project_state_machine_spec.rb +++ b/spec/state_machines/school_project_state_machine_spec.rb @@ -16,6 +16,21 @@ end describe 'transitions' do + it 'recalculates the parent lesson submitted projects count' do + teacher = create(:teacher, school:) + lesson = create(:lesson, school:, user_id: teacher.id) + remix = create(:project, school:, user_id: student.id, remixed_from_id: lesson.project.id) + state_machine = described_class.new(remix.school_project, transition_class: SchoolProjectTransition) + + expect do + state_machine.transition_to!(:submitted) + end.to change { lesson.reload.submitted_projects_count }.from(0).to(1) + + expect do + state_machine.transition_to!(:returned) + end.to change { lesson.reload.submitted_projects_count }.from(1).to(0) + end + context 'when in unsubmitted state' do it 'can transition to submitted' do expect(state_machine.can_transition_to?(:submitted)).to be true From 6ff71dbbb5f0c606e9309cfb6b226a25cd43d123 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 1 May 2026 16:23:51 +0100 Subject: [PATCH 2/2] Prevent stale submitted project count Add lock to prevent stale reads in the case this runs concurrently for the same project Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- app/models/lesson.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/lesson.rb b/app/models/lesson.rb index bca7a1e17..9c004b142 100644 --- a/app/models/lesson.rb +++ b/app/models/lesson.rb @@ -41,8 +41,10 @@ def submitted_count end def recalculate_submitted_projects_count! - count = school_projects.in_state(:submitted).count - update!(submitted_projects_count: count) + with_lock do + count = school_projects.in_state(:submitted).count + update!(submitted_projects_count: count) + end end private