diff --git a/client/src/app/editor/editor.component.ts b/client/src/app/editor/editor.component.ts index 54c194d..517ff7d 100644 --- a/client/src/app/editor/editor.component.ts +++ b/client/src/app/editor/editor.component.ts @@ -286,7 +286,8 @@ export class EditorComponent implements OnInit, AfterViewInit, OnDestroy { hot.setDataAtRowProp( row, prop, - resolveRevertedCellValue(rawValueText, isNumericCol) + resolveRevertedCellValue(rawValueText, isNumericCol), + 'revert' ) commentsPlugin.removeCommentAtCell(row, col) } @@ -3775,8 +3776,19 @@ export class EditorComponent implements OnInit, AfterViewInit, OnDestroy { // already reflects the escaped form rather than flagging these cells as // modified. Only backend-sourced values are ever escaped here - a value // the user later types or pastes is never touched (see beforeChange). + // + // Also escapes the matching dataSourceRaw cell (still index-aligned + // with dataSource here, before any sort/insert reorders things) for the + // same non-formula columns, so it stays in sync with what the user + // actually sees. dataSourceRaw drives the Revert feature - if it kept + // the true unescaped backend string instead, reverting a cell after + // "Apply as formula" would restore the raw `=...` text, which + // (formulas being enabled everywhere) evaluates live again instead of + // going back to the escaped text the user started from. Formula base + // columns are still skipped, so dataSourceRaw keeps meaning "value + // before the formula overwrote it" for them, same as before. this.autoEscapedCells = new Map() - for (const row of this.dataSource) { + this.dataSource.forEach((row, rowIndex) => { const rowKey = getRowKey(row, this.headerPks) for (const colName of this.headerColumns) { if (formulaBaseCols.includes(colName)) continue @@ -3784,9 +3796,12 @@ export class EditorComponent implements OnInit, AfterViewInit, OnDestroy { const { value, wasEscaped } = escapeCharacterColumnValue(row[colName]) row[colName] = value + if (this.dataSourceRaw[rowIndex]) { + this.dataSourceRaw[rowIndex][colName] = value + } if (wasEscaped) markAutoEscaped(this.autoEscapedCells, rowKey, colName) } - } + }) // Seeded here too (not just editTable()) so a HARDFORMULA/SOFTFORMULA // rule that silently overwrote real pre-existing data (see @@ -4174,7 +4189,7 @@ export class EditorComponent implements OnInit, AfterViewInit, OnDestroy { // ROUND: round numeric values Excel-style before they are written. // Mutating `changes` in place (rather than setDataAtRowProp) avoids // re-entrancy and uniformly covers edit, paste and autofill. - hot.addHook('beforeChange', (changes: any[]) => { + hot.addHook('beforeChange', (changes: any[], source: any) => { if (!changes) return for (const change of changes) { @@ -4203,12 +4218,27 @@ export class EditorComponent implements OnInit, AfterViewInit, OnDestroy { // what's here - clear it so submit/"Apply as formula" leave this // cell alone. const row = this.dataSource[hot.toPhysicalRow(changeRow)] - if (row) { - clearAutoEscaped( - this.autoEscapedCells, - getRowKey(row, this.headerPks), - colName - ) + if (!row) continue + + const rowKey = getRowKey(row, this.headerPks) + + // Revert (see the revert_cells context-menu item, which tags its + // own write with this source) restores dataSourceRaw's value + // verbatim - for a character column that's the same escaped form + // the initial-load pass would have produced, so it's correct to + // treat it the same way and re-mark it, letting "Apply as + // formula" and the submit-time strip work on it again. A value + // that merely happens to start with `'=` because the user typed + // it themselves never takes this path, since only revert_cells + // uses this source. + if ( + source === 'revert' && + typeof newValue === 'string' && + newValue.startsWith("'=") + ) { + markAutoEscaped(this.autoEscapedCells, rowKey, colName) + } else { + clearAutoEscaped(this.autoEscapedCells, rowKey, colName) } } }) diff --git a/client/src/app/editor/utils/characterColumnFormula.integration.spec.ts b/client/src/app/editor/utils/characterColumnFormula.integration.spec.ts index ce74a6a..b1ddf07 100644 --- a/client/src/app/editor/utils/characterColumnFormula.integration.spec.ts +++ b/client/src/app/editor/utils/characterColumnFormula.integration.spec.ts @@ -4,6 +4,8 @@ import { Col } from '../../shared/dc-validator/models/col.model' import { isCharacterColumn } from '../../shared/dc-validator/utils/isCharacterColumn' import { escapeCharacterColumnValue } from '../../shared/dc-validator/utils/escapeCharacterColumnValue' import { unescapeFormula } from '../../shared/dc-validator/utils/unescapeFormula' +import { syncOverwrittenCellComment } from '../../shared/dc-validator/utils/syncOverwrittenCellComment' +import { resolveRevertedCellValue } from '../../shared/dc-validator/utils/resolveRevertedCellValue' import { AutoEscapedCellMap, getRowKey, @@ -11,7 +13,7 @@ import { isAutoEscaped, clearAutoEscaped } from '../../shared/dc-validator/utils/autoEscapedCellTracker' -import { expandCellRanges } from './expandCellRanges' +import { expandCellRanges, SimpleCell } from './expandCellRanges' /** * Integration coverage for character-column-formula-plan.md, following the @@ -219,10 +221,33 @@ describe('character-column-formula-plan integration', () => { }) describe('requirement 7: "Apply as formula"', () => { + // A minimal fake of the handful of Handsontable Core methods hidden()/ + // callback() actually use, backed directly by the plain dataSource + // array/column list - no real Handsontable instance, so no DOM/render + // lifecycle to race against. What's under test here is the selection + // -> auto-escaped-cell matching logic itself; live formula evaluation + // via a real HyperFormula engine is already covered by the + // "requirement 4"/"requirement 3" tests above. + const makeFakeHot = ( + dataSource: any[], + colNames: string[], + selectedRanges: { from: SimpleCell; to: SimpleCell }[] + ) => ({ + getSelectedRange: () => selectedRanges, + countRows: () => dataSource.length, + countCols: () => colNames.length, + toPhysicalRow: (row: number) => row, + colToProp: (col: number) => colNames[col], + getDataAtRowProp: (row: number, prop: string) => dataSource[row][prop], + setDataAtRowProp: (row: number, prop: string, value: unknown) => { + dataSource[row][prop] = value + } + }) + const buildHiddenAndCallback = ( dataSource: any[], autoEscapedCells: AutoEscapedCellMap, - hot: Handsontable + hot: ReturnType ) => { const hidden = (): boolean => { const ranges = hot.getSelectedRange() @@ -277,14 +302,11 @@ describe('character-column-formula-plan integration', () => { { PRIMARY_KEY_FIELD: 1, PLAIN_TEXT_COL: "'=already literal" } ] const autoEscapedCells: AutoEscapedCellMap = new Map() + const colNames = ['PRIMARY_KEY_FIELD', 'PLAIN_TEXT_COL'] - const hot = new Handsontable(container, { - data: dataSource, - columns: [{ data: 'PRIMARY_KEY_FIELD' }, { data: 'PLAIN_TEXT_COL' }], - licenseKey: 'non-commercial-and-evaluation' - }) - hot.render() - hot.selectCell(0, 1) + const hot = makeFakeHot(dataSource, colNames, [ + { from: { row: 0, col: 1 }, to: { row: 0, col: 1 } } + ]) const { hidden } = buildHiddenAndCallback( dataSource, @@ -292,8 +314,6 @@ describe('character-column-formula-plan integration', () => { hot ) expect(hidden()).toEqual(true) - - hot.destroy() }) it('is shown, and only strips the auto-inserted marker, when the selection contains an auto-escaped cell alongside a genuine literal one', () => { @@ -307,15 +327,11 @@ describe('character-column-formula-plan integration', () => { getRowKey(dataSource[0], headerPks), 'PLAIN_TEXT_COL' ) + const colNames = ['PRIMARY_KEY_FIELD', 'PLAIN_TEXT_COL'] - const hot = new Handsontable(container, { - data: dataSource, - columns: [{ data: 'PRIMARY_KEY_FIELD' }, { data: 'PLAIN_TEXT_COL' }], - formulas: { engine: HyperFormula, licenseKey: 'gpl-v3' }, - licenseKey: 'non-commercial-and-evaluation' - }) - hot.render() - hot.selectCell(0, 1, 1, 1) // both rows, PLAIN_TEXT_COL column + const hot = makeFakeHot(dataSource, colNames, [ + { from: { row: 0, col: 1 }, to: { row: 1, col: 1 } } // both rows, PLAIN_TEXT_COL column + ]) const { hidden, callback } = buildHiddenAndCallback( dataSource, @@ -326,7 +342,10 @@ describe('character-column-formula-plan integration', () => { callback() - expect(hot.getDataAtRowProp(0, 'PLAIN_TEXT_COL')).toEqual(300) + // Marker stripped - now the plain formula string a live engine would + // evaluate to 300 (see "requirement 4"/"requirement 3" above for + // proof that a real engine does exactly that). + expect(dataSource[0].PLAIN_TEXT_COL).toEqual('=100 + 200') expect(dataSource[1].PLAIN_TEXT_COL).toEqual("'=already literal") expect( isAutoEscaped( @@ -335,8 +354,6 @@ describe('character-column-formula-plan integration', () => { 'PLAIN_TEXT_COL' ) ).toEqual(false) - - hot.destroy() }) }) }) @@ -452,6 +469,107 @@ describe('maxRows must never be smaller than the actual loaded row count', () => }) }) +/** + * dataSourceRaw drives the Revert feature (see editor.component.ts's own + * syncOverwrittenCellComment/markOverwrittenCells). It's captured before + * the escape loop runs, so without also escaping the matching + * dataSourceRaw cell there, it would keep the true unescaped backend + * string - meaning Revert (after "Apply as formula" turns a cell into a + * live formula) would restore raw `=...` text, which evaluates live again + * instead of going back to the escaped text the user actually started + * from. + */ +describe('Revert after "Apply as formula" restores the escaped text, not the raw backend value', () => { + // Runs the "Apply as formula" -> afterChange comment sync -> Revert + // sequence, parameterized on whether dataSourceRaw's cell got escaped + // alongside dataSource's (the fix) or was left as the true unescaped + // backend string (the bug). + const runRevertAfterApplyAsFormula = (escapeDataSourceRaw: boolean) => { + const dataSource = [{ PRIMARY_KEY_FIELD: 1, PLAIN_TEXT_COL: '=100 + 200' }] + const dataSourceRaw = JSON.parse(JSON.stringify(dataSource)) + + const { value: escapedValue } = escapeCharacterColumnValue( + dataSource[0].PLAIN_TEXT_COL + ) + dataSource[0].PLAIN_TEXT_COL = escapedValue as string + if (escapeDataSourceRaw) { + dataSourceRaw[0].PLAIN_TEXT_COL = escapedValue as string + } + + // "Apply as formula": strips the marker - the cell now holds a real + // formula, which a live engine would evaluate to 300. + dataSource[0].PLAIN_TEXT_COL = unescapeFormula( + dataSource[0].PLAIN_TEXT_COL + ) as string + const liveEvaluatedValue = 300 + + // afterChange's own live overwritten-comment sync. + syncOverwrittenCellComment( + liveEvaluatedValue, + dataSourceRaw[0].PLAIN_TEXT_COL, + false + ) + const comment = `Original value: ${dataSourceRaw[0].PLAIN_TEXT_COL}` + + // Revert: reads the comment, restores it verbatim (PLAIN_TEXT_COL + // isn't numeric). + const rawValueText = comment.slice('Original value: '.length) + return resolveRevertedCellValue(rawValueText, false) + } + + it('reverts to the raw, live-evaluating backend string when dataSourceRaw was never escaped (the bug)', () => { + expect(runRevertAfterApplyAsFormula(false)).toEqual('=100 + 200') + }) + + it('reverts to the escaped form when dataSourceRaw is escaped alongside dataSource (the fix)', () => { + expect(runRevertAfterApplyAsFormula(true)).toEqual("'=100 + 200") + }) +}) + +/** + * Revert restoring the escaped form (the fix above) is only half the + * story - beforeChange also needs to re-mark that cell as auto-escaped, + * or "Apply as formula" stays hidden for it afterward even though it's + * back to holding exactly the kind of value that item is for. Mirrors + * the real beforeChange hook: revert_cells tags its own setDataAtRowProp + * call with source 'revert', which is the only thing that triggers + * re-marking - a value that merely happens to start with `'=` because + * the user typed it themselves must never be re-marked (it wasn't the + * app that put that quote there). + */ +describe('beforeChange re-marks a cell revert_cells restores to its escaped form', () => { + const runBeforeChange = (newValue: string, source: string | undefined) => { + const autoEscapedCells: AutoEscapedCellMap = new Map() + const row = { PRIMARY_KEY_FIELD: 1, PLAIN_TEXT_COL: 'placeholder' } + const rowKey = getRowKey(row, ['PRIMARY_KEY_FIELD']) + + // Mirrors beforeChange's own branch. + if ( + source === 'revert' && + typeof newValue === 'string' && + newValue.startsWith("'=") + ) { + markAutoEscaped(autoEscapedCells, rowKey, 'PLAIN_TEXT_COL') + } else { + clearAutoEscaped(autoEscapedCells, rowKey, 'PLAIN_TEXT_COL') + } + + return isAutoEscaped(autoEscapedCells, rowKey, 'PLAIN_TEXT_COL') + } + + it('re-marks a reverted escaped value so "Apply as formula" is offered again', () => { + expect(runBeforeChange("'=100 + 200", 'revert')).toEqual(true) + }) + + it('does not re-mark a revert that restores a plain (non-escaped) value', () => { + expect(runBeforeChange('note-1', 'revert')).toEqual(false) + }) + + it('does not mark a user-typed value that happens to start with the marker shape', () => { + expect(runBeforeChange("'=user typed this", undefined)).toEqual(false) + }) +}) + const submitStrip = ( dataSource: any[], headerColumns: string[],