Preselect default supported permissions when creating new users #2442
Conversation
chrislo
force-pushed
the
preselect-default-supported-permissions-when-creating-new-users
branch
2 times, most recently
from
8024736 to
683ce89
Compare
There was a problem hiding this comment.
This all looks good apart from the implementation of User#grant_permission which I think should work consistently irrespective of whether the user is persisted.
I've suggested another couple of changes but I'll leave it to you to decide whether to make those.
| application_permissions.where(supported_permission_id: supported_permission.id).first_or_create! | ||
| else | ||
| supported_permissions << supported_permission | ||
| end |
There was a problem hiding this comment.
I think there's a potential problem here because it's possible to add the same supported permission multiple times to an unsaved user. Saving that user will then fail with ActiveRecord::RecordInvalid: Validation failed: Supported permission has already been taken. I wonder if it's better to use something like:
supported_permissions << supported_permission unless supported_permissions.include?(supported_permission)
Which works in both the persisted and unpersisted case.
There was a problem hiding this comment.
Thanks @chrisroos - I think I've addressed this in 513e5a0. I wasn't able to simplify the implementation to just the single line you suggest as another method relies on the return value of this method in the persisted case. But hopefully this is ok?
| @@ -122,6 +122,16 @@ def self.with_2sv_statuses(scope_names) | |||
| relation | |||
| end | |||
|
|
|||
| def self.with_default_permissions | |||
There was a problem hiding this comment.
I think this method can be simplified as new(supported_permissions: SupportedPermission.default).
| user.grant_permission(supported_permission) | ||
|
|
||
| assert user.has_permission?(supported_permission) | ||
| end |
There was a problem hiding this comment.
I think it might be worth adding another test here, to exercise the branch of code where the user already has the supported permission (i.e. the first bit of first_or_create).
There was a problem hiding this comment.
I expanded the test in 62b4a86 to test this branch.
chrislo
force-pushed
the
preselect-default-supported-permissions-when-creating-new-users
branch
from
683ce89 to
cbf2077
Compare
|
Thanks @chrisroos - I'll rebase this and get it merged. |
This method was previously untested and I'd like to change its behaviour so I'm adding a test first.
We want to change the user invitation page so that some permissions are "preselected". Since the User is not persisted before they are invited we'll need to be able to add permissions to non-persisted users.
This commit ensures that the default permissions defined by the SupportedPermission.default scope are checked when invititing a new user. These permissions are currently automatically added when inviting a new user but that mechanism is not very clear. By preselecting them we give the inviting user the option to configure them or add new ones via the application permissions UI. In a subsequent commit I'll remove the code that adds these automatically so everything is consistent. I've inlined the implementation of Devise::InvitationsController#new into InvitationsController#new so that I can set `self.resource` with `User.with_preselected_permissions` instead of `User.new`.
In the previous commit we exposed the default permissions via the user invitation form's checkboxes. To allow the inviting user to deselect these default permissions this commit stops granting them automatically on user create.
This commit ensures that the default permissions defined by the SupportedPermission.default scope are checked when adding a batch of new users. These permissions are currently automatically added when inviting batches of users but that mechanism is not very clear. By preselecting them we give the inviting user the option to configure them or add new ones via the application permissions UI. In a subsequent commit I'll remove the code that adds these automatically so everything is consistent.
In the previous commit we exposed the default permissions via the batch invitation form's checkboxes. To allow the inviting user to deselect these default permissions this commit stops granting them automatically.
chrislo
force-pushed
the
preselect-default-supported-permissions-when-creating-new-users
branch
from
cbf2077 to
e330339
Compare
chrislo
deleted the
preselect-default-supported-permissions-when-creating-new-users
branch
I noticed the existence of this rake task while working on #2442. As far as I can tell from the history (6333730, 3432e0c) this task was added when there was no other mechanism for adding users. We now have the /users/invitation/new and /batch_invitations/new pages to create individual and batches of users respectively. I've asked in the `#govuk-2ndline-tech` slack channel and no-one can remember using this task recently to add users. I think it's safe to remove it, the UserCreator class that contains its logic, the related tests and documentation.
Trello: https://trello.com/c/5tHKKbWq
This PR ensures that any SupportedPermission with
defaultset totruewill be preselected in the user invitation and batch user upload permissions pages. With this change we no longer grant these permissions by default automatically, allowing the inviting user to deselect some of them if they are not appropriate for the specific user.I haven't included a migration to make the permissions listed in the trello card default. I figured we can easily do this through the UI once this change has landed (and double-check that we want the same permissions by default on both integration and production).
User Invitation
Before
After
Batch Invitation Permissions
Before
After