Move invitations new to use GOV.UK Design System #2412
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
floehopper
force-pushed
the
move-invitations-new-to-use-govuk-design-system
branch
2 times, most recently
from
fc2059f
to
d353421
Compare
This fixes a bug where any non-signin permissions that were selected before the form was submitted were being "lost" when a validation error occurred and the form was re-rendered. The problem was that `User#permission_ids_for` only worked for users that had already been persisted. I've fixed it by changing the implementation to use the `User#application_permissions` in memory. Although `User#permission_ids_for` is used on a number of pages via the `app/views/shared/_user_permissions.html.erb` partial, I'm pretty sure all the other places related to persisted users. So I only added a failing test in InvitationsControllerTest which forced me to fix the problem. Note that signin permissions were not affected, because they are rendered in a checkbox separate from the select. However, I've added a test for this for completeness. Interestingly the reason this wasn't broken was that `User#has_access_to?` works OK even if the user is not persisted, because it operates on `User#application_permissions` in a similar way to my new implementation of `User#permission_ids_for`. It's possible that this change will lead to page that use the `app/views/shared/_user_permissions.html.erb` partial being a bit slower to load. However, it doesn't seem to be a significant problem when I run the app locally. If it does turn out to be a problem, we could add an if condition in `User#permission_ids_for` which checks whether the user is persisted or not. I've also taken the opportunity to add some test coverage for retaining other form field values when a validation error occurs.
I'm about to make some changes in this area and I thought it would be useful to capture this behaviour in tests.
Trello: https://trello.com/c/umBDQZUc I've re-used the accordion markup for setting permissions that @chrislo implemented in `app/views/batch_invitation_permissions/new.html.erb` in this commit [1] pretty much verbatim, although I've had to add a `checked` key/value pair to make sure any permissions that are set before submitting the form are not lost of there is a validation error. I've also had to change `User#has_permission?` so that it works when the user has not been persisted in order to support the validation error scenario mentioned above. Obviously all the above has introduced some duplication, but I plan to extract that in a subsequent commit. A few of the assertions for various form elements in `InvitationsControllerTest` have had to change slightly now we're using checkbox inputs rather than select elements. [1]: 539a70b
Trello: https://trello.com/c/umBDQZUc In this commit [1], @chrislo said: > I've had to use the "raw" HTML version of the accordian component[2] rather than the one in the publishing component library[3] since the latter doesn't support passing a block of arbitrary content into each section. However, it seems to work if you pass HTML as `content.html` attribute for an accordion item - I hope I'm not missing something! So I've used this approach in both `app/views/devise/invitations/new.html.erb` & `app/views/batch_invitation_permissions/new.html.erb`. I've also extracted `UsersHelper#items_for_permission_checkboxes` and used a `user` parameter with a default value to handle the slight difference between the behaviour needed in the two pages. Although there's still a little duplication between the two pages, I think the use of the library accordion component and the shared helper method has probably removed enough for now. I think extracting any more would make the template harder to understand. It's not ideal that I've had to include `PermissionsHelper` & `BatchInvitationPermissionsHelper` into `UsersHelperTest` - I think it's a sign that the relevant methods are not defined in the right places. I plan to address that in a subsequent commit. [1]: 539a70b [2] https://design-system.service.gov.uk/components/accordion/ [3] https://components.publishing.service.gov.uk/component-guide/accordion
The `PermissionsHelper#permissions_for` method was effectively duplicating the `Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui` method. Although you could argue that the sorting is a presentational concern, I think the filtering is definitely a model concern. And so on balance I think this behaviour is better implemented on the model. `Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui` didn't have any unit tests, so I've adapted the ones for `PermissionsHelper#permissions_for`.
The `BatchInvitationPermissionsHelper#formatted_permission_name` method is only used in `UsersHelper#items_for_permission_checkboxes` and so it makes more sense to define it in `UsersHelper` closer to where it's being used. We _could_ also make it private, but I decided it was worth keeping the tests around it and so left it as public.
floehopper
force-pushed
the
move-invitations-new-to-use-govuk-design-system
branch
from
d353421
to
3651d55
Compare
chrislo
approved these changes
Oct 9, 2023
floehopper
changed the title
Trello: https://trello.com/c/umBDQZUc I've belatedly noticed that I lost an h2 element for the permissions checkboxes in a previous commit. This wraps the permissions accordion in a fieldset with a legend which seems more semantically appropriate given the application names in the accordion are h2's.
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/umBDQZUc
This moves the "Create new user" page to use the GOV.UK Design System.
For the moment, we've agreed to keep it as one page (rather than splitting the permission assignment into a 2nd page) and we haven't included the filtering of the applications in the permissions assignment section.
I've managed to use the accordion component from the Design System gem for both this page and I've retrofitted it to the batch invitation permissions page to reduce duplication. However, it's worth noting that this component is marked as "experimental"!
I've also taken the opportunity to fix a bug where a number of the values in form fields (e.g. permissions checkboxes) were not being retained when a validation error occurred.
Screenshots (truncated)