Swapping beforeChange and `afterSetDataAtCell/afterSetDataAtRowProp…

…` hooks order (#10231)

Co-authored-by: Krzysztof Budnik <571316+budnix@users.noreply.github.com>

.changelogs/677.json

@@ -0,0 +1,8 @@
+{
+  "issuesOrigin": "private",
+  "title": "Swapping `beforeChange` and `afterSetDataAtCell`/`afterSetDataAtRowProp` hooks order",
+  "type": "changed",
+  "issueOrPR": 677,
+  "breaking": true,
+  "framework": "none"
+}

handsontable/src/core.js

@@ -1258,19 +1258,8 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
     }
 
     const activeEditor = instance.getActiveEditor();
-    const beforeChangeResult = instance.runHooks('beforeChange', changes, source || 'edit');
-    let shouldBeCanceled = true;
-
-    if (beforeChangeResult === false) {
-
-      if (activeEditor) {
-        activeEditor.cancelChanges();
-      }
-
-      return;
-    }
-
     const waitingForValidator = new ValidatorsQueue();
+    let shouldBeCanceled = true;
 
     waitingForValidator.onQueueEmpty = (isValid) => {
       if (activeEditor && shouldBeCanceled) {
@@ -1281,42 +1270,37 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
     };
 
     for (let i = changes.length - 1; i >= 0; i--) {
-      if (changes[i] === null) {
-        changes.splice(i, 1);
-      } else {
-        const [row, prop, , newValue] = changes[i];
-        const col = datamap.propToCol(prop);
-        const cellProperties = instance.getCellMeta(row, col);
+      const [row, prop, , newValue] = changes[i];
+      const col = datamap.propToCol(prop);
+      const cellProperties = instance.getCellMeta(row, col);
 
-        if (cellProperties.type === 'numeric' && typeof newValue === 'string' && isNumericLike(newValue)) {
-          changes[i][3] = getParsedNumber(newValue);
-        }
+      if (cellProperties.type === 'numeric' && typeof newValue === 'string' && isNumericLike(newValue)) {
+        changes[i][3] = getParsedNumber(newValue);
+      }
 
-        /* eslint-disable no-loop-func */
-        if (instance.getCellValidator(cellProperties)) {
-          waitingForValidator.addValidatorToQueue();
-          instance.validateCell(changes[i][3], cellProperties, (function(index, cellPropertiesReference) {
-            return function(result) {
-              if (typeof result !== 'boolean') {
-                throw new Error('Validation error: result is not boolean');
-              }
+      /* eslint-disable no-loop-func */
+      if (instance.getCellValidator(cellProperties)) {
+        waitingForValidator.addValidatorToQueue();
+        instance.validateCell(changes[i][3], cellProperties, (function(index, cellPropertiesReference) {
+          return function(result) {
+            if (typeof result !== 'boolean') {
+              throw new Error('Validation error: result is not boolean');
+            }
 
-              if (result === false && cellPropertiesReference.allowInvalid === false) {
-                shouldBeCanceled = false;
-                changes.splice(index, 1); // cancel the change
-                cellPropertiesReference.valid = true; // we cancelled the change, so cell value is still valid
+            if (result === false && cellPropertiesReference.allowInvalid === false) {
+              shouldBeCanceled = false;
+              changes.splice(index, 1); // cancel the change
+              cellPropertiesReference.valid = true; // we cancelled the change, so cell value is still valid
 
-                const cell = instance.getCell(cellPropertiesReference.visualRow, cellPropertiesReference.visualCol);
+              const cell = instance.getCell(cellPropertiesReference.visualRow, cellPropertiesReference.visualCol);
 
-                if (cell !== null) {
-                  removeClass(cell, tableMeta.invalidCellClassName);
-                }
-                // index -= 1;
+              if (cell !== null) {
+                removeClass(cell, tableMeta.invalidCellClassName);
               }
-              waitingForValidator.removeValidatorFormQueue();
-            };
-          }(i, cellProperties)), source);
-        }
+            }
+            waitingForValidator.removeValidatorFormQueue();
+          };
+        }(i, cellProperties)), source);
       }
     }
     waitingForValidator.checkIfQueueIsEmpty();
@@ -1528,6 +1512,32 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
     return [[row, propOrCol, value]];
   }
 
+  /**
+   * Process changes prepared for applying to the dataset (unifying list of changes, closing an editor - when needed,
+   * calling a hook).
+   *
+   * @private
+   * @param {Array} changes Array of changes in format `[[row, col, value],...]`.
+   * @param {string} [source] String that identifies how this change will be described in the changes array (useful in afterChange or beforeChange callback). Set to 'edit' if left empty.
+   * @returns {Array} List of changes finally applied to the dataset.
+   */
+  function processChanges(changes, source) {
+    const activeEditor = instance.getActiveEditor();
+    const beforeChangeResult = instance.runHooks('beforeChange', changes, source || 'edit');
+    // The `beforeChange` hook could add a `null` for purpose of cancelling some dataset's change.
+    const filteredChanges = changes.filter(change => change !== null);
+
+    if (beforeChangeResult === false || filteredChanges.length === 0) {
+      if (activeEditor) {
+        activeEditor.cancelChanges();
+      }
+
+      return [];
+    }
+
+    return filteredChanges;
+  }
+
   /**
    * @description
    * Set new value to a cell. To change many cells at once (recommended way), pass an array of `changes` in format
@@ -1575,10 +1585,12 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
       changeSource = column;
     }
 
-    instance.runHooks('afterSetDataAtCell', changes, changeSource);
+    const processedChanges = processChanges(changes, source);
 
-    validateChanges(changes, changeSource, () => {
-      applyChanges(changes, changeSource);
+    instance.runHooks('afterSetDataAtCell', processedChanges, changeSource);
+
+    validateChanges(processedChanges, changeSource, () => {
+      applyChanges(processedChanges, changeSource);
     });
   };
 
@@ -1614,10 +1626,12 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
       changeSource = prop;
     }
 
-    instance.runHooks('afterSetDataAtRowProp', changes, changeSource);
+    const processedChanges = processChanges(changes, source);
+
+    instance.runHooks('afterSetDataAtRowProp', processedChanges, changeSource);
 
-    validateChanges(changes, changeSource, () => {
-      applyChanges(changes, changeSource);
+    validateChanges(processedChanges, changeSource, () => {
+      applyChanges(processedChanges, changeSource);
     });
   };
 

handsontable/src/plugins/autofill/__tests__/autofill.spec.js

@@ -506,6 +506,38 @@ describe('AutoFill', () => {
     });
   });
 
+  describe('beforeChange hook autofill value overrides', () => {
+    it('should use a custom value when introducing changes', () => {
+      handsontable({
+        data: [
+          [1, 2, 3, 4, 5, 6],
+          [1, 2, 3, 4, 5, 6],
+          [1, 2, 3, 4, 5, 6],
+          [1, 2, 3, 4, 5, 6],
+        ],
+        beforeChange(changes) {
+          changes[0][3] = 'test2';
+          changes[1][3] = 'test3';
+          changes[2][3] = 'test4';
+        }
+      });
+      selectCell(0, 0);
+
+      spec().$container.find('.wtBorder.corner').simulate('mousedown');
+      spec().$container.find('tr:eq(1) td:eq(0)').simulate('mouseover');
+      spec().$container.find('tr:eq(3) td:eq(0)').simulate('mouseover');
+      spec().$container.find('.wtBorder.corner').simulate('mouseup');
+
+      expect(getSelected()).toEqual([[0, 0, 3, 0]]);
+      expect(getData()).toEqual([
+        [1, 2, 3, 4, 5, 6],
+        ['test2', 2, 3, 4, 5, 6],
+        ['test3', 2, 3, 4, 5, 6],
+        ['test4', 2, 3, 4, 5, 6]
+      ]);
+    });
+  });
+
   it('should pass correct arguments to `afterAutofill`', () => {
     const afterAutofill = jasmine.createSpy();
 

handsontable/src/plugins/formulas/__tests__/formulas.spec.js

@@ -2885,6 +2885,40 @@ describe('Formulas general', () => {
     expect(getDataAtCell(1, 4)).toEqual(3);
   });
 
+  it('should display calculated formula after changing value using `beforeChange` hook #6932', () => {
+    handsontable({
+      data: [
+        ['2016', 1, 1, 2, 3],
+        ['2017', 10, 11, 12, 13],
+        ['2018', 20, 11, 14, 13],
+        ['2019', 30, 15, 12, 13],
+      ],
+      rowHeaders: true,
+      colHeaders: true,
+      formulas: {
+        engine: HyperFormula
+      },
+      beforeChange(beforeChanges) {
+        beforeChanges[0][3] = '=SUM(B3:E3)';
+      },
+    });
+
+    setDataAtCell(0, 0, 1);
+
+    expect(getData()).toEqual([
+      [58, 1, 1, 2, 3],
+      ['2017', 10, 11, 12, 13],
+      ['2018', 20, 11, 14, 13],
+      ['2019', 30, 15, 12, 13],
+    ]);
+    expect(getSourceData()).toEqual([
+      ['=SUM(B3:E3)', 1, 1, 2, 3],
+      ['2017', 10, 11, 12, 13],
+      ['2018', 20, 11, 14, 13],
+      ['2019', 30, 15, 12, 13],
+    ]);
+  });
+
   describe('handling dates', () => {
     it('should handle date functions properly', () => {
       handsontable({

handsontable/test/e2e/Core_populateFromArray.spec.js

@@ -35,6 +35,29 @@ describe('Core_populateFromArray', () => {
     expect(output).toEqual([[0, 0, '', 'test'], [0, 1, 'Kia', 'test'], [1, 0, '2008', 'test'], [1, 1, 10, 'test']]);
   });
 
+  it('should override populated values using `beforeChange` hook', () => {
+    handsontable({
+      data: [
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+      ],
+      beforeChange(changes) {
+        changes[0][3] = 'test1';
+        changes[3][3] = 'test4';
+      }
+    });
+    populateFromArray(0, 0, [['test', 'test'], ['test', 'test']], 1, 1);
+
+    expect(getData()).toEqual([
+      ['test1', 'test', 3, 4, 5, 6],
+      ['test', 'test4', 3, 4, 5, 6],
+      [1, 2, 3, 4, 5, 6],
+      [1, 2, 3, 4, 5, 6],
+    ]);
+  });
+
   it('should populate single value for whole selection', () => {
     let output = null;
 

handsontable/test/e2e/Core_setDataAtCell.spec.js

@@ -317,6 +317,65 @@ describe('Core_setDataAtCell', () => {
     expect(errors).toBe(0);
   });
 
+  it('should trigger `beforeChange` and `afterSetDataAtCell` in the correct order', () => {
+    const beforeChange = jasmine.createSpy('beforeChange');
+    const afterSetDataAtCell = jasmine.createSpy('afterSetDataAtCell');
+
+    handsontable({
+      data: [[1, 1]],
+      beforeChange,
+      afterSetDataAtCell,
+    });
+
+    setDataAtCell(0, 0, 5);
+
+    expect(beforeChange).toHaveBeenCalledBefore(afterSetDataAtCell);
+  });
+
+  it('should trigger `beforeChange` and `afterSetDataAtRowProp` in the correct order', () => {
+    const beforeChange = jasmine.createSpy('beforeChange');
+    const afterSetDataAtRowProp = jasmine.createSpy('afterSetDataAtRowProp');
+
+    handsontable({
+      data: [[1, 1]],
+      beforeChange,
+      afterSetDataAtRowProp,
+    });
+
+    setDataAtRowProp(0, 0, 5);
+
+    expect(beforeChange).toHaveBeenCalledBefore(afterSetDataAtRowProp);
+  });
+
+  it('should override set values using `beforeChange` hook', () => {
+    handsontable({
+      data: [
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+      ],
+      beforeChange(changes) {
+        changes[0][3] = 'test';
+      }
+    });
+
+    setDataAtCell(0, 0, 5);
+    setDataAtCell(1, 1, 6);
+    setDataAtCell(2, 2, 7);
+    setDataAtRowProp(3, 3, 8);
+    setDataAtRowProp(4, 4, 9);
+
+    expect(getData()).toEqual([
+      ['test', 2, 3, 4, 5, 6],
+      [1, 'test', 3, 4, 5, 6],
+      [1, 2, 'test', 4, 5, 6],
+      [1, 2, 3, 'test', 5, 6],
+      [1, 2, 3, 4, 'test', 6],
+    ]);
+  });
+
   describe('Coordinates out of dataset', () => {
     it('should insert new column', () => {
       handsontable({

handsontable/test/e2e/core/emptySelectedCells.spec.js

@@ -129,4 +129,29 @@ describe('Core.emptySelectedCells', () => {
 
     expect(onBeforeChange).not.toHaveBeenCalled();
   });
+
+  it('should override cleared values using `beforeChange` hook', () => {
+    handsontable({
+      data: [
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+        [1, 2, 3, 4, 5, 6],
+      ],
+      beforeChange(changes) {
+        changes[0][3] = 'test';
+      }
+    });
+
+    selectCells([[0, 0, 2, 2]]);
+
+    emptySelectedCells();
+
+    expect(getData()).toEqual([
+      ['test', null, null, 4, 5, 6],
+      [null, null, null, 4, 5, 6],
+      [null, null, null, 4, 5, 6],
+      [1, 2, 3, 4, 5, 6],
+    ]);
+  });
 });