From 57b2da19d040957de3e45b53a3aeaf0aa9c6eb4e Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 7 Aug 2026 14:04:51 +0100 Subject: [PATCH 1/2] Support instruction steps format for project instructions Previously project instructions could only be a single markdown string, with no way to break guidance into discrete, orderable steps. Changing the existing instructions column type in place was tried first, but that is risky against a live database: a server process that queried the table before the migration ran keeps the old column type cached and keeps sending data the database no longer accepts, causing errors until it is restarted. Converting existing data to the new type can also take a while on a large table and holds a lock for that duration, causing errors on any concurrent read or write. This change avoids both problems by adding a new instruction_steps jsonb column instead, leaving the existing instructions text column untouched - it never needs to change type or be locked for a bulk conversion. Project#instructions reads instruction_steps when present and falls back to the legacy text column otherwise; the controller permits either a plain string or an array of {markdown_content} steps under the same instructions param, so no other call site needs to know about the split. Existing rows keep instruction_steps nil until a project is saved through the new format. A follow-up task can backfill instruction_steps for the remaining rows from the legacy column, at which point the instructions column can be dropped entirely. --- .../api/projects/remixes_controller.rb | 3 +- app/controllers/api/projects_controller.rb | 3 +- app/models/project.rb | 9 ++++++ ...20000_add_instruction_steps_to_projects.rb | 7 +++++ db/schema.rb | 3 +- lib/concepts/project/operations/update.rb | 9 +++++- spec/models/project_spec.rb | 29 +++++++++++++++++++ spec/requests/projects/show_spec.rb | 10 +++++++ spec/requests/projects/update_spec.rb | 8 +++++ 9 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20260807120000_add_instruction_steps_to_projects.rb diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb index acfcec769..797a10d82 100644 --- a/app/controllers/api/projects/remixes_controller.rb +++ b/app/controllers/api/projects/remixes_controller.rb @@ -82,7 +82,8 @@ def remix_params :instructions, { image_list: [], - components: [%i[id name extension content index]] + components: [%i[id name extension content index]], + instructions: [[:markdown_content]] }]) end end diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index b6897fc32..d9708f0aa 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -104,7 +104,8 @@ def base_params :locale, :instructions, { - components: %i[id name extension content index default] + components: %i[id name extension content index default], + instructions: [:markdown_content] }, scratch_component: {}, parent: {}, diff --git a/app/models/project.rb b/app/models/project.rb index 52b8631a4..915448773 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -74,6 +74,15 @@ def scratch_component=(value) super(value.is_a?(Hash) ? ScratchComponent.new(value) : value) end + def instructions + self[:instruction_steps].nil? ? self[:instructions] : self[:instruction_steps] + end + + def instructions=(value) + self[:instructions] = value unless value.is_a?(Array) + self[:instruction_steps] = value + end + def last_edited_at # datetime that the project or one of its components was last updated [updated_at, components.maximum(:updated_at)].compact.max diff --git a/db/migrate/20260807120000_add_instruction_steps_to_projects.rb b/db/migrate/20260807120000_add_instruction_steps_to_projects.rb new file mode 100644 index 000000000..75de32002 --- /dev/null +++ b/db/migrate/20260807120000_add_instruction_steps_to_projects.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddInstructionStepsToProjects < ActiveRecord::Migration[8.1] + def change + add_column :projects, :instruction_steps, :jsonb + end +end diff --git a/db/schema.rb b/db/schema.rb index 74dc5e40b..0403b6037 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[8.1].define(version: 2026_07_03_151018) do +ActiveRecord::Schema[8.1].define(version: 2026_08_07_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -235,6 +235,7 @@ create_table "projects", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.datetime "created_at", null: false t.string "identifier", null: false + t.jsonb "instruction_steps" t.text "instructions" t.uuid "lesson_id" t.string "locale" diff --git a/lib/concepts/project/operations/update.rb b/lib/concepts/project/operations/update.rb index 860f3cb0f..3c40cea6b 100644 --- a/lib/concepts/project/operations/update.rb +++ b/lib/concepts/project/operations/update.rb @@ -45,10 +45,17 @@ def validate_deletions(response) def student_project_instructions_updated?(response, update_hash, current_user) is_school_project = response[:project].school.present? user_is_student = current_user.student? - instructions_updated = response[:project].instructions != update_hash[:instructions] + instructions_updated = normalize_instructions(response[:project].instructions) != normalize_instructions(update_hash[:instructions]) is_school_project && user_is_student && instructions_updated end + # update_hash[:instructions] may be plain Ruby data or an ActionController::Parameters + # array/hash (for the instruction steps format); round-trip through JSON so both sides + # of the comparison are in the same plain, comparable shape. + def normalize_instructions(value) + ActiveSupport::JSON.decode(ActiveSupport::JSON.encode(value)) + end + def validate_update(response, update_hash, current_user) return unless student_project_instructions_updated?(response, update_hash, current_user) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 22bc48fda..86d5e1298 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -344,6 +344,35 @@ end end + describe '#instructions' do + let(:project) { create(:project, :with_instructions, school:, user_id: create(:teacher, school:).id) } + + it 'falls back to the legacy text column for rows that predate instruction_steps' do + project.instruction_steps = nil + project.save! + expect(project.instructions).to eq(project[:instructions]) + end + + it 'returns instruction_steps once set, without touching the legacy column for arrays' do + legacy_value = project[:instructions] + project.update!(instructions: [{ markdown_content: 'step 1' }]) + expect(project.instructions).to eq([{ 'markdown_content' => 'step 1' }]) + expect(project[:instructions]).to eq(legacy_value) + end + + it 'does not fall back to stale legacy text once instruction_steps is set to an empty array' do + project.update!(instructions: []) + expect(project.instructions).to eq([]) + end + + it 'also clears the legacy column when instructions are set to nil' do + project.update!(instructions: [{ markdown_content: 'step 1' }]) + project.update!(instructions: nil) + expect(project.instructions).to be_nil + expect(project[:instructions]).to be_nil + end + end + describe 'auditing' do let(:school) { create(:school) } let(:teacher) { create(:teacher, school:) } diff --git a/spec/requests/projects/show_spec.rb b/spec/requests/projects/show_spec.rb index 3cb701861..ac400d33f 100644 --- a/spec/requests/projects/show_spec.rb +++ b/spec/requests/projects/show_spec.rb @@ -54,6 +54,16 @@ get("/api/projects/#{project.identifier}", headers:) expect(response.parsed_body).not_to have_key('finished') end + + it 'returns instructions in the instruction steps format when saved that way' do + project.update!(instructions: [{ markdown_content: 'step 1' }, { markdown_content: 'step 2' }]) + + get("/api/projects/#{project.identifier}", headers:) + + expect(response.parsed_body['instructions']).to eq( + [{ 'markdown_content' => 'step 1' }, { 'markdown_content' => 'step 2' }] + ) + end end context 'when loading a student\'s project' do diff --git a/spec/requests/projects/update_spec.rb b/spec/requests/projects/update_spec.rb index 5cf4b3664..d7464627a 100644 --- a/spec/requests/projects/update_spec.rb +++ b/spec/requests/projects/update_spec.rb @@ -124,6 +124,14 @@ put("/api/projects/#{project.identifier}", params:, headers:) expect(response.body).to include('updated instructions') end + + it 'saves and returns instructions in the instruction steps format' do + params[:project][:instructions] = [{ markdown_content: 'step 1' }, { markdown_content: 'step 2' }] + put("/api/projects/#{project.identifier}", params:, headers:) + + expect(project.reload.instructions).to eq([{ 'markdown_content' => 'step 1' }, { 'markdown_content' => 'step 2' }]) + expect(response.parsed_body['instructions']).to eq([{ 'markdown_content' => 'step 1' }, { 'markdown_content' => 'step 2' }]) + end end context 'when authed user is a teacher updating a class project' do From b0cd27504eec3be8aacd7f4f4cf317b991fa8c4a Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Fri, 7 Aug 2026 10:40:46 +0100 Subject: [PATCH 2/2] Move student instructions restriction into strong params Previously Project::Update compared old and new instructions to detect and reject a student trying to change them, needing a current_user argument and a JSON-normalising comparison just to tell Parameters and plain Ruby values apart. This change drops :instructions from the permitted params entirely when the current user is a student, so their attempt is filtered out the same way Rails treats any other unpermitted param. The request now succeeds with the change silently ignored, instead of returning a 422. Project::Update no longer needs to know who the current user is. --- app/controllers/api/projects_controller.rb | 17 ++++++---- config/locales/en.yml | 1 - lib/concepts/project/operations/update.rb | 27 ++-------------- .../project/update_default_component_spec.rb | 3 +- .../project/update_delete_components_spec.rb | 3 +- spec/concepts/project/update_invalid_spec.rb | 3 +- spec/concepts/project/update_spec.rb | 31 ++----------------- spec/requests/projects/update_spec.rb | 6 ++-- 8 files changed, 24 insertions(+), 67 deletions(-) diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index d9708f0aa..e9919976a 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -41,7 +41,7 @@ def create end def update - result = Project::Update.call(project: @project, update_hash: project_params, current_user:) + result = Project::Update.call(project: @project, update_hash: project_params) if result.success? track_project_event('Project - Saved', @project) @@ -94,7 +94,11 @@ def project_params end def base_params - params.fetch(:project, {}).permit( + params.fetch(:project, {}).permit(*permitted_project_attributes) + end + + def permitted_project_attributes + attributes = [ :school_id, :lesson_id, :user_id, @@ -102,15 +106,16 @@ def base_params :name, :project_type, :locale, - :instructions, { - components: %i[id name extension content index default], - instructions: [:markdown_content] + components: %i[id name extension content index default] }, scratch_component: {}, parent: {}, image_list: [] - ) + ] + return attributes if current_user&.student? + + attributes + [:instructions, { instructions: [:markdown_content] }] end def school_owner? diff --git a/config/locales/en.yml b/config/locales/en.yml index 664fc5670..7c9c88e49 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -9,7 +9,6 @@ en: delete_default_component: "Cannot delete default file" change_default_name: "Cannot amend default file name" change_default_extension: "Cannot amend default file extension" - student_update_instructions: "Student cannot update project instructions" remixing: invalid_params: "Invalid parameters" cannot_save: "Cannot create project remix" diff --git a/lib/concepts/project/operations/update.rb b/lib/concepts/project/operations/update.rb index 3c40cea6b..1e0e64166 100644 --- a/lib/concepts/project/operations/update.rb +++ b/lib/concepts/project/operations/update.rb @@ -3,11 +3,11 @@ class Project class Update class << self - def call(project:, update_hash:, current_user:) + def call(project:, update_hash:) response = setup_response(project) setup_deletions(response, update_hash) - update_project_attributes(response, update_hash, current_user) + update_project_attributes(response, update_hash) update_component_attributes(response, update_hash) persist_changes(response) response @@ -42,28 +42,7 @@ def validate_deletions(response) response[:error] = I18n.t 'errors.project.editing.delete_default_component' end - def student_project_instructions_updated?(response, update_hash, current_user) - is_school_project = response[:project].school.present? - user_is_student = current_user.student? - instructions_updated = normalize_instructions(response[:project].instructions) != normalize_instructions(update_hash[:instructions]) - is_school_project && user_is_student && instructions_updated - end - - # update_hash[:instructions] may be plain Ruby data or an ActionController::Parameters - # array/hash (for the instruction steps format); round-trip through JSON so both sides - # of the comparison are in the same plain, comparable shape. - def normalize_instructions(value) - ActiveSupport::JSON.decode(ActiveSupport::JSON.encode(value)) - end - - def validate_update(response, update_hash, current_user) - return unless student_project_instructions_updated?(response, update_hash, current_user) - - response[:error] = I18n.t 'errors.project.editing.student_update_instructions' - end - - def update_project_attributes(response, update_hash, current_user) - validate_update(response, update_hash, current_user) + def update_project_attributes(response, update_hash) return if response.failure? response[:project].assign_attributes(update_hash.slice(:name, :instructions)) diff --git a/spec/concepts/project/update_default_component_spec.rb b/spec/concepts/project/update_default_component_spec.rb index 6685977ca..5960fcc61 100644 --- a/spec/concepts/project/update_default_component_spec.rb +++ b/spec/concepts/project/update_default_component_spec.rb @@ -3,9 +3,8 @@ require 'rails_helper' RSpec.describe Project::Update, type: :unit do - subject(:update) { described_class.call(project:, update_hash:, current_user:) } + subject(:update) { described_class.call(project:, update_hash:) } - let(:current_user) { create(:user) } let!(:project) { create(:project, :with_default_component) } let(:default_component) { project.components.first } diff --git a/spec/concepts/project/update_delete_components_spec.rb b/spec/concepts/project/update_delete_components_spec.rb index 0fb64af55..18a1aadb0 100644 --- a/spec/concepts/project/update_delete_components_spec.rb +++ b/spec/concepts/project/update_delete_components_spec.rb @@ -3,9 +3,8 @@ require 'rails_helper' RSpec.describe Project::Update, type: :unit do - subject(:update) { described_class.call(project:, update_hash:, current_user:) } + subject(:update) { described_class.call(project:, update_hash:) } - let(:current_user) { create(:user) } let!(:project) { create(:project, :with_default_component, :with_components) } let(:component_to_delete) { project.components.last } let(:default_component) { project.components.first } diff --git a/spec/concepts/project/update_invalid_spec.rb b/spec/concepts/project/update_invalid_spec.rb index 84a825140..7c2560f56 100644 --- a/spec/concepts/project/update_invalid_spec.rb +++ b/spec/concepts/project/update_invalid_spec.rb @@ -8,10 +8,9 @@ name: 'updated project name', components: [default_component_hash, edited_component_hash, new_component_hash] } - described_class.call(project:, update_hash:, current_user:) + described_class.call(project:, update_hash:) end - let(:current_user) { create(:user) } let!(:project) { create(:project, :with_default_component, :with_components, component_count: 2) } let(:editable_component) { project.components.last } let(:default_component) { project.components.first } diff --git a/spec/concepts/project/update_spec.rb b/spec/concepts/project/update_spec.rb index 153b68946..e7466bd57 100644 --- a/spec/concepts/project/update_spec.rb +++ b/spec/concepts/project/update_spec.rb @@ -9,10 +9,9 @@ components: component_hash, instructions: } - described_class.call(project:, update_hash:, current_user:) + described_class.call(project:, update_hash:) end - let(:current_user) { create(:user) } let!(:project) { create(:project, :with_default_component, :with_components) } let(:editable_component) { project.components.last } let(:default_component) { project.components.first } @@ -97,10 +96,9 @@ end end - context 'when the instructions have changed and the current user is a teacher' do + context 'when the instructions have changed' do let(:school) { create(:school) } - let!(:current_user) { create(:teacher, school:) } - let!(:project) { create(:project, :with_instructions, school:, user_id: current_user.id) } + let!(:project) { create(:project, :with_instructions, school:, user_id: create(:teacher, school:).id) } let(:instructions) { 'new instructions' } it 'returns success? true' do @@ -111,29 +109,6 @@ expect { update }.to change { project.reload.instructions }.to('new instructions') end end - - context 'when the instructions have changed and the current user is a student' do - let(:school) { create(:school) } - let!(:current_user) { create(:student, school:) } - let!(:project) { create(:project, :with_instructions, school:, user_id: current_user.id) } - let(:instructions) { 'new instructions' } - - it 'returns success? false' do - expect(update.success?).to be(false) - end - - it 'does not update project name' do - expect { update }.not_to change { project.reload.name } - end - - it 'does not update project instructions' do - expect { update }.not_to change { project.reload.instructions } - end - - it 'returns an error message' do - expect(update[:error]).to eq('Student cannot update project instructions') - end - end end def component_properties_hash(component) diff --git a/spec/requests/projects/update_spec.rb b/spec/requests/projects/update_spec.rb index d7464627a..dc184ddc3 100644 --- a/spec/requests/projects/update_spec.rb +++ b/spec/requests/projects/update_spec.rb @@ -205,10 +205,12 @@ expect(response).to have_http_status(:ok) end - it 'returns unprocessable entity if instructions updated' do + it 'ignores an attempt to update instructions' do params[:project][:instructions] = 'updated instructions' put("/api/projects/#{project.identifier}", params:, headers:) - expect(response).to have_http_status(:unprocessable_content) + + expect(response).to have_http_status(:ok) + expect(project.reload.instructions).to be_nil end it 'records a project saved event' do