[8.6] [Dashboard] [Controls] Allow control changes to be discarded (#147482) #147819
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…#147482) Closes elastic#147293 ## Summary Before this change, the Redux state `explicitInput` was getting out of sync with the embeddable `explicitInput` in scenarios where the new `explicitInput` was missing a key that the old `explicitInput` had - therefore, because they were out of sync, the changes that **should** have been discarded kept getting injected back into the embeddable `explicitInput`, which made it impossible to actually discard anything unless the key existed in both the before and after state. This PR fixes this by replacing the entire Redux state `explicitInput` with the embeddable `explicitInput` rather than spreading the new value. It also fixes a bug with the time slider control where changes to the embeddable's input were not reflected properly in the control's state, so nothing could be discarded even after the initial bug was fixed. #### Further Explanation When a control is first created, all the optional properties of the explicit input do not yet exist - for example, when creating an options list control, the `selections` key does not exist in the `explicitInput` until a selection is made. Therefore, imagine the following scenario: 1. You create an options list control (where the `selections` key does not exist) and save the dashboard 2. You make some selections, which causes `unsaved changes` because the `selections` key now exists and is equal to an array 3. You switch to view mode and choose to discard your changes, thus (supposedly) removing the `selections` key from the `explicitInput` object once again Unfortunately, the Redux embeddable state for each control was **not** accurately removing the `selections` key as expected - this was because, when trying to update the `explicitInput` via the old `updateEmbeddableReduxInput`, the new value was **spread** on top of the older value rather than replacing it. In a simplified scenario, this resulted in something like this: ```typescript const oldExplicitInput = { id: 'test_id', selections: ['test selection'] }; const newExplicitInput = { id: 'test_id' } const result = { ...oldExplicitInput, ...newExplicitInput }; ``` In this code, because `newExplicitInput` does not have the `selections` key, `result` will equal `{ id: 'test_id', selections: ['test selection'] }` - this is not the behaviour we want! Instead, we wanted to replace the entire old `explicitInput` with the new `explicitInput`. Effectively, that is what this PR does. Thanks to @ThomThomson for helping out with finding the root cause of this after I got lost :) ### How to Test For both options list and range slider controls, 1. Create a control of the desired type 2. Save the dashboard 3. Make some sort of change that causes unsaved changes - for example, make a selection or, if an options list control, set `exclude` to `true` 4. Switch to view mode, discarding the changes 5. Ensure that the changes made in step 3 are no longer applied ✅ 6. Switch back to edit mode 7. Ensure that there are no `unsaved changes` ✅ #### Flaky Test Runner <a href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1649"><img src="https://user-images.githubusercontent.com/8698078/207701101-69cdfada-77c6-4510-b254-1fd1fa13af5c.png"/></a> ### 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 - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 7a6eac8) # Conflicts: # src/plugins/controls/public/time_slider/embeddable/time_slider_embeddable.tsx
![https://user-images.githubusercontent.com/8698078/207701101-69cdfada-77c6-4510-b254-1fd1fa13af5c.png"/></a](https://user-images.githubusercontent.com/8698078/207701101-69cdfada-77c6-4510-b254-1fd1fa13af5c.png"/></a){kind=link}
3 tasks
Heenawter
force-pushed
the
backport/8.6/pr-147482
branch
from
b4a02a7
to
1b224a7
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport
This will backport the following commits from
mainto8.6:Questions ?
Please refer to the Backport tool documentation