[App Search] Role mappings migration part 2 #94461

scottybollinger · 2021-03-11T16:47:53Z

Summary

This is the part 2 of the Role Mappings to Kibana migration. This PR ports the logic file over. The full code can be found here, if QA is to be done.

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

The original asRoleMapping was merely for a smoke test in the shared component. Refactored to work better in App Search component

cee-chen

Thanks a ton for this Scotty! Some minor comments and general questions for you - I'm definitely not super familiar with the in-depth role mappings functionality, so appreciate any answers/knawledge you can toss me

x-pack/plugins/enterprise_search/public/applications/app_search/app_logic.ts

x-pack/plugins/enterprise_search/public/applications/shared/role_mapping/__mocks__/roles.ts

...rprise_search/public/applications/app_search/components/role_mappings/role_mappings_logic.ts

...e_search/public/applications/app_search/components/role_mappings/role_mappings_logic.test.ts

...rprise_search/public/applications/app_search/components/role_mappings/role_mappings_logic.ts

...e_search/public/applications/app_search/components/role_mappings/role_mappings_logic.test.ts

JasonStoltz

[EDIT] Just now realizing this PR is from you @scottybollinger . I reviewed this like I review any other App Search Logic file. If it seems too onerous, that is fine. I can make updates in a follow up PR or something. I realize you might not use the same patterns or standards on the WS half of things. So consider this optional.

I reviewed the test file only, a few general comments that they can all boil down to...

Consider making assertions in tests directly on .values and spreading DEFAULT_VALUES.
Consider using mount when setting up initial data, and consider doing it more often to get better, more understandable tests.
Consider using better descriptions on tests. It can be a valuable living documentation source.

My comments identify a few specific instances where that would help, but I think there are probably others as well.

...e_search/public/applications/app_search/components/role_mappings/role_mappings_logic.test.ts

Co-authored-by: Jason Stoltzfus <[email protected]>

scottybollinger

@JasonStoltz thanks for the edit above. I would actually like to make the changes you suggested, as I agree with them and they will help make our tests better in Workplace Search.

Refactoring the tests showed me that some parts of the state weren’t being reset.

I some how got the test coverage at 100% with my wrong way of doing tests before (scary). When I fixed it I noticed that noting I could do would trigger the fallback of just returning the `[ANY_AUTH_PROVIDER]` array. After talking with Constance, we could not come up with a way to trigger it either, given the conditions. She had suggested removing the first return statement but that caused an empty array being returned sometimes. Ultimately, I was able to get it working and covered with these changes.

The places where `role: 'superuser’` was deleted in the listeners was a side effect of using `setRoleMappingData` and not `mount`

scottybollinger · 2021-03-15T21:00:22Z

@elasticmachine merge upstream

scottybollinger · 2021-03-15T21:04:31Z

@JasonStoltz this should be ready for another look. I have updated the full-feature code on this branch if you want to do any QA

...ugins/enterprise_search/public/applications/app_search/components/role_mappings/constants.ts

Co-authored-by: Constance <[email protected]>

...e_search/public/applications/app_search/components/role_mappings/role_mappings_logic.test.ts

cee-chen

Mostly just minor test nits comments on my part - feel free to commit & auto-merge after. Thanks so much for the changes and awesome work Scotty!! You rock!

kibanamachine · 2021-03-16T21:06:17Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`enterpriseSearch`	2.1MB	2.1MB	+888.0B

History

💛 Build #113472 was flaky 5abe8d1
💚 Build #113232 succeeded 1eff8b5
💔 Build #113223 failed 37d07e8
💔 Build #113103 failed afd0acc
💚 Build #112803 succeeded 2ea8f82

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

* Add engines mock and fix mock role mapping The original asRoleMapping was merely for a smoke test in the shared component. Refactored to work better in App Search component * Add RoleMappingsLogic * Fix test description Co-authored-by: Jason Stoltzfus <[email protected]> * Fix test description Co-authored-by: Jason Stoltzfus <[email protected]> * Add flash messages when creating, updating or deleting * Add path and resetState calls Refactoring the tests showed me that some parts of the state weren’t being reset. * Refactor handleAuthProviderChange logic I some how got the test coverage at 100% with my wrong way of doing tests before (scary). When I fixed it I noticed that noting I could do would trigger the fallback of just returning the `[ANY_AUTH_PROVIDER]` array. After talking with Constance, we could not come up with a way to trigger it either, given the conditions. She had suggested removing the first return statement but that caused an empty array being returned sometimes. Ultimately, I was able to get it working and covered with these changes. * Refactor tests per PR feedback The places where `role: 'superuser’` was deleted in the listeners was a side effect of using `setRoleMappingData` and not `mount` * Add back deleted assertion * Copy nit Co-authored-by: Constance <[email protected]> Co-authored-by: Jason Stoltzfus <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Constance <[email protected]>

kibanamachine · 2021-03-16T21:09:22Z

💚 Backport successful

✅ 7.x / #94762

Successful backport PRs will be merged automatically after passing CI.

* Add engines mock and fix mock role mapping The original asRoleMapping was merely for a smoke test in the shared component. Refactored to work better in App Search component * Add RoleMappingsLogic * Fix test description Co-authored-by: Jason Stoltzfus <[email protected]> * Fix test description Co-authored-by: Jason Stoltzfus <[email protected]> * Add flash messages when creating, updating or deleting * Add path and resetState calls Refactoring the tests showed me that some parts of the state weren’t being reset. * Refactor handleAuthProviderChange logic I some how got the test coverage at 100% with my wrong way of doing tests before (scary). When I fixed it I noticed that noting I could do would trigger the fallback of just returning the `[ANY_AUTH_PROVIDER]` array. After talking with Constance, we could not come up with a way to trigger it either, given the conditions. She had suggested removing the first return statement but that caused an empty array being returned sometimes. Ultimately, I was able to get it working and covered with these changes. * Refactor tests per PR feedback The places where `role: 'superuser’` was deleted in the listeners was a side effect of using `setRoleMappingData` and not `mount` * Add back deleted assertion * Copy nit Co-authored-by: Constance <[email protected]> Co-authored-by: Jason Stoltzfus <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Constance <[email protected]> Co-authored-by: Scotty Bollinger <[email protected]> Co-authored-by: Jason Stoltzfus <[email protected]> Co-authored-by: Constance <[email protected]>

…-action * 'master' of github.com:elastic/kibana: (44 commits) Migrate the optimizer mixin to core (#94272) Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid (#92442) [ML] Anomaly Detection: Migrate validation messages links to use docLinks. (#94568) [Lists][Exceptions] - Adding basic linting, i18n and storybook support (#94772) [Fleet] Add test/fix for invalid/missing ids in bulk agent reassign (#94632) [Security Solution] [Cases] Move create page components and dependencies to Cases (#94444) [ML] Data Frame Analytics accessibility tests: fix flaky outlier creation test (#94735) [Security Solutions] Remove commented out old linter rules (#94753) [App Search] Role mappings migration part 2 (#94461) [CI] Update Backport action inputs to match updated ones (#94721) [chore] Remove the infra team from CODEOWNERS (#94740) [Connectors UI] Make UI use new connector APIs (#94180) [ML] Use indices options in anomaly detection job wizards (#91830) Remove `string` as a valid registry var type value (#94174) [Alerts] Replaces legacy es client with the ElasticsearchClient for alerts and triggers_actions_ui plugins. (#93364) [Reporting-CSV Export] Re-write CSV Export using SearchSource (#88303) chore(NA): upgrade bazel rules nodejs to v3.2.2 (#94726) [APM] Settings: Update layout and update/add descriptions (#94398) skip flaky suite (#94666) [XY axis] Integrates legend color picker with the eui palette (#90589) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/constants.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_context.tsx # x-pack/plugins/index_lifecycle_management/public/shared_imports.ts

scottybollinger added 2 commits March 11, 2021 10:27

Add engines mock and fix mock role mapping

e4f2ace

The original asRoleMapping was merely for a smoke test in the shared component. Refactored to work better in App Search component

Add RoleMappingsLogic

2ea8f82

scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 11, 2021

scottybollinger requested review from a team March 11, 2021 16:47

cee-chen reviewed Mar 15, 2021

View reviewed changes

JasonStoltz requested changes Mar 15, 2021

View reviewed changes

scottybollinger and others added 2 commits March 15, 2021 10:22

Fix test description

dbc9a2b

Co-authored-by: Jason Stoltzfus <[email protected]>

Fix test description

afd0acc

Co-authored-by: Jason Stoltzfus <[email protected]>

scottybollinger commented Mar 15, 2021

View reviewed changes

scottybollinger added 4 commits March 15, 2021 11:44

Add flash messages when creating, updating or deleting

2080296

Add path and resetState calls

0a150b6

Refactoring the tests showed me that some parts of the state weren’t being reset.

Refactor tests per PR feedback

eb1298a

The places where `role: 'superuser’` was deleted in the listeners was a side effect of using `setRoleMappingData` and not `mount`

Merge branch 'master' into scottybollinger/as-role-mappings-2

37d07e8

Add back deleted assertion

1eff8b5

JasonStoltz self-requested a review March 16, 2021 11:39

cee-chen reviewed Mar 16, 2021

View reviewed changes

...ugins/enterprise_search/public/applications/app_search/components/role_mappings/constants.ts Outdated Show resolved Hide resolved

Copy nit

5abe8d1

Co-authored-by: Constance <[email protected]>

scottybollinger enabled auto-merge (squash) March 16, 2021 18:11

scottybollinger disabled auto-merge March 16, 2021 18:12

scottybollinger enabled auto-merge (squash) March 16, 2021 18:23

cee-chen reviewed Mar 16, 2021

View reviewed changes

...e_search/public/applications/app_search/components/role_mappings/role_mappings_logic.test.ts Outdated Show resolved Hide resolved

cee-chen reviewed Mar 16, 2021

View reviewed changes

...e_search/public/applications/app_search/components/role_mappings/role_mappings_logic.test.ts Outdated Show resolved Hide resolved

cee-chen reviewed Mar 16, 2021

View reviewed changes

...e_search/public/applications/app_search/components/role_mappings/role_mappings_logic.test.ts Outdated Show resolved Hide resolved

cee-chen approved these changes Mar 16, 2021

View reviewed changes

Fix typo and use mock for window.confirm

59e9d93

JasonStoltz approved these changes Mar 16, 2021

View reviewed changes

scottybollinger merged commit 4aa7036 into elastic:master Mar 16, 2021

kibanamachine mentioned this pull request Mar 16, 2021

[7.x] [App Search] Role mappings migration part 2 (#94461) #94762

Merged

scottybollinger deleted the scottybollinger/as-role-mappings-2 branch March 16, 2021 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Role mappings migration part 2 #94461

[App Search] Role mappings migration part 2 #94461

scottybollinger commented Mar 11, 2021

cee-chen left a comment

JasonStoltz left a comment •

edited

Loading

scottybollinger left a comment

scottybollinger commented Mar 15, 2021

scottybollinger commented Mar 15, 2021 •

edited

Loading

cee-chen left a comment •

edited

Loading

kibanamachine commented Mar 16, 2021

kibanamachine commented Mar 16, 2021

[App Search] Role mappings migration part 2 #94461

[App Search] Role mappings migration part 2 #94461

Conversation

scottybollinger commented Mar 11, 2021

Summary

Checklist

cee-chen left a comment

Choose a reason for hiding this comment

JasonStoltz left a comment • edited Loading

Choose a reason for hiding this comment

scottybollinger left a comment

Choose a reason for hiding this comment

scottybollinger commented Mar 15, 2021

scottybollinger commented Mar 15, 2021 • edited Loading

cee-chen left a comment • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Mar 16, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Mar 16, 2021

💚 Backport successful

JasonStoltz left a comment •

edited

Loading

scottybollinger commented Mar 15, 2021 •

edited

Loading

cee-chen left a comment •

edited

Loading