From bad3f14c14c4369f6a25ef7fe045e3f9357f4a1a Mon Sep 17 00:00:00 2001 From: Ran Byron Date: Wed, 30 Oct 2019 11:29:11 +0200 Subject: [PATCH] Parameter feedback - #2 Client errors in query page --- .../EditParameterSettingsDialog.jsx | 1 + client/app/components/Parameters.jsx | 66 +++++++--- client/app/components/Parameters.less | 25 +++- client/app/pages/queries/query.html | 2 +- client/app/services/query-result.js | 6 +- client/app/services/query.js | 7 +- .../integration/query/parameter_spec.js | 120 +++++++++++++++++- 7 files changed, 204 insertions(+), 23 deletions(-) diff --git a/client/app/components/EditParameterSettingsDialog.jsx b/client/app/components/EditParameterSettingsDialog.jsx index fbae2eb71d..3d370ee1b9 100644 --- a/client/app/components/EditParameterSettingsDialog.jsx +++ b/client/app/components/EditParameterSettingsDialog.jsx @@ -179,6 +179,7 @@ function EditParameterSettingsDialog(props) { {param.type === 'enum' && ( setParam({ ...param, enumOptions: e.target.value })} diff --git a/client/app/components/Parameters.jsx b/client/app/components/Parameters.jsx index b77c47401c..f1ead2904e 100644 --- a/client/app/components/Parameters.jsx +++ b/client/app/components/Parameters.jsx @@ -1,12 +1,14 @@ import React from 'react'; import PropTypes from 'prop-types'; -import { size, filter, forEach, extend } from 'lodash'; +import { size, filter, forEach, extend, get } from 'lodash'; import { react2angular } from 'react2angular'; import { SortableContainer, SortableElement, DragHandle } from '@/components/sortable'; import { $location } from '@/services/ng'; import { Parameter } from '@/services/parameters'; import ParameterApplyButton from '@/components/ParameterApplyButton'; import ParameterValueInput from '@/components/ParameterValueInput'; +import Form from 'antd/lib/form'; +import Tooltip from 'antd/lib/tooltip'; import EditParameterSettingsDialog from './EditParameterSettingsDialog'; import { toHuman } from '@/filters'; @@ -29,6 +31,9 @@ export class Parameters extends React.Component { onValuesChange: PropTypes.func, onPendingValuesChange: PropTypes.func, onParametersEdit: PropTypes.func, + queryResultErrorData: PropTypes.shape({ + parameters: PropTypes.objectOf(PropTypes.string), + }), }; static defaultProps = { @@ -38,25 +43,35 @@ export class Parameters extends React.Component { onValuesChange: () => {}, onPendingValuesChange: () => {}, onParametersEdit: () => {}, + queryResultErrorData: {}, }; constructor(props) { super(props); const { parameters } = props; - this.state = { parameters }; + this.state = { + parameters, + touched: {}, + }; + if (!props.disableUrlUpdate) { updateUrl(parameters); } } componentDidUpdate = (prevProps) => { - const { parameters, disableUrlUpdate } = this.props; + const { parameters, disableUrlUpdate, queryResultErrorData } = this.props; if (prevProps.parameters !== parameters) { this.setState({ parameters }); if (!disableUrlUpdate) { updateUrl(parameters); } } + + // reset touched flags on new error data + if (prevProps.queryResultErrorData !== queryResultErrorData) { + this.setState({ touched: {} }); + } }; handleKeyDown = (e) => { @@ -69,14 +84,15 @@ export class Parameters extends React.Component { setPendingValue = (param, value, isDirty) => { const { onPendingValuesChange } = this.props; - this.setState(({ parameters }) => { + this.setState(({ parameters, touched }) => { if (isDirty) { param.setPendingValue(value); + touched[param.name] = true; } else { param.clearPendingValue(); } onPendingValuesChange(); - return { parameters }; + return { parameters, touched }; }); }; @@ -109,17 +125,32 @@ export class Parameters extends React.Component { EditParameterSettingsDialog .showModal({ parameter }) .result.then((updated) => { - this.setState(({ parameters }) => { + this.setState(({ parameters, touched }) => { + touched[parameter.name] = true; const updatedParameter = extend(parameter, updated); parameters[index] = Parameter.create(updatedParameter, updatedParameter.parentQueryId); onParametersEdit(); - return { parameters }; + return { parameters, touched }; }); }); }; + getParameterFeedback = (param) => { + // error msg + const { queryResultErrorData } = this.props; + const error = get(queryResultErrorData, ['parameters', param.name], false); + if (error) { + return [error, 'error']; + } + + return []; + }; + renderParameter(param, index) { const { editable } = this.props; + const touched = this.state.touched[param.name]; + const [feedback, status] = this.getParameterFeedback(param); + return (
)}
- this.setPendingValue(param, value, isDirty)} - /> + {feedback} : null} + > + this.setPendingValue(param, value, isDirty)} + /> + ); } diff --git a/client/app/components/Parameters.less b/client/app/components/Parameters.less index 338912fe80..22f968d637 100644 --- a/client/app/components/Parameters.less +++ b/client/app/components/Parameters.less @@ -3,7 +3,7 @@ .parameter-block { display: inline-block; background: white; - padding: 0 12px 6px 0; + padding: 0 12px 17px 0; vertical-align: top; z-index: 1; @@ -15,12 +15,31 @@ .parameter-container.sortable-container & { margin: 4px 0 0 4px; - padding: 3px 6px 6px; + padding: 3px 6px 19px; } &.parameter-dragged { box-shadow: 0 4px 9px -3px rgba(102, 136, 153, 0.15); } + + .ant-form-item { + margin-bottom: 0 !important; + } + + .ant-form-explain { + position: absolute; + left: 0; + right: 0; + bottom: -20px; + font-size: 12px; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + } + + .ant-form-item-control { + line-height: normal; + } } .parameter-heading { @@ -107,4 +126,4 @@ box-shadow: 0 0 0 1px white, -1px 1px 0 1px #5d6f7d85; } } -} +} \ No newline at end of file diff --git a/client/app/pages/queries/query.html b/client/app/pages/queries/query.html index 15ecb22e33..605d8fb6f1 100644 --- a/client/app/pages/queries/query.html +++ b/client/app/pages/queries/query.html @@ -199,7 +199,7 @@

-
diff --git a/client/app/services/query-result.js b/client/app/services/query-result.js index a267cee050..3fdf0cd34c 100644 --- a/client/app/services/query-result.js +++ b/client/app/services/query-result.js @@ -132,7 +132,7 @@ function QueryResultService($resource, $timeout, $q, QueryResultError, Auth) { this.status = 'processing'; } else if (this.job.status === 4) { this.status = statuses[this.job.status]; - this.deferred.reject(new QueryResultError(this.job.error)); + this.deferred.reject(new QueryResultError(this.job.error, this.job.error_data)); } else { this.status = undefined; } @@ -166,6 +166,10 @@ function QueryResultService($resource, $timeout, $q, QueryResultError, Auth) { return this.job.error; } + getErrorData() { + return this.job.error_data || undefined; + } + getLog() { if (!this.query_result.data || !this.query_result.data.log || this.query_result.data.log.length === 0) { return null; diff --git a/client/app/services/query.js b/client/app/services/query.js index 561af14526..c8f19ac176 100644 --- a/client/app/services/query.js +++ b/client/app/services/query.js @@ -140,8 +140,9 @@ class Parameters { function QueryResultErrorFactory($q) { class QueryResultError { - constructor(errorMessage) { + constructor(errorMessage, errorData = {}) { this.errorMessage = errorMessage; + this.errorData = errorData; this.updatedAt = moment.utc(); } @@ -153,6 +154,10 @@ function QueryResultErrorFactory($q) { return this.errorMessage; } + getErrorData() { + return this.errorData || undefined; + } + toPromise() { return $q.reject(this); } diff --git a/client/cypress/integration/query/parameter_spec.js b/client/cypress/integration/query/parameter_spec.js index 35fb9afd7c..4cea541a49 100644 --- a/client/cypress/integration/query/parameter_spec.js +++ b/client/cypress/integration/query/parameter_spec.js @@ -17,6 +17,14 @@ describe('Parameter', () => { }); }; + const expectValueValidationError = (edit, expectedInvalidString = 'Required parameter') => { + cy.getByTestId('ParameterName-test-parameter') + .find('.ant-form-item-control') + .should('have.class', 'has-error') + .find('.ant-form-explain') + .should('contain.text', expectedInvalidString); + }; + beforeEach(() => { cy.login(); }); @@ -28,7 +36,7 @@ describe('Parameter', () => { query: "SELECT '{{test-parameter}}' AS parameter", options: { parameters: [ - { name: 'test-parameter', title: 'Test Parameter', type: 'text' }, + { name: 'test-parameter', title: 'Test Parameter', type: 'text', value: 'text' }, ], }, }; @@ -56,6 +64,16 @@ describe('Parameter', () => { .type('Redash'); }); }); + + it('shows validation error when value is empty', () => { + cy.getByTestId('ParameterName-test-parameter') + .find('input') + .clear(); + + cy.getByTestId('ParameterApplyButton').click(); + + expectValueValidationError(); + }); }); describe('Number Parameter', () => { @@ -65,7 +83,7 @@ describe('Parameter', () => { query: "SELECT '{{test-parameter}}' AS parameter", options: { parameters: [ - { name: 'test-parameter', title: 'Test Parameter', type: 'number' }, + { name: 'test-parameter', title: 'Test Parameter', type: 'number', value: 1 }, ], }, }; @@ -103,6 +121,16 @@ describe('Parameter', () => { .type('{selectall}42'); }); }); + + it('shows validation error when value is empty', () => { + cy.getByTestId('ParameterName-test-parameter') + .find('input') + .clear(); + + cy.getByTestId('ParameterApplyButton').click(); + + expectValueValidationError(); + }); }); describe('Dropdown Parameter', () => { @@ -178,6 +206,36 @@ describe('Parameter', () => { .click(); }); }); + + it('shows validation error when empty', () => { + cy.getByTestId('ParameterSettings-test-parameter').click(); + cy.getByTestId('EnumTextArea').clear(); + cy.clickThrough(` + SaveParameterSettings + ExecuteButton + `); + + expectValueValidationError(); + }); + + it('shows validation error when multi-selection is empty', () => { + cy.clickThrough(` + ParameterSettings-test-parameter + AllowMultipleValuesCheckbox + QuotationSelect + DoubleQuotationMarkOption + SaveParameterSettings + `); + + cy.getByTestId('ParameterName-test-parameter') + .find('.ant-select-remove-icon') + .click(); + + cy.getByTestId('ParameterApplyButton') + .click(); + + expectValueValidationError(); + }); }); describe('Query Based Dropdown Parameter', () => { @@ -306,6 +364,22 @@ describe('Parameter', () => { it('sets dirty state when edited', () => { expectDirtyStateChange(() => selectCalendarDate('15')); }); + + it('shows validation error when value is empty', () => { + selectCalendarDate('15'); + + cy.getByTestId('ParameterApplyButton') + .click(); + + cy.getByTestId('ParameterName-test-parameter') + .find('.ant-calendar-picker-clear') + .click({ force: true }); + + cy.getByTestId('ParameterApplyButton') + .click(); + + expectValueValidationError(); + }); }); describe('Date and Time Parameter', () => { @@ -396,6 +470,32 @@ describe('Parameter', () => { .click(); }); }); + + it('shows validation error when value is empty', () => { + cy.getByTestId('ParameterName-test-parameter') + .find('input') + .as('Input') + .click({ force: true }); + + cy.get('.ant-calendar-date-panel') + .contains('.ant-calendar-date', '15') + .click(); + + cy.get('.ant-calendar-ok-btn') + .click(); + + cy.getByTestId('ParameterApplyButton') + .click(); + + cy.getByTestId('ParameterName-test-parameter') + .find('.ant-calendar-picker-clear') + .click({ force: true }); + + cy.getByTestId('ParameterApplyButton') + .click(); + + expectValueValidationError(); + }); }); describe('Date Range Parameter', () => { @@ -469,6 +569,22 @@ describe('Parameter', () => { it('sets dirty state when edited', () => { expectDirtyStateChange(() => selectCalendarDateRange('15', '20')); }); + + it('shows validation error when value is empty', () => { + selectCalendarDateRange('15', '20'); + + cy.getByTestId('ParameterApplyButton') + .click(); + + cy.getByTestId('ParameterName-test-parameter') + .find('.ant-calendar-picker-clear') + .click({ force: true }); + + cy.getByTestId('ParameterApplyButton') + .click(); + + expectValueValidationError(); + }); }); describe('Apply Changes', () => {