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

Kibana doesn't warn the user about panels losing their drilldowns when users edit viz in lens and do save and return #147646

Open
bhavyarm opened this issue Dec 15, 2022 · 11 comments
Labels
enhancement New value added to drive a business result Feature:Drilldowns Embeddable panel Drilldowns Feature:Lens Feature:Vis Editor Visualization editor issues impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@bhavyarm
Copy link
Contributor

Kibana version: 8.6.0 BC7

Elasticsearch version: 8.6.0 BC7

Server OS version: darwin_x86_64

Browser version: chrome latest

Browser OS version: OS X

Original install method (e.g. download page, yum, from source, etc.): from staging

Describe the bug: Lens doesn't display any message to user about their panels losing their drill-downs once user navigates to edit in lens from a panel and clicks save and return to dashboard.

Steps to reproduce:

  1. Make sure to have a panel with either classic aggregation or TSVB viz with a drilldown in dashboard
  2. Click edit on dashboard -> visualize->edit visualization in lens
  3. Click save and return to dashboard
  4. That panel has lost the drilldown

Expected behavior: Kibana should tell the user about them losing drilldowns.

Panel with drilldowns:
Screen Shot 2022-12-15 at 3 25 22 PM

Same panel in lens:
Screen Shot 2022-12-15 at 3 25 50 PM

Panel back in dashboard with drilldowns missing:
Screen Shot 2022-12-15 at 3 25 55 PM

@bhavyarm bhavyarm added bug Fixes for quality problems that affect the customer experience Feature:Vis Editor Visualization editor issues Feature:Lens Feature:Drilldowns Embeddable panel Drilldowns labels Dec 15, 2022
@botelastic botelastic bot added the needs-team Issues missing a team label label Dec 15, 2022
@bhavyarm bhavyarm added Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed needs-team Issues missing a team label labels Dec 15, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

@stratoula stratoula added enhancement New value added to drive a business result and removed bug Fixes for quality problems that affect the customer experience labels Dec 15, 2022
@stratoula
Copy link
Contributor

I am changing this to enhancement because it needs some discussion.

@elastic/kibana-presentation as you know, we allow the users to translate their legacy visualizations to Lens. These are replaced in the dashboard as by value visualizations. Can we somehow keep the drilldowns set?

I don't want to add a warning because I want this to be an appealing path for the users. So I would like to explore first if there is a way to do the replacement but keep the drilldowns.

@stratoula stratoula added the impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. label Dec 19, 2022
@ThomThomson
Copy link
Contributor

I think it should be completely possible to retain drilldowns on panel type change. The only complexity I can think of with this is the potential for the new type of panel to not support all of the same triggers - but as far as I can tell that shouldn't happen, due to Lens and TSVB being mostly 1 to 1 in their functionality. @stratoula is that the case?

If so, I think it's relatively low risk for us to just spread the enhancements on the embeddable even when changing types.

@stratoula
Copy link
Contributor

Yes @ThomThomson this is correct, Lens supports more drilldowns than the legacy visualizations (the same + discover drilldown) so it is safe to also transfer them to the replaced embeddable.

@stratoula
Copy link
Contributor

@ThomThomson the same applies for custom timerange, right? Does it require changes from your side too?

@ThomThomson
Copy link
Contributor

TimeRange is a strange one. When I edit a lens panel with a custom time range in Lens, we do pass it to Lens in the valueInput key of the state transfer service.

You can see this in embeddableEditorIncomingState in x-pack/plugins/lens/public/app_plugin/mounter.tsx. But by the time we get the Lens embeddable back the timeRange is missing. You can see after save and return in incomingEmbeddable in src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx ~line 272. The only reason the time range is retained in this case is because for same type embeddables, the older input gets spread (line 286).

Ideally, all input that gets passed into the editor should be returned on save and return. That way it can be updated properly.

@stratoula
Copy link
Contributor

@ThomThomson I could make the custom timerange to work for all cases expect from one

When I have a saved TSVB visualization --> Edit in TSVB --> Edit in Lens --> Save and return

At this point I dont have on the visualize editor the input for some reason. Here is the useEffect that I retrieve the embeddable input for the by ref visualizations https://github.com/elastic/kibana/blob/main/src/plugins/visualizations/public/visualize_app/components/visualize_editor.tsx#L76

Do you have a clue why I dont have this at this point?

Here is my PR #155113

@ThomThomson
Copy link
Contributor

Looks like we don't provide the valueInput for a by reference TSVB / visualization because on the embeddable side there is an explicit check for saved object id, and if the panel is by reference we don't add any input to the state transfer package at all.

Wondering out loud - would it mess anything up on the Lens or TSVB side if we included the valueInput all the time, even when editing the panel by reference? I did some preliminary testing with that condition removed and everything seemed to work fine with Visualize by reference editing, but I haven't covered every case.

@stratoula
Copy link
Contributor

Do you have a PR that enables that Devon? If yes I can also do some tests. I dont think it will cause any problem tbh.

@ThomThomson
Copy link
Contributor

ThomThomson commented Apr 19, 2023

I can open one that does that, sure! Edit - here's a draft PR

@dej611
Copy link
Contributor

dej611 commented Apr 26, 2023

Just noticed also tags are lost on Edit viz in Lens, when testing #155113

stratoula added a commit that referenced this issue Apr 27, 2023
#155113)

## Summary

Part of #147646

It passes the custom timerange to the converted Lens panel for both by
ref and by value legacy visualizations.
It works for all paths:
- Edit visualization--> Edit in Lens--> Replace in dashboard
- Convert to Lens --> Replace in dashboard


![2](https://user-images.githubusercontent.com/17003240/233287641-82fe190d-5b92-4368-ace8-0b576a46d32a.gif)

### Checklist

- [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: Marco Liberati <dej611@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Apr 27, 2023
elastic#155113)

## Summary

Part of elastic#147646

It passes the custom timerange to the converted Lens panel for both by
ref and by value legacy visualizations.
It works for all paths:
- Edit visualization--> Edit in Lens--> Replace in dashboard
- Convert to Lens --> Replace in dashboard

![2](https://user-images.githubusercontent.com/17003240/233287641-82fe190d-5b92-4368-ace8-0b576a46d32a.gif)

### Checklist

- [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: Marco Liberati <dej611@users.noreply.github.com>
(cherry picked from commit 61c82dc)
kibanamachine referenced this issue Apr 27, 2023
…d panel (#155113) (#155979)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Visualize2Lens] Transfers the custom timerange to the converted
panel (#155113)](#155113)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Stratoula
Kalafateli","email":"efstratia.kalafateli@elastic.co"},"sourceCommit":{"committedDate":"2023-04-27T09:11:35Z","message":"[Visualize2Lens]
Transfers the custom timerange to the converted panel (#155113)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/147646\r\n\r\nIt passes the
custom timerange to the converted Lens panel for both by\r\nref and by
value legacy visualizations.\r\nIt works for all paths:\r\n- Edit
visualization--> Edit in Lens--> Replace in dashboard\r\n- Convert to
Lens --> Replace in
dashboard\r\n\r\n\r\n![2](https://user-images.githubusercontent.com/17003240/233287641-82fe190d-5b92-4368-ace8-0b576a46d32a.gif)\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Liberati
<dej611@users.noreply.github.com>","sha":"61c82dc8682edc33bc7a8483befed136052d95dc","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","Feature:Visualizations","Team:Visualizations","Feature:Lens","backport:prev-minor","v8.8.0","v8.9.0"],"number":155113,"url":"https://github.com/elastic/kibana/pull/155113","mergeCommit":{"message":"[Visualize2Lens]
Transfers the custom timerange to the converted panel (#155113)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/147646\r\n\r\nIt passes the
custom timerange to the converted Lens panel for both by\r\nref and by
value legacy visualizations.\r\nIt works for all paths:\r\n- Edit
visualization--> Edit in Lens--> Replace in dashboard\r\n- Convert to
Lens --> Replace in
dashboard\r\n\r\n\r\n![2](https://user-images.githubusercontent.com/17003240/233287641-82fe190d-5b92-4368-ace8-0b576a46d32a.gif)\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Liberati
<dej611@users.noreply.github.com>","sha":"61c82dc8682edc33bc7a8483befed136052d95dc"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155113","number":155113,"mergeCommit":{"message":"[Visualize2Lens]
Transfers the custom timerange to the converted panel (#155113)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/147646\r\n\r\nIt passes the
custom timerange to the converted Lens panel for both by\r\nref and by
value legacy visualizations.\r\nIt works for all paths:\r\n- Edit
visualization--> Edit in Lens--> Replace in dashboard\r\n- Convert to
Lens --> Replace in
dashboard\r\n\r\n\r\n![2](https://user-images.githubusercontent.com/17003240/233287641-82fe190d-5b92-4368-ace8-0b576a46d32a.gif)\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Liberati
<dej611@users.noreply.github.com>","sha":"61c82dc8682edc33bc7a8483befed136052d95dc"}}]}]
BACKPORT-->

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Drilldowns Embeddable panel Drilldowns Feature:Lens Feature:Vis Editor Visualization editor issues impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

5 participants