[Dashboard] [Controls] Fix new controls causing unsaved changes bug #132850

Heenawter · 2022-05-24T19:19:47Z

Summary

Before this change, when a new field was selected via creating a new control, the optional parentFieldName and childFieldName properties were being injected as undefined to the panels property of the controlGroupInput via the following code in control_editor.tsx:

...
onSelectField={(field) => {
  onTypeEditorChange({
    fieldName: field.name,
    parentFieldName: fieldRegistry?.[field.name].parentFieldName,
    childFieldName: fieldRegistry?.[field.name].childFieldName,
  }); 
...

However, when the control group panels are saved as JSON, all keys that have undefined values are dropped. This means that, when the control group inputs were compared in diffDashboardState, the old state would not have parent/childFieldName but they would both be undefined in the new state.

This PR prevents the initial injection by ensuring that both the parentFieldName and childFieldName have values before setting them. Therefore, we no longer have the issue of an undefined value being different than the key simply not existing in an object, so deepEqual works as expected.

Video

2022-05-24_ControlsUnsavedChangesCleared.mp4

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 · 2022-05-24T23:09:04Z

test/functional/apps/dashboard_elements/controls/options_list.ts

  describe('Dashboard options list integration', () => {
    before(async () => {
      await common.navigateToApp('dashboard');
      await dashboard.gotoDashboardLandingPage();
      await dashboard.clickNewDashboard();
      await timePicker.setDefaultDataRange();
      await elasticChart.setNewChartUiDebugFlag();
+      await dashboard.saveDashboard(DASHBOARD_NAME, { exitFromEditMode: false });


This bug was only present for existing dashboards, so need to save the dashboard before adding controls and testing to ensure that the unsaved changes badge can be cleared.

kibana-ci · 2022-05-25T16:09:49Z

💛 Build succeeded, but was flaky

Failed CI Steps

FTR Configs #37

Test Failures

[job] [logs] FTR Configs #37 / console app console autocomplete feature with a missing comma in query should add a comma after the triple quoted strings

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`	426.4KB	426.4KB	+36.0B

History

💚 Build #47443 succeeded 4ca5180
💔 Build #47425 failed a8056aa
💔 Build #47394 failed ef977b3

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

cc @Heenawter

elasticmachine · 2022-05-25T18:46:00Z

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

ThomThomson

Really nice bugfix! Code only review and everything LGTM. Nice that it's mostly functional tests!

ThomThomson · 2022-05-25T18:48:55Z

src/plugins/controls/public/control_group/editor/control_editor.tsx

                onTypeEditorChange({
                  fieldName: field.name,
-                  parentFieldName: fieldRegistry?.[field.name].parentFieldName,
-                  childFieldName: fieldRegistry?.[field.name].childFieldName,
+                  ...(parentFieldName && { parentFieldName }),


ThomThomson · 2022-05-25T18:51:43Z

test/functional/apps/dashboard_elements/controls/range_slider.ts

@@ -102,6 +106,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
        validateRange('placeholder', secondId, '100', '1200');
        // data views should be properly propagated from the control group to the dashboard
        expect(await filterBar.getIndexPatterns()).to.be('logstash-*,kibana_sample_data_flights');
+        await dashboard.clearUnsavedChanges();


Nice addition to call this after each test.

kibanamachine · 2022-06-08T16:30:49Z

⚪ Backport skipped

The pull request was not backported as there were no branches to backport to. If this is a mistake, please apply the desired version labels or run the backport tool manually.

Manual backport

To create the backport manually run:

node scripts/backport --pr 132850

Questions ?

Please refer to the Backport tool documentation

…lastic#132850) * Prevent injection of undefined parent/field names * Add unsaved check to existing functional tests (cherry picked from commit f6086d9)

…132850) (#133922) * Prevent injection of undefined parent/field names * Add unsaved check to existing functional tests (cherry picked from commit f6086d9)

Heenawter self-assigned this May 24, 2022

Heenawter force-pushed the fix-controls-unsaved-changes_2022-05-24 branch 3 times, most recently from 65b3b7f to 4ca5180 Compare May 24, 2022 23:06

Heenawter commented May 24, 2022

View reviewed changes

Heenawter added 2 commits May 25, 2022 09:04

Prevent injection of undefined parent/field names

ec415bb

Add unsaved check to existing functional tests

d3859c9

Heenawter force-pushed the fix-controls-unsaved-changes_2022-05-24 branch from 4ca5180 to d3859c9 Compare May 25, 2022 15:04

Heenawter marked this pull request as ready for review May 25, 2022 18:45

Heenawter requested a review from a team as a code owner May 25, 2022 18:45

ThomThomson approved these changes May 25, 2022

View reviewed changes

Heenawter merged commit f6086d9 into elastic:main May 25, 2022

kibanamachine added the backport:skip This commit does not require backporting label May 25, 2022

bhavyarm mentioned this pull request Jun 8, 2022

Controls is making the dashboard go into unsaved state despite user saving the dashboard #133904

Closed

Heenawter added auto-backport Deprecated: Automatically backport this PR after it's merged and removed backport:skip This commit does not require backporting labels Jun 8, 2022

Heenawter mentioned this pull request Jun 8, 2022

[8.3] [Dashboard] [Controls] Fix new controls causing unsaved changes bug (#132850) #133922

Merged

Heenawter deleted the fix-controls-unsaved-changes_2022-05-24 branch June 8, 2022 16:34

Heenawter added backport:skip This commit does not require backporting and removed auto-backport Deprecated: Automatically backport this PR after it's merged labels Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] [Controls] Fix new controls causing unsaved changes bug #132850

[Dashboard] [Controls] Fix new controls causing unsaved changes bug #132850

Heenawter commented May 24, 2022 •

edited

Loading

Heenawter May 24, 2022

kibana-ci commented May 25, 2022

elasticmachine commented May 25, 2022

ThomThomson left a comment

ThomThomson May 25, 2022

ThomThomson May 25, 2022

kibanamachine commented Jun 8, 2022

[Dashboard] [Controls] Fix new controls causing unsaved changes bug #132850

[Dashboard] [Controls] Fix new controls causing unsaved changes bug #132850

Conversation

Heenawter commented May 24, 2022 • edited Loading

Summary

Video

Flaky Test Runner

Checklist

For maintainers

Heenawter May 24, 2022

Choose a reason for hiding this comment

kibana-ci commented May 25, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

elasticmachine commented May 25, 2022

ThomThomson left a comment

Choose a reason for hiding this comment

ThomThomson May 25, 2022

Choose a reason for hiding this comment

ThomThomson May 25, 2022

Choose a reason for hiding this comment

kibanamachine commented Jun 8, 2022

⚪ Backport skipped

Manual backport

Questions ?

Heenawter commented May 24, 2022 •

edited

Loading