Support instruction steps - #953
Conversation
cf0927d to
4f01c88
Compare
Test coverage92.18% line coverage reported by SimpleCov. |
4f01c88 to
c10b927
Compare
There was a problem hiding this comment.
Pull request overview
Adds support for storing project instructions as multi-step content (JSON) while adjusting update behavior so student attempts to change instructions are ignored rather than rejected. This fits into the Rails REST API layer (controllers + Jbuilder) and the Project::Update domain operation.
Changes:
- Convert
projects.instructionsfromtexttojsonbto support either a plain markdown string or an array of instruction steps. - Update request handling so students cannot submit
instructionsvia strong params (making instruction updates a no-op for students). - Extend request specs to cover reading/writing the instruction-steps format and update unit specs for the new
Project::Updatesignature.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/requests/projects/update_spec.rb | Adds coverage for saving/returning instruction steps; updates student behavior expectation to “ignore” instruction changes. |
| spec/requests/projects/show_spec.rb | Adds coverage for returning instruction steps when stored as an array. |
| spec/concepts/project/update_spec.rb | Updates unit specs for Project::Update signature change; removes student/teacher branching in operation tests. |
| spec/concepts/project/update_invalid_spec.rb | Updates unit spec call signature for Project::Update. |
| spec/concepts/project/update_delete_components_spec.rb | Updates unit spec call signature for Project::Update. |
| spec/concepts/project/update_default_component_spec.rb | Updates unit spec call signature for Project::Update. |
| lib/concepts/project/operations/update.rb | Removes current_user-based validation and always assigns :instructions if present in update_hash. |
| db/schema.rb | Reflects projects.instructions as jsonb. |
| db/migrate/20260807120000_change_projects_instructions_to_jsonb.rb | Migration to convert instructions from text to jsonb (and back). |
| config/locales/en.yml | Removes now-unused “student_update_instructions” translation. |
| app/controllers/api/projects/remixes_controller.rb | Permits instruction-step shape in remix request params. |
| app/controllers/api/projects_controller.rb | Moves instruction-update prevention for students into strong params; refactors permitted attributes list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c10b927 to
1562b28
Compare
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.
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.
1562b28 to
b0cd275
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
app/models/project.rb:83
- The setter writes every non-array value into the JSONB column, and the getter then returns it without the legacy text column's string coercion. Because
:instructionsis permitted as a scalar, values such as numbers or booleans now round-trip as JSON numbers/booleans even though the supported contract is a string or an array of{ markdown_content: ... }steps; nested parameters can also produce an object. Add format validation (including each array element) or normalize/reject unsupported values before assigninginstruction_steps.
def instructions=(value)
self[:instructions] = value unless value.is_a?(Array)
self[:instruction_steps] = value
app/controllers/api/projects/remixes_controller.rb:86
- This newly permits structured instructions on remix requests, but
Project::CreateRemixnever readsparams[:instructions];format_projectonly duplicates the original instructions and applies name/user/remix fields. A submitted steps array is therefore silently ignored. Either apply the permitted instructions when formatting the remix (with the intended student restriction) or remove this accepted parameter if remixes must always inherit the original.
instructions: [[:markdown_content]]
|
|
||
| class AddInstructionStepsToProjects < ActiveRecord::Migration[8.1] | ||
| def change | ||
| add_column :projects, :instruction_steps, :jsonb |
There was a problem hiding this comment.
What made you decide to add the extra column rather than modify and use the current instructions column?
There was a problem hiding this comment.
Good question, there's some details in the commit message:
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.
There was a problem hiding this comment.
This isn't true for all type changes, but was for this one when I tested it because Rails treats jsonb differently.
| class Update | ||
| class << self | ||
| def call(project:, update_hash:, current_user:) | ||
| def call(project:, update_hash:) |
There was a problem hiding this comment.
I'm curious, why are we removing the current_user everywhere here?
There was a problem hiding this comment.
There's detail in the commit message:
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.
I think it's simpler to do this in the controller and let Rails manage unexpected parameters rather than special casing this. Having it in the update required more complexity to see if the instructions had changed which was added in the first commit.
jamiebenstead
left a comment
jamiebenstead
left a comment
There was a problem hiding this comment.
LGTM. Left a few comments on things I am curious about
Status
What's changed?
The new instructions will be in a format such as:
This has been added to a new
instruction_stepscolumn that will eventually replace instructions.This change is backwards compatible with current versions of editor since sole strings are allowed in the JSONB column. It won't be at the point that we start storing instruction steps instead of strings in that column.
Created with help from Claude Code