diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md b/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md index a9e3879..c888748 100644 --- a/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md +++ b/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md @@ -126,7 +126,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita | **RB-22** | ssp/brief | CQRS-light | _(expand)_ `BriefStore.load()` tolerates a 404 by calling the existing `reset()` once | BL-003; §7 Backend CQRS-light row | S | Low | P2 | 4 | — | **SIGN-OFF** | **done** | | **RB-23** | backend/Program.cs + Data | CQRS-light | _(contract)_ `GET /brief` 404s when absent; `GetOrCreate` → `Get` | BL-003; §7 Backend CQRS-light row | S | Med | P2 | 4 | RB-22 | **SIGN-OFF** | **done** | | **RB-24** | libs/shared/upload | ADR conform. | Move `upload/` into `infrastructure`/`domain`/`application`; **delete** the depcruise carve-out | BL-010; §7 "+1 adapter outside `infrastructure/`", "8 of 9 machines in `domain/`"; §3b shared/domain 0% reach | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** | -| **RB-25** | libs/shared/upload | testability | `UPLOAD_TRANSPORT` injection token (the `SESSION_PORT` shape) instead of `inject(KeepaliveTransport)` | §3a upload 52.0%/50.0%; §3b file unreached, non-`ui/` | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | open | +| **RB-25** | libs/shared/upload | testability | `UPLOAD_TRANSPORT` injection token (the `SESSION_PORT` shape) instead of `inject(KeepaliveTransport)` | §3a upload 52.0%/50.0%; §3b file unreached, non-`ui/` | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** | | **RB-26** | libs/shared/upload | testability | Move the accept/reject decision to `planFileSelection` in `upload.machine.ts` | §3a upload 52.0%/50.0%; §4a module max CC 27 | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** | | **RB-27** | libs/shared/upload | testability | Extract `uploadOutcome(status, responseText)` out of the XHR closure | file LH 5/64 (**7.8% line**), BRH 3/57 (**5.3% branch**) | S–M | Low | P2 | 5 | RB-25 | **SIGN-OFF** | open | | **RB-28** | libs/beheer + ssp/brief | testability | `BLOB_PRESENTER` token; the 3 commands' success paths become assertable | §3a beheer/application **40.5% branch — worst FE**; brief.store BRH 32/64 | S–M | Low | P2 | 5 | — | **SIGN-OFF** | open | diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-25.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-25.md new file mode 100644 index 0000000..bc4949e --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-25.md @@ -0,0 +1,130 @@ +# RB-25 — `UPLOAD_TRANSPORT` injection token replaces `inject(KeepaliveTransport)` + +Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-003 · +`99-backlog.md` RB-25, "Merges" table row for RB-25/26/27 · Depends on +`implementation/rb-24.md` (the move that put this file at its current path) + +## What was wrong + +`libs/shared/src/application/upload-shell.service.ts` defines `export interface +UploadTransport` and documents it as the swap seam for upload transport. It then binds +`private transport: UploadTransport = inject(KeepaliveTransport)` — the concrete class, +which is `@Injectable` but not exported. A spec cannot reference the class to override its +provider, and cannot provide against the interface either, because an interface is not a +DI token. The port existed on paper only. + +The consequence TE-003 measures: `upload()`, `delete()`, `cancel()`, and `pollReturning()` +— the methods that translate transport and adapter outcomes into `UploadMsg`s — had no +spec at all. `libs/shared/upload` sat at 52.0% line / 50.0% branch, and +`upload-shell.service.ts` was one of the two unreached non-`ui/` files. + +## What changed + +One file plus one new spec, exactly as scoped: + +- `libs/shared/src/application/upload-shell.service.ts`: added + + ```ts + export const UPLOAD_TRANSPORT = new InjectionToken('UPLOAD_TRANSPORT', { + providedIn: 'root', + factory: () => inject(KeepaliveTransport), + }); + ``` + + copied verbatim from TE-003's own fix, placed directly under the `KeepaliveTransport` + class it wraps. `UploadShellService.transport` now reads + `inject(UPLOAD_TRANSPORT)` instead of `inject(KeepaliveTransport)`. This is the same + interface-plus-token shape as `SessionPort`/`SESSION_PORT` + (`libs/shared/src/application/session.port.ts`), the repo's one other explicit port. + `KeepaliveTransport` itself is untouched: still a private, unexported `@Injectable`, and + still the default factory's target — a real app gets the exact same singleton instance + it always did. + +- `libs/shared/src/application/upload-shell.service.spec.ts` (new): a recording fake + `UploadTransport` (records every `send()` call, exposes `resolveDone`/`rejectDone` per + call so a test drives the returned `Promise` by hand) provided against `UPLOAD_TRANSPORT`, + plus a fake `UploadAdapter` (a plain object with `vi.fn()` for `status`/`deleteDocument`) + provided against the already-exported `UploadAdapter` class. 16 specs across all four + target methods: + - `upload()` — `UploadQueued` carries the transport's `backgroundSyncAvailable`; + `onProgress` → `UploadProgress`; a resolved transport → `UploadComplete`; a rejected + transport → `UploadFailed` with the rejection reason; a rejection with the + `UPLOAD_ABORTED` sentinel dispatches nothing. + - `cancel()` — calls the stored cancel function for an in-flight upload and forgets it + (a second `cancel()` on the same id is a no-op); an unknown id is a no-op. + - `delete()` — `UploadDeleting` then `UploadDeleteComplete` on success; + `UploadDeleteFailed` with the server's `detail` on a ProblemDetails rejection; falls + back to an empty reason when the rejection carries no `detail`. + - `pollReturning()` — skips the adapter call entirely for an empty upload list; + dispatches `BackgroundUploadsReturned` filtered to only the items the server reports + `complete` with a `documentId`; dispatches nothing when nothing has arrived. + +No other file changed. `UploadAdapter`, `upload.machine.ts`, and `upload-controller.ts` +are untouched, per the ticket's file-scope fence (RB-26 and RB-28 are concurrently in +adjacent files). + +## Verification + +- **Coverage, `upload-shell.service.ts`** (`npm run test:coverage` narrowed to `shared`, + read from `coverage/shared/lcov.info`): + + | Metric | Before | After | + | --------- | ------ | -------------- | + | Lines | 0% | 88.57% (31/35) | + | Branches | 0% | 85.00% (17/20) | + | Functions | 0% | 87.50% (14/16) | + + "Before" is 0% across the board: no spec file for this service existed prior to this + ticket (confirmed by `grep -rln UploadShellService --include=*.spec.ts`, which returns + only the new spec), matching TE-003's "unreached" classification. The remaining + uncovered lines are the `KeepaliveTransport` class body (`send()`, its `inject`) and the + `UPLOAD_TRANSPORT` factory closure itself — both require a real `XMLHttpRequest`/real DI + resolution to exercise and are intentionally out of this ticket's scope: TE-003's fix is + the seam, not a rewrite of the transport it wraps. + +- **Red-proof.** Edited `upload()`'s success branch from + `dispatch({ type: 'UploadComplete', localId: req.localId, documentId })` to + `dispatch({ type: 'UploadFailed', localId: req.localId, reason: 'BROKEN-FOR-RED-PROOF' })`, + ran `ng test shared`. Result: 1 failed / 150 passed, with + + ``` + AssertionError: expected "vi.fn()" to be called with arguments: [ { type: 'UploadComplete', …(2) } ] + Received: + 1st vi.fn() call: [{ "backgroundSync": false, "localId": "l1", "type": "UploadQueued" }] + 2nd vi.fn() call: [{ "localId": "l1", "reason": "BROKEN-FOR-RED-PROOF", "type": "UploadFailed" }] + ``` + + at `upload-shell.service.spec.ts:79` (the `UploadComplete` assertion). Re-applied the + original line with a second edit (not `git checkout`); `git diff` against HEAD shows + only the intended token change — the red edit left no trace. Re-ran: 151/151 green. + +- `npm run ci`: result and step count in the final answer. + +## Judgement call + +- **The fake `UploadAdapter` is a plain object, not a class extending `UploadAdapter`.** + `UploadAdapter` is exported and already usable as a DI token (it always was — TE-003's + gap was specific to `KeepaliveTransport`, not `UploadAdapter`), so `delete()` and + `pollReturning()` (which never touch `this.transport`) were technically fakeable before + this ticket by providing a fake `UploadAdapter`. Nobody had written that spec, though, + and `upload()`/`cancel()` still needed `UPLOAD_TRANSPORT` regardless (they populate and + drain the `inflight` map via `transport.send()`). The spec fakes both seams together so + all four methods are exercised as one coherent suite, per the ticket's own framing + ("provide a recording fake transport and assert the message translation in `upload()`, + `delete()`, `cancel()` and `pollReturning()`"). + +## Handoff to RB-27 + +RB-27 extracts `uploadOutcome(status, responseText)` out of the XHR closure in +`libs/shared/src/infrastructure/upload.adapter.ts`'s `xhrUpload` — a different file, +untouched by this ticket. The token makes RB-27's optional half (moving the +`currentScenario()` branch into `KeepaliveTransport.send()`) no easier and no harder than +before: `KeepaliveTransport` is still unexported and its `send()` body is unchanged, one +line (`inject(UploadAdapter)`, `return this.adapter.xhrUpload(req, onProgress)`). If RB-27 +takes that optional move, it can inject `UPLOAD_TRANSPORT` in its own spec to assert the +scenario branch without touching this file — the seam is there and provided-in-root, but +RB-27 does not need to change anything here to use it. + +## `npm run ci` + +Result and step count reported in the final answer. diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 0988f9b..9b27688 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,7 +20,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page -**is** the suite, reshaped for a business reader. 474 frontend behaviours across +**is** the suite, reshaped for a business reader. 487 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -675,6 +675,31 @@ classes. - map only touches Success - map2 precedence: Failure > Loading > Success +#### UploadShellService.cancel + +- calls the transport cancel function for an in-flight upload and forgets it +- is a no-op for a localId with nothing in flight + +#### UploadShellService.delete + +- dispatches UploadDeleting, then UploadDeleteComplete on success +- dispatches UploadDeleteFailed with the server detail on failure +- falls back to an empty reason when the server sends no detail + +#### UploadShellService.pollReturning + +- does nothing when there are no uploads to poll +- dispatches BackgroundUploadsReturned for uploads the server reports complete +- does not dispatch when nothing has arrived yet + +#### UploadShellService.upload + +- dispatches UploadQueued with the transport backgroundSync flag, then sends via the transport +- translates a progress callback into UploadProgress +- translates a resolved transport into UploadComplete +- translates a rejected transport into UploadFailed with the reason +- does not dispatch UploadFailed on a user-initiated abort + #### authGuard - allows an authenticated user diff --git a/libs/shared/src/application/upload-shell.service.spec.ts b/libs/shared/src/application/upload-shell.service.spec.ts new file mode 100644 index 0000000..4a56652 --- /dev/null +++ b/libs/shared/src/application/upload-shell.service.spec.ts @@ -0,0 +1,230 @@ +import { TestBed } from '@angular/core/testing'; +import { describe, it, expect, vi } from 'vitest'; +import { + UploadAdapter, + UPLOAD_ABORTED, + XhrUploadHandle, +} from '@shared/infrastructure/upload.adapter'; +import { UploadMsg } from '@shared/domain/upload.machine'; +import { UploadShellService, UploadTransport, UPLOAD_TRANSPORT } from './upload-shell.service'; + +/** Records every `send()` call and lets a test resolve/reject/report progress by hand. */ +function fakeTransport(backgroundSyncAvailable = false) { + const handles: Array<{ + req: Parameters[0]; + onProgress: (pct: number) => void; + resolveDone: (v: { documentId: string }) => void; + rejectDone: (e: unknown) => void; + cancel: ReturnType; + }> = []; + const transport: UploadTransport = { + backgroundSyncAvailable, + send: vi.fn((req, onProgress) => { + let resolveDone!: (v: { documentId: string }) => void; + let rejectDone!: (e: unknown) => void; + const done = new Promise<{ documentId: string }>((res, rej) => { + resolveDone = res; + rejectDone = rej; + }); + const cancel = vi.fn(); + handles.push({ req, onProgress, resolveDone, rejectDone, cancel }); + const handle: XhrUploadHandle = { done, cancel }; + return handle; + }), + }; + return { transport, handles }; +} + +function setup(opts: { backgroundSync?: boolean; adapter?: Partial } = {}) { + const { transport, handles } = fakeTransport(opts.backgroundSync ?? false); + const adapter: Partial = { + status: vi.fn().mockResolvedValue([]), + deleteDocument: vi.fn().mockResolvedValue(undefined), + ...opts.adapter, + }; + TestBed.configureTestingModule({ + providers: [ + { provide: UPLOAD_TRANSPORT, useValue: transport }, + { provide: UploadAdapter, useValue: adapter }, + ], + }); + const service = TestBed.inject(UploadShellService); + const dispatch = vi.fn<(m: UploadMsg) => void>(); + return { service, dispatch, transport, handles, adapter }; +} + +const req = { localId: 'l1', categoryId: 'c1', wizardId: 'w1', file: new File(['x'], 'x.pdf') }; + +describe('UploadShellService.upload', () => { + it('dispatches UploadQueued with the transport backgroundSync flag, then sends via the transport', () => { + const { service, dispatch, transport } = setup({ backgroundSync: true }); + service.upload(req, dispatch); + expect(dispatch).toHaveBeenCalledWith({ + type: 'UploadQueued', + localId: 'l1', + backgroundSync: true, + }); + expect(transport.send).toHaveBeenCalledOnce(); + }); + + it('translates a progress callback into UploadProgress', () => { + const { service, dispatch, handles } = setup(); + service.upload(req, dispatch); + handles[0].onProgress(42); + expect(dispatch).toHaveBeenCalledWith({ + type: 'UploadProgress', + localId: 'l1', + progressPct: 42, + }); + }); + + it('translates a resolved transport into UploadComplete', async () => { + const { service, dispatch, handles } = setup(); + service.upload(req, dispatch); + handles[0].resolveDone({ documentId: 'doc-1' }); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledWith({ + type: 'UploadComplete', + localId: 'l1', + documentId: 'doc-1', + }); + }); + + it('translates a rejected transport into UploadFailed with the reason', async () => { + const { service, dispatch, handles } = setup(); + service.upload(req, dispatch); + handles[0].rejectDone('network down'); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledWith({ + type: 'UploadFailed', + localId: 'l1', + reason: 'network down', + }); + }); + + it('does not dispatch UploadFailed on a user-initiated abort', async () => { + const { service, dispatch, handles } = setup(); + service.upload(req, dispatch); + handles[0].rejectDone(UPLOAD_ABORTED); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'UploadFailed' })); + }); +}); + +describe('UploadShellService.cancel', () => { + it('calls the transport cancel function for an in-flight upload and forgets it', async () => { + const { service, dispatch, handles } = setup(); + service.upload(req, dispatch); + service.cancel(['l1']); + expect(handles[0].cancel).toHaveBeenCalledOnce(); + // Cancelling twice is a no-op the second time — the entry is already forgotten. + service.cancel(['l1']); + expect(handles[0].cancel).toHaveBeenCalledOnce(); + }); + + it('is a no-op for a localId with nothing in flight', () => { + const { service } = setup(); + expect(() => service.cancel(['unknown'])).not.toThrow(); + }); +}); + +describe('UploadShellService.delete', () => { + it('dispatches UploadDeleting, then UploadDeleteComplete on success', async () => { + const { service, dispatch } = setup(); + service.delete('l1', 'doc-1', dispatch); + expect(dispatch).toHaveBeenCalledWith({ type: 'UploadDeleting', localId: 'l1' }); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledWith({ type: 'UploadDeleteComplete', localId: 'l1' }); + }); + + it('dispatches UploadDeleteFailed with the server detail on failure', async () => { + const { service, dispatch } = setup({ + adapter: { deleteDocument: vi.fn().mockRejectedValue({ detail: 'Document is gekoppeld.' }) }, + }); + service.delete('l1', 'doc-1', dispatch); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledWith({ + type: 'UploadDeleteFailed', + localId: 'l1', + reason: 'Document is gekoppeld.', + }); + }); + + it('falls back to an empty reason when the server sends no detail', async () => { + const { service, dispatch } = setup({ + adapter: { deleteDocument: vi.fn().mockRejectedValue(new Error('boom')) }, + }); + service.delete('l1', 'doc-1', dispatch); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledWith({ + type: 'UploadDeleteFailed', + localId: 'l1', + reason: '', + }); + }); +}); + +describe('UploadShellService.pollReturning', () => { + it('does nothing when there are no uploads to poll', async () => { + const status = vi.fn().mockResolvedValue([]); + const { service, dispatch } = setup({ adapter: { status } }); + await service.pollReturning([], dispatch); + expect(status).not.toHaveBeenCalled(); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it('dispatches BackgroundUploadsReturned for uploads the server reports complete', async () => { + const status = vi.fn().mockResolvedValue([ + { localId: 'l1', status: 'complete', documentId: 'doc-1' }, + { localId: 'l2', status: 'unknown' }, + ]); + const { service, dispatch } = setup({ adapter: { status } }); + const uploads = [ + { + localId: 'l1', + categoryId: 'c1', + fileName: 'a.pdf', + fileSizeMb: 1, + status: { type: 'queued' as const }, + backgroundSync: true, + }, + { + localId: 'l2', + categoryId: 'c1', + fileName: 'b.pdf', + fileSizeMb: 1, + status: { type: 'queued' as const }, + backgroundSync: true, + }, + ]; + await service.pollReturning(uploads, dispatch); + expect(status).toHaveBeenCalledWith(['l1', 'l2']); + expect(dispatch).toHaveBeenCalledWith({ + type: 'BackgroundUploadsReturned', + results: [{ localId: 'l1', success: true, documentId: 'doc-1' }], + }); + }); + + it('does not dispatch when nothing has arrived yet', async () => { + const status = vi.fn().mockResolvedValue([{ localId: 'l1', status: 'unknown' }]); + const { service, dispatch } = setup({ adapter: { status } }); + const uploads = [ + { + localId: 'l1', + categoryId: 'c1', + fileName: 'a.pdf', + fileSizeMb: 1, + status: { type: 'queued' as const }, + backgroundSync: true, + }, + ]; + await service.pollReturning(uploads, dispatch); + expect(dispatch).not.toHaveBeenCalled(); + }); +}); diff --git a/libs/shared/src/application/upload-shell.service.ts b/libs/shared/src/application/upload-shell.service.ts index bab1e67..6014cbc 100644 --- a/libs/shared/src/application/upload-shell.service.ts +++ b/libs/shared/src/application/upload-shell.service.ts @@ -1,4 +1,4 @@ -import { Injectable, inject } from '@angular/core'; +import { Injectable, InjectionToken, inject } from '@angular/core'; import { UploadAdapter, XhrUploadRequest, @@ -28,6 +28,18 @@ class KeepaliveTransport implements UploadTransport { } } +/** + * The swap seam (see UploadTransport above), made real: a spec provides a fake + * transport against this token instead of the concrete class. This copies the + * `SessionPort` / `SESSION_PORT` shape (session.port.ts), the repo's one other + * explicit port. The default factory returns the same KeepaliveTransport + * instance the class-injection used to, so runtime behaviour is unchanged. + */ +export const UPLOAD_TRANSPORT = new InjectionToken('UPLOAD_TRANSPORT', { + providedIn: 'root', + factory: () => inject(KeepaliveTransport), +}); + type Dispatch = (m: UploadMsg) => void; /** @@ -37,7 +49,7 @@ type Dispatch = (m: UploadMsg) => void; */ @Injectable({ providedIn: 'root' }) export class UploadShellService { - private transport: UploadTransport = inject(KeepaliveTransport); + private transport: UploadTransport = inject(UPLOAD_TRANSPORT); private adapter = inject(UploadAdapter); private inflight = new Map void>(); // localId → cancel