Skip to content
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 bug fix: Kubernetes Role filter replace with explicit input filter #27178

Merged
merged 15 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/27178.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:change
ui/kubernetes: Update the roles filter-input to use explicit search.
```
26 changes: 26 additions & 0 deletions ui/lib/core/addon/components/filter-input-explicit.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
~}}

<div>
<Hds::SegmentedGroup as |S|>
<S.TextInput
@value={{@query}}
placeholder={{@placeholder}}
aria-label="Search by path"
size="32"
{{on "input" @handleInput}}
{{on "keydown" @handleKeyDown}}
data-test-filter-input-explicit
/>
<S.Button
@color="secondary"
@text="Search"
@icon="search"
type="submit"
{{on "click" @handleSearch}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we aren't wrapping these in a <form> element so that clicking this button submits (and then we also get a free submit when the user presses the "Enter" button)? I think that would be a nice ergonomic update to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests 😵‍💫 I originally had it wrapped as a form, but couldn't call the action correctly from the tests. There was an error about it being in a form element. I wish I would have saved that specific error. Let me see if I can resurface it. The enter functionality would be very nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashishaw This was the error, any thoughts? Only on the test, not on the UI.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, we just need to do evt.preventDefault in the triggered action so that it doesn't "send" the form data to the URL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Found an example in how we do it for sinon spies in toggle-test (thank you—you did that block of code).

data-test-filter-input-explicit-search
/>
</Hds::SegmentedGroup>
</div>
6 changes: 6 additions & 0 deletions ui/lib/core/app/components/filter-input-explicit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

export { default } from 'core/components/filter-input-explicit';
5 changes: 4 additions & 1 deletion ui/lib/kubernetes/addon/components/page/roles.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@
<TabPageHeader
@model={{@backend}}
@filterRoles={{not @promptConfig}}
@rolesFilterValue={{@filterValue}}
@query={{this.query}}
@breadcrumbs={{@breadcrumbs}}
@handleSearch={{this.handleSearch}}
@handleInput={{this.handleInput}}
@handleKeyDown={{this.handleKeyDown}}
>
{{#unless @promptConfig}}
<ToolbarLink @route="roles.create" @type="add" data-test-toolbar-roles-action>
Expand Down
38 changes: 36 additions & 2 deletions ui/lib/kubernetes/addon/components/page/roles.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,58 @@ import { action } from '@ember/object';
import { getOwner } from '@ember/application';
import errorMessage from 'vault/utils/error-message';
import { tracked } from '@glimmer/tracking';
import keys from 'core/utils/key-codes';

/**
* @module Roles
* RolesPage component is a child component to show list of roles
* RolesPage component is a child component to show list of roles.
* It also handles the filtering actions of roles.
*
* @param {array} roles - array of roles
* @param {boolean} promptConfig - whether or not to display config cta
* @param {array} pageFilter - array of filtered roles
* @param {string} filterValue - value of queryParam pageFilter
* @param {array} breadcrumbs - breadcrumbs as an array of objects that contain label and route
*/
export default class RolesPageComponent extends Component {
@service flashMessages;
@service router;
@tracked query;
@tracked roleToDelete = null;

constructor() {
super(...arguments);
this.query = this.args.filterValue;
hashishaw marked this conversation as resolved.
Show resolved Hide resolved
}

get mountPoint() {
return getOwner(this).mountPoint;
}

navigate(pageFilter) {
const route = `${this.mountPoint}.roles.index`;
const args = [route, { queryParams: { pageFilter: pageFilter || null } }];
this.router.transitionTo(...args);
}

@action
handleKeyDown(event) {
if (event.keyCode === keys.ESC) {
// On escape, transition to roles index route.
this.navigate();
}
// ignore all other key events
Comment on lines +48 to +51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is slick! nice job including keyboard navigation here. 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This originally comes from Chelsea's refactor of an old filter input for Kv (see KvListFilter). All the credit goes to her :)

}

@action handleInput(evt) {
this.query = evt.target.value;
}

@action
handleSearch(evt) {
evt.preventDefault();
this.navigate(this.query);
}

@action
async onDelete(model) {
try {
Expand Down
8 changes: 5 additions & 3 deletions ui/lib/kubernetes/addon/components/tab-page-header.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@
<Toolbar aria-label="items for managing kubernetes items">
{{#if @filterRoles}}
<ToolbarFilters>
<NavigateInput
@filter={{@rolesFilterValue}}
<FilterInputExplicit
@query={{@query}}
@placeholder="Filter roles"
@urls={{hash list="vault.cluster.secrets.backend.kubernetes.roles"}}
@handleSearch={{@handleSearch}}
@handleInput={{@handleInput}}
@handleKeyDown={{@handleKeyDown}}
/>
</ToolbarFilters>
{{/if}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import kubernetesScenario from 'vault/mirage/scenarios/kubernetes';
import kubernetesHandlers from 'vault/mirage/handlers/kubernetes';
import authPage from 'vault/tests/pages/auth';
import { fillIn, visit, currentURL, click, currentRouteName } from '@ember/test-helpers';
import { GENERAL } from 'vault/tests/helpers/general-selectors';

module('Acceptance | kubernetes | roles', function (hooks) {
setupApplicationTest(hooks);
Expand All @@ -30,7 +31,8 @@ module('Acceptance | kubernetes | roles', function (hooks) {
test('it should filter roles', async function (assert) {
await this.visitRoles();
assert.dom('[data-test-list-item-link]').exists({ count: 3 }, 'Roles list renders');
await fillIn('[data-test-component="navigate-input"]', '1');
await fillIn(GENERAL.filterInputExplicit, '1');
await click(GENERAL.filterInputExplicitSearch);
assert.dom('[data-test-list-item-link]').exists({ count: 1 }, 'Filtered roles list renders');
assert.ok(currentURL().includes('pageFilter=1'), 'pageFilter query param value is set');
});
Expand Down
2 changes: 2 additions & 0 deletions ui/tests/helpers/general-selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ export const GENERAL = {

filter: (name: string) => `[data-test-filter="${name}"]`,
filterInput: '[data-test-filter-input]',
filterInputExplicit: '[data-test-filter-input-explicit]',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

filterInputExplicitSearch: '[data-test-filter-input-explicit-search]',
confirmModalInput: '[data-test-confirmation-modal-input]',
confirmButton: '[data-test-confirm-button]',
confirmTrigger: '[data-test-confirm-action-trigger]',
Expand Down
57 changes: 57 additions & 0 deletions ui/tests/integration/components/filter-input-explicit-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render, typeIn, click } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
import { GENERAL } from 'vault/tests/helpers/general-selectors';
import sinon from 'sinon';

module('Integration | Component | filter-input-explicit', function (hooks) {
setupRenderingTest(hooks);

hooks.beforeEach(function () {
this.handleSearch = sinon.spy();
this.handleInput = sinon.spy();
this.handleKeyDown = sinon.spy();
this.query = '';
this.placeholder = 'Filter roles';

this.renderComponent = () => {
return render(
hbs`<FilterInputExplicit aria-label="test-component" @placeholder={{this.placeholder}} @query={{this.query}} @handleSearch={{this.handleSearch}} @handleInput={{this.handleInput}} @handleKeyDown={{this.handleKeyDown}} />`
);
};
});

test('it renders', async function (assert) {
this.query = 'foo';
await this.renderComponent();

assert
.dom(GENERAL.filterInputExplicit)
.hasAttribute('placeholder', 'Filter roles', 'Placeholder passed to input element');
assert.dom(GENERAL.filterInputExplicit).hasValue('foo', 'Value passed to input element');
});

test('it should call handleSearch on submit', async function (assert) {
this.handleSearch.calledOnce;

await this.renderComponent();
await typeIn(GENERAL.filterInputExplicit, 'bar');
await click(GENERAL.filterInputExplicitSearch);
});

test('it should send keydown event on keydown', async function (assert) {
assert.true(this.handleKeydown.calledTwice);

await this.renderComponent();
await typeIn(GENERAL.filterInputExplicit, 'a');
await typeIn(GENERAL.filterInputExplicit, 'b');

assert.true(this.handleSearch.notCalled);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { setupMirage } from 'ember-cli-mirage/test-support';
import { render, click } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
import { allowAllCapabilitiesStub } from 'vault/tests/helpers/stubs';
import { GENERAL } from 'vault/tests/helpers/general-selectors';

module('Integration | Component | kubernetes | Page::Roles', function (hooks) {
setupRenderingTest(hooks);
Expand Down Expand Up @@ -58,7 +59,7 @@ module('Integration | Component | kubernetes | Page::Roles', function (hooks) {
.dom('[data-test-toolbar-roles-action]')
.doesNotExist('Create role', 'Toolbar action does not render when not configured');
assert
.dom('[data-test-nav-input]')
.dom(GENERAL.filterInputExplicit)
.doesNotExist('Roles filter input does not render when not configured');
assert.dom('[data-test-config-cta]').exists('Config cta renders');
});
Expand All @@ -70,7 +71,7 @@ module('Integration | Component | kubernetes | Page::Roles', function (hooks) {
assert
.dom('[data-test-toolbar-roles-action] svg')
.hasClass('flight-icon-plus', 'Toolbar action has correct icon');
assert.dom('[data-test-nav-input]').exists('Roles filter input renders');
assert.dom(GENERAL.filterInputExplicit).exists('Roles filter input renders');
assert.dom('[data-test-empty-state-title]').hasText('No roles yet', 'Title renders');
assert
.dom('[data-test-empty-state-message]')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { setupEngine } from 'ember-engines/test-support';
import { setupMirage } from 'ember-cli-mirage/test-support';
import { render } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
import { GENERAL } from 'vault/tests/helpers/general-selectors';
import sinon from 'sinon';

module('Integration | Component | kubernetes | TabPageHeader', function (hooks) {
setupRenderingTest(hooks);
Expand All @@ -28,12 +30,18 @@ module('Integration | Component | kubernetes | TabPageHeader', function (hooks)
this.model = this.store.peekRecord('secret-engine', 'kubernetes-test');
this.mount = this.model.path.slice(0, -1);
this.breadcrumbs = [{ label: 'Secrets', route: 'secrets', linkExternal: true }, { label: this.mount }];
this.handleSearch = sinon.spy();
this.handleInput = sinon.spy();
this.handleKeyDown = sinon.spy();
});

test('it should render breadcrumbs', async function (assert) {
await render(hbs`<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}} />`, {
owner: this.engine,
});
await render(
hbs`<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}} @handleSearch={{this.handleSearch}} @handleInput={{this.handleInput}} @handleKeyDown={{this.handleKeyDown}} />`,
{
owner: this.engine,
}
);
assert.dom('[data-test-breadcrumbs] li:nth-child(1) a').hasText('Secrets', 'Secrets breadcrumb renders');

assert
Expand All @@ -42,36 +50,42 @@ module('Integration | Component | kubernetes | TabPageHeader', function (hooks)
});

test('it should render title', async function (assert) {
await render(hbs`<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}} />`, {
owner: this.engine,
});
await render(
hbs`<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}} @handleSearch={{this.handleSearch}} @handleInput={{this.handleInput}} @handleKeyDown={{this.handleKeyDown}} />`,
{
owner: this.engine,
}
);
assert
.dom('[data-test-header-title] svg')
.hasClass('flight-icon-kubernetes-color', 'Correct icon renders in title');
assert.dom('[data-test-header-title]').hasText(this.mount, 'Mount path renders in title');
});

test('it should render tabs', async function (assert) {
await render(hbs`<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}} />`, {
owner: this.engine,
});
await render(
hbs`<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}} @handleSearch={{this.handleSearch}} @handleInput={{this.handleInput}} @handleKeyDown={{this.handleKeyDown}}/>`,
{
owner: this.engine,
}
);
assert.dom('[data-test-tab="overview"]').hasText('Overview', 'Overview tab renders');
assert.dom('[data-test-tab="roles"]').hasText('Roles', 'Roles tab renders');
assert.dom('[data-test-tab="config"]').hasText('Configuration', 'Configuration tab renders');
});

test('it should render filter for roles', async function (assert) {
await render(
hbs`<TabPageHeader @model={{this.model}} @filterRoles={{true}} @rolesFilterValue="test" @breadcrumbs={{this.breadcrumbs}} />`,
hbs`<TabPageHeader @model={{this.model}} @filterRoles={{true}} @query="test" @breadcrumbs={{this.breadcrumbs}} @handleSearch={{this.handleSearch}} @handleInput={{this.handleInput}} @handleKeyDown={{this.handleKeyDown}} />`,
{ owner: this.engine }
);
assert.dom('[data-test-nav-input] input').hasValue('test', 'Filter renders with provided value');
assert.dom(GENERAL.filterInputExplicit).hasValue('test', 'Filter renders with provided value');
});

test('it should yield block for toolbar actions', async function (assert) {
await render(
hbs`
<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}}>
<TabPageHeader @model={{this.model}} @breadcrumbs={{this.breadcrumbs}} @handleSearch={{this.handleSearch}} @handleInput={{this.handleInput}} @handleKeyDown={{this.handleKeyDown}}>
<span data-test-yield>It yields!</span>
</TabPageHeader>
`,
Expand Down
Loading