fix(editor): own the delete-record column client-side #343

Merged
allan merged 3 commits from fix/getdata-delete-column-client-side into feat/release-7.18 2026-10-16 13:01:43 +00:00
Collaborator

Closes #309.

What

_____DELETE__THIS__RECORD_____ (the editor's Delete? column) is now created by the frontend, and editors/getdata no longer sends it.

Why

getdata.sas synthesised the column itself, in four places:

where what it did
the two filter data steps _____DELETE__THIS__RECORD_____="No"; on every row
the outdata build hoisted it in front of the PKs, attribed it, set it to "No "
the column metadata ctrlOptions='["No","Yes"]' for it
maxvarlengths forced MAXLEN=3

In the editor that column is a constant - it is only ever "No" on load. The only part that mattered, the Yes/No dropdown rule, is already injected client-side (deleteRecordColumnRule.ts, since #253's vertical-array fix), because %mp_getcols never has a row for it. So the backend was sending a column the client already owns, and its value was never read.

The column is meaningful elsewhere - postdata / getstagetable carry real Yes values, and file upload legitimately accepts the name (excel.md, files.md). None of that changes.

Changes

SAS - sas/sasjs/services/editors/getdata.sas

  • No longer emits the column: the two data-step assignments, the outdata hoist, the ctrlOptions branch and the maxvarlengths override are gone. (With the delete column gone, ctrlOptions had no other producer in this service, so the whole plumbing went with it.)

Frontend - deleteRecordColumn.ts (new)

  • prepareDeleteRecordColumn(response) asserts the response does not carry the name, then prepends it to COLHEADERS. Everything downstream (DcValidator's rules, columnHeader, headerArray) is keyed off COLHEADERS order, so the delete column is still index 0 and the grid is unchanged.
  • seedDeleteRecordColumn(rows) sets every row to 'No' before the validator or HyperFormula sees it.
  • A response that does carry the name throws, rather than rendering a table whose delete column would shadow a real column of that name. The throw lands in the existing getdata .catch, which sets getdataError and shows the error state - so the failure is visible, not silent.
  • multi-dataset.component.ts builds its own DcValidator from the same responses, so it goes through the same helper.

Mocks - the server and sas9 getdata mocks are updated to the new contract. postdata, getstagetable and makedata are untouched.

Screenshot

The editor, on the mocked estate, with the client-owned Delete? column first and its dropdown open. The spec asserts the header text and that the dropdown contains exactly No / Yes before it shoots.

The editor with the client-owned Delete? column first and its No/Yes dropdown open

Testing

  • client unit tests 570/570 (npm run test:headless), including 7 new cases for deleteRecordColumn.ts
  • npm run build (production) clean
  • sasjs lint clean on the changed SAS; sasjs compile -t server succeeds
  • Cypress against the mocked estate: coltype-delete-record.cy.ts 2/2, editor.cy.ts 55/55, excel.cy.ts 23/23, row-cell-limits.cy.ts 5/5

coltype-delete-record.cy.ts is the direct regression guard: it opens the editor on MPE_X_TEST and asserts the first column renders its hardcoded Yes/No dropdown end to end.

Deploy notes

  • The name is reserved. A table that contains a real column called _____DELETE__THIS__RECORD_____ will now fail to open in the editor, with the error state rather than a grid. That is the intended behaviour - the name would otherwise shadow the delete flag - and the docs PR adds the note.
  • No migration and no configuration change. The response is one column smaller, and the grid is identical.
  • Docs: docs.datacontroller.io PR updates the Developer Guide's pre-edit hook example (which mirrored the old data step) and adds the reserved-name note.
Closes #309. ## What `_____DELETE__THIS__RECORD_____` (the editor's **Delete?** column) is now created by the frontend, and `editors/getdata` no longer sends it. ## Why `getdata.sas` synthesised the column itself, in four places: | where | what it did | |---|---| | the two filter data steps | `_____DELETE__THIS__RECORD_____="No";` on every row | | the `outdata` build | hoisted it in front of the PKs, `attrib`ed it, set it to `"No "` | | the column metadata | `ctrlOptions='["No","Yes"]'` for it | | `maxvarlengths` | forced `MAXLEN=3` | In the editor that column is a **constant** - it is only ever `"No"` on load. The only part that mattered, the Yes/No dropdown rule, is already injected client-side (`deleteRecordColumnRule.ts`, since #253's vertical-array fix), because `%mp_getcols` never has a row for it. So the backend was sending a column the client already owns, and its value was never read. The column *is* meaningful elsewhere - `postdata` / `getstagetable` carry real `Yes` values, and file upload legitimately accepts the name (`excel.md`, `files.md`). None of that changes. ## Changes **SAS - `sas/sasjs/services/editors/getdata.sas`** - No longer emits the column: the two data-step assignments, the `outdata` hoist, the `ctrlOptions` branch and the `maxvarlengths` override are gone. (With the delete column gone, `ctrlOptions` had no other producer in this service, so the whole plumbing went with it.) **Frontend - `deleteRecordColumn.ts` (new)** - `prepareDeleteRecordColumn(response)` asserts the response does **not** carry the name, then prepends it to `COLHEADERS`. Everything downstream (`DcValidator`'s rules, `columnHeader`, `headerArray`) is keyed off `COLHEADERS` order, so the delete column is still index 0 and the grid is unchanged. - `seedDeleteRecordColumn(rows)` sets every row to `'No'` before the validator or HyperFormula sees it. - A response that *does* carry the name **throws**, rather than rendering a table whose delete column would shadow a real column of that name. The throw lands in the existing `getdata` `.catch`, which sets `getdataError` and shows the error state - so the failure is visible, not silent. - `multi-dataset.component.ts` builds its own `DcValidator` from the same responses, so it goes through the same helper. **Mocks** - the server and sas9 `getdata` mocks are updated to the new contract. `postdata`, `getstagetable` and `makedata` are untouched. ## Screenshot The editor, on the mocked estate, with the client-owned **Delete?** column first and its dropdown open. The spec asserts the header text and that the dropdown contains exactly `No` / `Yes` before it shoots. ![The editor with the client-owned Delete? column first and its No/Yes dropdown open](https://git.datacontroller.io/attachments/8338e427-abfe-40fe-863c-bc3faa1b629d) ## Testing - client unit tests **570/570** (`npm run test:headless`), including 7 new cases for `deleteRecordColumn.ts` - `npm run build` (production) clean - `sasjs lint` clean on the changed SAS; `sasjs compile -t server` succeeds - Cypress against the mocked estate: `coltype-delete-record.cy.ts` **2/2**, `editor.cy.ts` **55/55**, `excel.cy.ts` **23/23**, `row-cell-limits.cy.ts` **5/5** `coltype-delete-record.cy.ts` is the direct regression guard: it opens the editor on `MPE_X_TEST` and asserts the first column renders its hardcoded Yes/No dropdown end to end. ## Deploy notes - **The name is reserved.** A table that contains a real column called `_____DELETE__THIS__RECORD_____` will now fail to open in the editor, with the error state rather than a grid. That is the intended behaviour - the name would otherwise shadow the delete flag - and the docs PR adds the note. - No migration and no configuration change. The response is one column smaller, and the grid is identical. - Docs: `docs.datacontroller.io` PR updates the Developer Guide's pre-edit hook example (which mirrored the old data step) and adds the reserved-name note.
hermes left a comment
Author
Collaborator

Reviewed the full diff at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it), the head tree for the stagedata/mpe_loader positional contract, and the PR CI state.

  • client/src/app/multi-dataset/multi-dataset.component.ts:1084 - prepareDeleteRecordColumn(response) throws inside buildDatasetsObjects(), which runs in the Promise.allSettled(...).then(...) callback at line 359 with no .catch anywhere on that chain (the only .catch in the block, line 419, is the per-dataset excel-parse one). The new module's own doc comment says callers must surface the throw in a promise-chain .catch, and the editor does (editor.component.ts:3527 sets getdataError), but in the multi-dataset flow a getdata response carrying the reserved column becomes an unhandled promise rejection: the loop aborts, parsedDatasets stays empty, and no error state or modal is shown. Add a .catch to the allSettled chain (or wrap the buildDatasetsObjects call) that surfaces the error the way fetchDataset failures are surfaced.

  • client/src/app/multi-dataset/multi-dataset.component.ts:1111 - this rename block still uses the literal '_____DELETE__THIS__RECORD_____' while the same PR moves the identical block in editor.component.ts:3945 to DELETE_RECORD_COLUMN_NAME. Use the exported constant here too so the reserved name has a single definition.

2 findings above for review.

Reviewed the full diff at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it), the head tree for the stagedata/mpe_loader positional contract, and the PR CI state. - client/src/app/multi-dataset/multi-dataset.component.ts:1084 - `prepareDeleteRecordColumn(response)` throws inside `buildDatasetsObjects()`, which runs in the `Promise.allSettled(...).then(...)` callback at line 359 with no `.catch` anywhere on that chain (the only `.catch` in the block, line 419, is the per-dataset excel-parse one). The new module's own doc comment says callers must surface the throw in a promise-chain `.catch`, and the editor does (editor.component.ts:3527 sets `getdataError`), but in the multi-dataset flow a getdata response carrying the reserved column becomes an unhandled promise rejection: the loop aborts, `parsedDatasets` stays empty, and no error state or modal is shown. Add a `.catch` to the allSettled chain (or wrap the `buildDatasetsObjects` call) that surfaces the error the way `fetchDataset` failures are surfaced. - client/src/app/multi-dataset/multi-dataset.component.ts:1111 - this rename block still uses the literal `'_____DELETE__THIS__RECORD_____'` while the same PR moves the identical block in editor.component.ts:3945 to `DELETE_RECORD_COLUMN_NAME`. Use the exported constant here too so the reserved name has a single definition. 2 findings above for review.
Author
Collaborator

Both addressed in 2b8a2fa.

1. Unhandled rejection in the multi-dataset flow - wrapped the buildDatasetsObjects(datasets) call in a try/catch rather than adding a .catch to the allSettled chain. Chaining after that .then() re-indented its whole 60-line body (142 changed lines for a two-line fix); the wrap confines the diff to the call site. On a throw it logs and drops the parse, the same as fetchDataset's own .catch does for a failed getdata.

2. The name constant - the rename-to-Delete? block in multi-dataset.component.ts now uses DELETE_RECORD_COLUMN_NAME, matching the same block in editor.component.ts.

Re-verified after the change: client unit tests 570/570, npm run build clean. The full Cypress suite re-runs on this head in CI.

On the remaining literal occurrences of _____DELETE__THIS__RECORD_____ in these two components (the Excel-preview default, the submit-time reorder and the cell-change handler): those predate this PR and are untouched, so the diff stays on the sites the change actually moves. Happy to sweep them to the constant in a follow-up if you would rather the name had exactly one definition everywhere.

Both addressed in 2b8a2fa. **1. Unhandled rejection in the multi-dataset flow** - wrapped the `buildDatasetsObjects(datasets)` call in a `try`/`catch` rather than adding a `.catch` to the `allSettled` chain. Chaining after that `.then()` re-indented its whole 60-line body (142 changed lines for a two-line fix); the wrap confines the diff to the call site. On a throw it logs and drops the parse, the same as `fetchDataset`'s own `.catch` does for a failed getdata. **2. The name constant** - the rename-to-`Delete?` block in `multi-dataset.component.ts` now uses `DELETE_RECORD_COLUMN_NAME`, matching the same block in `editor.component.ts`. Re-verified after the change: client unit tests 570/570, `npm run build` clean. The full Cypress suite re-runs on this head in CI. On the remaining literal occurrences of `_____DELETE__THIS__RECORD_____` in these two components (the Excel-preview default, the submit-time reorder and the cell-change handler): those predate this PR and are untouched, so the diff stays on the sites the change actually moves. Happy to sweep them to the constant in a follow-up if you would rather the name had exactly one definition everywhere.
hermes left a comment
Author
Collaborator

Re-reviewed the full diff at 2b8a2fa against the previous review at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it). Scope of this pass: the delta commit (surfacing the reserved-column guard in the multi-dataset flow, using the name constant) plus a re-read of the full base...head diff against the head tree. Both previous findings (the unhandled rejection from buildDatasetsObjects in the allSettled chain, and the literal reserved name in the rename block) are addressed at this head and not re-raised. Verified against the head tree: the mpe_targetloader/bitemporal_dataloader macros that consume the delete flag on submit are untouched by this PR; stagedata.sas still synthesizes its own delete column server-side; the SAS delta leaves no dangling ctrloptions reference (the vars3 keep-list was updated in step with the ctrlOptions block's removal); the gitleaks pre-commit hook, .npmrc (ignore-scripts + save-exact) and the strict npm audit steps all stand.

  • client/src/app/multi-dataset/multi-dataset.component.ts:377 - the new catch logs and returns, but unlike fetchDataset's failures (which resolve to undefined and let the remaining datasets parse) it drops the whole parse with nothing shown to the user: uploadLoading is already false, parsedDatasets stays empty, and the only trace is a console.warn. A response carrying the reserved column means a real table column of that name - the editor flow refuses the load with an on-screen error state (editor.component.ts:3529 sets getdataError), so this flow should surface it the same way (eventService.showInfoModal is the pattern this file already uses elsewhere) rather than silently showing an empty "select tables" state with the file still attached.

  • client/src/app/editor/editor.component.ts:3894 - prepareDeleteRecordColumn(response) throws inside initSetup, and the comment at 3892 says "A throw here lands in the caller's .catch" - that only holds on the direct path. initSetup retries via setTimeout(() => this.initSetup(response, attempt + 1), 50) (line 3885) when hotInstance is not ready yet, and a throw from that retry runs outside the promise chain: the .catch at 3523 is not re-entered, so the reserved-column refusal becomes an unhandled rejection and the user is left on the loading spinner (getdataError never set, because tableTrue is only assigned at 3928, after the throw point). Guard for it: set this.getdataError = true (or route through the same catch) before rethrowing inside the retry branch too, so the refusal is surfaced whichever attempt it lands on.

2 findings above for review.

Re-reviewed the full diff at 2b8a2fa against the previous review at 56025cc (the editor takes ownership of the delete-record column; getdata.sas and the mocks stop emitting it). Scope of this pass: the delta commit (surfacing the reserved-column guard in the multi-dataset flow, using the name constant) plus a re-read of the full base...head diff against the head tree. Both previous findings (the unhandled rejection from buildDatasetsObjects in the allSettled chain, and the literal reserved name in the rename block) are addressed at this head and not re-raised. Verified against the head tree: the mpe_targetloader/bitemporal_dataloader macros that consume the delete flag on submit are untouched by this PR; stagedata.sas still synthesizes its own delete column server-side; the SAS delta leaves no dangling ctrloptions reference (the vars3 keep-list was updated in step with the ctrlOptions block's removal); the gitleaks pre-commit hook, .npmrc (ignore-scripts + save-exact) and the strict npm audit steps all stand. - client/src/app/multi-dataset/multi-dataset.component.ts:377 - the new catch logs and returns, but unlike fetchDataset's failures (which resolve to undefined and let the remaining datasets parse) it drops the whole parse with nothing shown to the user: uploadLoading is already false, parsedDatasets stays empty, and the only trace is a console.warn. A response carrying the reserved column means a real table column of that name - the editor flow refuses the load with an on-screen error state (editor.component.ts:3529 sets getdataError), so this flow should surface it the same way (eventService.showInfoModal is the pattern this file already uses elsewhere) rather than silently showing an empty "select tables" state with the file still attached. - client/src/app/editor/editor.component.ts:3894 - `prepareDeleteRecordColumn(response)` throws inside initSetup, and the comment at 3892 says "A throw here lands in the caller's .catch" - that only holds on the direct path. initSetup retries via `setTimeout(() => this.initSetup(response, attempt + 1), 50)` (line 3885) when hotInstance is not ready yet, and a throw from that retry runs outside the promise chain: the .catch at 3523 is not re-entered, so the reserved-column refusal becomes an unhandled rejection and the user is left on the loading spinner (getdataError never set, because tableTrue is only assigned at 3928, after the throw point). Guard for it: set this.getdataError = true (or route through the same catch) before rethrowing inside the retry branch too, so the refusal is surfaced whichever attempt it lands on. 2 findings above for review.
Author
Collaborator

Both addressed in b26f557.

1. The retry path in initSetup - you are right that a throw from the setTimeout retry is outside the promise chain. The retry call is now wrapped and routes through a new private handleGetdataFailure(err), which is also what the callService .catch now calls - so the refusal sets getdataError/tableTrue and shows the error state whichever attempt it lands on.

2. The multi-dataset catch - now surfaces eventService.showInfoModal('Upload error', err.message), the pattern this file already uses for the other upload problems, instead of logging and returning.

Re-verified after the change: client unit tests 570/570, npm run build clean. The Cypress suite re-runs on this head.

One note on the previous head: Build-and-test-development went red on 2b8a2fa, on excel.cy.ts test 18 (openTableFromTree waiting on .app-loading), which the same spec passed on 56025cc. It is the home-page boot spinner, not the upload path this PR touches, and the neighbouring tests in the same run passed - I have re-run the job. If it fails again on this head I will treat it as real and dig in.

Both addressed in b26f557. **1. The retry path in `initSetup`** - you are right that a throw from the `setTimeout` retry is outside the promise chain. The retry call is now wrapped and routes through a new private `handleGetdataFailure(err)`, which is also what the `callService` `.catch` now calls - so the refusal sets `getdataError`/`tableTrue` and shows the error state whichever attempt it lands on. **2. The multi-dataset catch** - now surfaces `eventService.showInfoModal('Upload error', err.message)`, the pattern this file already uses for the other upload problems, instead of logging and returning. Re-verified after the change: client unit tests 570/570, `npm run build` clean. The Cypress suite re-runs on this head. One note on the previous head: `Build-and-test-development` went red on 2b8a2fa, on `excel.cy.ts` test 18 (`openTableFromTree` waiting on `.app-loading`), which the same spec passed on 56025cc. It is the home-page boot spinner, not the upload path this PR touches, and the neighbouring tests in the same run passed - I have re-run the job. If it fails again on this head I will treat it as real and dig in.
hermes left a comment
Author
Collaborator

Re-reviewed the full base...head diff at b26f557, focused on the delta commit (surfacing the reserved-column refusal on every getdata path) plus a re-read of the earlier commits against the head tree. Both findings from the review at 2b8a2fa are addressed at this head and not re-raised: the initSetup retry branch (editor.component.ts:3899) now wraps the recursive call in a try/catch routed through handleGetdataFailure, so a throw from any retry attempt lands in the error state instead of an unhandled rejection; and the multi-dataset catch (multi-dataset.component.ts:380) now surfaces the refusal on screen via eventService.showInfoModal rather than only a console.warn. Verified in the delta and head tree: initSetup is synchronous so the try/catch spans the retry correctly; the cypress childNodes[N+1] position math matches the new mock COLHEADERS with the client-prepended column; the %length(&pk) guard around the attrib statement is needed now that the delete name no longer pads it.

No blocking issues.

Re-reviewed the full base...head diff at b26f557, focused on the delta commit (surfacing the reserved-column refusal on every getdata path) plus a re-read of the earlier commits against the head tree. Both findings from the review at 2b8a2fa are addressed at this head and not re-raised: the initSetup retry branch (editor.component.ts:3899) now wraps the recursive call in a try/catch routed through handleGetdataFailure, so a throw from any retry attempt lands in the error state instead of an unhandled rejection; and the multi-dataset catch (multi-dataset.component.ts:380) now surfaces the refusal on screen via eventService.showInfoModal rather than only a console.warn. Verified in the delta and head tree: initSetup is synchronous so the try/catch spans the retry correctly; the cypress childNodes[N+1] position math matches the new mock COLHEADERS with the client-prepended column; the %length(&pk) guard around the attrib statement is needed now that the delete name no longer pads it. No blocking issues.
hermes changed target branch from main to feat/release-7.18 2026-10-16 12:42:10 +00:00
hermes added 3 commits 2026-10-16 12:42:10 +00:00
fix(editor): own the delete-record column client-side
Build / Build-and-ng-test (pull_request) Successful in 5m36s
Lighthouse Checks / lighthouse (pull_request) Successful in 21m15s
Build / Build-and-test-development (pull_request) Successful in 30m57s
56025cc052
getdata.sas synthesised _____DELETE__THIS__RECORD_____ itself - it added
the column to work.out, hoisted it in front of the PKs, set it to "No" on
every row, gave it a No/Yes dropdown in the metadata and forced MAXLEN=3
in maxvarlengths. In the editor that column is a constant: it is only ever
"No" on load, and its Yes/No rule is already injected client-side
(deleteRecordColumnRule.ts, since #253's vertical-array fix). So the
backend was sending a column the client already owns.

The editor now creates the column, the same way EDIT_STATUS / dc.row_status
is client-only:

- getdata.sas no longer emits it (data steps, the outdata hoist, the
  ctrlOptions dropdown, the maxvarlengths override).
- deleteRecordColumn.ts asserts the response does NOT carry the name, then
  prepends it to COLHEADERS. Everything downstream (DcValidator's rules,
  columnHeader, headerArray) is keyed off COLHEADERS order, so the delete
  column is still index 0 and the grid is unchanged.
- Every row is seeded to 'No' before the validator or HyperFormula sees it.
- A response that does carry the name now throws instead of rendering a
  table whose delete column would shadow a real column of that name. The
  throw lands in the existing getdata .catch, which sets getdataError and
  shows the error state. The name is reserved; the docs note is in the
  sibling docs PR.

The multi-dataset loader (multi-dataset.component.ts) builds its own
DcValidator from the same responses, so it goes through the same helper.

Mocks updated to the new contract (server + sas9 getdata); postdata,
getstagetable and makedata are untouched - they consume or seed the flag
legitimately, and the upload contract in excel.md / files.md is unchanged.

Verified: client unit tests 570/570, npm run build clean, sasjs lint clean
on the changed SAS, sasjs compile -t server succeeds, and against the
mocked estate coltype-delete-record.cy.ts 2/2, editor.cy.ts 55/55,
excel.cy.ts 23/23 and row-cell-limits.cy.ts 5/5.
fix(multi-dataset): surface the reserved-column guard, use the name constant
Lighthouse Checks / lighthouse (pull_request) Successful in 21m1s
Build / Build-and-ng-test (pull_request) Successful in 5m14s
Build / Build-and-test-development (pull_request) Successful in 32m10s
2b8a2fa103
Review on #343:

- buildDatasetsObjects() runs inside the Promise.allSettled(...).then(...)
  chain in onStartParsingFile, which had no .catch. prepareDeleteRecordColumn
  throws on a response carrying the reserved delete-record column, so in this
  flow that became an unhandled rejection: the loop aborted, no dataset was
  parsed and no state was shown. Wrap the call and drop the parse, the way
  fetchDataset's own catch handles a failed getdata.
- The rename-to-Delete? block still used the literal name while the same
  block in editor.component.ts uses DELETE_RECORD_COLUMN_NAME. Use the
  constant here too.
fix(editor): surface the reserved-column refusal on every getdata path
Build / Build-and-ng-test (pull_request) Successful in 6m24s
Lighthouse Checks / lighthouse (pull_request) Successful in 22m18s
Build / Build-and-test-development (pull_request) Successful in 32m50s
b26f5571b6
Review on #343:

- initSetup's retry branch called itself from a bare setTimeout, outside the
  callService promise chain, so the reserved-column throw on a retry never
  reached the .catch: getdataError was never set and the user was left on the
  loading spinner. Wrap the retry call and route it through the same failure
  handler the .catch uses (now a private handleGetdataFailure, shared by
  both).
- The multi-dataset catch logged and returned, leaving an empty table
  selection with the file still attached and only a console.warn. Surface it
  with eventService.showInfoModal, the pattern this file already uses for
  other upload problems.
allan merged commit d6e0662b16 into feat/release-7.18 2026-10-16 13:01:43 +00:00
allan deleted branch fix/getdata-delete-column-client-side 2026-10-16 13:01:43 +00:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dc/dc#343