-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Show error page when accessing unavailable app #54656
Conversation
expect(getComponent()).toMatchInlineSnapshot(` | ||
<AppRouter | ||
history={ | ||
Object { | ||
"push": [MockFunction], | ||
} | ||
} | ||
mounters={Map {}} | ||
setAppLeaveHandler={[Function]} | ||
/> | ||
`); | ||
expect(getComponent()).toMatchSnapshot(); |
There was a problem hiding this comment.
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.
window.__coreAppStatus = startContract; | ||
return startContract; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Pinging @elastic/kibana-platform (Team:Platform) |
…able-app-not-found
…able-app-not-found
cb(); | ||
}, s); | ||
}; | ||
|
||
const navigateToApp = async (i: string): Promise<{ error?: string }> => { | ||
const navigateToApp = async (i: string) => { | ||
return (await browser.executeAsync(async (appId, cb: Function) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary await
?
window.__coreAppStatus = startContract; | ||
return startContract; |
There was a problem hiding this comment.
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.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* 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
* 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 ...
Summary
Fix #54366
Propagate the applications current states from the
ApplicationService
to theAppRouter
to be able to display thenotFound
page when mounting an application that is not available.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor 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