-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI: add pagination to new PKI #23193
Conversation
@@ -3,68 +3,114 @@ | |||
SPDX-License-Identifier: BUSL-1.1 | |||
~}} | |||
|
|||
{{#each @issuers as |pkiIssuer idx|}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this inside the <:list> section, otherwise is exactly the same
Build Results: |
CI Results: |
</:list> | ||
<:empty> | ||
<EmptyState @title="PKI not configured" @message={{this.notConfiguredMessage}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the empty state into here from the route template, so I needed to add a backing class for notConfiguredMessage
notConfiguredMessage = PKI_DEFAULT_EMPTY_STATE_MSG; | ||
|
||
// To prevent production build bug of passing D.actions to on "click": https://github.com/hashicorp/vault/pull/16983 | ||
@action onLinkClick(D: BasicDropdown) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from controller
|
||
{{#if @keyModels.length}} | ||
{{#each @keyModels as |pkiKey|}} | ||
<LinkedBlock class="list-item-row" @params={{array "keys.key.details" pkiKey.keyId}} @linkPrefix={{@mountPoint}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copypasta'd into <:list>
backend: this.secretMountPath.currentPath, | ||
responsePath: 'data.keys', | ||
page, | ||
skipCache: page === 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows the dataset to be refreshed whenever the list page is visited from a different route, or the user goes back to page 1
@@ -231,18 +231,21 @@ module('Acceptance | landing page dashboard', function (hooks) { | |||
}); | |||
|
|||
test('shows the correct actions and links associated with pki', async function (assert) { | |||
await mountSecrets.enable('pki', 'pki'); | |||
const backend = 'pki-dashboard'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was getting errors about the issuer already existing, so I made this backend name a bit more unique
@@ -379,7 +379,7 @@ module('Acceptance | pki workflow', function (hooks) { | |||
await visit(`/vault/secrets/${this.mountPath}/pki/overview`); | |||
await click(SELECTORS.issuersTab); | |||
assert.dom('[data-test-serial-number="0"]').exists({ count: 1 }, 'displays serial number tag'); | |||
assert.dom('[data-test-common-name="0"]').exists({ count: 1 }, 'displays cert common name tag'); | |||
assert.dom('[data-test-common-name="0"]').doesNotExist('does not display cert common name tag'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this locally with the given policy in both the current UI and in the shipped binary, and each time the issuer read failed and I wasn't able to see the common name. We learned with the kv workflow tests that the store doesn't always clear between logout/login so maybe this was working due to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I pulled down the branch and the empty states are showing. I'm seeing the pagination controls when there are items to display. I also created and deleted some keys to test the caching and they are added/removed from the list as expected!
One thing I was curious about was what might happen if I delete an item from page 2, wondering if it would return to the same page and then the cache would not be cleared but it goes back to page 1 so no troubles there. I think in the future, from a user perspective they should be taken back to the same page that they came from and caching could be disabled all together so the query is always made to ensure the UI stays in sync with the backend.
Great work 👏 🎉
ui/app/services/store.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully in the not so distance future we can sunset this but until we get pagination from the API this is a nice improvement to help deal with stale cache issues! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing here looks that specific to PKI but I think it's fine to have this be a scoped component for now until we can look at more usage examples and find all the commonalities. I think there would be an opportunity to have a block for the page header and breadcrumbs as well if it becomes a component in the core addon.
* UI: add pagination to new PKI (#23193) * fixes store type import * fixes tests --------- Co-authored-by: Jordan Reimer <[email protected]>
This PR adds pagination for PKI items which we missed as part of the PKI UI update.
To keep things relatively simple and consistent, I added a
PkiListPagination
component which standardizes the sections, and makes it very simple to add filtering in the future if it's needed. (At this point in time, the team has opted against, since users can go directly to items from the overview page). The component is PKI-specific so that we can make some assumptions about routing and not need to overload the component with parameters, but I can see a future where a version of this component is used in other list views to aid in general consistency across the list views.One other thing I added was a
skipCache
query option tolazyPaginatedQuery
, which will automatically clear the cache for the given model when querying, and fetch the data rather than using the cache. This was useful for PKI, where various form have non-standard side effects that are hard to predict.