From 8d0a98686d83f9b0d03357149b4d613d18cd444b Mon Sep 17 00:00:00 2001 From: Ran Byron Date: Wed, 30 Oct 2019 20:40:41 +0200 Subject: [PATCH 1/6] Parameter feedback - #5 Unsaved indication --- client/app/components/Parameters.jsx | 23 ++++++++++++++++++++--- client/app/pages/queries/query.html | 2 +- client/app/pages/queries/source-view.js | 17 ++++++++++++++++- client/app/services/query.js | 14 ++++++++++---- 4 files changed, 47 insertions(+), 9 deletions(-) diff --git a/client/app/components/Parameters.jsx b/client/app/components/Parameters.jsx index f1ead2904e..b07f7c7165 100644 --- 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 = {error}; + return [feedback, 'error']; + } + + // unsaved + const { unsavedParameters } = this.props; + if (includes(unsavedParameters, param.name)) { + const feedback = ( + <> + Unsaved{' '} + + + + + ); + return [feedback, 'warning']; } return []; @@ -172,7 +189,7 @@ export class Parameters extends React.Component { {feedback} : null} + help={feedback || null} >
+ on-values-change="executeQuery" on-pending-values-change="applyParametersChanges" on-parameters-edit="onParametersUpdated" unsaved-parameters="getUnsavedParameters()">
diff --git a/client/app/pages/queries/source-view.js b/client/app/pages/queries/source-view.js index f5d72898b1..138114a69a 100644 --- 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,21 @@ 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; + } + if (!isEqual(unsavedParameters, $scope.unsavedParameters)) { + $scope.unsavedParameters = unsavedParameters; + } + return $scope.unsavedParameters; + }; } export default function init(ngModule) { diff --git a/client/app/services/query.js b/client/app/services/query.js index c8f19ac176..15567cb8b6 100644 --- a/client/app/services/query.js +++ b/client/app/services/query.js @@ -3,7 +3,7 @@ import debug from 'debug'; import Mustache from 'mustache'; import { zipObject, isEmpty, map, includes, union, - uniq, has, identity, extend, each, some, + uniq, has, identity, extend, each, some, reject, } from 'lodash'; import { Parameter } from './parameters'; @@ -35,13 +35,14 @@ class Parameters { this.initFromQueryString(queryString); } - parseQuery() { + parseQuery(queryText = null) { const fallback = () => map(this.query.options.parameters, i => i.name); + queryText = queryText || this.query.query; let parameters = []; - if (this.query.query !== undefined) { + if (queryText !== undefined) { 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 +125,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 ''; From a1c2c5255a2efd7086bff1564ba9e12c3446f1f4 Mon Sep 17 00:00:00 2001 From: Ran Byron Date: Thu, 31 Oct 2019 10:06:45 +0200 Subject: [PATCH 2/6] Added ANGULAR_REMOVE_ME --- client/app/pages/queries/source-view.js | 1 + 1 file changed, 1 insertion(+) diff --git a/client/app/pages/queries/source-view.js b/client/app/pages/queries/source-view.js index 138114a69a..1d6257c000 100644 --- a/client/app/pages/queries/source-view.js +++ b/client/app/pages/queries/source-view.js @@ -119,6 +119,7 @@ function QuerySourceCtrl( if (isEmpty(unsavedParameters)) { return null; } + // avoiding Angular infdig (ANGULAR_REMOVE_ME) if (!isEqual(unsavedParameters, $scope.unsavedParameters)) { $scope.unsavedParameters = unsavedParameters; } From c3d2217f329357eded6d31e43dcce846b755909c Mon Sep 17 00:00:00 2001 From: Ran Byron Date: Thu, 31 Oct 2019 11:44:28 +0200 Subject: [PATCH 3/6] Added cypress test --- .../integration/query/parameter_spec.js | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/client/cypress/integration/query/parameter_spec.js b/client/cypress/integration/query/parameter_spec.js index a7c5c17eb4..f0ce1acc23 100644 --- a/client/cypress/integration/query/parameter_spec.js +++ b/client/cypress/integration/query/parameter_spec.js @@ -614,6 +614,26 @@ 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'); + 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(); From 1c92bb227758f005e80b2c361f7ee810e2f559be Mon Sep 17 00:00:00 2001 From: Ran Byron Date: Thu, 31 Oct 2019 12:52:45 +0200 Subject: [PATCH 4/6] Fixed percy screenshot --- client/cypress/integration/query/parameter_spec.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/client/cypress/integration/query/parameter_spec.js b/client/cypress/integration/query/parameter_spec.js index f0ce1acc23..5979c4c3a0 100644 --- a/client/cypress/integration/query/parameter_spec.js +++ b/client/cypress/integration/query/parameter_spec.js @@ -627,7 +627,10 @@ describe('Parameter', () => { .find('.ant-form-explain') .as('Feedback'); - cy.get('@Feedback').should('contain.text', 'Unsaved'); + 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(); From 0f90fb282c43ddee4557a159da4248f04c6abc0b Mon Sep 17 00:00:00 2001 From: Ran Byron Date: Mon, 4 Nov 2019 17:33:28 +0200 Subject: [PATCH 5/6] Some code improvements --- client/app/services/query.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/client/app/services/query.js b/client/app/services/query.js index 15567cb8b6..a43dfc7fa3 100644 --- a/client/app/services/query.js +++ b/client/app/services/query.js @@ -2,7 +2,7 @@ import moment from 'moment'; import debug from 'debug'; import Mustache from 'mustache'; import { - zipObject, isEmpty, map, includes, union, + zipObject, isEmpty, map, includes, union, isNil, uniq, has, identity, extend, each, some, reject, } from 'lodash'; @@ -35,12 +35,11 @@ class Parameters { this.initFromQueryString(queryString); } - parseQuery(queryText = null) { + parseQuery(queryText = this.query.query) { const fallback = () => map(this.query.options.parameters, i => i.name); - queryText = queryText || this.query.query; let parameters = []; - if (queryText !== undefined) { + if (!isNil(queryText)) { try { const parts = Mustache.parse(queryText); parameters = uniq(collectParams(parts)); From 56f494773c4890db0f00bb41131744754f66e019 Mon Sep 17 00:00:00 2001 From: Ran Byron Date: Fri, 21 Jul 2023 21:35:29 +0200 Subject: [PATCH 6/6] Parameter input feedback - #6 Better value normalization (#4327) --- client/app/components/ParameterValueInput.jsx | 10 ++++------ client/app/services/parameters/NumberParameter.js | 6 +++--- client/app/services/parameters/TextParameter.js | 9 ++++++++- .../services/parameters/tests/NumberParameter.test.js | 5 ----- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/client/app/components/ParameterValueInput.jsx b/client/app/components/ParameterValueInput.jsx index 7f28ad33a8..88aef8c41e 100644 --- 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 ( this.onSelect(normalize(val))} + value={value} + onChange={val => this.onSelect(val)} /> ); } diff --git a/client/app/services/parameters/NumberParameter.js b/client/app/services/parameters/NumberParameter.js index 997cdb4b29..17abe0cc65 100644 --- 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 index 645adbc8e3..0488392e88 100644 --- 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 index 2a292ea880..9cce92ef28 100644 --- 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(); - }); }); });