[Enterprise Search] Migrate shared role mapping components #91723

scottybollinger · 2021-02-17T18:40:59Z

Summary

Migrate shared role mapping components from ent-search. These will be used by both App Search and Workplace Search in their respective Role Mapping sections.

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

EUI has not updated their types to include this util and no one else in Kibana uses it so falling back to the lodash equivalent

Also cleaned up shared type import

Also added new lines to match other places in Kibana

- AddRoleMappingButton - DeleteMappingCallout - RoleSelector

When trying to achieve coverage it was impossible to create a state that these fallbacks were ever triggered. It’s my assumption that these were added in ent-search to get around TypeScript errors. Added typings instead.

scottybollinger · 2021-02-18T17:47:53Z

@JasonStoltz would you mind taking a look at this commit. It appears the change was made here in ent-search and I want to be sure my assumption and corresponding change are correct.

...ck/plugins/enterprise_search/public/applications/shared/role_mapping/role_mappings_table.tsx

x-pack/plugins/enterprise_search/public/applications/shared/role_mapping/attribute_selector.tsx

When actually implementing these I found that these types were wrong

scottybollinger · 2021-02-19T18:52:15Z

It won't let me reply to your comment so doing it here:

Where does tooltip.content come from and does it need to be i18ned?

It is sent from the API

x-pack/plugins/enterprise_search/public/applications/shared/role_mapping/role_selector.test.tsx

x-pack/plugins/enterprise_search/public/applications/shared/role_mapping/role_selector.tsx

scottybollinger · 2021-02-19T18:55:09Z

@elasticmachine merge upstream

x-pack/plugins/enterprise_search/public/applications/shared/role_mapping/role_selector.tsx

x-pack/plugins/enterprise_search/public/applications/shared/types.ts

JasonStoltz

I made a bunch of comments about small stuff, nothing major, but I thought it'd be good to clean this up now rather than needing to circle back to it later.

JasonStoltz · 2021-02-22T13:40:08Z

@elasticmachine merge upstream

JasonStoltz

LGTM, Thanks for making those changes.

Side note, I had a really hard time going back through my comments and figuring out which you addressed and which you didn't. It's OK if you don't address everything, but it helps me on re-review if you let me know what you decide. Thanks!

JasonStoltz · 2021-02-22T13:43:13Z

...lugins/enterprise_search/public/applications/shared/role_mapping/attribute_selector.test.tsx

+          selectedAuthProviders={['kbn_saml']}
+        />
+      );
+      const select = findAuthProvidersSelect(wrapper) as any;


I see you resolved this but didn't make the change. It's fine if you're choosing not to make this change, but could you just update the comments in the future so I know?

scottybollinger · 2021-02-22T14:38:27Z

@JasonStoltz for some reason GitHub still will not let me reply to your comments inline. In regards to the overlooked, resolved comment, that was an oversight on my part. Sorry for that. When I attempt to make the change locally, it causes type errors in other places.

There's actually another one on line 85 that I am not easily able to resolve without the any.

IMO it's ok to have any's in test files so I am going to go ahead and merge this on green. If you feel it warrants the effort to remove, we can address it in a separate PR.

kibanamachine · 2021-02-22T15:49:50Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #108138 succeeded 4f9bbd5
💚 Build #108040 succeeded 96e293e
💚 Build #108012 succeeded 6880c20
💔 Build #107865 failed 0167313
💚 Build #107814 succeeded fc839e6

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

…1723) * Initial copy/paste of components * Add types * Add constants * Replace EUI toSentenceCase with lodash startCase EUI has not updated their types to include this util and no one else in Kibana uses it so falling back to the lodash equivalent * Update paths * Fix test * Fix TypeScript issues * Remove ability check for non-federated users * Use Kibana React Router helpers Also cleaned up shared type import * Update comments * Fix failing test Also added new lines to match other places in Kibana * Add i18n * Add tests for smaller components - AddRoleMappingButton - DeleteMappingCallout - RoleSelector * Fix fallbacks When trying to achieve coverage it was impossible to create a state that these fallbacks were ever triggered. It’s my assumption that these were added in ent-search to get around TypeScript errors. Added typings instead. * Add tests for AttributeSelector * Add mocks and testSubj attrs * Add tests for RoleMappingsTable * Fix types When actually implementing these I found that these types were wrong * Refactor for better typing * Remove return type This was causing a TypeScript error and is not needed * Rename interface * Rename more interfaces * PR feedback * Add test for radio checked state * Add false radio assertion to * Update className Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-02-22T15:52:41Z

💚 Backport successful

✅ 7.x / #92214

Successful backport PRs will be merged automatically after passing CI.

* master: Ability to filter alerts by string parameters (elastic#92036) [APM] Fix for flaky correlations API test (elastic#91673) (elastic#92094) [Enterprise Search] Migrate shared role mapping components (elastic#91723) [file_upload] move ml Importer classes to file_upload plugin (elastic#91559) [Discover] Always show the "hide missing fields" toggle (elastic#91889) v2 migrations should exit process on corrupt saved object document (elastic#91465) [ML] Data Frame Analytics exploration page: filters improvements (elastic#91748) [ML] Data Frame Analytics: Improved error handling for scatterplot matrix. (elastic#91993) [coverage] speed up merging results of functional tests (elastic#92111) Adds a Reason indicator to the onClose handler in AddAlert and EditAlert (elastic#92149)

cee-chen · 2021-02-22T16:43:47Z

just popping in here to add my 2c, if there's a super quick fix/ alternative to any in tests I'll usually suggest it, but if it takes longer than that / the typing is super annoying, I'm totally good with any in tests 👍

…92214) * Initial copy/paste of components * Add types * Add constants * Replace EUI toSentenceCase with lodash startCase EUI has not updated their types to include this util and no one else in Kibana uses it so falling back to the lodash equivalent * Update paths * Fix test * Fix TypeScript issues * Remove ability check for non-federated users * Use Kibana React Router helpers Also cleaned up shared type import * Update comments * Fix failing test Also added new lines to match other places in Kibana * Add i18n * Add tests for smaller components - AddRoleMappingButton - DeleteMappingCallout - RoleSelector * Fix fallbacks When trying to achieve coverage it was impossible to create a state that these fallbacks were ever triggered. It’s my assumption that these were added in ent-search to get around TypeScript errors. Added typings instead. * Add tests for AttributeSelector * Add mocks and testSubj attrs * Add tests for RoleMappingsTable * Fix types When actually implementing these I found that these types were wrong * Refactor for better typing * Remove return type This was causing a TypeScript error and is not needed * Rename interface * Rename more interfaces * PR feedback * Add test for radio checked state * Add false radio assertion to * Update className Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>

scottybollinger added 11 commits February 17, 2021 10:32

Initial copy/paste of components

c6356a2

Add types

f62615f

Add constants

c2d6c4f

Replace EUI toSentenceCase with lodash startCase

9596b05

EUI has not updated their types to include this util and no one else in Kibana uses it so falling back to the lodash equivalent

Update paths

1dab689

Fix test

1b19482

Fix TypeScript issues

891abf3

Remove ability check for non-federated users

820a184

Use Kibana React Router helpers

e1d4fde

Also cleaned up shared type import

Update comments

b7ccfb9

Fix failing test

27c3591

Also added new lines to match other places in Kibana

scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated: Automatically backport this PR after it's merged labels Feb 17, 2021

scottybollinger added 6 commits February 17, 2021 13:07

Add i18n

3e23a4f

Add tests for smaller components

4f755d1

- AddRoleMappingButton - DeleteMappingCallout - RoleSelector

Fix fallbacks

9ebde24

When trying to achieve coverage it was impossible to create a state that these fallbacks were ever triggered. It’s my assumption that these were added in ent-search to get around TypeScript errors. Added typings instead.

Add tests for AttributeSelector

fba159d

Add mocks and testSubj attrs

235b368

Add tests for RoleMappingsTable

6c6b757

scottybollinger marked this pull request as ready for review February 18, 2021 17:43

scottybollinger requested review from a team as code owners February 18, 2021 17:43

scottybollinger commented Feb 18, 2021

View reviewed changes

...ck/plugins/enterprise_search/public/applications/shared/role_mapping/role_mappings_table.tsx Outdated Show resolved Hide resolved

JasonStoltz reviewed Feb 18, 2021

View reviewed changes

x-pack/plugins/enterprise_search/public/applications/shared/role_mapping/attribute_selector.tsx Outdated Show resolved Hide resolved

scottybollinger added 2 commits February 18, 2021 13:48

Fix types

fc839e6

When actually implementing these I found that these types were wrong

Refactor for better typing

0167313