Discovery: store and return Scratch data#720
Conversation
Rather than using the existing Component model, this adds a new Scratch component model. Projects expect to be able to update project metadata and component data at the same time, however for Scratch the lifecycle of project.json is managed independently by scratch-gui so it's best to keep that separate.
Test coverage89.61% line coverage reported by SimpleCov. |
mwtrew
changed the title
Only SVGs are working at present, as non-text formats like PNG are not recognised due to Scratch sending the incorrect content type header. Further work needed to support other file types.
This reverts commit 6d4d4bc. This was only partially working, we will revisit it in a subsequent PR.
There was a problem hiding this comment.
Pull request overview
Adds persistence for Scratch project.json by introducing a ScratchComponent model/table associated to Project, and wiring up API endpoints/tests to load and save that JSON content.
Changes:
- Add
scratch_componentstable +ScratchComponentmodel andProject#scratch_componentassociation. - Update project creation / permitted params to accept and build scratch component data.
- Implement
/api/scratch/projects/:idshow/update endpoints and request specs.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
app/controllers/api/scratch/scratch_controller.rb |
Adds shared load_project before-action for Scratch API controllers. |
app/controllers/api/scratch/projects_controller.rb |
Switches Scratch project show to return stored JSON and adds update to persist JSON. |
app/models/project.rb |
Introduces has_one :scratch_component and assignment helper. |
app/models/scratch_component.rb |
New AR model for storing Scratch JSON. |
db/migrate/20260309104851_create_scratch_components.rb |
Migration creating scratch_components table. |
db/schema.rb |
Schema update reflecting new table + FK. |
lib/concepts/project/operations/create.rb |
Extends project creation to build a scratch component. |
app/controllers/api/projects_controller.rb |
Permits scratch_component in project params. |
app/controllers/api/lessons_controller.rb |
Permits scratch_component within nested project_attributes. |
app/graphql/mutations/create_project.rb |
Passes scratch_component through to Project::Create. |
spec/factories/scratch_components.rb |
Adds factory for scratch components with default JSON shape. |
spec/features/scratch/showing_a_scratch_project_spec.rb |
Updates show spec to create real project + scratch component. |
spec/features/scratch/updating_a_scratch_project_spec.rb |
Updates update spec to persist scratch content and assert DB write. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| end | ||
|
|
||
| def update | ||
| @project.scratch_component&.content = params |
There was a problem hiding this comment.
@project.scratch_component&.content = params will persist framework/routing keys like controller, action, id, and potentially other untrusted request keys into content, which will then be echoed back by show. Extract and persist only the Scratch project payload (e.g., params.require(:content) or a permitted subset like targets/monitors/extensions/meta) and convert to a plain Hash before assignment.
| @project.scratch_component&.content = params | |
| scratch_content = params.require(:content) | |
| @project.scratch_component&.content = scratch_content.to_unsafe_h |
| end | ||
|
|
||
| def update | ||
| @project.scratch_component&.content = params |
There was a problem hiding this comment.
Just to note that I can't see any authorization checks on the project to make sure the user is allowed to write to it.
This doesn't have to be done as part of this PR, but we should make sure there's an issue for it if not.
We can't add auth to the show action yet (but I am coming up with a proposal for that)
There was a problem hiding this comment.
Yep, expecting this to be handled in a separate ticket.
There was a problem hiding this comment.
Could you please make sure it's covered by an issue (by adding it to one or creating a new one) - I don't want it to be lost.
zetter-rpf
left a comment
zetter-rpf
left a comment
There was a problem hiding this comment.
This looks great, I've left a few comments and I saw you're looking at the copilot ones, let me know when they are resolved and I can approve.
This avoids missing errors.
In the update case it won't matter, and in the show case an error would be better than nil.
This reverts commit e8b84ee. This causes an "unoptimised query" error that I'd rather not dive into just now.
Status
Points for consideration:
See the ADR for performance and security implications.
What's changed?
project.json). Does not yet enable these for assets.