Create models and controllers for lessons (rebased against main and fixed up)
#258
Conversation
Set the request body from the existing params that we know are valid. Otherwise, the request might fail with an error but the test would pass for the wrong reason.
This is so that we can present the user attributed to the lesson. This might not necessarily be a school-teacher if it is a public lesson authored by Raspberry Pi.
We already have this pattern in SchoolClasses controller so make it so that school-owner users are allowed to set the Lesson#user whereas regular users may not - the lesson is always assigned to themselves.
The authorisation logic varies by #visibility: - ‘private’ lessons can only be seen by the lesson’s user (e.g. a draft) - ‘public’ lessons can be seen by all users (e.g. for Raspberry Pi lessons) - ‘teachers’ lessons can be seen by all teachers within the school - ‘students’ lessons can be seen by students within the school, provided they are a member of the lesson’s class
The naming follows the ‘remixed_from’ convention on projects.
For consistency with projects.remixed_from_id
A project can have an optional school association, e.g. if it is a member of a school’s library of resources or if it is assigned to a lesson.
If the user has the school-owner or school-teacher role, they can create a project that is associated with a school (but not a school class). If the user is a school-owner they can assign a teacher to the project in the same was as lessons, otherwise the school-teacher is always set as the project’s owner (user).
These methods are class methods and so we should use a "." prefix rather than a "#" prefix which denotes an instance method.
Previously `User.temporarily_add_organisations_until_the_profile_app_is_updated` did not mutate the `hash` argument and the callsites ignored the return value and so the organisations were never added. This wasn't causing any spec failures, because all the relevant stubbing was using users from `spec/fixtures/users.json` all of which already had a value for the "organisations" key set. However, the real servers for `UserInfoApiClient.from_userinfo` & `HydraPublicApiClient.from_token` currently do *not* return a value for the "organisations" key. Indeed that's _why_ the `User.temporarily_add_organisations_until_the_profile_app_is_updated` method exists in the first place. I've added examples for `UserInfoApiClient.from_userinfo` & `HydraPublicApiClient.from_token` which make use of a new user in `spec/fixtures/users.json` which has no value for the "organisations" key set. These examples failed until I changed the implementation of `User.temporarily_add_organisations_until_the_profile_app_is_updated` to mutate the `hash` argument. I think there might be an argument to remove the value for the "organisations" key for all users in `spec/fixtures/users.json`, but this will do for now.
Strictly speaking this doesn't test that the current implementation returns `true` if the user has the "editor-admin" role for *any* organisation. However, that's already a bit of a bodge until we decide whether we want to properly support non-organisation-specific roles, so I think this is fine for now.
raspberrypiherokubot
temporarily deployed
to
editor-api-p-issues-239-usxxhw
There was a problem hiding this comment.
Nice spot 👍
Yeah, we had some discussions with the core team about them introducing the notion of organisations into the profile app so that user permissions could be scoped to a school (organisation). Greg or Daniel can probably explain more, I don't remember all the details.
|
I think this document has more details, although I no longer have access. |
Previously this spec was making use of a `User` model and the `User#serializable_hash` method. However, I'm pretty sure that was just a convenience and there is nothing actually tying the attributes returned from OmniAuth to the `User` model attributes. I've changed the setup so it uses an explicit `Hash` derived from observing the attributes that OmniAuth actually provides. This seems more realistic and I'm hoping it will make it easier to apply an unrelated fix in a subsequent commit. I've also changed the assertions to be more explicit and like those in the specs for `User.from_userinfo` & `User.from_token`.
We need to call `User.temporarily_add_organisations_until_the_profile_app_is_updated` from `User.from_auth` like we do from `User.from_userinfo` & `User.from_token` so that the `organisations` attribute gets populated.
floehopper
force-pushed
the
issues/239-Create_models_and_controllers_for_lessons-rebased-against-main-and-fixed-up
branch
from
5e0939a to
80e3c88
Compare
This fixes a problem that was introduced in this commit [1]. The call to
`skip_load_resource only: :create` was removed which meant that the call
to `load_and_authorize_resource` triggered an attempt to instantiate a
`Project` model using the params identified by CanCanCan. This included
the `image_list` param which did not exist as an attribute on a
`Project` and thus resulted in the following exception:
ActiveModel::UnknownAttributeError: unknown attribute 'image_list' for Project.
I did initially look at fixing the problem while maintaining the ability
to pass the `image_list` into `Project::Create#call`. However, the
functionality in `Project::Create#build_project` which loops through the
`image_list` was not previously tested and I can't really see that it
ever worked. Furthermore, @sra405 has pointed out that we're not
currently allowing users to add their own images to a project for
safeguarding reasons and so I thought it made more sense to fix the
problem by removing support for `image_list` from the create project
endpoint for now until we actually need it. /cc @loiswells97
Removing the `image_list` param from the permitted params in
`Api::ProjectsController#base_params` should mean it is never included
in the params supplied to either CanCanCan's
`load_and_authorize_resource` method or to `Project::Create#call`,
although it will mean that a warning will appear in the logs. So ideally
we'd remove it from the params being supplied by the UI.
Note that the images from the projects in the `raspberrypilearning`
GitHub organisation [2] are added to a project using a different
mechanism and so should not be affected by this change.
[1]: 39f45f7
I've had a go at doing this locally and this is what led me to finding and fixing the problem described in 0148d94. One I was concerned about was that logged-in users with no roles would still be able to do what they used to be able to do and that does indeed appear to be the case. And I've double-checked this by reading the code. You can see from the code snippets below that the only difference is that the code in the branch specifies that the
In editor-api/app/models/ability.rb Lines 8 to 9 in c0e104d In this branch: editor-api/app/models/ability.rb Lines 8 to 10 in 0148d94
In editor-api/app/models/ability.rb Lines 13 to 14 in c0e104d In this branch:
In editor-api/app/models/ability.rb Lines 13 to 14 in c0e104d In this branch: I've also confirmed that after this fix I can login to the admin dashboard with a user that has the "editor-admin" role. So I'm happy to tick this checkbox. |
Given that both @chrislo & I looked at a lot of the commits during our efforts to rebase the branch and all the manual testing I've done. I'm no longer convinced this is worthwhile, so I'm happy to delete this checkbox item. |
I've created an issue in the "Ideas" section of the project: RaspberryPiFoundation/editor-ui#984 I hope that makes sense. /cc @sra405 |
I've discovered that it is possible to associate multiple PRs with an issue, so I don't think this step is necessary. |
The branch is still up-to-date against |
Thanks @floehopper that makes sense to me 👍 thanks for adding the ticket, it's definitely one that needs mulling over |
floehopper
deleted the
issues/239-Create_models_and_controllers_for_lessons-rebased-against-main-and-fixed-up
branch
We created this PR as follows:
main.User.temporarily_add_organisations_until_the_profile_app_is_updated.TODO:
editor-uiapp and/or theeditor-standaloneapp against the code in this branch. In particular, it would be worth testing some operations on a user (e.g. logging in) and operations on projects (e.g. creating & updating).Although I have already reviewed these commits as part of my original review of Create models and controllers for lessons #241, it might be worth someone else reviewing the (much smaller number of) commits in this PR.We might want to replace the originalissues/239-Create_models_and_controllers_for_lessonsbranch with thisissues/239-Create_models_and_controllers_for_lessons-rebased-against-main-and-fixed-upbranch to keep GitHub projects happy before we merge these changes, but I'm not sure it's essential.mainjust before merging, force-push if necessary, and then merge with a merge commit (but not squashing) in order to preserve the original individual commits to make it easier to see what's happened./cc @sra405 @tuzz @chrislo @chrisroos