Support syncing ExperienceCS user projects - #979
Conversation
4f679d3 to
036934d
Compare
Test coverage93.46% line coverage reported by SimpleCov. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for migrating ExperienceCS user Scratch projects into their existing Code Classroom “stub” projects, using a service-authenticated endpoint and ensuring migrations (and related asset uploads) can be retried safely without granting broad access to other projects.
Changes:
- Adds
PUT /api/experience-cs/projects/:id/migrateto replace a locale-less legacy Scratch stub in place and mark it as migrated (experience_cs_migrated_at) for safe replays. - Introduces service-account abilities to allow migration + migration-asset upload while preventing general project access.
- Extends Scratch asset upload/view logic and test coverage to support migration assets and preserve project asset visibility rules.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/requests/experience_cs_project_migrations/update_spec.rb | Request specs for the new migration endpoint behavior (in-place replace, idempotency, auth/locale rules). |
| spec/models/project_spec.rb | Unit tests for Project#experience_cs_migration_target?. |
| spec/models/ability_spec.rb | Ability specs for the ExperienceCS service account migration permissions. |
| spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb | Feature specs for service-uploaded migration assets and migrated-asset visibility rules. |
| README.md | Documents the new migration endpoint and how project-scoped migration assets behave. |
| db/schema.rb | Schema version bump plus the new projects.experience_cs_migrated_at column. |
| db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb | Migration adding experience_cs_migrated_at to projects. |
| config/routes.rb | Adds the migration route under /api/experience-cs/projects/:id/migrate. |
| app/models/scratch_asset.rb | Allows Scratch assets to belong to ExperienceCS Scratch project types (legacy + code editor scratch). |
| app/models/project.rb | Adds experience_cs_migration_target? predicate used for authorization/eligibility. |
| app/models/ability.rb | Adds service-account abilities and expands school owner visibility for remixed lesson projects. |
| app/controllers/api/scratch/assets_controller.rb | Adds migration-asset upload support (service auth path, conflict checks, uploader attribution). |
| app/controllers/api/experience_cs_project_migrations_controller.rb | New controller implementing the migration update operation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
903b6b3 to
e2d8d15
Compare
| lesson_project_ids = Project.where( | ||
| school_id: school.id, | ||
| remixed_from_id: nil, | ||
| lesson_id: Lesson.where(school_id: school.id, visibility: %w[teachers students]).select(:id) | ||
| ).pluck(:id) | ||
| can(%i[read show_context], Project, school_id: school.id, remixed_from_id: lesson_project_ids) |
There was a problem hiding this comment.
Could you explain why this extra permission is needed? I can't see why the can(%i[read update show_context], Project permission above isn't sufficient and why this is important for School owners but not teachers.
If we do need it, I wonder if there's a better way to write it - currently it will do two queries on every page load for project owners even if the permission isn't used. Maybe Scopes can be used to simplify or cancancan's block mode can be used
There was a problem hiding this comment.
Student projects only link to lessons through their parent project, so this extra rule is needed; I'm rewritting it with your proposal to avoid extra queries.
There was a problem hiding this comment.
I still am a bit confused.
What endpoint or call to authorize won't work as needed if this doesn't exist?
Doesn't the more general permission above do everything we need? (which is now very similar except it doesn't check the parent relationship)
There was a problem hiding this comment.
/api/projects/:id could return 403 when a school owner who is not a class teacher opens a migrated student project.
A student project has no lesson_id; it reaches the lesson through its parent.
We only need this for the case where school owners must be able to view the migrated project.
this way we follow the permissions in the ticket -> the student, teachers in the class, owners can see the project
If “owners” does not mean school owners, or that access is not required, we can remove it.
| # frozen_string_literal: true | ||
|
|
||
| module Api | ||
| class ExperienceCsProjectMigrationsController < ApiController |
There was a problem hiding this comment.
Handling this in a new controller is a good idea and makes it easier to remove it when we have done the migration.
| def experience_cs_migration_target? | ||
| return false unless user_id.present? && school_id.present? | ||
|
|
||
| project_type == Types::SCRATCH || | ||
| (project_type == Types::CODE_EDITOR_SCRATCH && experience_cs_migrated_at.present?) |
There was a problem hiding this comment.
Could you explain the reasoning for introducing the experience_cs_migrated_at column?
Re-running the migration is good could be useful, however it could also cause data to be lost if updates have been made in Code Classroom after a project has been migrated.
I wonder if it would be simpler to just allow Types::SCRATCH to be migrated. If we do need to fix anything in the migration, we'll have to consider what to do when it comes up.
There was a problem hiding this comment.
I added it to identify previously migrated projects and allow retries, but you’re right that retrying could overwrite newer Code Classroom changes, i'm removing the column
| prepend_before_action :load_experience_cs_service_user, only: %i[create_global] | ||
| prepend_before_action :load_experience_cs_service_user, only: :create_global | ||
| prepend_before_action :load_project_asset_context, only: %i[show create] | ||
| before_action :authorize_user, except: %i[show] | ||
| prepend_before_action :load_project_from_header, only: %i[show create] | ||
| authorize_resource :project_from_header, except: %i[create_global] | ||
| before_action :authorize_project_from_header, except: %i[create_global] |
There was a problem hiding this comment.
I am finding this much harder to follow the loading and authorisation flows with these changes so have less confidence everything is authorized as expected.
Since this is only temporarily needed, I wonder if having a seperate action for adding assets to a project for the ExCS user could simplify this and reduce the risk of getting auth wrong. It would also mean it would be easier to remove the functionality once the migration is complete.
There was a problem hiding this comment.
Agreed, i'm going to move to its own action so it's simpler. Thanks for the comments, they do make sense, i was really focus on experience-cs to work.
e2d8d15 to
bbe3fe7
Compare
Status
What's changed?
Adds an endpoint for migrating an ExperienceCS project into its existing Code Classroom stub.
The migration: