[Discover] Address reverting unsaved changes issue to unpin filters #172063

jughosta · 2023-11-28T13:45:09Z

Closes Pin across all apps on filter triggers unsaved changes badge #172025

Summary

This PR changes filters diffing logic for the "Unsaved changes" badge and fixes "Revert changes" action of it. What to expect when testing:

If user pins a filter and the order of filters does not change, then the badge should not appear.
If user has 2 filters for example and pins the second one, then this action would change filters order and the badge would appear.
If user presses "Revert changes" action, then newly pinned filters would become unpinned.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

src/plugins/discover/public/application/main/utils/update_saved_search.ts


    savedSearch.searchSource
      .setField('query', state.query ?? undefined)
-      .setField('filter', [...appFilters, ...globalFilters]);
+      .setField('filter', [...appFilters, ...cleanedGlobalFilters]);


src/plugins/discover/public/application/main/services/discover_saved_search_container.ts

…_saved_search_container.ts

src/plugins/discover/public/application/main/services/discover_saved_search_container.ts

…_saved_search_container.ts

jughosta · 2023-11-30T16:05:34Z

src/plugins/discover/public/application/main/utils/update_saved_search.ts

@@ -52,7 +52,7 @@ export function updateSavedSearch({

    savedSearch.searchSource
      .setField('query', state.query ?? undefined)
-      .setField('filter', [...appFilters, ...globalFilters]);
+      .setField('filter', [...globalFilters, ...appFilters]);


In UI pinned filters are rendered first, then app filters. So we better pass them to Elasticsearch in the same order.

Following up about this topic, I just wanted to add some information, because it seems the order of query / filters doesn't matter from ES side, according to https://www.elastic.co/de/blog/elasticsearch-query-execution-order

Q: Does the order in which I put my queries/filters in the query DSL matter?
A: No, because they will be automatically reordered anyway based on their respective costs and match costs.

Thanks, @kertal! Then it's just helpful for the diffing logic.

elasticmachine · 2023-11-30T16:06:27Z

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

davismcphee

The code changes look good and it works as described in the summary, but I'm not sure I understand why a change in filter order should trigger the unsaved changes badge if it doesn't impact the request?

davismcphee · 2023-12-02T00:23:19Z

x-pack/test_serverless/functional/test_suites/common/discover/group3/_unsaved_changes_badge.ts

Thanks for adding to serverless too!

jughosta · 2023-12-04T11:59:10Z

@davismcphee

The code changes look good and it works as described in the summary, but I'm not sure I understand why a change in filter order should trigger the unsaved changes badge if it doesn't impact the request?

It's not a change for the request but it is a visual change in the UI. There could be another case when user adds a filter and disables it right away - also it would not impact the request but it's a change nevertheless. So I would rather show the badge then and let the user to revert modifications.

davismcphee · 2023-12-04T20:50:51Z

It's not a change for the request but it is a visual change in the UI. There could be another case when user adds a filter and disables it right away - also it would not impact the request but it's a change nevertheless. So I would rather show the badge then and let the user to revert modifications.

I can understand the thinking here, and maybe this is more in line with most user's expectations, but as a user I found it difficult to understand why sometimes pinning a filter would trigger unsaved changes and other times not. To phrase it better, I think more importantly than if it affects the request would be if it affects the saved state of the current saved search. The pinned states and order of filters don't, but a disabled filter would, which is why I view these situations differently.

Since @bhavyarm reported the issue, maybe we could get some feedback on what you think would make more sense for users? Pinning/unpinning filters doesn't affect the saved search state, but it will sometimes change the visual order of the filters in the UI. Do you think it makes sense to show the unsaved changes badge only when a pinning/unpinning action changes the visual order of filters, or do you think it would be better to always ignore pinning/unpinning since it doesn't affect the underlying state?

We can also quickly validate with the team tomorrow in our sync. If others agree that the visual order change should trigger the badge, I'm happy to move forward with merging this.

kibana-ci · 2023-12-07T12:36:51Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: b2231b1

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #67 / security APIs - Audit Log Audit Log logs audit events when reading and writing saved objects

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`	589.7KB	590.0KB	+240.0B

History

💔 Build #181265 failed e4b70f0
💚 Build #180569 succeeded 2d40a72
💚 Build #180521 succeeded 587f1d8
💔 Build #180488 failed ab11287
💚 Build #180420 succeeded 1d9daa4

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

cc @jughosta

jughosta · 2023-12-08T15:13:12Z

I think it can be still worth merging as the PR provides an improvement and fixes the "Revert" action. We could revisit whether the filters order should trigger the badge or not later based on users feedback.

davismcphee

I think it can be still worth merging as the PR provides an improvement and fixes the "Revert" action. We could revisit whether the filters order should trigger the badge or not later based on users feedback.

I agree 👍 We discussed this as a team, and while I think it would be ideal if filter order didn't matter, there are some technical constraints that make this more difficult, so I think this is fine for now. It solves the more significant issue of the revert not working, and we can revisit later if needed. LGTM, thanks!

…lastic#172063) - Closes elastic#172025 ## Summary This PR changes filters diffing logic for the "Unsaved changes" badge and fixes "Revert changes" action of it. What to expect when testing: - If user pins a filter and the order of filters does not change, then the badge should not appear. - If user has 2 filters for example and pins the second one, then this action would change filters order and the badge would appear. - If user presses "Revert changes" action, then newly pinned filters would become unpinned. ### 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 (cherry picked from commit c89c990)

kibanamachine · 2023-12-08T21:51:05Z

💚 All backports created successfully

Status	Branch	Result
✅	8.12

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…lters (#172063) (#172999) # Backport This will backport the following commits from `main` to `8.12`: - [[Discover] Address reverting unsaved changes issue to unpin filters (#172063)](#172063)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Julia Rechkunova <[email protected]>

bhavyarm · 2023-12-11T15:12:44Z

@davismcphee I am so sorry I missed your ping here. Thanks very much :) I read through the comments and the fix looks good to me. Thanks @jughosta.

[Discover] Address reverting unsaved changes issue

faf475a

jughosta added the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label Nov 28, 2023

jughosta self-assigned this Nov 28, 2023

jughosta commented Nov 28, 2023

View reviewed changes

jughosta added 8 commits November 29, 2023 16:19

Merge branch 'main' into 172025-unpin-filters-when-reverting-changes

8d4cda5

[Discover] Improve filters comparison

f657e2c

[Discover] Simplify the logic

5e7b528

[Discover] Revert some changes

2d0f382

Merge branch 'main' into 172025-unpin-filters-when-reverting-changes

1d9daa4

[Discover] Add a test and fix how global filters are applied

6df1795

[Discover] Fix deps

ab11287

[Discover] Update tests

587f1d8

jughosta commented Nov 30, 2023

View reviewed changes

src/plugins/discover/public/application/main/services/discover_saved_search_container.ts Outdated Show resolved Hide resolved

Update src/plugins/discover/public/application/main/services/discover…

5ed4a64

…_saved_search_container.ts

jughosta commented Nov 30, 2023

View reviewed changes

src/plugins/discover/public/application/main/services/discover_saved_search_container.ts Outdated Show resolved Hide resolved

Update src/plugins/discover/public/application/main/services/discover…

2d40a72

…_saved_search_container.ts

jughosta commented Nov 30, 2023

View reviewed changes

jughosta added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Nov 30, 2023

jughosta marked this pull request as ready for review November 30, 2023 16:06

jughosta requested a review from a team as a code owner November 30, 2023 16:06

jughosta requested a review from bhavyarm November 30, 2023 16:08

davismcphee reviewed Dec 2, 2023

View reviewed changes

Merge branch 'main' into 172025-unpin-filters-when-reverting-changes

e4b70f0

Merge branch 'main' into 172025-unpin-filters-when-reverting-changes

b2231b1

jughosta removed the backport:skip This commit does not require backporting label Dec 7, 2023

jughosta added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Dec 7, 2023

davismcphee approved these changes Dec 8, 2023

View reviewed changes

jughosta merged commit c89c990 into elastic:main Dec 8, 2023
40 checks passed

jughosta deleted the 172025-unpin-filters-when-reverting-changes branch December 8, 2023 21:45

kibanamachine added the v8.13.0 label Dec 8, 2023

kibanamachine mentioned this pull request Dec 8, 2023

[8.12] [Discover] Address reverting unsaved changes issue to unpin filters (#172063) #172999

Merged

kibanamachine added the v8.12.0 label Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Address reverting unsaved changes issue to unpin filters #172063

[Discover] Address reverting unsaved changes issue to unpin filters #172063

jughosta commented Nov 28, 2023 •

edited by kibanamachine

Loading

This comment was marked as outdated.

jughosta Nov 30, 2023

kertal Nov 30, 2023

jughosta Nov 30, 2023

elasticmachine commented Nov 30, 2023

davismcphee left a comment

davismcphee Dec 2, 2023

jughosta commented Dec 4, 2023

davismcphee commented Dec 4, 2023

kibana-ci commented Dec 7, 2023

jughosta commented Dec 8, 2023

davismcphee left a comment

kibanamachine commented Dec 8, 2023

bhavyarm commented Dec 11, 2023

[Discover] Address reverting unsaved changes issue to unpin filters #172063

[Discover] Address reverting unsaved changes issue to unpin filters #172063

Conversation

jughosta commented Nov 28, 2023 • edited by kibanamachine Loading

Summary

Checklist

This comment was marked as outdated.

jughosta Nov 30, 2023

Choose a reason for hiding this comment

kertal Nov 30, 2023

Choose a reason for hiding this comment

jughosta Nov 30, 2023

Choose a reason for hiding this comment

elasticmachine commented Nov 30, 2023

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Dec 2, 2023

Choose a reason for hiding this comment

jughosta commented Dec 4, 2023

davismcphee commented Dec 4, 2023

kibana-ci commented Dec 7, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

jughosta commented Dec 8, 2023

davismcphee left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 8, 2023

💚 All backports created successfully

Questions ?

bhavyarm commented Dec 11, 2023

jughosta commented Nov 28, 2023 •

edited by kibanamachine

Loading