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

[discover] Enable data view editing from flyout #149453

Merged
merged 14 commits into from
Jan 30, 2023

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Jan 24, 2023

Summary

Currently, changes to a data view require a round trip to management when you're in discover. This PR allows editing of data views via flyout from within discover.

Closes #144801

Checklist

Delete any items that are not applicable to this PR.

@mattkime mattkime changed the title enable edit data view from flyout [discover] Enable data view editing from flyout Jan 24, 2023
@mattkime mattkime added v8.7.0 Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. release_note:skip Skip the PR/issue when compiling release notes and removed Feature:Data Views Data Views code and UI - index patterns before 8.0 labels Jan 24, 2023
@@ -289,8 +289,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await testSubjects.click('indexPattern-manage-field');
await PageObjects.header.waitUntilLoadingHasFinished();

const titleElem = await testSubjects.find('currentIndexPatternTitle');
expect(await titleElem.getVisibleText()).to.equal(dataView);
const titleElem = await testSubjects.find('createIndexPatternTitleInput');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests change because its opening the flyout instead of navigating to data view management.

@mattkime mattkime marked this pull request as ready for review January 25, 2023 04:19
@mattkime mattkime requested a review from a team as a code owner January 25, 2023 04:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@mattkime mattkime requested a review from kertal January 26, 2023 23:44
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Looks nice and green! What currently doesn't work is, when renaming the data view, the change isn't displayed in the data view select button

Kapture.2023-01-27.at.14.15.33.mp4

Might be the case that when a data view is changed, it's mutated, so the reference to the object doesn't change? because this wouldn't trigger an update then

I might be wrong, but I think it worked in #148283


// refetch data view to force react state update
// by creating new data view reference in the state
dataViews.clearInstanceCache(dataViewId);
stateContainer.actions.setDataView(await dataViews.get(dataViewId));

However, if there's an option to clone the data view on edit, I think it would also work?

@mattkime
Copy link
Contributor Author

mattkime commented Jan 28, 2023

@kertal The issue I saw and fixed was a bit different from the one you show in the recording. Either its ready to go OR there's something I've failed to reproduce.

Screen.Recording.2023-01-27.at.7.39.21.PM.mov

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Pulled and tested locally, and works as expected. Whatever the issue was with the selector seems to be working now. LGTM!

@kertal kertal self-requested a review January 30, 2023 07:20
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thx for bringing this over the finishing line. One thing that IMO is worth an issue, when changing the index pattern of the data view, I think the fields are not refetched, which leads to unmapped fields when e.g. switching from "commerce" to "flights" index pattern
Bildschirmfoto 2023-01-30 um 11 20 39
However, before users are doing so, they are warned, that this kind of change can lead to breaking changes. So this should not block this PR, but I think it shouldn't be a big effort to fix this, and I bet once users have to option the change the data view with a flyout, they will use it more often to apply changes like this

@kertal
Copy link
Member

kertal commented Jan 30, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 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
discover 376.1KB 376.3KB +127.0B

History

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

@mattkime mattkime merged commit a64027d into elastic:main Jan 30, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 30, 2023
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
## Summary

Currently, changes to a data view require a round trip to management
when you're in discover. This PR allows editing of data views via flyout
from within discover.

Closes elastic#144801


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Matthias Wilhelm <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Allow data view editing without switching the management
6 participants