Issues/1263 scope assets to project in scratch#783
Conversation
Test coverage89.76% line coverage reported by SimpleCov. |
ef4870e to
ddfaa05
Compare
5ef7e67 to
d2dd13c
Compare
There was a problem hiding this comment.
Pull request overview
Scopes Scratch assets to projects (via project_id) so assets are no longer globally shared by default, while preserving remix lineage access and keeping library assets usable as global fallbacks. It also tightens Scratch project access control and avoids duplicate remix creation.
Changes:
- Added
project_idtoscratch_assets(with partial unique indexes) and a migration to backfill legacy assets onto referencing projects while reusing blobs. - Updated Scratch asset endpoints to require
X-Project-ID, enforce project/remix visibility, and adjust SVG content-type behavior (trusted only for global/library SVGs). - Updated Scratch project endpoints to authorize access and to reuse an existing remix instead of creating duplicates.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/controllers/api/scratch/assets_controller.rb |
Requires X-Project-ID, enforces authorization, scopes asset reads/writes to project/remix visibility, and can auto-create a remix for uploads. |
app/controllers/api/scratch/projects_controller.rb |
Adds explicit authorization for show/update and reuses an existing remix on remix create. |
app/controllers/concerns/scratch_remix_creation.rb |
New concern to create a remix with fallback Scratch content when needed (e.g., asset upload before first save). |
app/models/scratch_asset.rb |
Introduces project scoping, global/project scopes, lineage-based asset lookup, and SVG content-type policy. |
app/models/project.rb |
Adds self_and_ancestor_ids to support remix-lineage asset resolution. |
db/migrate/20260410110000_scope_scratch_assets_to_projects.rb |
Adds project_id, removes old uniqueness, backfills legacy assets to projects, and adds partial unique indexes. |
db/schema.rb |
Reflects scratch_assets.project_id and new indexes/foreign key. |
lib/scratch_asset_importer.rb |
Ensures imports only short-circuit on existing global assets and creates imported assets as global (project_id: nil). |
spec/factories/scratch_assets.rb |
Sets default project: nil for assets in tests. |
spec/lib/scratch_asset_importer_spec.rb |
Adds expectations around global imports and filename collisions with project assets. |
spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb |
Expands coverage for X-Project-ID, project scoping, remix/ancestor fallback, SVG content-type policy, and concurrent/idempotent uploads. |
spec/features/scratch/creating_a_scratch_asset_spec.rb |
Updates request headers/tests for X-Project-ID requirement. |
spec/features/scratch/creating_a_scratch_project_spec.rb |
Updates authorization expectations and tests remix reuse behavior. |
spec/features/scratch/showing_a_scratch_project_spec.rb |
Adds private-project access control tests (unauthenticated vs owner). |
spec/features/scratch/updating_a_scratch_project_spec.rb |
Updates cat_mode setup and adds “forbidden when cannot update” coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aa15b2d to
9c7b3e6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9c7b3e6 to
bf21101
Compare
bf21101 to
e5086d9
Compare
Status
What's changed?
Scratch assets are no longer treated as shared by everyone.
Uploaded assets are now linked to a Scratch project using project_id. This means:
The Scratch asset endpoints now:
Scratch project access:
There is also a fix for students remixing teacher projects:
I think this is the most complex part to understand, so i will add some extra words, since a student may want to add an asset but hasn't clicked yet the save button (i.e. remixed) we need to somehow be able to predo it so when the user clicks the button it's already associated to the future remix, since the user wouldn't be able to add an asset to the teacher's project. There are alternative solutions, like forcing the user to save the project before saving assets, but the way it works, i believe it can cause more issues. Another way would be preventely save immediately the project to detach from the teacher. So happy to discuss this part if we want to do it in another way.
⚠️ I found out this when i tested the first implementation and i was getting errors as the student.
The migration adds project_id to scratch_assets and backfills old assets onto the projects that reference them.
If the same old asset is used by multiple projects, the migration creates one asset row per project but reuses the same stored blob, so files are not copied. Global assets are represented by project_id = nil.
We can remove the service and the backfill task if we believe it doesn't belong here.
This makes Scratch assets private to the right project instead of globally readable, while still keeping: