Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show error page when accessing unavailable app #54656

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jan 13, 2020

Summary

Fix #54366

Propagate the applications current states from the ApplicationService to the AppRouter to be able to display the notFound page when mounting an application that is not available.

Checklist

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

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

expect(getComponent()).toMatchInlineSnapshot(`
<AppRouter
history={
Object {
"push": [MockFunction],
}
}
mounters={Map {}}
setAppLeaveHandler={[Function]}
/>
`);
expect(getComponent()).toMatchSnapshot();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AppRouter now have an observable as a property. This causes the snapshot to be incredibly long, as all inner properties of the observer are rendered. I would love to be able to exclude some properties from the snapshot, but that doesn't seem doable, so I moved from inline to file snapshot.

Comment on lines +67 to +68
window.__coreAppStatus = startContract;
return startContract;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to be able to test NP to NP app navigation. As we are not loading LP plugins in non-legacy mode, I can't use the core_provider_plugin plugin anymore and I'm forced to expose my own API to the global scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add buttons to click on instead, but this is probably less flaky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for navigateToApp I could (even if for core FT I think I prefer using directly our APIs), but not for the setAppStatus I'm also using

@pgayvallet pgayvallet added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.7.0 v8.0.0 labels Jan 15, 2020
@elasticmachine
Copy link
Contributor

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

@pgayvallet pgayvallet marked this pull request as ready for review January 15, 2020 14:16
@pgayvallet pgayvallet requested a review from a team as a code owner January 15, 2020 14:16
cb();
}, s);
};

const navigateToApp = async (i: string): Promise<{ error?: string }> => {
const navigateToApp = async (i: string) => {
return (await browser.executeAsync(async (appId, cb: Function) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary await?

Comment on lines +67 to +68
window.__coreAppStatus = startContract;
return startContract;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add buttons to click on instead, but this is probably less flaky.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@pgayvallet pgayvallet merged commit de7a22d into elastic:master Jan 20, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jan 20, 2020
* display not found page instead of throwing an error when accessible unavailable app

* move types to public folder

* fix types import

* remove updater from start app

* remove unnecessary await
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
* 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)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
* 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
  ...
pgayvallet added a commit that referenced this pull request Jan 20, 2020
* display not found page instead of throwing an error when accessible unavailable app

* move types to public folder

* fix types import

* remove updater from start app

* remove unnecessary await
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ApplicationService: inaccessible applications improvements
4 participants