Allow Experience CS admins to create public projects #549
Conversation
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
raspberrypiherokubot
temporarily deployed
to
editor-api-p-allow-expe-u7yd7q
We want to allow experience-cs admins to create "public" projects via experience-cs administrate UI [1]. When I say "public" projects, I mean those that are accessible to all users, because `Project#user_id` is set to `nil`. Public projects for Python & HTML are currently added via the `GithubWebhooksController#github_push` [2] action and/or via the (possibly defunct) `projects:create_all` rake task [3] both of which make use of `ProjectImporter` [4]. However, these mechanisms are not suitable for our use case. This commit introduces a new `Api::PublicProjectsController` which is somewhat based on `Api::ProjectsController`. However, it differs in a couple of key respects: only experience-cs admin users should be authorized to use it; it should not be possible to set `Project#user_id`; and we want some sensible validation to restrict the values entered into the administrate UI and provide sensible error messages to the user. Conceivably this could be achieved by enhancing the existing `Api::ProjectsController`, but this is already pretty complicated and I wanted to minimize the chances of breaking existing behaviour. I've tried to roughly follow the existing conventions however much I disagree with them, e.g. I've introduced a `PublicProject::Create` class in `lib/concepts/public_project/operations/create.rb` along the same lines as `Project::Create` in `lib/concepts/project/operations/create.rb`. This is just a skeletal starting point - I plan to flesh it out in subsequent commits. [1]: RaspberryPiFoundation/experience-cs#405 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/app/controllers/github_webhooks_controller.rb#L6-L8 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/tasks/projects.rake#L4-L7 [4]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/project_importer.rb
The original version was copied from `Api::ProjectsController#base_params`, but that seems to be an abberation introduced [1] for no obviously discernable reason. Using `ActionController::Parameters#require` [2] is more idiomatic Rails. [1]: #86 [2]: https://api.rubyonrails.org/v7.1.3.4/classes/ActionController/Parameters.html#method-i-require
I'm planning to introduce another role-related predicate method in a subsequent commit. Extracting this method first will make that change easier. I'm pretty convinced the call to `#to_s` was made redundant when the safe navigation operator was introduced in this commit [1]. However, I suppose it's theoretically possible for the parsed JSON returned from `HydraPublicApiClient.fetch_oauth_user` via `User.from_token` to contain non-String values, so I'm going to leave it in place for now. [1]: 9a1fdb1
I'm planning to make use of this to grant access to `Api::PublicProjectsController#create` so that admin users on the `experience-cs` website can create public Scratch projects via the `editor-api` API.
floehopper
force-pushed
the
allow-experience-cs-admins-to-create-public-projects
branch
from
0698fa0 to
d297533
Compare
I plan to use this to restrict creating public projects to experience-cs admin users.
For the moment, we only want to allow experience-cs admin users to be able to create public projects.
I found the previous name confusing.
The `saved_project` wasn't really being used and it's sufficient to assert the final state of `Project#identifier` rather than aserting that it's changed.
When creating a public project we want the admin user to be able to specify the identifier and apply validation rules against it. However, prior to this change, if the supplied identifier was `nil`, the `before_validation` callback, `Project#generate_identifier`, would kick in and generate an identifier. Thus the presence validation on `Project#identifier` never failed. The new `Project#skip_identifier_generation` attribute gives us the ability in specific contexts to prevent the identifier being generated and thus allow the presence validation to fail.
This makes use of the recently added `Project#skip_identifier_generation` attribute to ensure the presence validation on `Project#identifier` can fail. In this case we want to report this problem back to the user so they can correct it rather than just generating a fallback identifier and continuing.
Accepting the `product_type` param means that this controller can legitimately be named more generically, i.e. `PublicProjectsController` vs e.g. `ScratchPublicProjectsController`. However, the immediate requirement is for admin users in the experience-cs app to be able to create public Scratch projects and so I've only implemented the functionality necessary to support that use case. If we want to expand the capability of this controller to support the creation of HTML and/or Python projects, we'll need to support other params, e.g. `components`.
The `PhraseIdentifier.generate` method is used to generate an identifier for projects created by teachers and students. However, public projects, e.g. those created via the `GithubWebhooksController#github_push` [1] action and those created via the (possibly defunct) `projects:create_all` rake task [2], have identifiers specified in their project config via `ProjectImporter` [3]. While there is no *explicit* validation to enforce it, the identifiers for these public projects seems to follow the pattern described by the regular expression I have defined in `PhraseIdentifier::PATTERN`, i.e. kebab-case with digits allowed, at least in the 2nd word [4]. The pattern could probably be stricter, but since I don't have access to the production data, it seems safer for it to be more relaxed. We want admin users in experience-cs to be able to create public scratch projects via the editor-api API and I think it makes sense to restrict them to using identifiers following a similar pattern. I plan to add some validation to enforce that and so we can provide feedback to the admin user if they choose an identifier with the wrong format. I'll be able to use this `PhraseIdentifier::PATTERN` in that validation. [1]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/app/controllers/github_webhooks_controller.rb#L6-L8 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/tasks/projects.rake#L4-L7 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/project_importer.rb [4]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/tasks/project_components/top_5_emoji_list/project_config.yml#L2
I want to add some validations to the `Project` model to provide some sensible constraints on the values that an experience-cs admin user enters into the administrate UI so that I can provide sensible error messages. However, since I don't have access [1] to staging or production data for `editor-api`, I can't be sure that some existing projects would become invalid. So as a workaround I've implemented this `PublicProject` wrapper class which allows me to add validations for use only in the context of the `Api::PublicProjectsController`. To start with, I've just added a validation to check the format of `Project#identifier`, although this isn't yet being used in the controller action. Hopefully implementing these validations as ActiveModel validations will make it easier to move them onto the `Project` model once it's deemed safe to do so. [1]: https://raspberrypifoundation.slack.com/archives/C08831T17H6/p1747900618263229
This uses the recently added `PublicProject` wrapper class to add the validation just in the context of the `Api::PublicProjectsController`. The validation is based on the regular expression in `PhaseIdentifier::PATTERN`, i.e. kebab-case.
Somewhat surprisingly there is currently no validation for the presence of `name` on the `Project` model itself, although I suspect the name is probably implicitly guaranteed not to be blank by the clients of editor-api. Since admin users in experience-cs are going to be entering the name manually in the administrate UI, I want to protect against a blank name and I want to be able to provide a sensible error message if none is set. Ideally I'd add a validation to the `Project` model itself. However, since I don't have access [1] to staging or production data for `editor-api`, I can't be sure that some existing projects would become invalid. So as before I've added the validation to the `PublicProject` wrapper class which means the validation will only be used in the context of the `Api::PublicProjectsController`. [1]: https://raspberrypifoundation.slack.com/archives/C08831T17H6/p1747900618263229
A public project, i.e. one that is accessible to all users, should not have `Project#user_id` set. This is already implicitly enforced by `Api::PublicProjectsController#project_params` which excludes the `user_id` param. However, I thought it was worth adding an example to guarantee it remains the case.
I'm about to add an update action which needs different params, so renaming `project_params` to `create_params` will make that change simpler.
floehopper
force-pushed
the
allow-experience-cs-admins-to-create-public-projects
branch
from
d297533 to
2b6f331
Compare
We want to allow experience-cs admins to update "public" projects via experience-cs administrate UI [1]. When I say "public" projects, I mean those that are accessible to all users, because `Project#user_id` is set to `nil`. Public projects for Python & HTML are currently updated via the `GithubWebhooksController#github_push` [2] action and/or via the (possibly defunct) `projects:create_all` rake task [3] both of which make use of `ProjectImporter` [4]. However, these mechanisms are not suitable for our use case. This commit introduces a new `Api::PublicProjectsController#update` action which is somewhat based on `Api::ProjectsController#update`. I've tried to roughly follow the existing conventions however much I disagree with them, e.g. I've introduced a `PublicProject::Update` class in `lib/concepts/public_project/operations/update.rb` along the same lines as `Project::Update` in `lib/concepts/project/operations/update.rb`. This is just a skeletal starting point - I plan to flesh it out in subsequent commits. Note that I've had to limit the `restrict_project_type` before action to just the create action, because it relies on the `project.project_type` param which will not be accepted by the `update` action. [1]: RaspberryPiFoundation/experience-cs#405 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/app/controllers/github_webhooks_controller.rb#L6-L8 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/tasks/projects.rake#L4-L7 [4]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/project_importer.rb
This is closely based on how `Api::ProjectsController` works although it's not obvious that its behaviour is tested. However, there is a spec for `ProjectLoader` itself, so I've just added examples to make sure it's wired up to the controller correctly. One thing I've added is to raise `ActiveRecord::RecordNotFound` if `ProjectLoader#load` returns `nil` which seems possible. This will result in a 404 Not Found response which is what I was already asserting in `spec/features/public_project/updating_a_public_project_spec.rb`. As far as I can tell, `Api::ProjectsController` actions which make use of `ProjectLoader#load` (e.g. the `update` action) do not check for the project being `nil` and so will probably respond with a 500 Internal Server Error when the code (e.g. in `Project::Update`) calls a method on the `nil` project. This doesn't seem great!
The immediate use case for `Api::PublicProjectsController#update` is for experience-cs admin users to change the name of a public project. Since experience-cs only deals with Scratch projects, it makes sense to use projects of this type in the specs. However, since we're using `ProjectLoader` to find the project and the default scope is still in place to hide Scratch projects, we need to make use of the `ProjectScoping` concern to use the `project_type` param to set `Current.project_scope` to include Scratch projects.
I plan to use this to restrict updating public projects to experience-cs admin users.
For the moment, we only want to allow experience-cs admin users to be able to update public projects.
This `Api::PublicProjectsController` is focussed on managing "public" projects, i.e. those that have no `Project#user_id` set. I think it's sensible to protect against the `update` action being used to update a non-public project, because it makes the code easier to reason about.
There's currently no need to be able to *update* the `locale` or `project_type` from experience-cs, but since we are supplying them for the `create` action, I think it's useful to highlight that they're not accepted by the `update` action. It doesn't make much sense to allow the `user_id` to be updated via this controller that specifically relates to "public" projects, so I think it's useful to add an example to highlight that `user_id` is not accepted by the `update` action. This behaviour is currently effectively implemented by omitting these attibute keys from the permitted params in `Api::PublicProjectsController#update_params`.
We can re-use the validations in the `PublicProject` wrapper class to ensure that, in `Api::PublicProjectsController#update`, the project identifier is updated to a non-blank value in the correct format and the project name is updated to a non-blank value.
We want to allow experience-cs admins to update "public" projects via experience-cs administrate UI [1]. When I say "public" projects, I mean those that are accessible to all users, because `Project#user_id` is set to `nil`. Public projects for Python & HTML are currently updated via the `GithubWebhooksController#github_push` [2] action and/or via the (possibly defunct) `projects:create_all` rake task [3] both of which make use of `ProjectImporter` [4]. However, it doesn't look as if there's a way to delete an existing project through that mechanism and in any case it's not very suitable for our use case. This commit introduces a new `Api::PublicProjectsController#destroy` action which is somewhat based on `Api::ProjectsController#destroy`. As before I've tried to roughly follow the existing conventions however much I disagree with them. Unlike in `Api::ProjectsController#destroy`, the action responds with 422 Unprocessable Entity if `Project#destroy` returns `false`. This is just a skeletal starting point - I plan to flesh it out in subsequent commits. However, I have already re-used the `ProjectLoader` for finding the project by identifier and locale. [1]: RaspberryPiFoundation/experience-cs#405 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/app/controllers/github_webhooks_controller.rb#L6-L8 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/tasks/projects.rake#L4-L7 [4]: https://github.com/RaspberryPiFoundation/editor-api/blob/f348dfc92fdd8b19fbfd5434e7751340f33b4093/lib/project_importer.rb
I plan to use this to restrict destroying public projects to experience-cs admin users.
For the moment, we only want to allow experience-cs admin users to be able to destroy public projects.
The immediate use case for `Api::PublicProjectsController#destroy` is for experience-cs admin users to destroy a public project. Since experience-cs only deals with Scratch projects, it makes sense to use projects of this type in the specs. However, since we're using `ProjectLoader` to find the project and the default scope is still in place to hide Scratch projects, we need to make use of the `ProjectScoping` concern to use the `project_type` param to set `Current.project_scope` to include Scratch projects.
This `Api::PublicProjectsController` is focussed on managing "public" projects, i.e. those that have no `Project#user_id` set. I think it's sensible to protect against the `destroy` action being used to update a non-public project, because it makes the code easier to reason about.
The only use case for `Api::PublicProjectsController#destroy` is currently to allow experience-cs admins to delete a project. Although the `Project` model allows such a deletion, it's not obvious that it's entirely sensible. And in any case, while the Learning Team is using the experience-cs admin UI to create projects for the curriculum, there's little danger that they will have remixes until they're associated with lessons, etc. So this seems like a safer option for now. We can always relax it later if it becomes a problem.
floehopper
force-pushed
the
allow-experience-cs-admins-to-create-public-projects
branch
from
b8f31da to
1b08bbd
Compare
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.
No description provided.