Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[8.6] [Dashboard] [Controls] Allow control changes to be discarded (#…
…147482) (#147819) # Backport This will backport the following commits from `main` to `8.6`: - [[Dashboard] [Controls] Allow control changes to be discarded (#147482)](#147482) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2022-12-19T21:55:31Z","message":"[Dashboard] [Controls] Allow control changes to be discarded (#147482)\n\nCloses https://github.com/elastic/kibana/issues/147293\r\n\r\n## Summary\r\n\r\nBefore this change, the Redux state `explicitInput` was getting out of\r\nsync with the embeddable `explicitInput` in scenarios where the new\r\n`explicitInput` was missing a key that the old `explicitInput` had -\r\ntherefore, because they were out of sync, the changes that **should**\r\nhave been discarded kept getting injected back into the embeddable\r\n`explicitInput`, which made it impossible to actually discard anything\r\nunless the key existed in both the before and after state.\r\n\r\nThis PR fixes this by replacing the entire Redux state `explicitInput`\r\nwith the embeddable `explicitInput` rather than spreading the new value.\r\nIt also fixes a bug with the time slider control where changes to the\r\nembeddable's input were not reflected properly in the control's state,\r\nso nothing could be discarded even after the initial bug was fixed.\r\n\r\n#### Further Explanation \r\n\r\nWhen a control is first created, all the optional properties of the\r\nexplicit input do not yet exist - for example, when creating an options\r\nlist control, the `selections` key does not exist in the `explicitInput`\r\nuntil a selection is made. Therefore, imagine the following scenario:\r\n\r\n1. You create an options list control (where the `selections` key does\r\nnot exist) and save the dashboard\r\n2. You make some selections, which causes `unsaved changes` because the\r\n`selections` key now exists and is equal to an array\r\n3. You switch to view mode and choose to discard your changes, thus\r\n(supposedly) removing the `selections` key from the `explicitInput`\r\nobject once again\r\n\r\nUnfortunately, the Redux embeddable state for each control was **not**\r\naccurately removing the `selections` key as expected - this was because,\r\nwhen trying to update the `explicitInput` via the old\r\n`updateEmbeddableReduxInput`, the new value was **spread** on top of the\r\nolder value rather than replacing it. In a simplified scenario, this\r\nresulted in something like this:\r\n\r\n```typescript\r\nconst oldExplicitInput = { id: 'test_id', selections: ['test selection'] };\r\nconst newExplicitInput = { id: 'test_id' }\r\nconst result = { ...oldExplicitInput, ...newExplicitInput };\r\n```\r\n\r\nIn this code, because `newExplicitInput` does not have the `selections`\r\nkey, `result` will equal `{ id: 'test_id', selections: ['test\r\nselection'] }` - this is not the behaviour we want! Instead, we wanted\r\nto replace the entire old `explicitInput` with the new `explicitInput`.\r\nEffectively, that is what this PR does.\r\n\r\nThanks to @ThomThomson for helping out with finding the root cause of\r\nthis after I got lost :)\r\n\r\n### How to Test\r\nFor both options list and range slider controls, \r\n1. Create a control of the desired type\r\n2. Save the dashboard \r\n3. Make some sort of change that causes unsaved changes - for example,\r\nmake a selection or, if an options list control, set `exclude` to `true`\r\n4. Switch to view mode, discarding the changes\r\n5. Ensure that the changes made in step 3 are no longer applied ✅ \r\n6. Switch back to edit mode\r\n7. Ensure that there are no `unsaved changes` ✅ \r\n\r\n#### Flaky Test Runner\r\n\r\n<a\r\nhref=\"https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1649\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/207701101-69cdfada-77c6-4510-b254-1fd1fa13af5c.png\"/></a>\r\n\r\n### Checklist\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- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"7a6eac8d1bfe492b8dce83c7a0f47dc26706e388","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Dashboard","release_note:fix","Feature:Input Control","Team:Presentation","loe:days","impact:high","Project:Controls","backport:prev-minor","v8.7.0"],"number":147482,"url":"https://github.com/elastic/kibana/pull/147482","mergeCommit":{"message":"[Dashboard] [Controls] Allow control changes to be discarded (#147482)\n\nCloses https://github.com/elastic/kibana/issues/147293\r\n\r\n## Summary\r\n\r\nBefore this change, the Redux state `explicitInput` was getting out of\r\nsync with the embeddable `explicitInput` in scenarios where the new\r\n`explicitInput` was missing a key that the old `explicitInput` had -\r\ntherefore, because they were out of sync, the changes that **should**\r\nhave been discarded kept getting injected back into the embeddable\r\n`explicitInput`, which made it impossible to actually discard anything\r\nunless the key existed in both the before and after state.\r\n\r\nThis PR fixes this by replacing the entire Redux state `explicitInput`\r\nwith the embeddable `explicitInput` rather than spreading the new value.\r\nIt also fixes a bug with the time slider control where changes to the\r\nembeddable's input were not reflected properly in the control's state,\r\nso nothing could be discarded even after the initial bug was fixed.\r\n\r\n#### Further Explanation \r\n\r\nWhen a control is first created, all the optional properties of the\r\nexplicit input do not yet exist - for example, when creating an options\r\nlist control, the `selections` key does not exist in the `explicitInput`\r\nuntil a selection is made. Therefore, imagine the following scenario:\r\n\r\n1. You create an options list control (where the `selections` key does\r\nnot exist) and save the dashboard\r\n2. You make some selections, which causes `unsaved changes` because the\r\n`selections` key now exists and is equal to an array\r\n3. You switch to view mode and choose to discard your changes, thus\r\n(supposedly) removing the `selections` key from the `explicitInput`\r\nobject once again\r\n\r\nUnfortunately, the Redux embeddable state for each control was **not**\r\naccurately removing the `selections` key as expected - this was because,\r\nwhen trying to update the `explicitInput` via the old\r\n`updateEmbeddableReduxInput`, the new value was **spread** on top of the\r\nolder value rather than replacing it. In a simplified scenario, this\r\nresulted in something like this:\r\n\r\n```typescript\r\nconst oldExplicitInput = { id: 'test_id', selections: ['test selection'] };\r\nconst newExplicitInput = { id: 'test_id' }\r\nconst result = { ...oldExplicitInput, ...newExplicitInput };\r\n```\r\n\r\nIn this code, because `newExplicitInput` does not have the `selections`\r\nkey, `result` will equal `{ id: 'test_id', selections: ['test\r\nselection'] }` - this is not the behaviour we want! Instead, we wanted\r\nto replace the entire old `explicitInput` with the new `explicitInput`.\r\nEffectively, that is what this PR does.\r\n\r\nThanks to @ThomThomson for helping out with finding the root cause of\r\nthis after I got lost :)\r\n\r\n### How to Test\r\nFor both options list and range slider controls, \r\n1. Create a control of the desired type\r\n2. Save the dashboard \r\n3. Make some sort of change that causes unsaved changes - for example,\r\nmake a selection or, if an options list control, set `exclude` to `true`\r\n4. Switch to view mode, discarding the changes\r\n5. Ensure that the changes made in step 3 are no longer applied ✅ \r\n6. Switch back to edit mode\r\n7. Ensure that there are no `unsaved changes` ✅ \r\n\r\n#### Flaky Test Runner\r\n\r\n<a\r\nhref=\"https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1649\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/207701101-69cdfada-77c6-4510-b254-1fd1fa13af5c.png\"/></a>\r\n\r\n### Checklist\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- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"7a6eac8d1bfe492b8dce83c7a0f47dc26706e388"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/147482","number":147482,"mergeCommit":{"message":"[Dashboard] [Controls] Allow control changes to be discarded (#147482)\n\nCloses https://github.com/elastic/kibana/issues/147293\r\n\r\n## Summary\r\n\r\nBefore this change, the Redux state `explicitInput` was getting out of\r\nsync with the embeddable `explicitInput` in scenarios where the new\r\n`explicitInput` was missing a key that the old `explicitInput` had -\r\ntherefore, because they were out of sync, the changes that **should**\r\nhave been discarded kept getting injected back into the embeddable\r\n`explicitInput`, which made it impossible to actually discard anything\r\nunless the key existed in both the before and after state.\r\n\r\nThis PR fixes this by replacing the entire Redux state `explicitInput`\r\nwith the embeddable `explicitInput` rather than spreading the new value.\r\nIt also fixes a bug with the time slider control where changes to the\r\nembeddable's input were not reflected properly in the control's state,\r\nso nothing could be discarded even after the initial bug was fixed.\r\n\r\n#### Further Explanation \r\n\r\nWhen a control is first created, all the optional properties of the\r\nexplicit input do not yet exist - for example, when creating an options\r\nlist control, the `selections` key does not exist in the `explicitInput`\r\nuntil a selection is made. Therefore, imagine the following scenario:\r\n\r\n1. You create an options list control (where the `selections` key does\r\nnot exist) and save the dashboard\r\n2. You make some selections, which causes `unsaved changes` because the\r\n`selections` key now exists and is equal to an array\r\n3. You switch to view mode and choose to discard your changes, thus\r\n(supposedly) removing the `selections` key from the `explicitInput`\r\nobject once again\r\n\r\nUnfortunately, the Redux embeddable state for each control was **not**\r\naccurately removing the `selections` key as expected - this was because,\r\nwhen trying to update the `explicitInput` via the old\r\n`updateEmbeddableReduxInput`, the new value was **spread** on top of the\r\nolder value rather than replacing it. In a simplified scenario, this\r\nresulted in something like this:\r\n\r\n```typescript\r\nconst oldExplicitInput = { id: 'test_id', selections: ['test selection'] };\r\nconst newExplicitInput = { id: 'test_id' }\r\nconst result = { ...oldExplicitInput, ...newExplicitInput };\r\n```\r\n\r\nIn this code, because `newExplicitInput` does not have the `selections`\r\nkey, `result` will equal `{ id: 'test_id', selections: ['test\r\nselection'] }` - this is not the behaviour we want! Instead, we wanted\r\nto replace the entire old `explicitInput` with the new `explicitInput`.\r\nEffectively, that is what this PR does.\r\n\r\nThanks to @ThomThomson for helping out with finding the root cause of\r\nthis after I got lost :)\r\n\r\n### How to Test\r\nFor both options list and range slider controls, \r\n1. Create a control of the desired type\r\n2. Save the dashboard \r\n3. Make some sort of change that causes unsaved changes - for example,\r\nmake a selection or, if an options list control, set `exclude` to `true`\r\n4. Switch to view mode, discarding the changes\r\n5. Ensure that the changes made in step 3 are no longer applied ✅ \r\n6. Switch back to edit mode\r\n7. Ensure that there are no `unsaved changes` ✅ \r\n\r\n#### Flaky Test Runner\r\n\r\n<a\r\nhref=\"https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1649\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/207701101-69cdfada-77c6-4510-b254-1fd1fa13af5c.png\"/></a>\r\n\r\n### Checklist\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- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"7a6eac8d1bfe492b8dce83c7a0f47dc26706e388"}}]}] BACKPORT-->
- Loading branch information