Let owners toggle Scratch for their school#839
Open
zetter-rpf wants to merge 9 commits into
Open
Conversation
Test coverage91.34% line coverage reported by SimpleCov. |
zetter-rpf
force-pushed
the
enable-scratch-endpoint
branch
2 times, most recently
from
9fd2598 to
f486e88
Compare
zetter-rpf
force-pushed
the
enable-scratch-endpoint
branch
from
34c6dd7 to
5215472
Compare
This update schools endpoint isn't used, and there were no requests to it the last 30 days. We've chosen to limit the params so we don't get any unexpected behaviour, but can add old ones back in if we want to allow school owners to update them in the future. We've also removed the 422 test as can't seem to make a similar failing test. We've also checked the abilities and only school owners can update the school, but owners, teachers and students can view it.
zetter-rpf
force-pushed
the
enable-scratch-endpoint
branch
from
a689dd6 to
e351e91
Compare
zetter-rpf
changed the title
zetter-rpf
force-pushed
the
enable-scratch-endpoint
branch
from
b406d4b to
8b57ca5
Compare
zetter-rpf
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces the cat_mode Flipper feature flag with a new per-school scratch_enabled boolean column. School members can now view/remix/update existing Scratch projects they have access to regardless of any feature flag; only the creation of new Scratch lessons is gated by scratch_enabled. The schools update endpoint is repurposed to toggle this flag, the lessons controller restricts what params can be updated, and the Scratch controllers are refactored to use the standard CanCan authorize_resource pattern.
Changes:
- New
scratch_enabledboolean onschools(migration + schema + jbuilder view), exposed viaPUT /api/schools/:id(now only permittingscratch_enabled). - Scratch controllers: replaced the
cat_modefilter with a school-membership check and switched toauthorize_resourceforProject/original_project/project_from_header; added averify_can_create_scratch_projectsguard in the lessons controller and splitlesson_paramsintocreate_params/update_params. - Updated and reorganized request specs to reflect the new behavior; removed the now-duplicate
creating_a_scratch_asset_spec.rbfile.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| db/migrate/20260528141937_add_scratch_enabled_to_school.rb | Adds scratch_enabled boolean column to schools. |
| db/schema.rb | Reflects the new column and bumps schema version. |
| app/views/api/schools/_school.json.jbuilder | Exposes scratch_enabled in school JSON. |
| app/controllers/api/schools_controller.rb | Splits permitted params into create_params / update_params (update is restricted to scratch_enabled). |
| app/controllers/api/lessons_controller.rb | Adds verify_can_create_scratch_projects filter, splits create_params/update_params, and routes update through the restricted set. |
| app/controllers/api/scratch/scratch_controller.rb | Replaces check_scratch_feature Flipper check with a school-membership check; drops load_project (moved to projects controller). |
| app/controllers/api/scratch/projects_controller.rb | Switches to authorize_resource :project / :original_project; refactors load_original_project. |
| app/controllers/api/scratch/assets_controller.rb | Uses authorize_resource :project_from_header instead of inline authorize! calls. |
| spec/features/my_school/showing_my_school_spec.rb | Adds scratch_enabled to expected JSON. |
| spec/features/school/updating_a_school_spec.rb | Reworks update spec to assert scratch_enabled flips; removes invalid-name test. |
| spec/features/lesson/creating_a_lesson_spec.rb | Adds tests for Scratch project creation gated by scratch_enabled. |
| spec/features/lesson/updating_a_lesson_spec.rb | Updates expectation: re-assigning class is now silently ignored rather than 422. |
| spec/features/scratch/creating_a_scratch_project_spec.rb | Replaces cat_mode flag setup with school-membership scenarios; updates expected status code to 403. |
| spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb | Removes cat_mode flag setup; adds 401/404 cases for unauthenticated / non-school users. |
| spec/features/scratch/showing_a_scratch_project_spec.rb | Requires authentication and adds 401/403 coverage. |
| spec/features/scratch/updating_a_scratch_project_spec.rb | Removes cat_mode setup; adds 403 case for inaccessible project. |
| spec/features/scratch/creating_a_scratch_asset_spec.rb | Deletes duplicate spec file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The behaviour we want is to allow any existing scratch projects to be accessed updated and so I've removed all the checks from the Scratch Controller, except that they are logged in and part of a school. The school check could be removed in the future if we want to open up access to Scratch. Note that some controllers were missing the logged in check so I've updated the code and tests
We're adding a new setting to allow Scratch to be toggled by school owners since Flipper is not designed for gating large amounts of individual actors.
Previously any params could be updated in lessons which was dangerous - we don't expect many of these params to be updated to changing them could create odd behaviour or allow other data to be accessed. I've checked the frontend, and only the name and visibility can be updated. I've done this now so that users can't turn their projects into Scratch projects unexpectedly. We could add attributes back in in the future if we want to.
This is only relevant to the projects controller, so there's no need to have it in the shared controller
While we have been working on this we have ended up with two test files that cover creating assets. Use the more comprehensive file for now and move in the only one that wasn't covered.
Using authorize_resource directly makes it easier to remove authorization concerns from controller actions, provide better defaults for the controller, and give us consistent behaviour for unauthorized requests.
zetter-rpf
force-pushed
the
enable-scratch-endpoint
branch
from
8b57ca5 to
2b204ba
Compare
Contributor
Author
|
This is deployable while I'm off once reviewed, I suggest deploying it just before @DNR500's work for https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1375 otherwise it will be hard for us to test enabling scratch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
What's changed?
Previously members of schools could access Scratch projects if they had the
cat_modefeature was enabled.This change allows members of schools to view/remix/update existing scratch projects they have access to, regardless of the feature flag. There is a new
scratch_enabledflag in the database that decides if school members can create new scratch projects.As part of this we have:
scratch_enabledflag in the lessons controllerAs part of deploy
We should deploy the frontend changes soon after this to make it easier to toggle the Scratch feature