fixing updating editor state (#22869)

* editor state update

* remove lockDirty

* Add functional tests

* Add data to functional tests

src/core_plugins/kibana/public/visualize/wizard/step_1.html

@@ -50,6 +50,7 @@
           tooltip="{{ getVisTypeTooltip(type) }}"
           tooltip-placement="{{ getVisTypeTooltipPosition($parent.$index) }}"
           aria-describedby="visDescription_{{ ::getVisTypeId(type) }}"
+          data-test-subj="visType-{{::type.name}}"
         >
           <div class="kuiGalleryItem__image">
             <img

src/ui/public/vis/editors/default/default.js

@@ -31,7 +31,6 @@ import { DefaultEditorSize } from '../../editor_size';
 
 import { VisEditorTypesRegistryProvider } from '../../../registry/vis_editor_types';
 import { getVisualizeLoader } from '../../../visualize/loader/visualize_loader';
-import { updateEditorStateWithChanges } from './update_editor_state';
 
 
 const defaultEditor = function ($rootScope, $compile) {
@@ -115,18 +114,10 @@ const defaultEditor = function ($rootScope, $compile) {
             }
           };
 
-          let lockDirty = false;
           $scope.$watch(() => {
             return $scope.vis.getSerializableState($scope.state);
           }, function (newState) {
-            // when visualization updates its `vis.params` we need to update the editor, but we should
-            // not set the dirty flag (as this change came from vis itself and is already applied)
-            if (lockDirty) {
-              lockDirty = false;
-            } else {
-              $scope.vis.dirty = !angular.equals(newState, $scope.oldState);
-            }
-
+            $scope.vis.dirty = !angular.equals(newState, $scope.oldState);
             $scope.responseValueAggs = null;
             try {
               $scope.responseValueAggs = $scope.state.aggs.getResponseAggs().filter(function (agg) {
@@ -143,8 +134,9 @@ const defaultEditor = function ($rootScope, $compile) {
           $scope.$watch(() => {
             return $scope.vis.getCurrentState(false);
           }, (newState) => {
-            if (updateEditorStateWithChanges(newState, $scope.oldState, $scope.state)) {
-              lockDirty = true;
+            if (!_.isEqual(newState, $scope.oldState)) {
+              $scope.state = $scope.vis.copyCurrentState(true);
+              $scope.oldState = newState;
             }
           }, true);
 

src/ui/public/vis/editors/default/update_editor_state.js

@@ -1,51 +0,0 @@
-/*
- * Licensed to Elasticsearch B.V. under one or more contributor
- * license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright
- * ownership. Elasticsearch B.V. licenses this file to you under
- * the Apache License, Version 2.0 (the "License"); you may
- * not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *    http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-import { cloneDeep } from 'lodash';
-
-export const updateEditorStateWithChanges = (newState, oldState, editorState) => {
-  let dirty = false;
-  for (const prop in newState) {
-    if (newState.hasOwnProperty(prop)) {
-      const newStateValue = newState[prop];
-      const oldStateValue = oldState[prop];
-      const editorStateValue = editorState[prop];
-
-      if (newStateValue && typeof newStateValue === 'object') {
-        if (editorStateValue) {
-          // Keep traversing.
-          if (updateEditorStateWithChanges(newStateValue, oldStateValue, editorStateValue)) {
-            dirty = true;
-          }
-          break;
-        }
-
-        const newStateValueCopy = cloneDeep(newStateValue);
-        editorState[prop] = newStateValueCopy;
-        oldState[prop] = newStateValueCopy;
-        dirty = true;
-      } else if (newStateValue !== oldStateValue) {
-        oldState[prop] = newStateValue;
-        editorState[prop] = newStateValue;
-        dirty = true;
-      }
-    }
-  }
-  return dirty;
-};

src/ui/public/vis/editors/default/update_editor_state.test.js

@@ -1,78 +0,0 @@
-/*
- * Licensed to Elasticsearch B.V. under one or more contributor
- * license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright
- * ownership. Elasticsearch B.V. licenses this file to you under
- * the Apache License, Version 2.0 (the "License"); you may
- * not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *    http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-import { cloneDeep } from 'lodash';
-import { updateEditorStateWithChanges } from './update_editor_state';
-
-// Parts of the tests in this file are generated more dynamically, based on the
-// values inside the Status object.Make sure this object has one function per entry
-// in Status, that actually change on the passed $scope, what needs to be changed
-// so that we expect the getUpdateStatus function to actually detect a change.
-const oldState = {
-  booleanValue: false,
-  intValue: 0,
-  stringValue: '',
-  nullValue: null,
-  undefinedValue: undefined,
-  objectValue: {},
-  arrayValue: [],
-  testObject: {
-    booleanValue: false,
-    intValue: 0,
-    nullValue: null,
-    arrayValue: [1, 2, 3, { nullValue: null, stringValue: 'hello' }],
-  }
-};
-
-describe('updateEditorStateWithChanges', () => {
-  let editorState;
-  let newState;
-
-  beforeEach(() => {
-    editorState = cloneDeep(oldState);
-    newState = cloneDeep(oldState);
-  });
-
-  it('should be a function', () => {
-    expect(typeof updateEditorStateWithChanges).toBe('function');
-  });
-
-  it('should return false if no changes are detected', () => {
-    const dirty = updateEditorStateWithChanges(newState, oldState, editorState);
-    expect(dirty).toBe(false);
-  });
-
-  it('should return true if changes are detected', () => {
-    let dirty;
-
-    newState.booleanValue = true;
-    dirty = updateEditorStateWithChanges(newState, oldState, editorState);
-    expect(dirty).toBe(true);
-
-    dirty = updateEditorStateWithChanges(newState, oldState, editorState);
-    expect(dirty).toBe(false);
-
-    newState.nullValue = 'test';
-    dirty = updateEditorStateWithChanges(newState, oldState, editorState);
-    expect(dirty).toBe(true);
-
-    dirty = updateEditorStateWithChanges(newState, oldState, editorState);
-    expect(dirty).toBe(false);
-  });
-});

test/functional/page_objects/visualize_page.js

@@ -59,6 +59,11 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
       });
     }
 
+    async clickVisType(type) {
+      await testSubjects.click(`visType-${type}`);
+      await PageObjects.header.waitUntilLoadingHasFinished();
+    }
+
     async clickAreaChart() {
       await find.clickByPartialLinkText('Area');
       await PageObjects.header.waitUntilLoadingHasFinished();

test/plugin_functional/config.js

@@ -30,6 +30,7 @@ export default async function ({ readConfigFile }) {
   return {
     testFiles: [
       require.resolve('./test_suites/app_plugins'),
+      require.resolve('./test_suites/custom_visualizations'),
       require.resolve('./test_suites/embedding_visualizations'),
       require.resolve('./test_suites/panel_actions'),
     ],

test/plugin_functional/plugins/custom_visualizations/README.md

@@ -0,0 +1,3 @@
+# Custom visualizations
+
+This sample plugin contains exampels on how to create custom visualizations in Kibana.

test/plugin_functional/plugins/custom_visualizations/index.js

@@ -0,0 +1,28 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export default function (kibana) {
+  return new kibana.Plugin({
+    uiExports: {
+      visTypes: [
+        'plugins/custom_visualziations/self_changing_vis/self_changing_vis',
+      ],
+    },
+  });
+}

test/plugin_functional/plugins/custom_visualizations/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "custom_visualziations",
+  "version": "kibana"
+}

test/plugin_functional/plugins/custom_visualizations/public/self_changing_vis/self_changing_components.js

@@ -0,0 +1,55 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+
+import {
+  EuiBadge,
+} from '@elastic/eui';
+
+export class SelfChangingComponent extends React.Component {
+
+  onClick = () => {
+    this.props.vis.params.counter++;
+    this.props.vis.updateState();
+  }
+
+  render() {
+    return (
+      <div>
+        <EuiBadge
+          data-test-subj="counter"
+          onClick={this.onClick}
+          onClickAriaLabel="Increase counter"
+          color="primary"
+        >
+          {this.props.vis.params.counter}
+        </EuiBadge>
+      </div>
+    );
+  }
+
+  componentDidMount() {
+    this.props.renderComplete();
+  }
+
+  componentDidUpdate() {
+    this.props.renderComplete();
+  }
+}

test/plugin_functional/plugins/custom_visualizations/public/self_changing_vis/self_changing_editor.js

@@ -0,0 +1,47 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+
+import {
+  EuiFieldNumber,
+  EuiFormRow,
+} from '@elastic/eui';
+
+export class SelfChangingEditor extends React.Component {
+
+  onCounterChange = (ev) => {
+    this.props.stageEditorParams({
+      counter: parseInt(ev.target.value),
+    });
+  }
+
+  render() {
+    return (
+      <EuiFormRow label="Counter">
+        <EuiFieldNumber
+          value={this.props.editorState.params.counter}
+          onChange={this.onCounterChange}
+          step={1}
+          data-test-subj="counterEditor"
+        />
+      </EuiFormRow>
+    );
+  }
+}

test/plugin_functional/plugins/custom_visualizations/public/self_changing_vis/self_changing_vis.js

@@ -0,0 +1,53 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { VisFactoryProvider } from 'ui/vis/vis_factory';
+import { VisTypesRegistryProvider } from 'ui/registry/vis_types';
+
+import { SelfChangingEditor } from './self_changing_editor';
+import { SelfChangingComponent } from './self_changing_components';
+
+function SelfChangingVisType(Private) {
+  const VisFactory = Private(VisFactoryProvider);
+
+  return VisFactory.createReactVisualization({
+    name: 'self_changing_vis',
+    title: 'Self Changing Vis',
+    icon: 'visControls',
+    description: 'This visualization is able to change its own settings, that you could also set in the editor.',
+    visConfig: {
+      component: SelfChangingComponent,
+      defaults: {
+        counter: 0,
+      },
+    },
+    editorConfig: {
+      optionTabs: [
+        {
+          name: 'options',
+          title: 'Options',
+          editor: SelfChangingEditor,
+        },
+      ],
+    },
+    requestHandler: 'none',
+  });
+}
+
+VisTypesRegistryProvider.register(SelfChangingVisType);

test/plugin_functional/test_suites/custom_visualizations/index.js

@@ -0,0 +1,35 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export default function ({ getService, loadTestFile }) {
+  const remote = getService('remote');
+  const esArchiver = getService('esArchiver');
+  const kibanaServer = getService('kibanaServer');
+
+  describe('custom visualizations', function () {
+    before(async () => {
+      await esArchiver.loadIfNeeded('../functional/fixtures/es_archiver/logstash_functional');
+      await esArchiver.loadIfNeeded('../functional/fixtures/es_archiver/visualize');
+      await kibanaServer.uiSettings.replace({ 'dateFormat:tz': 'Australia/North', 'defaultIndex': 'logstash-*' });
+      await remote.setWindowSize(1300, 900);
+    });
+
+    loadTestFile(require.resolve('./self_changing_vis'));
+  });
+}

test/plugin_functional/test_suites/custom_visualizations/self_changing_vis.js

@@ -0,0 +1,68 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import expect from 'expect.js';
+
+export default function ({ getService, getPageObjects }) {
+  const testSubjects = getService('testSubjects');
+  const renderable = getService('renderable');
+  const PageObjects = getPageObjects(['common', 'visualize']);
+
+  async function getCounterValue() {
+    return await testSubjects.getVisibleText('counter');
+  }
+
+  async function getEditorValue() {
+    const editor = await testSubjects.find('counterEditor');
+    return await editor.getProperty('value');
+  }
+
+  describe('self changing vis', function describeIndexTests() {
+
+    before(async () => {
+      await PageObjects.visualize.navigateToNewVisualization();
+      await PageObjects.visualize.clickVisType('self_changing_vis');
+    });
+
+    it('should allow updating params via the editor', async () => {
+      const editor = await testSubjects.find('counterEditor');
+      await editor.clearValue();
+      await editor.type('10');
+      const isApplyEnabled = await PageObjects.visualize.isApplyEnabled();
+      expect(isApplyEnabled).to.be(true);
+      await PageObjects.visualize.clickGo();
+      const counter = await getCounterValue();
+      expect(counter).to.be('10');
+    });
+
+    it('should allow changing params from within the vis', async () => {
+      await testSubjects.click('counter');
+      await renderable.waitForRender();
+      const visValue = await getCounterValue();
+      expect(visValue).to.be('11');
+      const editorValue = await getEditorValue();
+      expect(editorValue).to.be('11');
+      // If changing a param from within the vis it should immediately apply and not bring editor in an unchanged state
+      const isApplyEnabled = await PageObjects.visualize.isApplyEnabled();
+      expect(isApplyEnabled).to.be(false);
+    });
+
+  });
+
+}