diff --git a/.changelogs/8138.json b/.changelogs/8138.json new file mode 100644 index 00000000000..8d2fc7a56fe --- /dev/null +++ b/.changelogs/8138.json @@ -0,0 +1,7 @@ +{ + "title": "Fixed an issue where the validator function is called twice when the Formulas plugin is enabled.", + "type": "fixed", + "issue": 8138, + "breaking": false, + "framework": "none" +} diff --git a/src/plugins/formulas/__tests__/validation.spec.js b/src/plugins/formulas/__tests__/validation.spec.js index 65a174ef3a9..fb670b8a84a 100644 --- a/src/plugins/formulas/__tests__/validation.spec.js +++ b/src/plugins/formulas/__tests__/validation.spec.js @@ -6,6 +6,7 @@ describe('Formulas general', () => { beforeEach(function() { this.$container = $(`
`).appendTo('body'); + this.$container2 = $(`
`).appendTo('body'); }); afterEach(function() { @@ -14,9 +15,35 @@ describe('Formulas general', () => { } if (this.$container) { - destroy(); + try { + if (this.$container.handsontable('getInstance')) { + destroy(); + } + } catch (e) { + // In some of the test cases we're manually destroying the Handsontable instances, so 'getInstance' may + // throw a post-mortem error. + if (!e.message.includes('instance has been destroyed')) { + throw e; + } + } + this.$container.remove(); } + + if (this.$container2) { + try { + if (this.$container2.handsontable('getInstance')) { + this.$container2.handsontable('getInstance').destroy(); + } + } catch (e) { + // In some of the test cases we're manually destroying the Handsontable instances, so 'getInstance' may + // throw a post-mortem error. + if (!e.message.includes('instance has been destroyed')) { + throw e; + } + } + this.$container2.remove(); + } }); describe('cooperation with validation', () => { @@ -53,100 +80,138 @@ describe('Formulas general', () => { }); it('should validate result of formula for dependant cells properly (formula returns text)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ [0, '=A1', '=B1', '=C1'] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.setDataAtCell(0, 0, 'text'); + setDataAtCell(0, 0, 'text'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 1)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 1, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('text', 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 1)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); }); it('should validate result of formula and dependant cells properly (formula returns error)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ ['=B1+5', 2, '=A1', '=C1'] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.setDataAtCell(0, 0, '=B1+5a'); + setDataAtCell(0, 0, '=B1+5a'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).toHaveBeenCalledWith('#ERROR!', 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#ERROR!', 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#ERROR!', 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); - hot.setDataAtCell(0, 0, '=B1+5'); + setDataAtCell(0, 0, '=B1+5'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(false); }); it('should validate result of formula and dependant cells properly (formula create circular dependency)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ ['=B1+5', 2, '=A1', '=C1'] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.setDataAtCell(0, 0, '=C1'); + setDataAtCell(0, 0, '=C1'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).toHaveBeenCalledWith('#CYCLE!', 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#CYCLE!', 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#CYCLE!', 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); - hot.setDataAtCell(0, 0, '=B1+5'); + setDataAtCell(0, 0, '=B1+5'); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 2, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(7, 0, 3, void 0, void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(false); }); it('should validate result of formula and dependant cells properly (formula create #REF! error)', async() => { - const hot = handsontable({ + const beforeValidate = jasmine.createSpy('beforeValidate'); + + handsontable({ data: [ ['=E1', 2, '=A1', '=C1', 22] ], formulas: { engine: HyperFormula }, - type: 'numeric' + type: 'numeric', + beforeValidate, }); - hot.alter('remove_col', 4); + alter('remove_col', 4); await sleep(100); // Validator is asynchronous. - expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); - expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); + expect(beforeValidate).not.toHaveBeenCalled(); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(false); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(false); + + await new Promise(resolve => validateCells(resolve)); + + expect(beforeValidate).toHaveBeenCalledWith('#REF!', 0, 0, 'validateCells', void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#REF!', 0, 2, 'validateCells', void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith('#REF!', 0, 3, 'validateCells', void 0, void 0); + expect($(getCell(0, 0)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 2)).hasClass(getSettings().invalidCellClassName)).toBe(true); + expect($(getCell(0, 3)).hasClass(getSettings().invalidCellClassName)).toBe(true); }); it('should not automatically validate changes when the engine is modified from the outside code', async() => { @@ -168,8 +233,7 @@ describe('Formulas general', () => { expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); expect($(getCell(0, 3)).hasClass(hot.getSettings().invalidCellClassName)).toBe(false); - hot.validateCells(); - await sleep(100); // Validator is asynchronous. + await new Promise(resolve => hot.validateCells(resolve)); expect($(getCell(0, 0)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); expect($(getCell(0, 2)).hasClass(hot.getSettings().invalidCellClassName)).toBe(true); @@ -201,5 +265,82 @@ describe('Formulas general', () => { expect(afterValidate).toHaveBeenCalledWith(false, '=A1', 0, 6, 'validateCells', void 0); expect(afterValidate).toHaveBeenCalledWith(false, '12/1/2016', 0, 7, 'validateCells', void 0); }); + + it('should only call the validator once for modified cells', async() => { + const validator1 = jasmine.createSpy('validator1').and.callFake((value, callback) => callback(true)); + const validator2 = jasmine.createSpy('validator2').and.callFake((value, callback) => callback(true)); + + handsontable({ + data: [ + ['=B1+5', 2, '=D1', 'text', 'foo'] + ], + formulas: { + engine: HyperFormula, + sheetName: 'Sheet1' + }, + validator: validator1, + }); + spec().$container2.handsontable({ + data: [ + ['=Sheet1!A1', 2, '=D1', 'D', '=A1+1'] + ], + formulas: { + engine: getPlugin('formulas').engine, + sheetName: 'Sheet2' + }, + validator: validator2, + }); + + setDataAtCell(0, 1, 6); + + await sleep(100); // Validator is asynchronous. + + expect(validator1).toHaveBeenCalledTimes(2); + expect(validator1).toHaveBeenCalledWith(6, jasmine.any(Function)); + expect(validator1).toHaveBeenCalledWith(11, jasmine.any(Function)); + expect(validator2).toHaveBeenCalledTimes(2); + expect(validator2).toHaveBeenCalledWith(11, jasmine.any(Function)); + expect(validator2).toHaveBeenCalledWith(12, jasmine.any(Function)); + + setDataAtCell(0, 4, 'bar'); + + await sleep(100); // Validator is asynchronous. + + expect(validator1).toHaveBeenCalledTimes(3); + expect(validator1).toHaveBeenCalledWith('bar', jasmine.any(Function)); + expect(validator2).toHaveBeenCalledTimes(2); + }); + + it('should validate correct visual cells', async() => { + const beforeValidate = jasmine.createSpy('beforeValidate'); + const hot = handsontable({ + data: [ + ['1', 2, '=D1', 'text1', 'foo1'], + ['2', 2, '=D2', 'text2', 'foo2'], + ['3', 2, '=D3', 'text3', 'foo3'], + ['4', 2, '=D4', 'text4', 'foo4'], + ['5', 2, '=D5', 'text5', '=A1+3'], + ], + formulas: { + engine: HyperFormula + }, + validator(value, callback) { + callback(false); + }, + beforeValidate, + }); + + hot.columnIndexMapper.indexesSequence.setValues([0, 2, 3, 4, 1]); + hot.rowIndexMapper.indexesSequence.setValues([0, 2, 3, 4, 1]); + + render(); + setDataAtCell(0, 0, 6); + + await sleep(100); // Validator is asynchronous. + + expect(beforeValidate).toHaveBeenCalledTimes(2); + expect(beforeValidate).toHaveBeenCalledWith(6, 0, 0, void 0, void 0, void 0); + expect(beforeValidate).toHaveBeenCalledWith(9, 3, 4, void 0, void 0, void 0); + }); }); }); diff --git a/src/plugins/formulas/engine/register.js b/src/plugins/formulas/engine/register.js index d15745fe76e..93cf1ccc6c1 100644 --- a/src/plugins/formulas/engine/register.js +++ b/src/plugins/formulas/engine/register.js @@ -130,13 +130,16 @@ export function registerEngine(engineClass, hotSettings, hotInstance) { } /** + * Returns the list of the Handsontable instances linked to the specific engine instance. + * * @param {object} engine The engine instance. - * @returns {Handsontable[]} Returns an array with Handsontable instances. + * @returns {Map} Returns Map with Handsontable instances. */ export function getRegisteredHotInstances(engine) { const engineRegistry = getEngineRelationshipRegistry(); + const hotInstances = engineRegistry.size === 0 ? [] : Array.from(engineRegistry.get(engine) ?? []); - return engineRegistry.size === 0 ? [] : Array.from(engineRegistry.get(engine) ?? []); + return new Map(hotInstances.map(hot => [hot.getPlugin('formulas').sheetId, hot])); } /** diff --git a/src/plugins/formulas/formulas.js b/src/plugins/formulas/formulas.js index e673b32dbf1..0e413d40da1 100644 --- a/src/plugins/formulas/formulas.js +++ b/src/plugins/formulas/formulas.js @@ -146,10 +146,12 @@ export class Formulas extends BasePlugin { this.addHook('afterLoadData', (...args) => this.onAfterLoadData(...args)); this.addHook('modifyData', (...args) => this.onModifyData(...args)); this.addHook('modifySourceData', (...args) => this.onModifySourceData(...args)); - this.addHook('afterSetSourceDataAtCell', (...args) => this.onAfterSetSourceDataAtCell(...args)); - this.addHook('beforeChange', (...args) => this.onBeforeChange(...args)); this.addHook('beforeValidate', (...args) => this.onBeforeValidate(...args)); + this.addHook('afterSetSourceDataAtCell', (...args) => this.onAfterSetSourceDataAtCell(...args)); + this.addHook('afterSetDataAtCell', (...args) => this.onAfterSetDataAtCell(...args)); + this.addHook('afterSetDataAtRowProp', (...args) => this.onAfterSetDataAtCell(...args)); + this.addHook('beforeCreateRow', (...args) => this.onBeforeCreateRow(...args)); this.addHook('beforeCreateCol', (...args) => this.onBeforeCreateCol(...args)); @@ -386,13 +388,13 @@ export class Formulas extends BasePlugin { * recalculated dependent cells. * * @private - * @param {object[]} changedCells The values and location of applied changes within HF engine. + * @param {object[]} dependentCells The values and location of applied changes within HF engine. * @param {boolean} [renderSelf] `true` if it's supposed to render itself, `false` otherwise. */ - renderDependentSheets(changedCells, renderSelf = false) { + renderDependentSheets(dependentCells, renderSelf = false) { const affectedSheetIds = new Set(); - changedCells.forEach((change) => { + dependentCells.forEach((change) => { // For the Named expression the address is empty, hence the `sheetId` is undefined. const sheetId = change?.address?.sheet; @@ -400,22 +402,10 @@ export class Formulas extends BasePlugin { if (!affectedSheetIds.has(sheetId)) { affectedSheetIds.add(sheetId); } - - if (!this.#internalOperationPending && sheetId === this.sheetId) { - const { row, col } = change.address; - - // It will just re-render certain cell when necessary. - this.hot.validateCell(this.hot.getDataAtCell(row, col), this.hot.getCellMeta(row, col), () => {}); - } } }); - const hotInstances = new Map( - getRegisteredHotInstances(this.engine) - .map(hot => [hot.getPlugin('formulas').sheetId, hot]) - ); - - hotInstances.forEach((relatedHot, sheetId) => { + getRegisteredHotInstances(this.engine).forEach((relatedHot, sheetId) => { if ( (renderSelf || (sheetId !== this.sheetId)) && affectedSheetIds.has(sheetId) @@ -426,6 +416,48 @@ export class Formulas extends BasePlugin { }); } + /** + * Validates dependent cells based on the cells that are modified by the change. + * + * @private + * @param {object[]} dependentCells The values and location of applied changes within HF engine. + * @param {object[]} [changedCells] The values and location of applied changes by developer (through API or UI). + */ + validateDependentCells(dependentCells, changedCells = []) { + const stringifyAddress = (change) => { + const { + row, + col, + sheet + } = change?.address ?? {}; + + return isDefined(sheet) ? `${sheet}:${row}x${col}` : ''; + }; + const changedCellsSet = new Set(changedCells.map(change => stringifyAddress(change))); + + dependentCells.forEach((change) => { + // For the Named expression the address is empty, hence the `sheetId` is undefined. + const sheetId = change?.address?.sheet; + const addressId = stringifyAddress(change); + + // Validate the cells that depend on the calculated formulas. Skip that cells + // where the user directly changes the values - the Core triggers those validators. + if (sheetId !== void 0 && !changedCellsSet.has(addressId)) { + const { row, col } = change.address; + const visualRow = this.hot.toVisualRow(row); + const visualColumn = this.hot.toVisualColumn(col); + const hot = getRegisteredHotInstances(this.engine).get(sheetId); + + // It will just re-render certain cell when necessary. + hot.validateCell( + hot.getDataAtCell(visualRow, visualColumn), + hot.getCellMeta(visualRow, visualColumn), + () => {} + ); + } + }); + } + /** * Sync a change from the change-related hooks with the engine. * @@ -471,7 +503,10 @@ export class Formulas extends BasePlugin { sheet: this.sheetId, }; - return this.engine.getCellValue(address); + const cellValue = this.engine.getCellValue(address); + + // If `cellValue` is an object it is expected to be an error + return (typeof cellValue === 'object' && cellValue !== null) ? cellValue.value : cellValue; } return value; @@ -623,24 +658,34 @@ export class Formulas extends BasePlugin { } /** - * `onBeforeChange` hook callback. + * `onAfterSetDataAtCell` hook callback. * * @private * @param {Array[]} changes An array of changes in format [[row, prop, oldValue, value], ...]. */ - onBeforeChange(changes) { + onAfterSetDataAtCell(changes) { const dependentCells = []; const outOfBoundsChanges = []; + const changedCells = []; changes.forEach(([row, prop, , newValue]) => { const column = this.hot.propToCol(prop); + const physicalRow = this.hot.toPhysicalRow(row); + const physicalColumn = this.hot.toPhysicalColumn(column); + const address = { + row: physicalRow, + col: physicalColumn, + sheet: this.sheetId, + }; - if (this.hot.toPhysicalRow(row) !== null && this.hot.toPhysicalColumn(column) !== null) { + if (physicalRow !== null && physicalColumn !== null) { dependentCells.push(...this.syncChangeWithEngine(row, column, newValue)); } else { outOfBoundsChanges.push([row, column, newValue]); } + + changedCells.push({ address }); }); if (outOfBoundsChanges.length) { @@ -658,6 +703,7 @@ export class Formulas extends BasePlugin { } this.renderDependentSheets(dependentCells); + this.validateDependentCells(dependentCells, changedCells); } /** @@ -668,6 +714,7 @@ export class Formulas extends BasePlugin { */ onAfterSetSourceDataAtCell(changes) { const dependentCells = []; + const changedCells = []; changes.forEach(([row, column, , newValue]) => { const address = { @@ -682,10 +729,12 @@ export class Formulas extends BasePlugin { return; } + changedCells.push({ address }); dependentCells.push(...this.engine.setCellContents(address, newValue)); }); this.renderDependentSheets(dependentCells); + this.validateDependentCells(dependentCells, changedCells); } /** diff --git a/test/helpers/common.js b/test/helpers/common.js index c22ca03a9d2..b117509565e 100644 --- a/test/helpers/common.js +++ b/test/helpers/common.js @@ -92,6 +92,7 @@ export const getSelected = handsontableMethodFactory('getSelected'); export const getSelectedLast = handsontableMethodFactory('getSelectedLast'); export const getSelectedRange = handsontableMethodFactory('getSelectedRange'); export const getSelectedRangeLast = handsontableMethodFactory('getSelectedRangeLast'); +export const getSettings = handsontableMethodFactory('getSettings'); export const getSourceData = handsontableMethodFactory('getSourceData'); export const getSourceDataArray = handsontableMethodFactory('getSourceDataArray'); export const getSourceDataAtCell = handsontableMethodFactory('getSourceDataAtCell'); @@ -114,14 +115,16 @@ export const selectColumns = handsontableMethodFactory('selectColumns'); export const selectRows = handsontableMethodFactory('selectRows'); export const setCellMeta = handsontableMethodFactory('setCellMeta'); export const setDataAtCell = handsontableMethodFactory('setDataAtCell'); -export const setSourceDataAtCell = handsontableMethodFactory('setSourceDataAtCell'); export const setDataAtRowProp = handsontableMethodFactory('setDataAtRowProp'); +export const setSourceDataAtCell = handsontableMethodFactory('setSourceDataAtCell'); export const spliceCellsMeta = handsontableMethodFactory('spliceCellsMeta'); export const spliceCol = handsontableMethodFactory('spliceCol'); export const spliceRow = handsontableMethodFactory('spliceRow'); export const toVisualRow = handsontableMethodFactory('toVisualRow'); -export const updateSettings = handsontableMethodFactory('updateSettings'); export const undo = handsontableMethodFactory('undo'); +export const updateSettings = handsontableMethodFactory('updateSettings'); +export const validateCell = handsontableMethodFactory('validateCell'); +export const validateCells = handsontableMethodFactory('validateCells'); const specContext = {};