From 399eabd98771a441df621fee369cd58e3233ab5d Mon Sep 17 00:00:00 2001 From: Yash Vibhandik Date: Tue, 23 Nov 2021 11:35:01 +0530 Subject: [PATCH] fix: #4758 added EvaluatedValuePopup over PrimaryColumnsControl to show errors and handled duplicate column name (#8770) * fix: #4758 added EvaluatedValuePopup over PrimaryColumnsControl to show errors and handled duplicate column name * highlighted duplicate column labels, show error message only when duplicate column label focused * updated test case * updated duplicate label indicator logic for test case --- .../Table_Duplicate_ColumnName_spec.js | 33 +++ .../components/ads/DraggableListComponent.tsx | 5 + .../PrimaryColumnsControl.tsx | 248 ++++++++++++++++-- app/client/src/entities/Widget/utils.test.ts | 12 + .../TableWidget/widget/propertyConfig.ts | 14 +- .../TableWidget/widget/propertyUtils.ts | 24 ++ 6 files changed, 315 insertions(+), 21 deletions(-) create mode 100644 app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_Duplicate_ColumnName_spec.js diff --git a/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_Duplicate_ColumnName_spec.js b/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_Duplicate_ColumnName_spec.js new file mode 100644 index 0000000000..55c26fc072 --- /dev/null +++ b/app/client/cypress/integration/Smoke_TestSuite/ClientSideTests/DisplayWidgets/Table_Duplicate_ColumnName_spec.js @@ -0,0 +1,33 @@ +const widgetsPage = require("../../../../locators/Widgets.json"); +const dsl = require("../../../../fixtures/tableNewDsl.json"); +const commonlocators = require("../../../../locators/commonlocators.json"); + +describe("prevent duplicate column name in table", function() { + before(() => { + cy.addDsl(dsl); + }); + + it("evaluted value popup should show when focus on duplicate column input", function() { + cy.openPropertyPane("tablewidget"); + // Updating the column name ; "id" > "TestUpdated" + cy.tableColumnPopertyUpdate("id", "TestUpdated"); + // Updating the column name ; "email" > "TestUpdated" + cy.tableColumnPopertyUpdate("email", "TestUpdated"); + cy.wait("@updateLayout"); + cy.get(commonlocators.evaluatedTypeTitle).should("exist"); + + // Updating the column name ; "userName" > "TestUpdated2" + // this will move focus of input to another column input and let popup close + cy.tableColumnPopertyUpdate("userName", "TestUpdated2"); + + // duplicate column's border should remain red + cy.get("[data-rbd-draggable-id='email'] > div > div").should( + "have.class", + "has-duplicate-label", + ); + }); + + afterEach(() => { + // put your clean up code if any + }); +}); diff --git a/app/client/src/components/ads/DraggableListComponent.tsx b/app/client/src/components/ads/DraggableListComponent.tsx index 978f5c7140..c4e778ac6b 100644 --- a/app/client/src/components/ads/DraggableListComponent.tsx +++ b/app/client/src/components/ads/DraggableListComponent.tsx @@ -12,6 +12,7 @@ type RenderComponentProps = { updateOption: (index: number, value: string) => void; toggleVisibility?: (index: number) => void; onEdit?: (index: number) => void; + updateFocus?: (index: number, isFocused: boolean) => void; }; interface DroppableComponentProps { @@ -23,6 +24,7 @@ interface DroppableComponentProps { toggleVisibility?: (index: number) => void; updateItems: (items: Array>) => void; onEdit?: (index: number) => void; + updateFocus?: (index: number, isFocused: boolean) => void; } export class DroppableComponent extends React.Component< @@ -46,6 +48,7 @@ export class DroppableComponent extends React.Component< id: item.id, label: item.label, isVisible: item.isVisible, + isDuplicateLabel: item.isDuplicateLabel, }; } @@ -60,11 +63,13 @@ export class DroppableComponent extends React.Component< onEdit, renderComponent, toggleVisibility, + updateFocus, updateOption, } = this.props; return renderComponent({ deleteOption, + updateFocus, updateOption, toggleVisibility, onEdit, diff --git a/app/client/src/components/propertyControls/PrimaryColumnsControl.tsx b/app/client/src/components/propertyControls/PrimaryColumnsControl.tsx index 567e026a33..b89d05a979 100644 --- a/app/client/src/components/propertyControls/PrimaryColumnsControl.tsx +++ b/app/client/src/components/propertyControls/PrimaryColumnsControl.tsx @@ -1,4 +1,9 @@ -import React, { useCallback, useEffect, useState } from "react"; +import React, { useCallback, useEffect, useState, Component } from "react"; +import { AppState } from "reducers"; +import { connect } from "react-redux"; +import { Placement } from "popper.js"; +import * as Sentry from "@sentry/react"; +import _ from "lodash"; import BaseControl, { ControlProps } from "./BaseControl"; import { StyledDragIcon, @@ -10,22 +15,38 @@ import { StyledOptionControlInputGroup, } from "./StyledControls"; import styled from "constants/DefaultTheme"; +import { Indices } from "constants/Layers"; import { DroppableComponent } from "components/ads/DraggableListComponent"; -import { ColumnProperties } from "widgets/TableWidget/component/Constants"; +import { Size, Category } from "components/ads/Button"; import EmptyDataState from "components/utils/EmptyDataState"; -import { getNextEntityName } from "utils/AppsmithUtils"; +import EvaluatedValuePopup from "components/editorComponents/CodeEditor/EvaluatedValuePopup"; +import { EditorTheme } from "components/editorComponents/CodeEditor/EditorConfig"; +import { CodeEditorExpected } from "components/editorComponents/CodeEditor"; +import { ColumnProperties } from "widgets/TableWidget/component/Constants"; import { getDefaultColumnProperties, getTableStyles, } from "widgets/TableWidget/component/TableUtilities"; -import { debounce } from "lodash"; -import { Size, Category } from "components/ads/Button"; import { reorderColumns } from "widgets/TableWidget/component/TableHelpers"; +import { DataTree } from "entities/DataTree/dataTreeFactory"; +import { getDataTreeForAutocomplete } from "selectors/dataTreeSelectors"; +import { + EvaluationError, + getEvalErrorPath, + getEvalValuePath, + PropertyEvaluationErrorType, +} from "utils/DynamicBindingUtils"; +import { getNextEntityName } from "utils/AppsmithUtils"; +import { Colors } from "constants/Colors"; +import { noop } from "utils/AppsmithUtils"; const ItemWrapper = styled.div` display: flex; justify-content: flex-start; align-items: center; + &.has-duplicate-label > div:nth-child(2) { + border: 1px solid ${Colors.DANGER_SOLID}; + } `; const TabsWrapper = styled.div` @@ -44,13 +65,33 @@ const AddColumnButton = styled(StyledPropertyPaneButton)` } `; +interface ReduxStateProps { + dynamicData: DataTree; + datasources: any; +} + +type EvaluatedValuePopupWrapperProps = ReduxStateProps & { + isFocused: boolean; + theme: EditorTheme; + popperPlacement?: Placement; + popperZIndex?: Indices; + dataTreePath?: string; + evaluatedValue?: any; + expected?: CodeEditorExpected; + hideEvaluatedValue?: boolean; + useValidationMessage?: boolean; + children: JSX.Element; +}; + type RenderComponentProps = { index: number; item: { label: string; isDerived?: boolean; isVisible?: boolean; + isDuplicateLabel?: boolean; }; + updateFocus?: (index: number, isFocused: boolean) => void; updateOption: (index: number, value: string) => void; onEdit?: (index: number) => void; deleteOption: (index: number) => void; @@ -84,10 +125,12 @@ function ColumnControlComponent(props: RenderComponentProps) { item, onEdit, toggleVisibility, + updateFocus, updateOption, } = props; const [visibility, setVisibility] = useState(item.isVisible); - const debouncedUpdate = debounce(updateOption, 1000); + const debouncedUpdate = _.debounce(updateOption, 1000); + const debouncedFocus = updateFocus ? _.debounce(updateFocus, 400) : noop; const onChange = useCallback( (index: number, value: string) => { setValue(value); @@ -96,11 +139,19 @@ function ColumnControlComponent(props: RenderComponentProps) { [updateOption], ); - const onFocus = () => setEditing(true); - const onBlur = () => setEditing(false); + const onFocus = () => { + setEditing(true); + debouncedFocus(index, true); + }; + const onBlur = () => { + setEditing(false); + debouncedFocus(index, false); + }; return ( - + { +type State = { + focusedIndex: number | null; + duplicateColumnIds: string[]; +}; + +class PrimaryColumnsControl extends BaseControl { + constructor(props: ControlProps) { + super(props); + + const columns: Record = props.propertyValue || {}; + const columnOrder = Object.keys(columns); + const reorderedColumns = reorderColumns(columns, columnOrder); + const tableColumnLabels = _.map(reorderedColumns, "label"); + const duplicateColumnIds = []; + + for (let index = 0; index < tableColumnLabels.length; index++) { + const currLabel = tableColumnLabels[index] as string; + const duplicateValueIndex = tableColumnLabels.indexOf(currLabel); + if (duplicateValueIndex !== index) { + // get column id from columnOrder index + duplicateColumnIds.push(reorderedColumns[columnOrder[index]].id); + } + } + + this.state = { + focusedIndex: null, + duplicateColumnIds, + }; + } + render() { // Get columns from widget properties const columns: Record = @@ -183,22 +263,38 @@ class PrimaryColumnsControl extends BaseControl { isVisible: column.isVisible, isDerived: column.isDerived, index: column.index, + isDuplicateLabel: _.includes( + this.state.duplicateColumnIds, + column.id, + ), }; }, ); + const column: ColumnProperties | undefined = Object.values( + reorderedColumns, + ).find( + (column: ColumnProperties) => column.index === this.state.focusedIndex, + ); + // show popup on duplicate column label input focused + const isFocused = + !_.isNull(this.state.focusedIndex) && + _.includes(this.state.duplicateColumnIds, column?.id); return ( - + + + { propertiesToDelete.push(`columnOrder[${columnOrderIndex}]`); this.deleteProperties(propertiesToDelete); + // if column deleted, clean up duplicateIndexes + let duplicateColumnIds = [...this.state.duplicateColumnIds]; + duplicateColumnIds = duplicateColumnIds.filter( + (id) => id !== originalColumn.id, + ); + this.setState({ duplicateColumnIds }); } }; @@ -317,12 +419,118 @@ class PrimaryColumnsControl extends BaseControl { `${this.props.propertyName}.${originalColumn.id}.label`, updatedLabel, ); + // check entered label is unique or duplicate + const tableColumnLabels = _.map(columns, "label"); + let duplicateColumnIds = [...this.state.duplicateColumnIds]; + // if duplicate, add into array + if (_.includes(tableColumnLabels, updatedLabel)) { + duplicateColumnIds.push(originalColumn.id); + this.setState({ duplicateColumnIds }); + } else { + duplicateColumnIds = duplicateColumnIds.filter( + (id) => id !== originalColumn.id, + ); + this.setState({ duplicateColumnIds }); + } } }; + updateFocus = (index: number, isFocused: boolean) => { + this.setState({ focusedIndex: isFocused ? index : null }); + }; + static getControlType() { return "PRIMARY_COLUMNS"; } } export default PrimaryColumnsControl; + +/** + * wrapper component on dragable primary columns + * render popup if primary column labels are not unique + * show unique name error in PRIMARY_COLUMNS + */ +class EvaluatedValuePopupWrapperClass extends Component< + EvaluatedValuePopupWrapperProps +> { + getPropertyValidation = ( + dataTree: DataTree, + dataTreePath?: string, + ): { + isInvalid: boolean; + errors: EvaluationError[]; + pathEvaluatedValue: unknown; + } => { + if (!dataTreePath) { + return { + isInvalid: false, + errors: [], + pathEvaluatedValue: undefined, + }; + } + + const errors = _.get( + dataTree, + getEvalErrorPath(dataTreePath), + [], + ) as EvaluationError[]; + + const filteredLintErrors = errors.filter( + (error) => error.errorType !== PropertyEvaluationErrorType.LINT, + ); + + const pathEvaluatedValue = _.get(dataTree, getEvalValuePath(dataTreePath)); + + return { + isInvalid: filteredLintErrors.length > 0, + errors: filteredLintErrors, + pathEvaluatedValue, + }; + }; + + render = () => { + const { + dataTreePath, + dynamicData, + evaluatedValue, + expected, + hideEvaluatedValue, + useValidationMessage, + } = this.props; + const { + errors, + isInvalid, + pathEvaluatedValue, + } = this.getPropertyValidation(dynamicData, dataTreePath); + let evaluated = evaluatedValue; + if (dataTreePath) { + evaluated = pathEvaluatedValue; + } + + return ( + + {this.props.children} + + ); + }; +} +const mapStateToProps = (state: AppState): ReduxStateProps => ({ + dynamicData: getDataTreeForAutocomplete(state), + datasources: state.entities.datasources, +}); + +const EvaluatedValuePopupWrapper = Sentry.withProfiler( + connect(mapStateToProps)(EvaluatedValuePopupWrapperClass), +); diff --git a/app/client/src/entities/Widget/utils.test.ts b/app/client/src/entities/Widget/utils.test.ts index 12847e339e..5c76980964 100644 --- a/app/client/src/entities/Widget/utils.test.ts +++ b/app/client/src/entities/Widget/utils.test.ts @@ -123,6 +123,7 @@ describe("getAllPathsFromPropertyConfig", () => { defaultSearchText: EvaluationSubstitutionType.TEMPLATE, defaultSelectedRow: EvaluationSubstitutionType.TEMPLATE, isVisible: EvaluationSubstitutionType.TEMPLATE, + primaryColumns: "TEMPLATE", isSortable: EvaluationSubstitutionType.TEMPLATE, compactMode: EvaluationSubstitutionType.TEMPLATE, delimiter: EvaluationSubstitutionType.TEMPLATE, @@ -199,6 +200,16 @@ describe("getAllPathsFromPropertyConfig", () => { isVisible: { type: "BOOLEAN", }, + primaryColumns: { + params: { + expected: { + autocompleteDataType: AutocompleteDataType.STRING, + example: "abc", + type: "Unique Column Names", + }, + }, + type: "FUNCTION", + }, isSortable: { type: "BOOLEAN", params: { @@ -222,6 +233,7 @@ describe("getAllPathsFromPropertyConfig", () => { // Note: Removing until we figure out how functions are represented here. delete result.validationPaths.defaultSelectedRow.params?.fn; + delete result.validationPaths.primaryColumns.params?.fn; expect(result).toStrictEqual(expected); }); diff --git a/app/client/src/widgets/TableWidget/widget/propertyConfig.ts b/app/client/src/widgets/TableWidget/widget/propertyConfig.ts index e4e44fec90..19b3611727 100644 --- a/app/client/src/widgets/TableWidget/widget/propertyConfig.ts +++ b/app/client/src/widgets/TableWidget/widget/propertyConfig.ts @@ -18,6 +18,7 @@ import { updateIconAlignment, getBasePropertyPath, hideByColumnType, + uniqueColumnNameValidation, } from "./propertyUtils"; export default [ @@ -49,8 +50,19 @@ export default [ label: "Columns", updateHook: updateDerivedColumnsHook, dependencies: ["derivedColumns", "columnOrder"], - isBindProperty: false, + isBindProperty: true, isTriggerProperty: false, + validation: { + type: ValidationTypes.FUNCTION, + params: { + fn: uniqueColumnNameValidation, + expected: { + type: "Unique Column Names", + example: "abc", + autocompleteDataType: AutocompleteDataType.STRING, + }, + }, + }, panelConfig: { editableTitle: true, titlePropertyName: "label", diff --git a/app/client/src/widgets/TableWidget/widget/propertyUtils.ts b/app/client/src/widgets/TableWidget/widget/propertyUtils.ts index 7060ba35cf..3cbd1aa8a5 100644 --- a/app/client/src/widgets/TableWidget/widget/propertyUtils.ts +++ b/app/client/src/widgets/TableWidget/widget/propertyUtils.ts @@ -125,6 +125,30 @@ export function totalRecordsCountValidation( }; } +export function uniqueColumnNameValidation( + value: unknown, + props: TableWidgetProps, + _?: any, +) { + const tableColumns = _.map(value, "label"); + const duplicates = tableColumns.filter( + (val: string, index: number, arr: string[]) => arr.indexOf(val) !== index, + ); + const hasError = !!duplicates.length; + if (value && hasError) { + return { + isValid: false, + parsed: value, + messages: ["Column names should be unique."], + }; + } + return { + isValid: true, + parsed: value, + messages: [], + }; +} + // A hook to update all column styles when global table styles are updated export const updateColumnStyles = ( props: TableWidgetProps,