-
Notifications
You must be signed in to change notification settings - Fork 4
Display spreadsheet errors and disable math.js internal functions #3439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2ad106c
2e0e645
0f5a7fd
1e026ea
9d39628
73e7477
d84499b
c18a6ac
8e68192
5e8e901
057267e
c8790b1
a40f92b
0d1f0ec
5f2fae4
da7637a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import { isBlankOrEmpty } from 'components/utils/validation-functions'; | |
| import { ICellRendererParams } from 'ag-grid-community'; | ||
| import { CustomCellRendererProps } from 'ag-grid-react'; | ||
| import { mergeSx, type MuiStyles } from '@gridsuite/commons-ui'; | ||
| import { IntlShape } from 'react-intl'; | ||
|
|
||
| const styles = { | ||
| tableCell: (theme) => ({ | ||
|
|
@@ -35,6 +36,17 @@ const styles = { | |
| }, | ||
| } as const satisfies MuiStyles; | ||
|
|
||
| const FORMULA_ERROR_KEY = 'spreadsheet/formula/error'; | ||
|
|
||
| interface BaseCellRendererProps { | ||
| value: string | undefined; | ||
| tooltip?: string; | ||
| } | ||
|
|
||
| interface ErrorCellRendererParams extends ICellRendererParams { | ||
| intl: IntlShape; | ||
| } | ||
|
|
||
| export const BooleanCellRenderer = (props: any) => { | ||
| const isChecked = props.value; | ||
| return ( | ||
|
|
@@ -105,15 +117,23 @@ export const NumericCellRenderer = (props: NumericCellRendererProps) => { | |
| ); | ||
| }; | ||
|
|
||
| const BaseCellRenderer = ({ value, tooltip }: BaseCellRendererProps) => ( | ||
| <Box sx={mergeSx(styles.tableCell)}> | ||
| <Tooltip disableFocusListener disableTouchListener title={tooltip || value || ''}> | ||
| <Box sx={styles.overflow}>{value}</Box> | ||
| </Tooltip> | ||
| </Box> | ||
| ); | ||
|
|
||
| export const ErrorCellRenderer = (props: ErrorCellRendererParams) => { | ||
| const errorMessage = props.intl.formatMessage({ id: props.value?.error }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are in react context here already, you can use useIntl directly you don't need to pass it through props |
||
| const errorValue = props.intl.formatMessage({ id: FORMULA_ERROR_KEY }); | ||
| return <BaseCellRenderer value={errorValue} tooltip={errorMessage} />; | ||
| }; | ||
|
|
||
| export const DefaultCellRenderer = (props: CustomCellRendererProps) => { | ||
| const cellValue = formatCell(props); | ||
| return ( | ||
| <Box sx={mergeSx(styles.tableCell)}> | ||
| <Tooltip disableFocusListener disableTouchListener title={cellValue.value?.toString()}> | ||
| <Box sx={styles.overflow}>{cellValue.value?.toString()}</Box> | ||
| </Tooltip> | ||
| </Box> | ||
| ); | ||
| const cellValue = formatCell(props).value?.toString(); | ||
| return <BaseCellRenderer value={cellValue} />; | ||
| }; | ||
|
|
||
| export const NetworkModificationNameCellRenderer = (props: CustomCellRendererProps) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { ColumnMenuProps } from '../../spreadsheet-view/columns/column-menu'; | |
| import { SortParams } from '../hooks/use-custom-aggrid-sort'; | ||
| import { COLUMN_TYPES, CustomCellType } from '../custom-aggrid-header.type'; | ||
| import type { UUID } from 'node:crypto'; | ||
| import { ValidationResult } from '../../spreadsheet-view/columns/utils/formula-validator'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. import type |
||
|
|
||
| export enum FILTER_DATA_TYPES { | ||
| TEXT = 'text', | ||
|
|
@@ -67,7 +68,7 @@ export interface ColumnContext<F extends CustomAggridFilterParams = CustomAggrid | |
| } | ||
|
|
||
| export interface CustomColDef<TData = any, F extends CustomAggridFilterParams = CustomAggridFilterParams> | ||
| extends ColDef<TData, boolean | string | number | CustomCellType> { | ||
| extends ColDef<TData, boolean | string | number | CustomCellType | ValidationResult> { | ||
| colId: string; | ||
| context?: ColumnContext<F>; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,14 +14,22 @@ import { | |
| numberColumnDefinition, | ||
| textColumnDefinition, | ||
| } from '../common-column-definitions'; | ||
| import { validateFormulaResult } from './formula-validator'; | ||
| import { isValidationError, validateFormulaResult, ValidationResult } from './formula-validator'; | ||
| import { ColumnDefinition, SpreadsheetTabDefinition } from '../../types/spreadsheet.type'; | ||
| import { CustomColDef } from '../../../custom-aggrid/custom-aggrid-filters/custom-aggrid-filter.type'; | ||
| import { isCalculationRow } from '../../utils/calculation-utils'; | ||
| import { ErrorCellRenderer } from '../../../custom-aggrid/cell-renderers'; | ||
| import { IntlShape, useIntl } from 'react-intl'; | ||
| import { useMemo } from 'react'; | ||
|
|
||
| export const useColumnDefinitions = (tableDefinition: SpreadsheetTabDefinition) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is not useful |
||
| const intl = useIntl(); | ||
| return useMemo(() => addFormulaErrorsRenderer(intl, mapColumns(tableDefinition)), [intl, tableDefinition]); | ||
| }; | ||
|
|
||
| const createValueGetter = | ||
| (colDef: ColumnDefinition) => | ||
| (params: ValueGetterParams): boolean | string | number | undefined => { | ||
| (params: ValueGetterParams): ValidationResult | boolean | string | number | undefined => { | ||
| try { | ||
| // Skip formula processing for pinned rows and use raw value | ||
| if (isCalculationRow(params.node?.data?.rowType)) { | ||
|
|
@@ -37,15 +45,15 @@ const createValueGetter = | |
| const validation = validateFormulaResult(result, colDef.type); | ||
|
|
||
| if (!validation.isValid) { | ||
| return undefined; | ||
| return validation; | ||
| } | ||
| return result; | ||
| } catch (e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we could manage any "evaluate" error from MathJS in this catch. Next step ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Precisely :) I elaborated on this in the going further section or the summary |
||
| return undefined; | ||
| } | ||
| }; | ||
|
|
||
| export const mapColumns = (tableDefinition: SpreadsheetTabDefinition) => | ||
| const mapColumns = (tableDefinition: SpreadsheetTabDefinition) => | ||
| tableDefinition?.columns.map((colDef): CustomColDef => { | ||
| let baseDefinition: ColDef; | ||
|
|
||
|
|
@@ -87,3 +95,11 @@ export const mapColumns = (tableDefinition: SpreadsheetTabDefinition) => | |
| enableCellChangeFlash: true, | ||
| }; | ||
| }); | ||
|
|
||
| const addFormulaErrorsRenderer = (intl: IntlShape, columns: CustomColDef[]): CustomColDef[] => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't you add it in mapColumns directly ? |
||
| return columns.map((col) => ({ | ||
| ...col, | ||
| cellRendererSelector: (params) => | ||
| isValidationError(params.value) ? { component: ErrorCellRenderer, params: { intl } } : undefined, //Returning undefined make it so the originally defined renderer is used | ||
| })); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,34 +7,47 @@ | |||||
| import { COLUMN_TYPES } from 'components/custom-aggrid/custom-aggrid-header.type'; | ||||||
| import { MAX_FORMULA_CHARACTERS } from '../../constants'; | ||||||
|
|
||||||
| interface ValidationResult { | ||||||
| export interface ValidationResult { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe name it ValidationError directly with |
||||||
| isValid: boolean; | ||||||
| error?: string; | ||||||
| } | ||||||
|
|
||||||
| export function isValidationResult(value: unknown): value is ValidationResult { | ||||||
| return ( | ||||||
| typeof value === 'object' && value !== null && value.hasOwnProperty('isValid') && value.hasOwnProperty('error') | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| export function isValidationError(value: unknown): value is ValidationResult { | ||||||
| return isValidationResult(value) && !value.isValid; | ||||||
| } | ||||||
|
|
||||||
| export const formatValidationResult = (isValid: boolean, messageId?: string): ValidationResult => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not really helpful |
||||||
| return { isValid: isValid, error: messageId }; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| }; | ||||||
|
|
||||||
| export const validateFormulaResult = (value: any, type: COLUMN_TYPES): ValidationResult => { | ||||||
| if (isValidationResult(value)) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this method it could be, if check passes returns the orginal value, if not returns a ValidationError |
||||||
| return value; | ||||||
| } | ||||||
|
|
||||||
| switch (type) { | ||||||
| case COLUMN_TYPES.NUMBER: | ||||||
| return { | ||||||
| isValid: | ||||||
| (typeof value === 'number' && !isNaN(value)) || | ||||||
| (typeof value !== 'boolean' && !isNaN(Number(value))), | ||||||
| error: 'Formula must evaluate to a number', | ||||||
| }; | ||||||
| return formatValidationResult( | ||||||
| (typeof value === 'number' && !isNaN(value)) || (typeof value !== 'boolean' && !isNaN(Number(value))), | ||||||
| 'spreadsheet/formula/type/number' | ||||||
| ); | ||||||
| case COLUMN_TYPES.BOOLEAN: | ||||||
| return { | ||||||
| isValid: typeof value === 'boolean', | ||||||
| error: 'Formula must evaluate to a boolean', | ||||||
| }; | ||||||
| return formatValidationResult(typeof value === 'boolean', 'spreadsheet/formula/type/boolean'); | ||||||
| case COLUMN_TYPES.ENUM: | ||||||
| return { | ||||||
| isValid: typeof value === 'string' || typeof value === 'number', | ||||||
| error: 'Formula must evaluate to a string', | ||||||
| }; | ||||||
| return formatValidationResult( | ||||||
| typeof value === 'string' || typeof value === 'number', | ||||||
| 'spreadsheet/formula/type/enum' | ||||||
| ); | ||||||
| case COLUMN_TYPES.TEXT: | ||||||
| return { isValid: true }; // Text accepts any type | ||||||
| return formatValidationResult(true); // Text accepts any type | ||||||
| default: | ||||||
| return { isValid: false, error: 'Unknown column type' }; | ||||||
| return formatValidationResult(false, 'spreadsheet/formula/type/unknown'); | ||||||
| } | ||||||
| }; | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import type