[Discover] Fix "Unsaved changes" badge for ES|QL #174645
Conversation
jughosta
added
Feature:Discover
|
/ci |
|
/ci |
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
| @@ -318,7 +318,9 @@ function getSearchSourceFieldValueForComparison( | |||
| searchSourceFieldName: keyof SearchSourceFields | |||
| ) { | |||
| if (searchSourceFieldName === 'index') { | |||
| return searchSource.getField('index')?.id; | |||
| const query = searchSource.getField('query'); | |||
| // ad-hoc data view id can change, so we rather compare the ES|QL query itself here | |||
There was a problem hiding this comment.
this makes sense, I've got one question, when does the id change in ES|QL and the query.esql doesn't? A quick thought, maybe, when initializing the ad-hoc data view, we could use the used index-pattern (with a prefix) as id. This could also maybe fix the problem of having multiple data views with the same index pattern on a dashboard. So ids wouldn't be random but derived from the used index pattern
There was a problem hiding this comment.
@kertal I have not found yet why/when it changes
There was a problem hiding this comment.
maybe if you change the index pattern in the ES|QL query, and then go back to the previous query. Then it will high likely get a new ID
So if we would assign an id derived by the used index pattern here, then it should be stable (it's not something for this PR, but I'm now continuing thinking, because this might also fix the problem of having multiple data views on a dashboard with the same index pattern) ... FYI @stratoula ... is there a reason not to use a data view id generated by the index pattern of the ES|QL query? like esql-{indexpattern} or esql-{indexpatternHash}
There was a problem hiding this comment.
This can actually work Matthias, very good idea 👏
There was a problem hiding this comment.
I can give it a try tomorrow to see how it will work!
|
@stratoula Sure! If your fix can be also backported to 8.12 then it would be great! |
|
I would prefer to not backport it, I want to merge it and have this tested (I did the same change for Lens charts). ES|QL is on tech preview so it is fine if there are some bugs before GA. |
|
@stratoula Okay, then it still makes sense to backport this PR to fix the badge which we introduced in 8.12. |
|
Yes this makes sense 👍 Checking your code, I like your change. It makes more sense to check the query rather than the id for ES|QL (taken under consideration that ES|QL works without dataviews). So on second thoughts I feel we should proceed with this change in general. Wdyt? |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @jughosta |
## Summary This PR fixes a bug where "Unsaved changes" badge would appear after page refresh for an ES|QL saved search. To reproduce on main: - Create a new ES|QL saved search - Reload the page => Notice that the badge appeared With this PR the issue should be resolved. It was caused by the fact that adhoc data view id might change internally. ### 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 900ab21)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…74645) (#174832) # Backport This will backport the following commits from `main` to `8.12`: - [[Discover] Fix "Unsaved changes" badge for ES|QL (#174645)](#174645) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Julia Rechkunova","email":"julia.rechkunova@elastic.co"},"sourceCommit":{"committedDate":"2024-01-15T11:12:56Z","message":"[Discover] Fix \"Unsaved changes\" badge for ES|QL (#174645)\n\n## Summary\r\n\r\nThis PR fixes a bug where \"Unsaved changes\" badge would appear after\r\npage refresh for an ES|QL saved search.\r\n\r\nTo reproduce on main:\r\n- Create a new ES|QL saved search\r\n- Reload the page => Notice that the badge appeared\r\n\r\nWith this PR the issue should be resolved. It was caused by the fact\r\nthat adhoc data view id might change internally.\r\n\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","sha":"900ab217a2cba446c923ac3bd46795d6c70fe106","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","Team:DataDiscovery","backport:prev-minor","v8.13.0"],"title":"[Discover] Fix \"Unsaved changes\" badge for ES|QL","number":174645,"url":"#174645 Fix \"Unsaved changes\" badge for ES|QL (#174645)\n\n## Summary\r\n\r\nThis PR fixes a bug where \"Unsaved changes\" badge would appear after\r\npage refresh for an ES|QL saved search.\r\n\r\nTo reproduce on main:\r\n- Create a new ES|QL saved search\r\n- Reload the page => Notice that the badge appeared\r\n\r\nWith this PR the issue should be resolved. It was caused by the fact\r\nthat adhoc data view id might change internally.\r\n\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","sha":"900ab217a2cba446c923ac3bd46795d6c70fe106"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"#174645 Fix \"Unsaved changes\" badge for ES|QL (#174645)\n\n## Summary\r\n\r\nThis PR fixes a bug where \"Unsaved changes\" badge would appear after\r\npage refresh for an ES|QL saved search.\r\n\r\nTo reproduce on main:\r\n- Create a new ES|QL saved search\r\n- Reload the page => Notice that the badge appeared\r\n\r\nWith this PR the issue should be resolved. It was caused by the fact\r\nthat adhoc data view id might change internally.\r\n\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","sha":"900ab217a2cba446c923ac3bd46795d6c70fe106"}}]}] BACKPORT--> Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
|
This PR didn't make it into the latest BC for 8.12.0. Updating the labels. |
## Summary This PR fixes a bug where "Unsaved changes" badge would appear after page refresh for an ES|QL saved search. To reproduce on main: - Create a new ES|QL saved search - Reload the page => Notice that the badge appeared With this PR the issue should be resolved. It was caused by the fact that adhoc data view id might change internally. ### 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
Summary
This PR fixes a bug where "Unsaved changes" badge would appear after page refresh for an ES|QL saved search.
To reproduce on main:
With this PR the issue should be resolved. It was caused by the fact that adhoc data view id might change internally.
Checklist