migrate saved objects management edition view to react/typescript/eui by pgayvallet · Pull Request #59490 · elastic/kibana

pgayvallet · 2020-03-05T22:21:50Z

Summary

Preliminary work for #50308, this converts the saved objects edition/view page to react, typescript, EUI, and new platform APIs.

Extracted from my #59110 sandbox

before

after

Checklist

Delete any items that are not applicable to this PR.

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
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

elasticmachine · 2020-03-05T22:21:53Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2020-03-05T22:24:22Z

src/legacy/core_plugins/kibana/public/management/sections/objects/_view.js

+  $scope.$$postDigest(() => {
+    const node = document.getElementById(REACT_OBJECTS_VIEW_DOM_ELEMENT_ID);
+    if (!node) {
+      return;
+    }
+
+    render(
+      <I18nContext>


Not sure this $$postDigest approach is the best, or even useful, but I copied the way it's already done in src/legacy/core_plugins/kibana/public/management/sections/objects/_objects.js, and this angular->react bridge is meant to be temporary anyway.

pgayvallet · 2020-03-05T22:27:40Z

src/legacy/core_plugins/kibana/public/management/sections/objects/_view.js

 uiRoutes.when('/management/kibana/objects/:service/:id', {
  template: objectViewHTML,
  k7Breadcrumbs: getViewBreadcrumbs,
  requireUICapability: 'management.kibana.objects',
 });

+function createReactView($scope, $routeParams) {
+  const { service: serviceName, id: objectId, notFound } = $routeParams;


TIL: src/legacy/ui/public/url/redirect_when_missing.js for the notFound undeclared route param.

pgayvallet · 2020-03-05T22:31:50Z

src/legacy/core_plugins/kibana/public/management/sections/objects/saved_object_view.tsx

+    if (confirmed) {
+      await savedObjectsClient.delete(type, id);
+      notifications.toasts.addSuccess(`Deleted '${object!.attributes.title}' ${type} object`);
+      window.location.hash = '/management/kibana/objects';


So, I did not like doing that, however

I don't want any angular services here, so $location or kbnUrl (which was used before) is not an option

We have no core APIs to do that

The management kibana app is still using angular hash-based routing, meaning that even if core was exposing an API to change to url, this wouldn't work (we need to redirect to {basePath}/app/kibana#/management/kibana/objects)

Have you looked at using react-router-dom to do the redirect? I think it will provide the right interface so that changes to the url structure won't break this.

edit - I think this is fine as an intermediate step but once this becomes a new platform management app we'll want to change this.

@mattkime I'm planning to do that in the next step, when I actually migrate these two pages to NP plugins (draft in #59110). ATM, the routes are still provided/handled by angular so imho it doesn't really make sense (if even possible?) to use react router to perform the redirect.

pgayvallet · 2020-03-05T22:34:05Z

...egacy/core_plugins/kibana/public/management/sections/objects/components/object_view/form.tsx

+    if ((service as any).Class) {
+      addFieldsFromClass((service as any).Class, fields);
+    }


The js code was accessing the SavedObjectLoader.Class property, but it's declared as private in the TS definition.

pgayvallet · 2020-03-06T08:32:36Z

test/functional/apps/saved_objects_management/edit_saved_object.ts

+  const focusAndClickButton = async (buttonSubject: string) => {
+    const button = await testSubjects.find(buttonSubject);
+    await button.scrollIntoViewIfNecessary();
+    await delay(10);
+    await button.focus();
+    await delay(10);
+    await button.click();
+  };


I don't know the exact cause, but only clicking on the buttons was very flaky (it seems the texteditor are catching the focus / blocking the click during the driver auto-scrolling to the button). I had to use that to remove the flakyness...

gchaps · 2020-03-06T17:09:00Z

For consistency, please use init caps for the field names:

Name
Description
Hits
...

Also, use this capitalization for the button name:

Save search object

pgayvallet · 2020-03-06T17:09:27Z

@elastic/kibana-design As said on slack

the main goal of the PR is to perform the react migration to be able to move that to a NP plugin. We don't plan to do any heavy UI changes in the short term on this page, but as I migrated the old kbn classes to Eui components, any feedback on that is very welcome

pgayvallet · 2020-03-06T17:22:59Z

@gchaps

For consistency, please use init caps for the field names:

These are the exact field name from the saved object structure, not labels. Capitalizing that might not be a good idea, as copy/pasting the field name (to perform an ES query for example) would result in copying something wrong. WDYT?

gchaps · 2020-03-06T23:06:30Z

@pgayvallet Ah, I see. My preference would be to use capitalization for the labels because to me it makes the UI look more polished. We do use capitalization for the labels when editing a field in an index pattern. However, as this is for advanced users "with intimate knowledge of the code" and they might want to copy the field name for use in their queries, it's ok to leave as is.

…gement-object-view-to-react

joshdover · 2020-03-09T16:20:23Z

@elastic/kibana-design Do you want to take a pass at this? Not sure if it's important enough since it is a "advanced" screen, but thought I'd flag it just in case.

cchaos

I made a design PR for you mainly just cleanup: pgayvallet#1

src/legacy/core_plugins/kibana/public/management/sections/objects/saved_object_view.tsx

…object Wrapping whole view in page content panel and removing legacy classes

…gement-object-view-to-react

pgayvallet · 2020-03-10T09:02:40Z

retest

…gement-object-view-to-react

pgayvallet · 2020-03-11T11:32:05Z

retest

…gement-object-view-to-react

joshdover

What's the testing plan for these changes? I see you added e2e tests which is great. Are we going to hold off on unit tests until later?

joshdover · 2020-03-15T21:57:45Z

src/legacy/core_plugins/kibana/public/management/sections/objects/_view.js

-          $location.path('/management/kibana/objects').search({
-            _a: rison.encode({
-              tab: serviceObj.title,
-            }),
-          });


Is this URL state unneeded now? I don't see it in the new implementation

From my understanding, it is. I think this was a forgotten thing from an older version, as the rison formatted search doesn't seems to be used (functionally or technically) in the SO management table view.

pgayvallet · 2020-03-16T06:54:05Z

@joshdover

Are we going to hold off on unit tests until later?

If we want to add UT, we should probably do it in this PR. What kind of unit test would you like to see added here? snapshot testing of the introduced lower-level react components ?

pgayvallet · 2020-03-16T06:55:00Z

@cchaos I think the asked changes are done. PTAL

…gement-object-view-to-react

cchaos

There are still a few unnecessary legacy classes that need to be removed in favor of EUI components.

cchaos · 2020-03-17T13:48:56Z

...egacy/core_plugins/kibana/public/management/sections/objects/components/object_view/form.tsx

+            />
+          ))}
+        </div>
+        <EuiFormRow fullWidth={true}>


You don't need a EuiFormRow if you're not providing an input as a direct descendent or a label. It will just cause extraneous wrappers.

cchaos · 2020-03-17T13:49:19Z

...gacy/core_plugins/kibana/public/management/sections/objects/components/object_view/field.tsx

+    const { name } = this.props;
+
+    return (
+      <div className="kuiFormSection">


Can you remove these legacy kui classes? Instead, you can just use the <EuiFormRow label=""> wrapper around the field.

cchaos · 2020-03-17T13:52:27Z

...gacy/core_plugins/kibana/public/management/sections/objects/components/object_view/intro.tsx

+          defaultMessage="Proceed with caution!"
+        />
+      }
+      iconType="bolt"


Suggested change

iconType="bolt"

iconType="alert"

pgayvallet · 2020-03-17T15:34:02Z

@cchaos the remaning kui classes has been removed and suggested changes done.

cchaos

👍 Thanks!

pgayvallet · 2020-03-18T06:31:52Z

@joshdover added UT for new components. Only skipped the high-level form component as it's more efficiently covered by the existing FTs.

joshdover

LGTM. Left some suggestions for some improvements, but nothing blocking this for merging. I'll leave it up to you whether these optional suggestions are worth it on this page.

joshdover · 2020-03-18T19:16:19Z

...core_plugins/kibana/public/management/sections/objects/components/object_view/field.test.tsx

+  });
+
+  it('renders a EuiFieldText as fallback', () => {
+    const mounted = mountField({ ...defaultProps, type: 'unkown' as any });


super nit, doesn't really matter (could also make more obvious that it's supposed to be an unexpected value by making it blah or something)

Suggested change

const mounted = mountField({ ...defaultProps, type: 'unkown' as any });

const mounted = mountField({ ...defaultProps, type: 'unknown' as any });

joshdover · 2020-03-18T19:17:39Z

...gacy/core_plugins/kibana/public/management/sections/objects/components/object_view/field.tsx

+              !!currentValue ? (
+                <FormattedMessage id="kbn.management.objects.field.onLabel" defaultMessage="On" />
+              ) : (
+                <FormattedMessage id="kbn.management.objects.field.offLabel" defaultMessage="Off" />
+              )


joshdover · 2020-03-18T19:18:27Z

...egacy/core_plugins/kibana/public/management/sections/objects/components/object_view/form.tsx

+ * @param {array} parents The parent keys to the field
+ * @returns {array}
+ */
+const recursiveCreateFields = (key: string, value: any, parents: string[] = []): ObjectField[] => {


Should this have a max depth safety mechanism to avoid recursing deeply nested objects if one were to exist?

Doesn't cost much to add. Also extracted the field generation logic and added tests

joshdover · 2020-03-18T19:19:58Z

src/legacy/core_plugins/kibana/public/management/sections/objects/saved_object_view.tsx

+            <Intro />
+          </>
+        )}
+        {object && (


Should we use Suspense or show a spinner while this loads?

Will keep this one as a later improvement

joshdover · 2020-03-18T19:21:23Z

...egacy/core_plugins/kibana/public/management/sections/objects/components/object_view/form.tsx

+                  },
+                })}
+                onClick={this.onSubmit}
+                disabled={!isValid}


optional: Should this also be disabled while the save operation is running to avoid double-saves?

…gement-object-view-to-react

kibanamachine · 2020-03-19T12:52:34Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: b63934f
Flaky suites:
- xpack-kibana-ciGroup6

Test Failures

Kibana Pipeline / kibana-xpack-agent / X-Pack OpenID Connect API Integration Tests (Implicit Flow).x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth·ts.apis OpenID Connect Implicit Flow authentication finishing handshake should succeed if both the OpenID Connect response and the cookie are provided

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 3 times on tracked branches: https://github.com/elastic/kibana/issues/43938

[00:00:00]       │
[00:00:00]         └-: apis
[00:00:00]           └-> "before all" hook
[00:00:00]           └-: OpenID Connect Implicit Flow authentication
[00:00:00]             └-> "before all" hook
[00:00:00]             └-: finishing handshake
[00:00:00]               └-> "before all" hook
[00:00:00]               └-> should return an HTML page that will parse URL fragment
[00:00:00]                 └-> "before each" hook: global before each
[00:00:00]                 └-> "before each" hook
[00:00:00]                 └- ✓ pass  (51ms) "apis OpenID Connect Implicit Flow authentication finishing handshake should return an HTML page that will parse URL fragment"
[00:00:00]               └-> should fail if OpenID Connect response is not complemented with handshake cookie
[00:00:00]                 └-> "before each" hook: global before each
[00:00:00]                 └-> "before each" hook
[00:00:00]                 └- ✓ pass  (133ms) "apis OpenID Connect Implicit Flow authentication finishing handshake should fail if OpenID Connect response is not complemented with handshake cookie"
[00:00:00]               └-> should fail if state is not matching
[00:00:00]                 └-> "before each" hook: global before each
[00:00:00]                 └-> "before each" hook
[00:00:00]                 │ info [o.e.x.s.a.AuthenticationService] [kibana-ci-immutable-ubuntu-16-tests-xl-1584614707406488139] Authentication to realm oidc1 failed - Failed to authenticate user with OpenID Connect (Caused by org.elasticsearch.ElasticsearchSecurityException: Invalid state parameter [$someothervalue], while [7NkdMpkC-g3WVZ7SZReQdRW9sA-gAkoVVpgsmjiz9MA] was expected)
[00:00:00]                 └- ✓ pass  (199ms) "apis OpenID Connect Implicit Flow authentication finishing handshake should fail if state is not matching"
[00:00:00]               └-> should succeed if both the OpenID Connect response and the cookie are provided
[00:00:00]                 └-> "before each" hook: global before each
[00:00:00]                 └-> "before each" hook
[00:00:00]                 │ info [o.e.x.s.a.AuthenticationService] [kibana-ci-immutable-ubuntu-16-tests-xl-1584614707406488139] Authentication to realm oidc1 failed - Failed to authenticate user with OpenID Connect (Caused by org.elasticsearch.ElasticsearchSecurityException: Failed to get claims from the Userinfo Endpoint.)
[00:00:00]                 └- ✖ fail: "apis OpenID Connect Implicit Flow authentication finishing handshake should succeed if both the OpenID Connect response and the cookie are provided"
[00:00:00]                 │

Stack Trace

Error: expected 302 "Found", got 401 "Unauthorized"
    at Test._assertStatus (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:268:12)
    at Test._assertFunction (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:283:11)
    at Test.assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:173:18)
    at assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:131:12)
    at /dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:128:5
    at Test.Request.callback (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:718:3)
    at parser (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:906:18)
    at IncomingMessage.res.on (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/parsers/json.js:19:7)
    at endReadableNT (_stream_readable.js:1145:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

History

💚 Build #33829 succeeded 0cf9e80
💚 Build #33795 succeeded 44f59a6
💚 Build #33333 succeeded 9e31d14
💚 Build #32441 succeeded 5e9cfef
💔 Build #32369 failed 0015f2a

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

…elastic#59490) * migrate so management edition view to react * fix bundle name + add forgotten data-test-subj * add FTR tests for edition page * EUIfy react components * wrap form with EuiPanel + caps btns labels * Wrapping whole view in page content panel and removing legacy classes * improve delete confirmation modal * update translations * improve delete popin * add unit test on view components * remove kui classes & address comments * extract createFieldList and add tests * disable form submit during submition Co-authored-by: cchaos <caroline.horn@elastic.co>

…pt/eui (#59490) (#60621) * migrate saved objects management edition view to react/typescript/eui (#59490) * migrate so management edition view to react * fix bundle name + add forgotten data-test-subj * add FTR tests for edition page * EUIfy react components * wrap form with EuiPanel + caps btns labels * Wrapping whole view in page content panel and removing legacy classes * improve delete confirmation modal * update translations * improve delete popin * add unit test on view components * remove kui classes & address comments * extract createFieldList and add tests * disable form submit during submition Co-authored-by: cchaos <caroline.horn@elastic.co> * update dataset and fix test description Co-authored-by: cchaos <caroline.horn@elastic.co>

* master: [ML] Use a new ML endpoint to estimate a model memory (elastic#60376) [Logs UI] Correctly update the expanded log rate table rows (elastic#60306) fixes drag and drop flakiness (elastic#60625) Removing isEmptyState from embeddable input (elastic#60511) [Cross Cluster Replication] NP Shim (elastic#60121) Clear changes when canceling an edit to an alert (elastic#60518) Update workflow syntax (elastic#60626) Updating project assigner workflows to v2.0.0 of the action and back to default tokens (elastic#60577) migrate saved objects management edition view to react/typescript/eui (elastic#59490)

migrate so management edition view to react

b6aab43

pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 5, 2020

pgayvallet requested a review from a team as a code owner March 5, 2020 22:21

pgayvallet requested a review from a team March 5, 2020 22:22

pgayvallet commented Mar 5, 2020

View reviewed changes

pgayvallet added 2 commits March 6, 2020 07:54

fix bundle name + add forgotten data-test-subj

81c0623

add FTR tests for edition page

ed158a4

pgayvallet commented Mar 6, 2020

View reviewed changes

pgayvallet mentioned this pull request Mar 6, 2020

Euified Management Saved Objects view component. #47652

Closed

2 tasks

EUIfy react components

4f60ecf

pgayvallet changed the title ~~migrate saved objects management edition view to react and typescript~~ migrate saved objects management edition view to react/typescript/eui Mar 6, 2020

pgayvallet requested a review from a team March 6, 2020 12:33

wrap form with EuiPanel + caps btns labels

330a0c0

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

c477b61

…gement-object-view-to-react

rudolf mentioned this pull request Mar 9, 2020

[Platform] SavedObject Management #50308

Closed

5 tasks

mattkime approved these changes Mar 9, 2020

View reviewed changes

Wrapping whole view in page content panel and removing legacy classes

6e049cf

cchaos suggested changes Mar 9, 2020

View reviewed changes

src/legacy/core_plugins/kibana/public/management/sections/objects/saved_object_view.tsx Show resolved Hide resolved

pgayvallet and others added 2 commits March 10, 2020 08:25

Merge pull request #1 from cchaos/pgayvallet-kbn-50308-so-management-…

cdd7825

…object Wrapping whole view in page content panel and removing legacy classes

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

4313bd2

…gement-object-view-to-react

update translations

1d7c60f

pgayvallet added 2 commits March 11, 2020 10:57

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

4898fab

…gement-object-view-to-react

improve delete popin

0015f2a

pgayvallet requested a review from cchaos March 11, 2020 10:21

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

5e9cfef

…gement-object-view-to-react

joshdover reviewed Mar 15, 2020

View reviewed changes

pgayvallet added 2 commits March 16, 2020 07:55

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

9e31d14

…gement-object-view-to-react

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

5743a70

…gement-object-view-to-react

cchaos suggested changes Mar 17, 2020

View reviewed changes

pgayvallet added 2 commits March 17, 2020 15:07

add unit test on view components

44f59a6

remove kui classes & address comments

0cf9e80

cchaos approved these changes Mar 17, 2020

View reviewed changes

joshdover approved these changes Mar 18, 2020

View reviewed changes

pgayvallet added 3 commits March 19, 2020 11:18

extract createFieldList and add tests

8ac573a

disable form submit during submition

1a68b30

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

b63934f

…gement-object-view-to-react

pgayvallet merged commit 395d621 into elastic:master Mar 19, 2020

This was referenced Mar 19, 2020

[7.x] migrate saved objects management edition view to react/typescript/eui (#59490) #60621

Merged

fix test description #60638

Merged

pgayvallet mentioned this pull request May 14, 2020

Editing a saved object from Management UI can corrupt the saved object #66542

Closed

	const mounted = mountField({ ...defaultProps, type: 'unkown' as any });
	const mounted = mountField({ ...defaultProps, type: 'unknown' as any });

Conversation

pgayvallet commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

before

after

Checklist

Uh oh!

elasticmachine commented Mar 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchaps commented Mar 6, 2020

Uh oh!

pgayvallet commented Mar 6, 2020

Uh oh!

pgayvallet commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gchaps commented Mar 6, 2020

Uh oh!

joshdover commented Mar 9, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgayvallet commented Mar 10, 2020

Uh oh!

pgayvallet commented Mar 11, 2020

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Mar 16, 2020

Uh oh!

pgayvallet commented Mar 16, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Mar 17, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Mar 18, 2020

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pgayvallet commented Mar 5, 2020 •

edited

Loading

pgayvallet Mar 5, 2020 •

edited

Loading

mattkime Mar 7, 2020 •

edited

Loading

pgayvallet commented Mar 6, 2020 •

edited

Loading