fix(editor): one predicate for the CASE marker, and no stacking in the tooltip

Addresses the retroactive review of #447.

- afterGetColHeader appended the case hint to th.title on every render, and
  Handsontable reuses the th, so the tooltip grew a copy per render. The hint
  is composed by composeCaseHeaderTitle, which strips any known hint first, so
  a repeat call is a no-op and a rule change replaces the sentence.
- the marker is now gated on the same predicate the conversion uses,
  resolveEnteredCaseTransform, so a formula, read-only or derived column no
  longer advertises a conversion the hook refuses to make. The class is removed
  when the predicate stops matching.
- resolvePkFormula and the unescapeFormula write (given an explicit source) join
  CASE_TRANSFORM_SKIPPED_SOURCES: neither is an entry, and re-casing a computed
  key or a live `=...` string corrupts it. force_cell_validation_value keeps
  converting on purpose, stated in the docstring.

client/src/app/editor/editor.component.ts

@@ -61,7 +61,10 @@ import { getHotDataSchema } from '../shared/dc-validator/utils/getHotDataSchema'
 import { excelRound } from '../shared/dc-validator/utils/excelRound'
 import {
   applyCaseTransform,
-  isCaseTransformSkipped
+  CaseTransform,
+  composeCaseHeaderTitle,
+  isCaseTransformSkipped,
+  resolveEnteredCaseTransform
 } from '../shared/dc-validator/utils/caseTransform'
 import { isEmpty } from '../shared/dc-validator/utils/isEmpty'
 import { hasFormulaRules } from '../shared/dc-validator/utils/hasFormulaRules'
@@ -364,7 +367,11 @@ export class EditorComponent
                 hot.setDataAtRowProp(
                   row,
                   colName,
-                  unescapeFormula(hot.getDataAtRowProp(row, colName))
+                  unescapeFormula(hot.getDataAtRowProp(row, colName)),
+                  // a named source so the CASE transform leaves this write
+                  // alone: it restores a live `=...` formula, which re-casing
+                  // would corrupt
+                  'unescapeFormula'
                 )
                 clearAutoEscaped(this.autoEscapedCells, rowKey, colName)
               }
@@ -1130,6 +1137,20 @@ export class EditorComponent
     return this.headerPks.indexOf(col) > -1
   }
 
+  /**
+   * The case transform to apply to values *entered* into `colName`, or
+   * undefined when the column must neither convert them nor advertise that it
+   * does. One predicate for the conversion (the beforeChange hook) and the
+   * header marker (afterGetColHeader) - see `resolveEnteredCaseTransform`.
+   */
+  getEnteredCaseTransform(colName: string): CaseTransform | undefined {
+    return resolveEnteredCaseTransform(
+      colName,
+      this.dcValidator,
+      !!colName && this.isDerivedCol(colName)
+    )
+  }
+
   isReadonlyCol(col: string | number) {
     const colRules = this.dcValidator?.getRule(col)
 
@@ -4307,17 +4328,19 @@ export class EditorComponent
           // case (see the beforeChange hook) - mark the header so the user can
           // tell the capitals or the lower-case came from the grid and not
           // from them. The cell shows the value the grid actually holds, so
-          // nothing here re-cases a value the user has not touched.
+          // nothing here re-cases a value the user has not touched. Gated on
+          // the hook's own predicate, so a formula, read-only or derived column
+          // carries no marker claiming a conversion that will not happen; the
+          // title is composed rather than appended because Handsontable reuses
+          // this th across renders.
           const caseTransform = column
-            ? this.dcValidator?.getCaseTransform(column)
+            ? this.getEnteredCaseTransform(column)
             : undefined
           if (caseTransform) {
             th.classList.add('dc-case-header')
-            const caseHint =
-              caseTransform === 'upper'
-                ? 'Values are converted to upper case on entry'
-                : 'Values are converted to lower case on entry'
-            th.title = th.title ? `${th.title}; ${caseHint}` : caseHint
+            th.title = composeCaseHeaderTitle(th.title, caseTransform)
+          } else {
+            th.classList.remove('dc-case-header')
           }
 
           // Remove header arrow from Delete column
@@ -4521,18 +4544,13 @@ export class EditorComponent
         // touched them (the unchanged baseline is cloned from what the grid
         // hands back). Undo and redo restore a previous value rather than
         // entering a new one, and re-casing it would hand back something other
-        // than what the user asked to get back. A formula column holds a live
-        // `=...` string, which is re-cased into nonsense - HARDFORMULA is
-        // read-only so it never reaches here, but SOFTFORMULA is editable.
+        // than what the user asked to get back. The rest of the skipped list
+        // and the formula/read-only exclusions live in
+        // CASE_TRANSFORM_SKIPPED_SOURCES and getEnteredCaseTransform, which the
+        // header marker uses too.
         if (!isCaseTransformSkipped(source) && typeof newValue === 'string') {
-          const caseTransform = this.dcValidator?.getCaseTransform(colName)
-          if (
-            caseTransform &&
-            !this.dcValidator?.hasDqRules(colName, [
-              'HARDFORMULA',
-              'SOFTFORMULA'
-            ])
-          ) {
+          const caseTransform = this.getEnteredCaseTransform(colName)
+          if (caseTransform) {
             change[3] = applyCaseTransform(newValue, caseTransform)
           }
         }

client/src/app/editor/utils/caseTransform.integration.spec.ts

@@ -1,7 +1,9 @@
 import Handsontable from 'handsontable'
 import {
   applyCaseTransform,
+  CASE_HEADER_HINTS,
   CaseTransform,
+  composeCaseHeaderTitle,
   isCaseTransformSkipped
 } from '../../shared/dc-validator/utils/caseTransform'
 
@@ -189,4 +191,59 @@ describe('CASE rule transform on entry integration', () => {
 
     expect(hot.getDataAtRowProp(0, 'UPPER_COL')).toEqual('keep')
   })
+
+  it('leaves a value the pk-formula resolver writes back alone', () => {
+    buildGrid([row()])
+
+    // resolvePkFormula re-writes a computed key into its own cell on every row
+    // event. The key has to keep matching the backend, so it is not converted -
+    // and converting it would have the pass fighting itself.
+    hot.setDataAtCell(0, 1, 'mixedKey', 'resolvePkFormula')
+
+    expect(hot.getDataAtRowProp(0, 'UPPER_COL')).toEqual('mixedKey')
+  })
+
+  describe('header marker', () => {
+    const buildHeaderGrid = () => {
+      const headerHot = new Handsontable(container, {
+        data: [row()],
+        columns: columnNames.map((name) => ({ data: name })),
+        // the hook under test only fires for a rendered column header
+        colHeaders: true,
+        licenseKey: 'non-commercial-and-evaluation'
+      })
+
+      // mirrors editor.component.ts's afterGetColHeader branch
+      headerHot.addHook('afterGetColHeader', (col: number, th: any) => {
+        const transform = caseRules[columnNames[col]]
+        if (transform) th.title = composeCaseHeaderTitle(th.title, transform)
+      })
+
+      headerHot.render()
+
+      return headerHot
+    }
+
+    it('carries one hint per header however often the grid re-renders', () => {
+      hot = buildHeaderGrid()
+
+      // the th element is reused across renders, which is how appending the
+      // hint each time grew the tooltip
+      hot.render()
+      hot.setDataAtCell(0, 1, 'x', 'edit')
+      hot.render()
+
+      // the grid paints more than one copy of a header (the master table and
+      // its clone), so the unit under test is the th element, not the document
+      const headers = Array.from(container.querySelectorAll('th'))
+      const hinted = headers.filter((th) =>
+        (th.title || '').includes(CASE_HEADER_HINTS.upper)
+      )
+
+      expect(hinted.length).toBeGreaterThan(0)
+      for (const th of hinted) {
+        expect(th.title.split(CASE_HEADER_HINTS.upper).length - 1).toEqual(1)
+      }
+    })
+  })
 })

client/src/app/shared/dc-validator/utils/caseTransform.spec.ts

@@ -1,4 +1,11 @@
-import { applyCaseTransform, isCaseTransformSkipped } from './caseTransform'
+import {
+  applyCaseTransform,
+  CASE_HEADER_HINTS,
+  CaseTransform,
+  composeCaseHeaderTitle,
+  isCaseTransformSkipped,
+  resolveEnteredCaseTransform
+} from './caseTransform'
 
 describe('applyCaseTransform', () => {
   it('upper-cases a string when the column rule is UPCASE', () => {
@@ -46,9 +53,134 @@ describe('isCaseTransformSkipped', () => {
     expect(isCaseTransformSkipped('revert')).toBeTrue()
   })
 
+  it('skips the computed writers, whose values nobody typed', () => {
+    // a resolved primary key has to keep the case the formula produced, and it
+    // is rewritten on every row event, so converting it would fight the pass
+    expect(isCaseTransformSkipped('resolvePkFormula')).toBeTrue()
+    // this one restores a live `=...` formula, which re-casing corrupts
+    expect(isCaseTransformSkipped('unescapeFormula')).toBeTrue()
+  })
+
+  it('converts a forced validation value on purpose', () => {
+    // a forced value is a configured default written as though entered, so it
+    // is converted to satisfy the column's own rule
+    expect(isCaseTransformSkipped('force_cell_validation_value')).toBeFalse()
+  })
+
   it('does not skip a value the user enters', () => {
     expect(isCaseTransformSkipped('edit')).toBeFalse()
     expect(isCaseTransformSkipped('paste')).toBeFalse()
     expect(isCaseTransformSkipped('autofill')).toBeFalse()
   })
 })
+
+describe('resolveEnteredCaseTransform', () => {
+  const stub = (opts: {
+    readOnly?: boolean
+    formula?: boolean
+    transform?: CaseTransform
+  }) => ({
+    getRule: () => ({ readOnly: opts.readOnly }),
+    hasDqRules: (col: string, rules: string[]) =>
+      !!opts.formula && rules.includes('SOFTFORMULA'),
+    getCaseTransform: () => opts.transform
+  })
+
+  it('returns the transform for an editable CASE column', () => {
+    expect(
+      resolveEnteredCaseTransform('CODE', stub({ transform: 'upper' }), false)
+    ).toEqual('upper')
+  })
+
+  it('refuses a read-only column, whose entries never reach the hook', () => {
+    expect(
+      resolveEnteredCaseTransform(
+        'CODE',
+        stub({ readOnly: true, transform: 'upper' }),
+        false
+      )
+    ).toBeUndefined()
+  })
+
+  it('refuses a formula column, which holds a live =... string', () => {
+    expect(
+      resolveEnteredCaseTransform(
+        'CODE',
+        stub({ formula: true, transform: 'upper' }),
+        false
+      )
+    ).toBeUndefined()
+  })
+
+  it('refuses a derived column, which is computed at read time', () => {
+    expect(
+      resolveEnteredCaseTransform('CODE', stub({ transform: 'lower' }), true)
+    ).toBeUndefined()
+  })
+
+  it('is undefined without a validator, a column name or a CASE rule', () => {
+    expect(
+      resolveEnteredCaseTransform('CODE', undefined, false)
+    ).toBeUndefined()
+    expect(
+      resolveEnteredCaseTransform('', stub({ transform: 'upper' }), false)
+    ).toBeUndefined()
+    expect(resolveEnteredCaseTransform('CODE', stub({}), false)).toBeUndefined()
+  })
+})
+
+describe('composeCaseHeaderTitle', () => {
+  it('is the hint alone when the header has no title of its own', () => {
+    expect(composeCaseHeaderTitle('', 'upper')).toEqual(CASE_HEADER_HINTS.upper)
+    expect(composeCaseHeaderTitle(undefined as any, 'lower')).toEqual(
+      CASE_HEADER_HINTS.lower
+    )
+  })
+
+  it('appends the hint to a title the grid already set', () => {
+    expect(composeCaseHeaderTitle('Account Code', 'upper')).toEqual(
+      `Account Code; ${CASE_HEADER_HINTS.upper}`
+    )
+  })
+
+  it('does not stack the hint when the header is re-rendered', () => {
+    // afterGetColHeader runs on every render over the same th element, which is
+    // how the tooltip grew a copy per render
+    let title = composeCaseHeaderTitle('Account Code', 'upper')
+    for (let render = 0; render < 5; render++) {
+      title = composeCaseHeaderTitle(title, 'upper')
+    }
+
+    expect(title).toEqual(`Account Code; ${CASE_HEADER_HINTS.upper}`)
+    expect(title.split(CASE_HEADER_HINTS.upper).length - 1).toEqual(1)
+  })
+
+  it('replaces the hint when a metadata reload changes the rule', () => {
+    const upper = composeCaseHeaderTitle('Account Code', 'upper')
+
+    expect(composeCaseHeaderTitle(upper, 'lower')).toEqual(
+      `Account Code; ${CASE_HEADER_HINTS.lower}`
+    )
+  })
+
+  it('collapses a title that already carries several copies', () => {
+    const stacked = [
+      'Account Code',
+      CASE_HEADER_HINTS.upper,
+      CASE_HEADER_HINTS.upper,
+      CASE_HEADER_HINTS.upper
+    ].join('; ')
+
+    expect(composeCaseHeaderTitle(stacked, 'upper')).toEqual(
+      `Account Code; ${CASE_HEADER_HINTS.upper}`
+    )
+  })
+
+  it('keeps a derived column label in front of the hint', () => {
+    const derived = 'Total value - derived variable, computed at read time'
+
+    expect(composeCaseHeaderTitle(derived, 'lower')).toEqual(
+      `${derived}; ${CASE_HEADER_HINTS.lower}`
+    )
+  })
+})

client/src/app/shared/dc-validator/utils/caseTransform.ts

@@ -23,17 +23,99 @@ export type CaseTransform = 'upper' | 'lower'
  * - `revert` is the Revert menu item, which puts back the value the service
  *   supplied for that cell. Converting it would leave the cell modified
  *   against the unchanged baseline, i.e. the revert would not have reverted.
+ * - `resolvePkFormula` is the pass that writes a computed primary-key value
+ *   back into its own cell. The value is already the one the formula
+ *   produced; re-casing it would change a key that has to match the backend,
+ *   and the pass would keep rewriting it on every row event.
+ * - `unescapeFormula` writes an auto-escaped value back as the live `=...`
+ *   formula it came from. Re-casing a formula string corrupts it.
+ *
+ * `force_cell_validation_value` is deliberately NOT on this list: a forced
+ * value is a configured default written into a cell as though the user had
+ * entered it, so it is converted to satisfy the column's own rule rather than
+ * arriving pre-flagged as invalid.
  */
 export const CASE_TRANSFORM_SKIPPED_SOURCES = [
   'loadData',
   'UndoRedo.undo',
   'UndoRedo.redo',
-  'revert'
+  'revert',
+  'resolvePkFormula',
+  'unescapeFormula'
 ]
 
 export const isCaseTransformSkipped = (source: string): boolean =>
   CASE_TRANSFORM_SKIPPED_SOURCES.includes(source)
 
+/** The tooltip text a CASE column's header carries. */
+export const CASE_HEADER_HINTS: Record<CaseTransform, string> = {
+  upper: 'Values are converted to upper case on entry',
+  lower: 'Values are converted to lower case on entry'
+}
+
+/**
+ * The parts of `DcValidator` the entry predicate needs, as a structural type so
+ * this module stays free of the validator (and the predicate stays testable
+ * without a grid).
+ */
+export interface CaseTransformSource {
+  getRule(col: string | number): { readOnly?: boolean } | undefined
+  hasDqRules(col: string, rules: string[]): boolean
+  getCaseTransform(col: string | number): CaseTransform | undefined
+}
+
+/**
+ * The case transform to apply to values *entered* into `colName`, or undefined
+ * when the column must neither convert them nor advertise that it does.
+ *
+ * One predicate for both the conversion (the beforeChange hook) and the header
+ * marker (afterGetColHeader), so a badge and tooltip cannot claim a conversion
+ * the hook refuses to make:
+ *  - a HARDFORMULA or SOFTFORMULA column holds a live `=...` string, which
+ *    re-casing corrupts (HARDFORMULA is read-only, SOFTFORMULA is editable and
+ *    so does reach the hook);
+ *  - a read-only or derived column never receives an entry at all.
+ */
+export const resolveEnteredCaseTransform = (
+  colName: string,
+  validator: CaseTransformSource | undefined,
+  isDerived: boolean
+): CaseTransform | undefined => {
+  if (!colName || !validator || isDerived) return undefined
+  if (validator.getRule(colName)?.readOnly) return undefined
+  if (validator.hasDqRules(colName, ['HARDFORMULA', 'SOFTFORMULA'])) {
+    return undefined
+  }
+
+  return validator.getCaseTransform(colName)
+}
+
+/**
+ * Composes a column header's title with its case hint, idempotently.
+ *
+ * `afterGetColHeader` runs on every render and Handsontable reuses the header
+ * element it is handed, so appending the hint each time grew the tooltip by a
+ * copy per render (one after the first render, two after the next, and so on).
+ * Stripping the known hints before appending makes a repeated call a no-op and
+ * lets the hint change (a metadata reload that swaps UPCASE for LOWCASE)
+ * without leaving the old sentence behind.
+ */
+export const composeCaseHeaderTitle = (
+  title: string,
+  transform: CaseTransform
+): string => {
+  const hint = CASE_HEADER_HINTS[transform]
+  const known = Object.values(CASE_HEADER_HINTS)
+
+  let base = title || ''
+  for (const previous of known) {
+    base = base.split(`; ${previous}`).join('').split(previous).join('')
+  }
+  base = base.replace(/;\s*$/, '').trim()
+
+  return base ? `${base}; ${hint}` : hint
+}
+
 /**
  * Applies the column's case transform to a pending cell value.
  *