Move managing tokens for API users to use GOV.UK Design System #2616
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-managing-tokens-for-api-users-to-use-govuk-design-system
branch
2 times, most recently
from
09d803a
to
c455212
Compare
I think this makes the code more readable.
I think this makes the code more readable.
I think this makes them more consistent with other pages and/or more readable.
To make them consistent with the "edit API user" page.
This page was converted to use the GOV.UK Design System before we'd established a pattern for breadcrumb links. This makes the page consistent with other API user related pages.
Previously the actions in this controller were using predicate methods on `ApiUserPolicy` to determine whether a user had permission to carry out the relevant action. This meant that the same `ApiUserPolicy#new?` predicate method was being used for both `ApiUsersController#new` and `AuthorisationsController#new` actions. While it makes sense that the logic is the same (superadmin access only) for both actions, it's not very idiomatic and I found it quite confusing. Additionally I'm planning to add some more actions to this controller which would exacerbate the problem. The authorisations referenced by this controller are actually instances of `Doorkeeper::AccessToken` which is quite confusing. However, the terminology is quite widespread across the codebase and so I've decided to stick with it for now. This meant that I had to use the `policy_class` option in the call to `Pundit::Authorization#authorize` in `AuthorisationsController#authorize_authorisation` so that I could name the policy class `AuthorisationPolicy` vs `Doorkeeper::AccessTokenPolicy`.
None of the methods defined in `UserPermissionsControllerMethods` seem to be used in the `AuthorisationsController` or its only view template, `app/views/authorisations/new.html.erb`, so it should be safe to stop including it. Looking through the git history it's not immediately obvious to me that this module was ever needed, but I might have missed something.
Although the `authorisation` is an instance of `Doorkeeper::AccessToken`, everything else in this controller and its associated view template is named based on "authorisation". While I'm not wild about this terminology, it's so pervasive across the codebase that making this smaller change to make things more consistent seems worthwhile.
This moves the action-related contexts up to the top-level and the contexts related to the type of the current user down to the next level. I think this is easier to reaad and it is more consistent with other controller tests. All the tests remain unchanged except for the one which was sending a GET request to the `revoke` action. I've corrected this so that it now sends a POST request which is specified by the route. I plan to add more tests for the `revoke` action and so I've deleted the comment, because it *is* in fact possible to test non-RESTful actions from a controller test!
Add test coverage for existing behaviour.
To make it more consistent with other similar pages.
To make it consistent with the rest of the test.
…Test This is more idiomatic and consistent with other controller tests.
Add/improve test coverage for existing behaviour.
I want to use this in the controller tests for `AuthorisationsController#revoke` that I'm about to add.
Add test coverage for existing behaviour.
floehopper
force-pushed
the
move-managing-tokens-for-api-users-to-use-govuk-design-system
branch
3 times, most recently
from
081cd9f
to
e4bbffc
Compare
This commit changes the "manage tokens for API user" page to link to a new page where you can revoke a specific token for an API user. This is a small step on the way to changing the "manage tokens for API user" page to use the GOV.UK Design System. There wasn't any design for this page, so I've just made it up. I thought it was useful to include some details about the access token that's about to be revoked and so I've used a `summary_list` component for this purpose. I hope that makes sense!
The extra invisible text should improve accessibility and it makes the test assertion simpler. It should also make the assertion robust against the changes I'm about to make to move this page to use the GOV.UK Design System.
Trello: https://trello.com/c/75Jyg8zR The main change to the page is that the access tokens for the API user are now displayed as "Summary card" components rather than as rows in a table. The "Summary card" component is documented here [1] as wrapper around a `summary_list` component, but it doesn't currently exist in the Rails component library, so I've implemented it using lower-level GOV.UK Design System CSS classes. Similarly, I've had to use lower-level CSS classes for the summary lists inside each summary card, because there's no support for a custom link (i.e. "Revoke") in the Rails component library version of `summary_list`. The "Add application token" button has moved to the top of the page as per the design. I've replaced the "danger" & "success" alerts with a single `success_alert` component [1]. There is no real equivalent to the "danger" alert in the GOV.UK Design System - the `error_alert` component [2] isn't appropriate, because there's not actually an error in this case - it's just trying to draw attention to the fact that the access token code will only be visible temporarily - so using a single `success_alert` made the most sense to me. I think the copy could be improved, but I'm going to say that's out-of-scope for now. In order to display the `success_alert` in the right place, I've introduced a new `custom_alerts` `content_for` call in the `admin_layout`. I've replaced the functionality provided by `ZeroClipboard` (and the `zeroclipboard-rails` gem) in this alert with the `copy_to_clipboard` component [3]. Note that the `ZeroClipboard` behaviour wasn't working for me (at least in Firefox), but the new component works fine. Lastly I've had to tweak a few of the assertions in existing tests to account for the changes in the markup. [1]: https://design-system.service.gov.uk/components/summary-list/#summary-cards [2]: https://components.publishing.service.gov.uk/component-guide/success_alert [3]: https://components.publishing.service.gov.uk/component-guide/error_alert [4]: https://components.publishing.service.gov.uk/component-guide/copy_to_clipboard
And associated JavaScript config. This was originally added in this PR [1] and I can't see any other remnants. This seems like a good thing, because the functionality it was supposed to provide wasn't working for me (at least in Firefox) and the GitHub repo [2] has been archived since 2021! We're now using the `copy_to_clipboard` component [3] instead. [1]: #248 [2]: https://github.com/zeroclipboard/zeroclipboard-rails [3]: https://components.publishing.service.gov.uk/component-guide/copy_to_clipboard
floehopper
force-pushed
the
move-managing-tokens-for-api-users-to-use-govuk-design-system
branch
from
e4bbffc
to
179e6db
Compare
chrisroos
approved these changes
Jan 4, 2024
floehopper
deleted the
move-managing-tokens-for-api-users-to-use-govuk-design-system
branch
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/75Jyg8zR
The majority of the commits in this branch are refactorings or other improvements intended to make the actual changes easier. The two key commits where the actual changes are made are:
I've also made a few tweaks to other pages relating to API users in order to make all the pages more consistent with each other and with other pages across the site.
I considered displaying each token's expired/valid status on the manage tokens page using the
tagcomponent, but since it wasn't in the Figma design, I decided it was out-of-scope for the moment.I think the copy in the success alerts isn't great, but I've left it unchanged for now. I'm keen that we do a review/audit of all flash messages across the site once we've moved the whole app to use the GOV.UK Design System.
I considered renaming the
AuthorisationsControllerto be more about access tokens, but the most obvious name would beDoorkeeper::AccessTokensController(to match the model class), but this might imply that the controller is somehow provided by Doorkeeper and in any case, I think Doorkeeper might already provide such a controller!The Figma design never envisaged the "manage permission" and "manage tokens" pages being split out. We split them to make it easier to make the changes incrementally. Once the work on the "manage permissions" page is complete, we might want to consider combining them back into the "edit API user" page as per the Figma design.
Manage tokens for API user page
Create new access token page
Manage tokens for API user page with new token details
Revoke token for API user page
Manage tokens for API user page with revocation success message