[Dashboard] [Controls] Allow control changes to be discarded

[Heenawter]

Closes #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:

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

Checklist

• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[Heenawter]

The timeslider control wasn't actually discarding the changes to the selected time range value on input change because it was only syncing with the time range when the global time range changed. Moving this syncwithTimeRange call outside of the if statement fixed this 👍

[nreese]

this.syncWithTimeRange() should stay in the if statement so syncWithTimeRange is not called every time input changes.

I could not reproduce the problem you mentioned and was able to do the following

1. edit code, moving syncwithTimeRange back into if statement
2. create dashboard with timeslider control
3. save dashboard
4. switch to view mode
5. switch back to edit mode
6. change time range - verified time slider control updated to new time range
7. Click "Switch to view mode" and select discard changes in modal
8. verified time slider control updated to previous time range.

[Heenawter]

Oh... hrm. Very odd.

Here is me replicating the behaviour when I move the syncwithTimeRange back into the if:

BadTimeSlider.mov

I will try with a freshly bootstrapped Kibana to see if something funky is happening....

[Heenawter]

Yup, tried it with a fresh install (updated my branch, pulled the changes, and built Kibana with yarn cache clean && yarn kbn clean && yarn kbn bootstrap) and I can still replicate that the changes to the time slider are not being discarded when I move syncwithTimeRange back into the if. I'm not sure why you aren't able to replicate it? Very odd....

Here it is being replicated in Firefox (the previous video was taken in Chrome) after the fresh install:

BadTimeSlider2.mov

Can you try replicating it after a fresh install on your end @nreese?

Agreed that calling syncWithTimeRange may not be the best way to do it but, from my digging into this bug, the componentState and explicitInput became out of sync when discarding changes. Basically, discarding changes resets the explicitInput to the old value directly - so, while the getInput$() subscription is fired appropriately when changes are discarded, because this.onInputChanged() is all contained within the if, the component state is not updated as it should be. Hence, the changes appearing to not be discarded.

[nreese]

Thank you for the video, that helped. I was changing the time range in the global time picker, not changing the selected timeslice in the time slider. I can see the problem now.

Calling syncWithTimeRange on any input change is a problem. syncWithTimeRange publishes timeslice and updates a bunch of redux state that does not need to be updated.

How about wrapping syncWithTimeRange in an if statement to only call when this case happens. See 97887a5 as an example.

[Heenawter]

Good idea! :) I added something along those lines in 62f2f0d

It's definitely a bit messy - not sure if there is a better way to go about it. Because componentState stores the value but explicitInput stores the percentage, there isn't a super easy way to reset the component state back to its previous value when the explicitInput is changed.... Interested in your thoughts on how this might be cleaned up!

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[nreese]

I would not recommend adding closePopover parameter to gotoNextTimeSlice. It takes away from the readability of the functional tests. For example

• gotoNextTimeSlice(false) means goto next time slice
• gotoNextTimeSlice() means goto next time slice and close popover.

When you come back to look at these tests in a few months, you will not remember what passing false and not passing false actually do.
I think this does not really save a lot and showing explicit steps in functional tests really helps with readability.

[Heenawter]

Ignore my previous comment :) Cleaned this up in cfdb008

[nreese]

prevTimesliceAsPercentage needs to be set either in constructor or syncWithTimeRange (which is called by constructor)

[Heenawter]

Good catch! Done in 3297a73 👍

[nreese]

prevTimesliceAsPercentage is always set and can be typed as just

private prevTimesliceAsPercentage:
    {
        timesliceStartAsPercentageOfTimeRange?: number;
        timesliceEndAsPercentageOfTimeRange?: number;
      };

[Heenawter]

Right WOOP sorry :) Done in 23c0da7

[nreese]

LGTM
code review, tested in chrome

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 23c0da7

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
controls	454.3KB	455.4KB	+1.0KB
presentationUtil	130.4KB	130.3KB	-37.0B
total			+1.0KB

Unknown metric groups

ESLint disabled in files

id	before	after	diff
osquery	1	2	+1

ESLint disabled line counts

id	before	after	diff
enterpriseSearch	19	21	+2
fleet	61	67	+6
osquery	109	115	+6
securitySolution	439	445	+6
total			+20

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	20	22	+2
fleet	70	76	+6
osquery	110	117	+7
securitySolution	516	522	+6
total			+21

History

• 💚 Build #95770 succeeded ff0690e
• 💚 Build #95233 succeeded 970d914
• 💚 Build #94801 succeeded 1196dc2
• 💚 Build #94748 succeeded f3ca231
• 💚 Build #94543 succeeded d47bb4b

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

cc @Heenawter

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.6	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 147482

Questions ?

Please refer to the Backport tool documentation

[Heenawter]

💚 All backports created successfully

Status	Branch	Result
✅	8.6

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

Questions ?

Please refer to the Backport tool documentation