Management API - redirect on disabled app path by mattkime · Pull Request #55136 · elastic/kibana

mattkime · 2020-01-16T23:15:08Z

Summary

Previously a disabled app didn't show in the side nav but its path was still available to the user. This PR fixes that, but it will still be up to management apps to handle the case where a user is already viewing an app when its disabled.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes [i18n support]~~(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

elasticmachine · 2020-01-16T23:22:31Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

streamich

LGTM. One comment below that needs addressing.

streamich · 2020-01-17T09:08:04Z

src/plugins/management/public/management_app.tsx

+            params.element
+          );
+        }
        return async () => {


Seems appUnmount() and ReactDOM.unmountComponentAtNode(params.element) will be called even if React was not mounted. Maybe you could do an early return

if (!this.enabledStatus) { window.location.hash = '/management'; // Early return return () => {}; }

and leave the rest of the function as before.

azasypkin

Works as expected for Security plugin, thanks! Just two optional nits and +1 to comment about appUnmount that may be undefined now.

azasypkin · 2020-01-17T11:59:03Z

src/plugins/management/public/management_app.tsx

  readonly order: number;
  readonly mount: ManagementSectionMount;
-  protected enabledStatus: boolean = true;
+  private enabledStatus: boolean = true;


optioanal nit: I believe :boolean is not needed as the correct type is derived from the initial value.

azasypkin · 2020-01-17T12:05:15Z

src/plugins/management/public/management_app.tsx

+        if (!this.enabledStatus) {
+          window.location.hash = '/management';
+        } else {
+          async function setBreadcrumbs(crumbs: ChromeBreadcrumb[]) {


optional nit: it was here before, so feel free to ignore, but technically we can move setBreadcrumbs function one level up to create it only once per app (or even make it a private class method assuming getStartServices can become a private readonly as well).

I'll likely address this in a future refactor. Gave it a try but it twas changing a bit more code than I'd like for this PR.

kibanamachine · 2020-01-17T18:43:17Z

💚 Build Succeeded

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

History

💔 Build #21106 failed f3925da
💚 Build #21089 succeeded 9447514
💚 Build #20979 succeeded 4714152

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

* redirect on disabled management app path

* upstream/master: (24 commits) Show error page when accessing unavailable app (elastic#54656) [ML] Improving job wizards with datafeed aggregations (elastic#55180) remove flaly assetion. a license presence tested anyway (elastic#55289) fix commonly used ranges uptime (elastic#54930) [SIEM] Use proper icons on Detections view (elastic#55215) Fix: invalid translation referenced (elastic#54901) [State Management] Remove AppState from edit_index_pattern page (elastic#54104) Implements `getStartServices` on server-side (elastic#55156) Move vis_vega_type/data_model tests to jest (elastic#55186) [SIEM] [Detection Engine] Update status on rule details page (elastic#55201) Fix KQL value suggestions for nested fields (elastic#54820) Enforce camelCase format for a plugin id (elastic#53759) [SIEM] Detection engine cleanup for rule details/creation/edit page (elastic#55069) Remove nested root from index pattern (elastic#54978) [Reporting/Migration] ReportingSetup, LegacySetup (elastic#54198) [SIEM] [Detection Engine] Fixes duplicate rule action (elastic#55252) [SIEM] Detections add alert & signal tab (elastic#55127) Management API - redirect on disabled app path (elastic#55136) [SIEM][Detection Engine] Fixes critical regression on the backend with immutable and tags update local (elastic#55177) ...

* master: (108 commits) [ML] Single Metric Viewer: Fix job check. (elastic#55191) Show error page when accessing unavailable app (elastic#54656) [ML] Improving job wizards with datafeed aggregations (elastic#55180) remove flaly assetion. a license presence tested anyway (elastic#55289) fix commonly used ranges uptime (elastic#54930) [SIEM] Use proper icons on Detections view (elastic#55215) Fix: invalid translation referenced (elastic#54901) [State Management] Remove AppState from edit_index_pattern page (elastic#54104) Implements `getStartServices` on server-side (elastic#55156) Move vis_vega_type/data_model tests to jest (elastic#55186) [SIEM] [Detection Engine] Update status on rule details page (elastic#55201) Fix KQL value suggestions for nested fields (elastic#54820) Enforce camelCase format for a plugin id (elastic#53759) [SIEM] Detection engine cleanup for rule details/creation/edit page (elastic#55069) Remove nested root from index pattern (elastic#54978) [Reporting/Migration] ReportingSetup, LegacySetup (elastic#54198) [SIEM] [Detection Engine] Fixes duplicate rule action (elastic#55252) [SIEM] Detections add alert & signal tab (elastic#55127) Management API - redirect on disabled app path (elastic#55136) [SIEM][Detection Engine] Fixes critical regression on the backend with immutable and tags ...

redirect on disabled management app path

4714152

mattkime changed the title ~~redirect on disabled management app path~~ Management API - redirect on disabled app path Jan 16, 2020

mattkime requested a review from streamich January 16, 2020 23:18

mattkime added v8.0.0 v7.6.0 release_note:skip Skip the PR/issue when compiling release notes Team:AppArch labels Jan 16, 2020

mattkime requested a review from azasypkin January 16, 2020 23:26

mattkime marked this pull request as ready for review January 16, 2020 23:26

mattkime requested a review from a team as a code owner January 16, 2020 23:26

streamich approved these changes Jan 17, 2020

View reviewed changes

azasypkin approved these changes Jan 17, 2020

View reviewed changes

mattkime added 4 commits January 17, 2020 09:33

early return

9447514

use navigateToApp

f3925da

fix functional test

be4490f

Update management_plugin.js

e8a02ab

azasypkin mentioned this pull request Jan 17, 2020

Migrate Management views to Kibana Platform plugin #53880

Merged

mattkime merged commit c1ccb30 into elastic:master Jan 17, 2020

mattkime mentioned this pull request Jan 17, 2020

[7.x] Management API - redirect on disabled app path (#55136) #55258

Merged

mattkime added a commit to mattkime/kibana that referenced this pull request Jan 17, 2020

Management API - redirect on disabled app path (elastic#55136)

6ce837b

* redirect on disabled management app path

mattkime added a commit to mattkime/kibana that referenced this pull request Jan 17, 2020

Management API - redirect on disabled app path (elastic#55136)

f838eca

* redirect on disabled management app path

mattkime mentioned this pull request Jan 17, 2020

[7.6] Management API - redirect on disabled app path (#55136) #55259

Merged

mattkime added a commit that referenced this pull request Jan 18, 2020

Management API - redirect on disabled app path (#55136) (#55259)

f7f28b7

* redirect on disabled management app path

mattkime added a commit that referenced this pull request Jan 18, 2020

Management API - redirect on disabled app path (#55136) (#55258)

587f64f

* redirect on disabled management app path

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Management API - redirect on disabled app path#55136

Management API - redirect on disabled app path#55136
mattkime merged 5 commits intoelastic:masterfrom
mattkime:management_api_fix_disabled_path

mattkime commented Jan 16, 2020 •

edited

Loading

Uh oh!

elasticmachine commented Jan 16, 2020

Uh oh!

streamich left a comment

Uh oh!

streamich Jan 17, 2020

Uh oh!

azasypkin left a comment

Uh oh!

azasypkin Jan 17, 2020

Uh oh!

azasypkin Jan 17, 2020

Uh oh!

mattkime Jan 17, 2020

Uh oh!

kibanamachine commented Jan 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

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

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Jan 16, 2020

Uh oh!

streamich left a comment

Choose a reason for hiding this comment

Uh oh!

streamich Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

azasypkin Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

azasypkin Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

mattkime Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 17, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mattkime commented Jan 16, 2020 •

edited

Loading