-
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
[Dashboard] Add Analytics No Data Page #132188
Conversation
…and dashboard listing page
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
LGTM
reviewed app services change + ran locally
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.
Tested locally + code review - one tiny question but otherwise works great! 👍
// loadTestFile(require.resolve('./dashboard_time')); | ||
// loadTestFile(require.resolve('./dashboard_listing')); | ||
// loadTestFile(require.resolve('./dashboard_clone')); |
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.
Is there a reason these other tests are commented out?
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.
🤭
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 commented them out just for testing purposes. That would have been bad. Thank you for catching this.
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.
Let's not merge this until the issue with permissions is resolved, I'll keep you updated.
return true; | ||
} | ||
|
||
const defaultDataView = await dataViews.getDefaultDataView(); |
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.
is this check necessary for Dashboard?
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.
That's a good question. The older version of this code had an ensureDefaultDataView
check, so it seems to have been required before, but I can't exactly understand why. If there are any data views, surely one of them should be default, no?
Is there a reason this was required for Discover?
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.
if memory serves well, in Discover, you save a saved search with a data view. if all data views get deleted and you try opening a saved search, that would throw an error. That's why the check. That being said, it was there before checking for existence of any data view, so I am not sure if it's relevant any more.
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.
Seems like it should all work the same. I will remove this from the dashboard side!
); | ||
}; | ||
|
||
export const isDashboardAppEmpty = async (dataViews: DataPublicPluginStart['dataViews']) => { |
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.
just a nit: we're trying to standardize terminology in a way that by "no data" we refer to "there is no data in ES or there are no data views". by "empty" we refer to "this app is empty", aka there is data, but there are no dashboards yet. can we rename this to something like "noDataCheck" or "hasData" or even "canUseDashboard"?
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.
Ah, I will rename all of these. Naming is important, thank you for pointing this out!
@@ -47,7 +47,7 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) { | |||
indices: [ | |||
{ | |||
names: ['ecommerce', 'kibana_sample_data_ecommerce'], | |||
privileges: ['read'], | |||
privileges: ['read', 'view_index_metadata'], |
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.
Let's not change permissions here as this is a valid test case that caught a pretty big error.
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.
Aye aye, will undo this now that the fix is merged in!
The Can you try removing the additional privilege in the functional test and see if this works? |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Unknown metric groupsasync chunk count
miscellaneous assets size
History
To update your PR or re-run it, just comment with: |
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 pushed a commit that re-introduces a check for default data view. That actually does more than just check for one; it loads a default data view if there is none. This is needed in case of switching spaces, where a wrong data view could stay associated with the dashboard and dashboard would try to load the data from the data view from a previous space (which doesn't exist in a current space). Sorry for leading you down the wrong path.
Also approving this for no data changes, so that it doesn't block merging.
Fixes #132144
Summary
Adopts the Shared UX no data component in the Dashboard listing page, and in the dashboard app.
Add integrations callout in dashboard
Creating a Data View from the create Dashboard route
Creating a Data View from the Dashboard Listing route
Checklist
Delete any items that are not applicable to this PR.