Parameter feedback - #5 Unsaved indication (#4322)

* Parameter feedback - #5 Unsaved indication * Added ANGULAR_REMOVE_ME * Added cypress test * Fixed percy screenshot * Some code improvements * Parameter input feedback - #6 Better value normalization (#4327)
getredash · Jul 21, 2023 · a33f615 · a33f615
1 parent a336c5b
commit a33f615
Show file tree

Hide file tree

Showing 9 changed files with 86 additions and 25 deletions.
diff --git a/client/app/components/ParameterValueInput.jsx b/client/app/components/ParameterValueInput.jsx
@@ -5,7 +5,7 @@ import Input from 'antd/lib/input';
 import InputNumber from 'antd/lib/input-number';
 import DateParameter from '@/components/dynamic-parameters/DateParameter';
 import DateRangeParameter from '@/components/dynamic-parameters/DateRangeParameter';
-import { isEqual } from 'lodash';
+import { isEqual, trim } from 'lodash';
 import { QueryBasedParameterInput } from './QueryBasedParameterInput';
 
 import './ParameterValueInput.less';
@@ -59,7 +59,7 @@ class ParameterValueInput extends React.Component {
   }
 
   onSelect = (value) => {
-    const isDirty = !isEqual(value, this.props.value);
+    const isDirty = !isEqual(trim(value), trim(this.props.value));
     this.setState({ value, isDirty });
     this.props.onSelect(value, isDirty);
   }
@@ -140,13 +140,11 @@ class ParameterValueInput extends React.Component {
     const { className } = this.props;
     const { value } = this.state;
 
-    const normalize = val => (isNaN(val) ? undefined : val);
-
     return (
       <InputNumber
         className={className}
-        value={normalize(value)}
-        onChange={val => this.onSelect(normalize(val))}
+        value={value}
+        onChange={val => this.onSelect(val)}
       />
     );
   }

diff --git a/client/app/components/Parameters.jsx b/client/app/components/Parameters.jsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import { size, filter, forEach, extend, get } from 'lodash';
+import { size, filter, forEach, extend, get, includes } from 'lodash';
 import { react2angular } from 'react2angular';
 import { SortableContainer, SortableElement, DragHandle } from '@/components/sortable';
 import { $location } from '@/services/ng';
@@ -34,6 +34,7 @@ export class Parameters extends React.Component {
     queryResultErrorData: PropTypes.shape({
       parameters: PropTypes.objectOf(PropTypes.string),
     }),
+    unsavedParameters: PropTypes.arrayOf(PropTypes.string),
   };
 
   static defaultProps = {
@@ -44,6 +45,7 @@ export class Parameters extends React.Component {
     onPendingValuesChange: () => {},
     onParametersEdit: () => {},
     queryResultErrorData: {},
+    unsavedParameters: null,
   };
 
   constructor(props) {
@@ -140,7 +142,22 @@ export class Parameters extends React.Component {
     const { queryResultErrorData } = this.props;
     const error = get(queryResultErrorData, ['parameters', param.name], false);
     if (error) {
-      return [error, 'error'];
+      const feedback = <Tooltip title={error}>{error}</Tooltip>;
+      return [feedback, 'error'];
+    }
+
+    // unsaved
+    const { unsavedParameters } = this.props;
+    if (includes(unsavedParameters, param.name)) {
+      const feedback = (
+        <>
+          Unsaved{' '}
+          <Tooltip title='Click the "Save" button to preserve this parameter.'>
+            <i className="fa fa-question-circle" />
+          </Tooltip>
+        </>
+      );
+      return [feedback, 'warning'];
     }
 
     return [];
@@ -172,7 +189,7 @@ export class Parameters extends React.Component {
         </div>
         <Form.Item
           validateStatus={touched ? '' : status}
-          help={feedback ? <Tooltip title={feedback}>{feedback}</Tooltip> : null}
+          help={feedback || null}
         >
           <ParameterValueInput
             type={param.type}

diff --git a/client/app/pages/queries/query.html b/client/app/pages/queries/query.html
@@ -200,7 +200,7 @@ <h3>
             <div class="d-flex flex-column p-b-15 p-absolute static-position__mobile" style="left: 0; top: 0; right: 0; bottom: 0;">
               <div class="p-t-15 p-b-5" ng-if="query.hasParameters()">
                 <parameters parameters="query.getParametersDefs()" query-result-error-data="queryResult.getErrorData()" editable="sourceMode && canEdit" disable-url-update="query.isNew()"
-                  on-values-change="executeQuery" on-pending-values-change="applyParametersChanges" on-parameters-edit="onParametersUpdated"></parameters>
+                  on-values-change="executeQuery" on-pending-values-change="applyParametersChanges" on-parameters-edit="onParametersUpdated" unsaved-parameters="getUnsavedParameters()"></parameters>
               </div>
               <!-- Query Execution Status -->
 

diff --git a/client/app/pages/queries/source-view.js b/client/app/pages/queries/source-view.js
@@ -1,4 +1,4 @@
-import { map, debounce } from 'lodash';
+import { map, debounce, isEmpty, isEqual } from 'lodash';
 import template from './query.html';
 import EditParameterSettingsDialog from '@/components/EditParameterSettingsDialog';
 
@@ -109,6 +109,22 @@ function QuerySourceCtrl(
   $scope.$watch('query.query', (newQueryText) => {
     $scope.isDirty = newQueryText !== queryText;
   });
+
+  $scope.unsavedParameters = null;
+  $scope.getUnsavedParameters = () => {
+    if (!$scope.isDirty || !queryText) {
+      return null;
+    }
+    const unsavedParameters = $scope.query.$parameters.getUnsavedParameters(queryText);
+    if (isEmpty(unsavedParameters)) {
+      return null;
+    }
+    // avoiding Angular infdig (ANGULAR_REMOVE_ME)
+    if (!isEqual(unsavedParameters, $scope.unsavedParameters)) {
+      $scope.unsavedParameters = unsavedParameters;
+    }
+    return $scope.unsavedParameters;
+  };
 }
 
 export default function init(ngModule) {

diff --git a/client/app/services/parameters/NumberParameter.js b/client/app/services/parameters/NumberParameter.js
@@ -1,4 +1,4 @@
-import { toNumber, isNull } from 'lodash';
+import { toNumber, trim } from 'lodash';
 import { Parameter } from '.';
 
 class NumberParameter extends Parameter {
@@ -9,11 +9,11 @@ class NumberParameter extends Parameter {
 
   // eslint-disable-next-line class-methods-use-this
   normalizeValue(value) {
-    if (isNull(value)) {
+    if (!trim(value)) {
       return null;
     }
     const normalizedValue = toNumber(value);
-    return !isNaN(normalizedValue) ? normalizedValue : null;
+    return !isNaN(normalizedValue) ? normalizedValue : value;
   }
 }
 

diff --git a/client/app/services/parameters/TextParameter.js b/client/app/services/parameters/TextParameter.js
@@ -1,4 +1,4 @@
-import { toString, isEmpty } from 'lodash';
+import { toString, isEmpty, trim } from 'lodash';
 import { Parameter } from '.';
 
 class TextParameter extends Parameter {
@@ -15,6 +15,13 @@ class TextParameter extends Parameter {
     }
     return normalizedValue;
   }
+
+  getExecutionValue() {
+    if (!trim(this.value)) {
+      return null;
+    }
+    return this.value;
+  }
 }
 
 export default TextParameter;
diff --git a/client/app/services/parameters/tests/NumberParameter.test.js b/client/app/services/parameters/tests/NumberParameter.test.js
@@ -17,10 +17,5 @@ describe('NumberParameter', () => {
       const normalizedValue = param.normalizeValue(42);
       expect(normalizedValue).toBe(42);
     });
-
-    test('returns null when not possible to convert to number', () => {
-      const normalizedValue = param.normalizeValue('notanumber');
-      expect(normalizedValue).toBeNull();
-    });
   });
 });
diff --git a/client/app/services/query.js b/client/app/services/query.js
@@ -2,8 +2,8 @@ import moment from 'moment';
 import debug from 'debug';
 import Mustache from 'mustache';
 import {
-  zipObject, isEmpty, map, includes, union,
-  uniq, has, identity, extend, each, some,
+  zipObject, isEmpty, map, includes, union, isNil,
+  uniq, has, identity, extend, each, some, reject,
 } from 'lodash';
 
 import { Parameter } from './parameters';
@@ -35,13 +35,13 @@ class Parameters {
     this.initFromQueryString(queryString);
   }
 
-  parseQuery() {
+  parseQuery(queryText = this.query.query) {
     const fallback = () => map(this.query.options.parameters, i => i.name);
 
     let parameters = [];
-    if (this.query.query !== undefined) {
+    if (!isNil(queryText)) {
       try {
-        const parts = Mustache.parse(this.query.query);
+        const parts = Mustache.parse(queryText);
         parameters = uniq(collectParams(parts));
       } catch (e) {
         logger('Failed parsing parameters: ', e);
@@ -124,6 +124,11 @@ class Parameters {
     each(this.get(), p => p.applyPendingValue());
   }
 
+  getUnsavedParameters(queryText) {
+    const savedParameters = this.parseQuery(queryText);
+    return reject(this.get(), p => includes(savedParameters, p.name)).map(p => p.name);
+  }
+
   toUrlParams() {
     if (this.get().length === 0) {
       return '';

diff --git a/client/cypress/integration/query/parameter_spec.js b/client/cypress/integration/query/parameter_spec.js
@@ -614,6 +614,29 @@ describe('Parameter', () => {
       cy.percySnapshot('Validation error in query page');
     });
 
+    it('shows unsaved feedback in query page', function () {
+      cy.visit(`/queries/${this.query.id}/source`);
+
+      cy.getByTestId('QueryEditor')
+        .get('.ace_text-input')
+        .type(' {{ newparam }}', { force: true, parseSpecialCharSequences: false });
+
+      cy.getByTestId('ParameterName-newparam')
+        .find('.ant-form-item-control')
+        .should('have.class', 'has-warning')
+        .find('.ant-form-explain')
+        .as('Feedback');
+
+      cy.get('@Feedback')
+        .should('contain.text', 'Unsaved')
+        .should('not.have.class', 'show-help-appear'); // assures ant animation ended for screenshot
+
+      cy.percySnapshot('Unsaved feedback in query page');
+
+      cy.getByTestId('SaveButton').click();
+      cy.get('@Feedback').should('not.exist');
+    });
+
     it('shows validation error in visualization embed', function () {
       cy.visit(`/embed/query/${this.query.id}/visualization/${this.vizId}?api_key=${this.query.api_key}`);
       expectValueValidationError();