Migrate batch user creation to design system #2361
Merged
Conversation
This file contains 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
chrislo
force-pushed
the
migrate-batch-user-creation-to-design-system
branch
5 times, most recently
from
2f96bb0
to
c5c809d
Compare
mike29736
approved these changes
Sep 20, 2023
This method only returns true if the user is a `govuk_admin`, which is the same logic as the `new?` method in this policy.
The `BatchInvitationPolicy#assign_organisation_from_csv?` method is an alias for `BatchInvitationPolicy#new` which means that only people who can view this page can assign organisations from the CSV file. So the second branch (where `assign_organisation_from_csv` is `false`) is not reachable in this view template and can therefore be removed.
`BatchInvitationPolicy#assign_organisation_from_csv?` is an alias for `BatchInvitationPolicy#create` so this method always returns `true` in this controller action. We can therefore remove the conditional and inline it into the argument hash.
With the previous commits this method is no longer used so can be removed.
The new (design-system) designs for this page don't include the table of recent batches, so in preparation for migrating this page to the design system, I'm removing them. With the removal of the table the `batch_invite_status_link` helper method is no longer used so can also be removed.
This commit switches the first "page" of the batch creation of users workflow to the design system. All of the actions in BatchInvitationsController now use the `admin_layout` so I've removed the `only` array rather than add `new` to it. The `form_for` helper needs `multipart: true` for file uploads to work. This confused me for a while - I think rails default form builder knows to set `multipart: true` implicitly if you use the `file_field` helper, but as we've removed that in order to use the `file_upload` component we now have to set it explicitly. I've inlined the relevant `policy_scope` line from `UserHelpers#organisation_options` to build the list of `Organisations` in the select. This helper method is still used elsewhere though, so I can't remove it yet.
I want to inline the shared template, doing this will make that change more obvious.
We want to make fairly extensive changes to how the permissions are selected for this page of the batch user creation process and convert it to the design system. Inlining the shared form will let me make some changes to the logic first and then hopefully make it easier to see the design-system changes that follow.
`user_object` in this view is always `User.new` and `User.new.api_user?` is `false`, so the predicate conditions on this method can be simplified.
`user_object` in this view is always `User.new` and `User.new.persisted?` is `false`, so the predicate conditions on this method can be removed.
We don't think it makes sense for users to be granted permissions to applications that have been retired, certainly in this scenario of bulk creating users. So rather than stricking-through the application name, simply skip it altogether from the table.
This block variable is not actually used inside the block, so I'm replacing it with a dummy variable to avoid confusion.
The second block variable yielded by this helper method is not used, instead we only need the applications. This list of applications was previously obtained from `UserPermissionsControllerMethods#applications_and_permissions` which in turn called `UserPermissionsControllerMethods#visible_applications` with the `user_object` which in this view is always `User.new`. Because `User.new.api_user? == false` the implementation of `UserPermissionsControllerMethods#visible_applications` fell through to return `policy_scope(:user_permission_manageable_application)`. So we can inline this in the view instead of the more convoluted chain of methods.
The `user_object` in this template is always `User.new` and `User.new.has_access_to?(application)` always returns `false` since `User.new` has no associated `application_permissions`. So we can replace this method call with the literal `false` indicating that this checkbox is not selected.
In this `user_object` is always `User.new` and `User.new.permission_ids_for(application)` will always return `[]` since a new User does not have any associated `application_permissions`. This means that no applications need to be pre-selected in this call to `options_for_select` and we can rely on the default value for the second argument (which is `nil`).
This variable is no longer used in this template, so there's no need to assign it.
In the new designs for this page we want to have a list of checkboxes rather than the current `chosen`-powered select box. As a step towards that this commit replaces the select box with checkboxes. I haven't made any attempt to style these new checkboxes since they will shortly be replaced by design-system ones. With this commit it is less obvious that we need to have a distinction between the sign-on permission and the others, but I'm going to live with the rather ugly chained `reject` method here and tidy that up next.
In the new designs we intend to present these checkboxes together in a single list, so as a step towards that this commit changes the existing design to group all of the checkboxes together.
I want to control the sort order of the permissions that appear in the list of permissions so I'm first extracting a helper method and writing a test to make that change easier.
The most commonly assigned permission is the signin permission so we want to have that at the top of the list of selectable permissions. The others are sorted alphabetically to make them easier to find.
This commit migrates the BatchInvitationPermissionsController#new page to the design system. Hopefully with the preparatory commits this diff isn't too hard to follow. I've had to use the "raw" HTML version of the accordian component[1] rather than the one in the publishing component library[2] since the latter doesn't support passing a block of arbitrary content into each section. I think it's a small enough amount of HTML to take this route rather than try to extend the component. The breadcrumb trail could be improved - ideally it would link to the previous "step" in this flow which would involve editing the CSV file and organisation - but we don't have an edit route for this yet and I'm concerned already about the size of this change. I think it's one we could revisit in a subsequent enhancement. [1] https://design-system.service.gov.uk/components/accordion/ [2] https://components.publishing.service.gov.uk/component-guide/accordion
|
Rebasing/autosquashing on main |
chrislo
force-pushed
the
migrate-batch-user-creation-to-design-system
branch
from
600f2f1
to
539a70b
Compare
|
Thanks for the review @mike29736! |
floehopper
added a commit
that referenced
this pull request
Oct 11, 2023
Trello: https://trello.com/c/SfIc6Q2q The main motivation for these changes is the Licensing app which has a large number of permissions. The recent changes [1,2] which moved the UI for granting permissions to the GOV.UK Design System have made managing the permissions for the Licensing unmanageable. This replaces the accordion component [3] where each item of the accordion was a checkboxes component [4] (i.e. a set of checkboxes) with a list of option-select components [5] (each of which has a set of checkboxes within it). We've made this change to *both* the new invitation ("Create new user") page and the new batch invitation permissions page. The option-select component gives us something similar to the show/hide toggle which was previously provided by the accordion which is why it no longer makes sense to use the accordion. Also it makes the permissions section more compact which is no bad thing given how long it is! Most importantly the option-select component gives us the option to enable a search filter for the permissions for a given app. I plan to make use of this in a subsequent commit. This will address the primary motivation which is the unmanagability of the Licensing app permissions. Note that I've set the `key` attribute of the option-select components to "not-used" for now, because it's a required attribute but the name of the checkboxes is being derived from each individual option hash. I plan to address this in a subsequent commit. The use of the option-select component isn't really what it was intended for in that it doesn't *control* a list of things. However, I don't think it's too far away and it gives us a much better UI. [1]: #2361 [2]: #2412 [3]: https://components.publishing.service.gov.uk/component-guide/accordion [4]: https://components.publishing.service.gov.uk/component-guide/checkboxes [5]: https://components.publishing.service.gov.uk/component-guide/option_select
floehopper
added a commit
that referenced
this pull request
Oct 11, 2023
Trello: https://trello.com/c/SfIc6Q2q The main motivation for these changes is the Licensing app which has a large number of permissions. The recent changes [1,2] which moved the UI for granting permissions to the GOV.UK Design System have made managing the permissions for the Licensing unmanageable. This replaces the accordion component [3] where each item of the accordion was a checkboxes component [4] (i.e. a set of checkboxes) with a list of option-select components [5] (each of which has a set of checkboxes within it). We've made this change to *both* the new invitation ("Create new user") page and the new batch invitation permissions page. The option-select component gives us something similar to the show/hide toggle which was previously provided by the accordion which is why it no longer makes sense to use the accordion. Also it makes the permissions section more compact which is no bad thing given how long it is! Most importantly the option-select component gives us the option to enable a search filter for the permissions for a given app. I plan to make use of this in a subsequent commit. This will address the primary motivation which is the unmanagability of the Licensing app permissions. Note that I've set the `key` attribute of the option-select components to "not-used" for now, because it's a required attribute but the name of the checkboxes is being derived from each individual option hash. I plan to address this in a subsequent commit. The use of the option-select component isn't really what it was intended for in that it doesn't *control* a list of things. However, I don't think it's too far away and it gives us a much better UI. [1]: #2361 [2]: #2412 [3]: https://components.publishing.service.gov.uk/component-guide/accordion [4]: https://components.publishing.service.gov.uk/component-guide/checkboxes [5]: https://components.publishing.service.gov.uk/component-guide/option_select
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.
Trello: https://trello.com/c/fShAozUE
This PR migrates the batch user upload/invite pages to the Design system. The two main commits in this branch are
but in order to make those two commits as small as I could there's several commits of tidying up and refactoring before each one.
A small change of behaviour introduced in this PR (other than the UX changes) is that we now exclude retired applications from the permission-selection list. We had a discussion about this last week, and it doesn't seem to make sense to give users permissions to retired applications, so this change makes the choice of applications a little easier.
There's a few things that I think could be improved and that I haven't tacked yet from our Figma concepts.
I feel like it is worth landing this as it currently is and then having a round of enhancements/improvements. This is already quite a large change and it introduces some value in its current state.
Before
After