refactor: split org-template-editor by output cluster (RD-25)
org-template-editor.component.ts carried an eslint-disable for max-lines, padded by a dead sample-letter constant, 13 label inputs that were never bindable, and two self-contained mutation clusters. Split all three out: - SAMPLE_LETTER_BRIEF moves to brief/domain/sample-letter.ts. It is production content (the letter the admin previews), not a test fixture, so it stays out of brief.testing.ts (no-testing-in-production forbids production code from reaching a *.testing.ts file). - 11 of the 13 label inputs become inline i18n template text. The two that interpolate MARGIN_MIN_MM/MARGIN_MAX_MM (marginsLegend, invalidHint) stay in TS, because moving an interpolated $localize call into a template renames the xlf placeholder and breaks the translation merge. Every id is preserved; messages.en.xlf is unchanged. - logo-upload.component.ts and version-history.component.ts each take one output cluster. The parent still declares and re-emits all 11 outputs — org-template.page.ts binds them directly on <app-org-template-editor> and is out of this ticket's file scope, so the parent's public surface cannot shrink. Correction to the ticket while executing it: its acceptance check for "= output" on the parent read "MUST be 7", copying decision 4's cluster count instead of decision 5's (and the ticket's own Risks section's) explicit requirement that the parent keep all 11 declarations. Fixed the ticket's acceptance section to the correct number. npm run ci --full is green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Claude Sonnet 5
@@ -0,0 +1,176 @@
|
||||
# RD-25 — Split `org-template-editor` by output cluster
|
||||
|
||||
Status: done
|
||||
Source: PLAN.md 3f, order step 7
|
||||
|
||||
## Why
|
||||
|
||||
`org-template-editor.component.ts` measures ~330 effective lines against a limit of 250, and
|
||||
carries `/* eslint-disable max-lines */`. Three separate things pad it:
|
||||
|
||||
- a 44-line sample letter constant, exported but used only in this file;
|
||||
- 13 label `input()`s that are all declared `protected`, so nothing can ever bind them — they
|
||||
are constants wearing input ceremony;
|
||||
- two self-contained blocks, the logo uploader and the version history.
|
||||
|
||||
The **11 `output()`s are the tell**: each cluster is a mutation family, and two of them lift out
|
||||
whole.
|
||||
|
||||
## Read first
|
||||
|
||||
- `org-template-editor.component.ts:27` — `SAMPLE_LETTER_BRIEF`, and line 284, its only use.
|
||||
- `org-template-editor.component.ts:272-297` — the inputs and the 11 outputs.
|
||||
- `org-template-editor.component.ts:337-353` — the 13 label inputs.
|
||||
- `org-template-editor.component.ts:195-235` — the logo block and the history block, the two
|
||||
that become children.
|
||||
- `.dependency-cruiser.base.js:113-120` — `no-testing-in-production`. Decision 1 depends on it.
|
||||
- PLAN.md 3e — the `$localize` boundary, which decision 2 applies.
|
||||
|
||||
## Decisions (pre-made, don't relitigate)
|
||||
|
||||
1. **`SAMPLE_LETTER_BRIEF` moves to `apps/ssp/src/app/brief/domain/sample-letter.ts`.** It is
|
||||
production content — the letter the admin previews — not a test fixture.
|
||||
|
||||
**It must not go into `brief.testing.ts`, and nothing may import it from there.**
|
||||
`.dependency-cruiser.base.js`'s `no-testing-in-production` rule forbids production code
|
||||
reaching any `*.testing.ts`, so putting it there fails `npm run dep:check`. A new
|
||||
`domain/sample-letter.ts` beside `brief.ts` is the right home; `domain/` is pure TS, and this
|
||||
is data.
|
||||
|
||||
2. **11 of the 13 label inputs become inline `i18n` attributes in the template. Two stay.**
|
||||
|
||||
The two that stay are parameterised, and PLAN 3e explains why moving them breaks the build:
|
||||
|
||||
| Keep in TS | Id | Why |
|
||||
| --------------- | ----------------------- | -------------------------------------------- |
|
||||
| `marginsLegend` | `@@orgTemplate.margins` | interpolates `MARGIN_MIN_MM`/`MARGIN_MAX_MM` |
|
||||
| `invalidHint` | `@@orgTemplate.invalid` | same two interpolations |
|
||||
|
||||
The `.xlf` stores an interpolation as `<x id="min" equiv-text="MARGIN_MIN_MM"/>`. Moving such
|
||||
a message into a template renames the placeholder to `INTERPOLATION`, the translation merge
|
||||
no longer matches, and `ng build --localize` fails.
|
||||
|
||||
**Every id is preserved.** A plain `protected foo = input($localize`:@@id:Text`)` used as
|
||||
`{{ foo() }}` becomes the literal text in the template with an `i18n="@@id"` attribute, or
|
||||
`i18n-label="@@id"` when it feeds an attribute. Neither `messages.en.xlf` may change.
|
||||
|
||||
3. **Two new children, each taking one output cluster:**
|
||||
|
||||
| File | Class | Inputs | Outputs |
|
||||
| ------------------------------ | ---------------- | ----------------------------------------- | ------------------------------------------ |
|
||||
| `logo-upload.component.ts` | `LogoUpload` | `logoUrl`, `uploadState`, `previewUrlFor` | `logoSelected`, `logoRemoved`, `logoRetry` |
|
||||
| `version-history.component.ts` | `VersionHistory` | `history`, `publishedVersion` | `rollback` |
|
||||
|
||||
They live beside the parent, in `apps/ssp/src/app/brief/ui/org-template-editor/`.
|
||||
|
||||
4. **The parent keeps seven outputs, not the five PLAN estimated.** `selectSubOrg`,
|
||||
`templateEdit`, `marginEdit`, `requestPublish`, `confirmPublish`, `cancelPublish`,
|
||||
`proefbrief`. The publish trio stays with the parent because publishing acts on the whole
|
||||
draft, not on the version list; only `rollback` is history's own verb. PLAN's "→ 5" was an
|
||||
estimate made before the outputs were mapped to blocks — its line estimate (~222) is the part
|
||||
that matches this split.
|
||||
|
||||
5. **The parent's public surface does not change.** All 11 outputs still exist on the parent and
|
||||
still fire; two clusters are simply re-emitted from children. `org-template.page.ts` and the
|
||||
story bind exactly what they bind today.
|
||||
|
||||
6. **Delete `/* eslint-disable max-lines */`.** Mandatory — `reportUnusedDisableDirectives` is
|
||||
`error`, so the two rules pin each other in both directions.
|
||||
|
||||
7. **No new stories.** `org-template-editor.stories.ts` already renders both blocks through the
|
||||
parent. The 13 label inputs were `protected`, so no story could bind them and none does.
|
||||
|
||||
## Files
|
||||
|
||||
- `apps/ssp/src/app/brief/domain/sample-letter.ts` (new)
|
||||
- `apps/ssp/src/app/brief/ui/org-template-editor/logo-upload.component.ts` (new)
|
||||
- `apps/ssp/src/app/brief/ui/org-template-editor/version-history.component.ts` (new)
|
||||
- `apps/ssp/src/app/brief/ui/org-template-editor/org-template-editor.component.ts`
|
||||
|
||||
## Steps
|
||||
|
||||
1. Move `SAMPLE_LETTER_BRIEF` to `domain/sample-letter.ts` and import it in the parent
|
||||
(decision 1).
|
||||
2. Inline the 11 plain labels, keeping every id (decision 2).
|
||||
3. Extract `logo-upload.component.ts`, then `version-history.component.ts` (decision 3), wiring
|
||||
each child's outputs to the parent's existing ones.
|
||||
4. Delete the disable (decision 6).
|
||||
5. `git add -A`, then run the acceptance commands.
|
||||
6. Update this ticket's `Status:` to `done` and the README's RD-25 row to `done`.
|
||||
7. Commit all of it together.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
Measured against the tree before handover. Run after `git add -A`.
|
||||
|
||||
```bash
|
||||
P=apps/ssp/src/app/brief/ui/org-template-editor/org-template-editor.component.ts
|
||||
git ls-files 'apps/ssp/src/app/brief/ui/org-template-editor/*.component.ts' | wc -l # is 1 -> MUST be 3
|
||||
git ls-files apps/ssp/src/app/brief/domain/sample-letter.ts | wc -l # is 0 -> MUST be 1
|
||||
git grep -c "eslint-disable max-lines" -- $P # is 1 -> MUST be 0
|
||||
```
|
||||
|
||||
The constant moved, and the parent still uses it (import line plus use line is two lines, so 2
|
||||
is the correct number here):
|
||||
|
||||
```bash
|
||||
git grep -c "export const SAMPLE_LETTER_BRIEF" -- apps/ssp/src/app/brief/domain/sample-letter.ts # MUST be 1
|
||||
git grep -c "export const SAMPLE_LETTER_BRIEF" -- $P # is 1 -> MUST be 0
|
||||
git grep -c "SAMPLE_LETTER_BRIEF" -- $P # is 2 -> MUST still be 2
|
||||
```
|
||||
|
||||
The ceremony is gone and the clusters left (decisions 2, 3, 4):
|
||||
|
||||
```bash
|
||||
git grep -c "protected .* = input(" -- $P # is 13 -> MUST be 2
|
||||
git grep -c "= output" -- $P # is 11 -> MUST still be 11 (decision 5, see below)
|
||||
```
|
||||
|
||||
**Correction found while executing this ticket.** This check originally read `MUST be 7`,
|
||||
copying decision 4's output count. That count is decision 4's classification of which cluster
|
||||
owns each output, not the count of `output()` declarations on the parent class. Decision 5 and
|
||||
this ticket's own Risks section both require the parent to keep declaring all 11 — a child's
|
||||
output is re-emitted, not removed, and `org-template.page.ts` (out of scope, Files list excludes
|
||||
it) binds all 11 directly on `<app-org-template-editor>`. Removing 4 declarations would break
|
||||
that binding. Verified: `npx ng build ssp --localize` and `npm run dep:check` both pass with all
|
||||
11 outputs present, and no other acceptance number changes.
|
||||
|
||||
The translation seam did not move (decision 2):
|
||||
|
||||
```bash
|
||||
git grep -ho "@@orgTemplate[a-zA-Z0-9_.]*" -- apps/ssp/src/app/brief/ui/org-template-editor/ | sort -u | wc -l # is 18 -> MUST still be 18
|
||||
git status --short -- '*.xlf' | wc -l # MUST be 0
|
||||
```
|
||||
|
||||
```bash
|
||||
npm run ci --full # exits 0
|
||||
```
|
||||
|
||||
## Verification
|
||||
|
||||
`ng build --localize` inside the gate is the real check on decision 2: a renamed placeholder or
|
||||
a lost id fails it. The `.xlf` files are hand-maintained, so **if you find yourself editing one,
|
||||
you have changed an id and should undo it instead**.
|
||||
|
||||
`npm run dep:check` inside the gate is the real check on decision 1.
|
||||
|
||||
**Do not add a line-count command.** `npm run lint` is the exact check (decision 6).
|
||||
|
||||
## Out of scope
|
||||
|
||||
- `letter-canvas`. RD-26 owns it, and it keeps its disable.
|
||||
- Changing any label text, any id, or any output name.
|
||||
- The `orgTemplate.publish.impact` message. It is not one of the 13 labels and does not move.
|
||||
- Reworking the upload controller or the publish flow.
|
||||
|
||||
## Risks
|
||||
|
||||
- **Moving a parameterised `$localize` into a template breaks the build** (decision 2). The two
|
||||
named messages stay in TS. If a third turns out to interpolate, leave it in TS too and say so
|
||||
in the commit message.
|
||||
- **`brief.testing.ts` is the wrong home for the sample letter** (decision 1), and the failure
|
||||
is a dependency-cruiser error rather than a type error, so it will not show up until
|
||||
`dep:check`.
|
||||
- **The parent must keep all 11 outputs** (decision 5). A child's output is re-emitted, not
|
||||
removed — `org-template.page.ts` binds them.
|
||||
- **Deleting the disable is mandatory** (decision 6).
|
||||
Reference in New Issue
Block a user