[App Search] Credentials: implement working flyout form #81541

cee-chen · 2020-10-22T21:59:03Z

PR sequence

This PR is part of a set of 3 PRs that implements the Credentials flyout form + functionality. It's broken up into PRs in the 400-800 line range so as not to open a 1700~ line PR.

List of all PRs (may be helpful for obtaining more context / QAing the entire final feature)

PR 1 - FlashMessages & flyout stub
PR 2 - Logic & server routes
PR 3 - All working credential form views (you are here)

Summary

This PR achieves:

Adding all Credentials form fields as sub-components
Entire form working and submitting - users should be able to create and edit keys from the flyout

Screencaps

Creating, updating, and deleting a key:

Example errors returned by API:

^ Note - I don't super love how the errors are "off screen" and sometimes not visible on smaller screens. I wish there was a way to address this, but it's probably out of scope for the migration. We also have the same issue on some scrolling modals so it's potentially something to address all at once later.

QA

Creating a key
- Confirm that creating a key works (success message, credentials table is updated)
- Admin keys
  - Confirm that when creating an admin key, you only see the Name and Type fields
- Private keys
  - Confirm that when creating a private key, you see all fields (Name, Type, Read/Write access, Engine access)
- Search keys
  - Confirm that when creating a search key, you see the engine access field but not the read/write access field
Updating a key
- Confirm that when you click the pencil icon to edit an existing key, the Name and Type form fields are disabled / cannot be modified
- Confirm that you can edit a private key's read/write access levels. Confirm that on refreshing the page, your updated settings are 'remembered'
- Confirm that you can edit a search key's engine access. Confirm that on refreshing the page, your updated settings are 'remembered'
Deleting a key
- Confirm that deleting a key works and shows a flash message
Expected errors
- Confirm that trying to create a key with no name generates an understandable error
- Confirm that trying to save a private key with neither read nor write access generates an understandable error
- Confirm that trying to save a search key without access to any engines generates an understandable error

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

cee-chen · 2020-10-22T22:05:54Z

...c/applications/app_search/components/credentials/credentials_flyout/form_components/index.ts

+export { FormKeyName } from './key_name';
+export { FormKeyType } from './key_type';
+export { FormKeyReadWriteAccess } from './key_read_write_access';
+export { FormKeyEngineAccess } from './key_engine_access';
+export { FormKeyUpdateWarning } from './key_update_warning';


Super quick note about the naming schema I went with - I'm not in love with it or anything so definitely happy to take suggestions. One thing that drives me slightly nuts (but no idea how to fix lol) is the alternating terminology we use:

"Credentials" in the routing/file/folder names

"Tokens" in our actual code vars

"API Keys" in our user-facing copy (and now I guess these file names)

Anyway, don't mind me, I'm just dead inside over here

cee-chen · 2020-10-22T22:08:20Z

...plications/app_search/components/credentials/credentials_flyout/form_components/key_type.tsx

+          <p>
+            {tokenDescription && <strong>{tokenDescription}</strong>}{' '}
+            <EuiLink href={DOCS_HREF}>
+              {i18n.translate('xpack.enterpriseSearch.appSearch.tokens.documentationLink1', {
+                defaultMessage: 'Visit the documentation',
+              })}
+            </EuiLink>{' '}
+            {i18n.translate('xpack.enterpriseSearch.appSearch.tokens.documentationLink2', {
+              defaultMessage: 'to learn more about keys.',
+            })}
+          </p>


I'm not sure if there's a better way of i18n'ing sentences broken up by JSX/HTML (a link in this case). Hoping our amazing i18n folks are ok handling this 😬

JasonStoltz

The way you broke these down into independent form components makes the code, and this PR, way more manageable. Thank you for doing that.

I don't have any blocking feedback, but I did make some comments to consider.

Great work on this.

...s/app_search/components/credentials/credentials_flyout/form_components/key_engine_access.tsx

...tions/app_search/components/credentials/credentials_flyout/form_components/key_type.test.tsx

...plications/app_search/components/credentials/credentials_flyout/form_components/key_type.tsx

JasonStoltz · 2020-10-26T14:57:21Z

...s/app_search/components/credentials/credentials_flyout/form_components/key_engine_access.tsx

+  );
+};
+
+export const EngineSelection: React.FC = () => {


Are you OK with the fact that we're exporting two components from a single file? Seems counter to common practice.

Definitely my opinion, but I don't think there's a standard/best practice for this. I generally prefer taking advantage of multiple exports (the same way we export multiple string consts, utility fns, etc.) and I don't 100% see why components are significantly different.

In this case EngineSelection is a subcomponent and primarily exported to make unit testing easier - but I think that's a feature, not a bug, of unit testing, because it helps us break code down into substantially more readable blocks. I also don't think exporting for the sake of a test file is a major faux pas or anything - conceptually it's not any different from mocks or other const's exported to make tests easier.

JasonStoltz · 2020-10-26T15:07:37Z

...enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts

@@ -21,7 +21,7 @@ import { IMeta } from '../../../../../common/types';
 import { IEngine } from '../../types';
 import { IApiToken, ICredentialsDetails, ITokenReadWrite } from './types';

-const defaultApiToken: IApiToken = {
+export const defaultApiToken: IApiToken = {


Nice way to clean up tests 👍

kibanamachine · 2020-10-26T18:14:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dab0456

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`enterpriseSearch`	429	435	+6

async chunks size

id	before	after	diff
`enterpriseSearch`	657.3KB	670.1KB	+12.8KB

History

💚 Build #83488 succeeded 05b1625

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Add key name field * Add key type field * Add key read/write fields * Add key engine access / selection * Add key update warning callout * Update flyout body with form components * [PR feedback] i18n - change appSearch.tokens to appSearch.credentials * [PR feedback] Remove unnecessary conditional

) * Add key name field * Add key type field * Add key read/write fields * Add key engine access / selection * Add key update warning callout * Update flyout body with form components * [PR feedback] i18n - change appSearch.tokens to appSearch.credentials * [PR feedback] Remove unnecessary conditional Co-authored-by: Kibana Machine <[email protected]>

* master: (37 commits) [ILM] Migrate Warm phase to Form Lib (elastic#81323) [Security Solutions][Detection Engine] Fixes critical bug with error reporting that was doing a throw (elastic#81549) [Detection Rules] Add 7.10 rules (elastic#81676) [kbn/optimizer] ignore missing metrics when updating limits with --focus (elastic#81696) [SECURITY SOLUTIONS] Bugs overview page + investigate eql in timeline (elastic#81550) [Maps] fix unable to edit cluster vector styles styled by count when switching to super fine grid resolution (elastic#81525) Fixed migration issue for case specific actions, by extending email action migrator checks (elastic#81673) [CI] Preparation for APM tracking on CI (elastic#80399) [Home] Fixes Kibana app description order on home page and updates Canvas copy (elastic#80057) Make sure `to` is 'now' and not the same as `from` (elastic#81524) Nitpicking the 8.0 Breaking Change issue template (elastic#81678) [SECURITY_SOLUTION] Fix text on onboarding screen (elastic#81672) [data.search] Skip async search tests in build candidates and production builds (elastic#81547) Fix previousStartedAt by not changing when execution fails (elastic#81388) [Monitoring] Fix a couple of issues with the cpu usage alert (elastic#80737) Telemetry collection xpack to ts project references (elastic#81269) Elasticsearch: don't use url authentication for new client (elastic#81564) [App Search] Credentials: implement working flyout form (elastic#81541) Properly encode links to edit user page (elastic#81562) [Alerting UI] Don't wait for health check before showing Create Alert flyout (elastic#80996) ...

cee-chen added 5 commits October 22, 2020 14:55

Add key name field

c2fbd47

Add key type field

2692d17

Add key read/write fields

8672aff

Add key engine access / selection

94db91a

Add key update warning callout

6f81b63

cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 22, 2020

cee-chen requested review from JasonStoltz and a team October 22, 2020 21:59

This was referenced Oct 22, 2020

[App Search] Credentials: add FlashMessages, stub out credentials flyout #81391

Merged

[App Search] Credentials: Add final Logic and server routes #81519

Merged

cee-chen commented Oct 22, 2020

View reviewed changes

Update flyout body with form components

05b1625

cee-chen force-pushed the credentials-flyout-3 branch from 005f93d to 05b1625 Compare October 22, 2020 22:10

JasonStoltz approved these changes Oct 26, 2020

View reviewed changes

cee-chen added 3 commits October 26, 2020 09:32

[PR feedback] i18n - change appSearch.tokens to appSearch.credentials

947f9f5

[PR feedback] Remove unnecessary conditional

ee377e2

Merge remote-tracking branch 'upstream/master' into credentials-flyout-3

dab0456

cee-chen merged commit 7508321 into elastic:master Oct 26, 2020

cee-chen mentioned this pull request Oct 26, 2020

[7.x] [App Search] Credentials: implement working flyout form (#81541) #81657

Merged

cee-chen deleted the credentials-flyout-3 branch October 26, 2020 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Credentials: implement working flyout form #81541

[App Search] Credentials: implement working flyout form #81541

cee-chen commented Oct 22, 2020 •

edited

Loading

cee-chen Oct 22, 2020 •

edited

Loading

cee-chen Oct 22, 2020

JasonStoltz left a comment

JasonStoltz Oct 26, 2020

cee-chen Oct 26, 2020

JasonStoltz Oct 26, 2020

kibanamachine commented Oct 26, 2020

[App Search] Credentials: implement working flyout form #81541

[App Search] Credentials: implement working flyout form #81541

Conversation

cee-chen commented Oct 22, 2020 • edited Loading

PR sequence

Summary

Screencaps

QA

Checklist

cee-chen Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

cee-chen Oct 22, 2020

Choose a reason for hiding this comment

JasonStoltz left a comment

Choose a reason for hiding this comment

JasonStoltz Oct 26, 2020

Choose a reason for hiding this comment

cee-chen Oct 26, 2020

Choose a reason for hiding this comment

JasonStoltz Oct 26, 2020

Choose a reason for hiding this comment

kibanamachine commented Oct 26, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

History

cee-chen commented Oct 22, 2020 •

edited

Loading

cee-chen Oct 22, 2020 •

edited

Loading