feat(infakt): consume invoice_marked_as_paid webhook + payment-status sync (#1354)

[norbert-kulus-blockydevs]

What changed

The inFakt invoice_marked_as_paid (+ _via_async_api) webhook was recognized but dropped, so OL never learned when an invoice became paid. This routes it onto the invoicing domain and refreshes payment status via an authoritative provider re-read (never trusting the webhook body), mirroring the KSeF-status reconciliation pattern.

• CORE (invoicing): neutral PaymentStatus (unknown|unpaid|partially-paid|paid) + PaymentStatusResult; paymentStatus on InvoiceRecord (+ isPaid); PaymentStatusReader sub-capability + guard; PaymentStatusRefreshService (by-id, authoritative re-read, write-on-change, graceful no-op); ORM column + repo mapping + findByProviderInvoiceId; migration 1818000000004-add-invoice-payment-status.ts.
• CORE (sync): new CanonicalInboundEvent domain invoice-payment; job type invoicing.paymentStatus.refreshByExternalId; InboundRoutingPolicy routes it to the refresh job, gated on Invoicing.
• Worker: PaymentStatusRefreshHandler.
• Integration (inFakt): adapter implements PaymentStatusReader (maps inFakt status/paid_date via authoritative GET); translator maps both payment events to invoice-payment; decoder derives externalId from the invoice uuid.

How to test

• Unit: pnpm --filter @openlinker/core test (1450 pass), pnpm --filter @openlinker/integrations-infakt test (153 pass) - translator/routing/refresh-service/adapter/decoder specs.
• Migration ordering check passes. Run pnpm --filter @openlinker/api migration:run on a test DB to apply the paymentStatus column.

Scope

• Inbound half (consume the webhook + refresh status) is implemented - the must-have.
• Outbound half (InvoicePaymentMarker -> POST /async/invoices/{uuid}/paid.json + endpoint/UI) is deferred; follow-up filed and linked below.
• Part of the inFakt accounting epic feat(infakt): Infakt accounting integration [EPIC] #1279.

Closes #1354

🤖 Generated with Claude Code

[piotrswierzy]

/pr-review — systematic review

Textbook-clean feature — the only thing standing between it and merge is a cross-PR migration-timestamp collision.

🔴 BLOCKING — 1818000000004 collides with PR #1341

Both PRs claim the same 13-digit prefix on an invoice_records migration:

• this PR: 1818000000004-add-invoice-payment-status.ts
• #1341 (open, closes #1338): 1818000000004-backfill-ksef-provider-invoice-number.ts

That's the #374 collision class (docs/migrations.md § Timestamp uniqueness invariant). The class names differ so there's no duplicate-migrations-row hazard, but the shared prefix violates the uniqueness + strictly-greater invariants. check-migration-timestamps.mjs compares each branch only against origin/main, so both pass lint in isolation and the second-to-merge fails pnpm lint. Latest prefix on main is 1818000000003, so …004 is otherwise the correct next step. Fix: whichever lands second re-prefixes to 1818000000005 + updates its class suffix — recommend this PR pre-empt to 1818000000005, since #1341 is a smaller self-contained bugfix. (I've flagged the same on #1341.)

✅ Everything else verified clean

• Migration safety: up()/down() present; ADD COLUMN IF NOT EXISTS "paymentStatus" text NOT NULL DEFAULT 'unknown' — idempotent, metadata-only in PG11+, existing rows backfill to unknown (never falsely unpaid); ORM @Column default matches the DDL (no migration:show drift).
• ADR-026: PaymentStatusValues (unknown|unpaid|partially-paid|paid), PaymentStatusResult, PaymentStatusReader, and the sync invoice-payment domain carry zero inFakt/PL vocabulary — all status/paid_date mapping confined to toPaymentStatus() in the adapter.
• Authoritative re-read: refreshByExternalId re-reads via adapter.getPaymentStatus(record) and never touches the webhook body (payload carries only externalInvoiceId); write-on-change only; graceful no-ops (unsupported warn when !isPaymentStatusReader, not-found when no record) — only a transport failure propagates for retry. Mirrors the webhook=trigger / poll=source-of-truth principle and the RegulatoryStatusReader precedent.
• Sub-capability + routing: payment-status-reader.capability.ts + co-located isPaymentStatusReader guard, barrel-exported; InboundRoutingPolicy gates invoice-payment on capability Invoicing (not platformType), switch stays exhaustive; runtime-detected (correctly not in supportedCapabilities), so no manifest/routing-int-spec ripple.
• Layers/types: no any; service implements IPaymentStatusRefreshService in a separate interface file; Symbol token in invoicing.tokens.ts (export *'d); worker handler registered in both the module and handler-registration.service.ts; cross-context imports barrel-only; findByProviderInvoiceId backed by the pre-existing partial index, newest-first.
• Tests: colocated and branch-complete (refresh-service updated/unchanged/not-found/unsupported; adapter mapping + 503 propagation; decoder; translator both events + dead-letter).

✅ Positives

Clean orthogonal modelling — paymentStatus is separate from issuance status and regulatoryStatus, and invoice-payment is deliberately split from the invoicing domain so a paid document nudges the by-id refresh rather than the regulatory sweep. Fiscal-safe unknown default applied consistently across type doc, entity, ORM, and migration.

Verdict: 🔄 Approve with changes — re-prefix the migration to 1818000000005 to clear the #1341 collision; the code is otherwise merge-ready.

[norbert-kulus-blockydevs]

Thanks for the review, @piotrswierzy. All findings addressed - in this case the single 🔴 BLOCKING item (there were no inline comments or open suggestions to fold in):

🔴 Migration-timestamp collision with #1341 - fixed (bc7bb058)

Re-prefixed the migration 1818000000004 -> 1818000000005 as recommended, so this PR pre-empts and #1341 keeps the …004 slot:

• File: 1818000000004-add-invoice-payment-status.ts -> 1818000000005-add-invoice-payment-status.ts
• Class: AddInvoicePaymentStatus1818000000004 -> AddInvoicePaymentStatus1818000000005
• name property updated to match.

…005 is the correct next step over main's latest …003 regardless of which of the two PRs merges first, so the timestamp uniqueness + strictly-greater invariant now holds either way.

Verification:

• node scripts/check-migration-timestamps.mjs -> OK (ordering vs origin/main: checked)
• pnpm --filter @openlinker/api type-check -> clean

No functional change - up()/down() DDL, the ORM column default, and every other reviewed aspect are untouched.

[norbert-kulus-blockydevs]

/tech-review - independent pass

Independent tech-lead review of PR #1361, formed from scratch against the diff and the OpenLinker docs. Not an echo of any prior review.

Summary

Solid, well-scoped work. The inbound-half implementation is faithful to the documented patterns: a distinct invoice-payment canonical domain, capability-gated routing to a by-id refresh job, and an authoritative provider re-read (PaymentStatusReader) that never trusts the webhook body - a clean mirror of the RegulatoryStatusReader reconcile pattern (#1121). CORE/Integration boundaries hold (no libs/integrations import in core; no paid_date/faktura vocabulary leaks past the adapter, honouring ADR-026), the service codes against ports only, and the Symbol-token + separate-interface conventions are respected. The migration timestamp collision that was flagged earlier is correctly fixed (1818000000005, strictly greater than main's tail ...003, class suffix matches, node scripts/check-migration-timestamps.mjs is green). I found no BLOCKING issues and no NEW correctness defects; the remainder are minor.

Issues

[SUGGESTION] - apps/worker/src/sync/handlers/payment-status-refresh.handler.ts (line 145)

getPayload carries non-trivial branching validation (schemaVersion, non-empty externalInvoiceId, SyncJobExecutionError on both invalid-payload and downstream failure) but has no unit spec. Handler specs are not the norm here (only 2 of 23 handlers have one), so this is not blocking - but the direct sibling this PR mirrors, regulatory-status-reconcile.handler.spec.ts, does have one, and the core service/routing/adapter paths are all otherwise well covered. A short spec pinning the reject path and the success delegation would match that sibling.

[SUGGESTION] - libs/core/src/invoicing/application/services/payment-status-refresh.service.ts (line 502)

Payment status is persisted through repo.updateOutcome(record.id, { paymentStatus }). Reusing the generic InvoiceOutcomePatch update works and avoids method proliferation, but updateOutcome is a slightly misleading name for a payment-lifecycle write that is orthogonal to the issuance outcome. Not worth churn on its own; if the patch surface grows, consider renaming to a neutral applyPatch/update. engineering-standards.md naming guidance, informational only.

[SUGGESTION] - libs/core/src/invoicing/application/services/payment-status-refresh.service.interface.ts (line 234)

PaymentStatusRefreshOutcomeValues (a runtime as const array) and PaymentStatusRefreshResult live in the .service.interface.ts file rather than a dedicated *.types.ts, which is the literal engineering-standards.md rule ("all types must be defined in separate files"). I am flagging it only as a SUGGESTION because there is an established in-context precedent - RegulatoryStatusReconcileResult is defined the same way in its service-interface file - so this is consistent with the surrounding code. Worth a note, not a change.

[SUGGESTION] - libs/integrations/infakt/src/infrastructure/adapters/infakt-invoicing.adapter.ts (line 165)

toPaymentStatus classifies "partial" via status.includes('partial') || status.includes('partly') and treats every other non-paid status as unpaid. This is a reasonable, tested heuristic and correctly contained in the adapter (ADR-026), but substring matching is fragile if inFakt introduces a new status string. Acceptable for now; a comment enumerating the known inFakt status vocabulary (or matching against a known set) would make future drift obvious. The paid_date-present fallback to paid is a sensible defensive belt.

Notes verified (no action)

• Migration correctness: up/down are symmetric and idempotent (ADD COLUMN IF NOT EXISTS / DROP COLUMN IF EXISTS); NOT NULL DEFAULT 'unknown' backfills existing rows to a state that never falsely asserts "unpaid", matching the documented intent. No ORM drift - the ORM column (text, non-null, default 'unknown') matches the DDL, and no new index is required because IDX_invoice_records_provider_invoice_id (partial, providerInvoiceId IS NOT NULL) already exists on main and backs findByProviderInvoiceId. The repo comment referencing that index is accurate.
• Backward compatibility: the new paymentStatus constructor parameter on InvoiceRecord is last and defaulted (= 'unknown'), so existing constructor call sites are unaffected.
• Authoritative re-read / retry-safety: the service resolves the adapter, narrows with isPaymentStatusReader (clean unsupported no-op before any DB hit), looks up the record (benign not-found no-op), then re-reads provider state and writes only on change. A transport failure propagates for job retry - correct.
• Exhaustiveness: adding invoice-payment to the closed domain union keeps the default: never guard in the routing policy intact; the translator refactor (domain === null gate) preserves the existing send_to_ksef_* -> invoicing behaviour.
• Security: no secrets, capability-gated routing, webhook body treated as trigger only.

Verdict

✅ Approve - ready to merge. All open items are SUGGESTIONs; none block. If any is worth acting on, the handler unit spec is the highest-value follow-up.

[norbert-kulus-blockydevs]

Tech-review follow-up (all 4 SUGGESTIONs addressed)

Thanks for the pass. Addressed in 4f309826.

Fixed

#1 - Missing handler unit spec (apps/worker/src/sync/handlers/payment-status-refresh.handler.ts)
Added payment-status-refresh.handler.spec.ts, mirroring the sibling regulatory-status-reconcile.handler.spec.ts. It pins:

• every getPayload reject branch (undefined / null / non-object payload, wrong schemaVersion, missing / non-string / empty externalInvoiceId) throwing a SyncJobExecutionError and never calling the service;
• the success path delegating to refreshByExternalId(connectionId, externalInvoiceId) and returning { outcome: 'ok' };
• a thrown downstream error re-wrapped in an OL-shaped SyncJobExecutionError carrying jobId / jobType / connectionId.

9 tests, all green.

#4 - toPaymentStatus substring heuristic (libs/integrations/infakt/src/infrastructure/adapters/infakt-invoicing.adapter.ts)
Reworked the classifier to match against documented known-token sets (INFAKT_PAID_TOKENS / INFAKT_PARTIAL_TOKENS) instead of a bare === 'paid' plus ad-hoc substring. Added an enumerating comment listing both inFakt vocabularies, verified live against the meta dictionaries:

• invoice status (invoice_statuses): draft | sent | printed | paid
• payment status (payment_statuses): paid | unpaid | partial_payment | payment_not_applicable

Behaviour is unchanged (paid -> paid; partial/partly token -> partially-paid; paid_date belt -> paid; else unpaid). Extended the adapter spec table with partial_payment (the payment_statuses-dictionary token), draft, and printed (no paid_date) cases so future drift is caught. 51 tests, all green. Vocabulary stays entirely in the adapter (ADR-026 clean).

Deliberately kept (with reasons)

#2 - updateOutcome naming (payment-status-refresh.service.ts)
Kept. updateOutcome is the shared InvoiceRecordRepositoryPort write used by 10+ call sites across three services (InvoiceService, RegulatoryStatusReconciliationService, this one), the port, the repo impl, the SourceDocumentImmutableError, and the type docs. Renaming to a neutral applyPatch would be exactly the kind of sweeping ripple across unrelated files the review flagged as "not worth churn on its own". If the patch surface grows we can revisit then.

#3 - result types in .service.interface.ts (payment-status-refresh.service.interface.ts)
Kept. Moving PaymentStatusRefreshOutcomeValues + PaymentStatusRefreshResult to a dedicated *.types.ts would diverge from the direct in-context precedent - RegulatoryStatusReconcileResult lives in its own service-interface file the same way. Splitting only this one would make the invoicing context less internally consistent for a marginal literal-rule gain. Consistency with the established sibling pattern wins here.

Checks (scoped to touched packages)

• pnpm --filter @openlinker/integrations-infakt type-check - clean
• pnpm --filter @openlinker/worker type-check - clean
• pnpm --filter @openlinker/integrations-infakt test -- infakt-invoicing.adapter.spec - 51 passed
• pnpm --filter @openlinker/worker test -- payment-status-refresh.handler.spec - 9 passed

No migrations touched.

[piotrswierzy]

/pr-review — delta re-review (9b1cac40 → 4f309826)

The non-migration delta is straight approve-quality. The sole remaining blocker is the migration collision — which moved but wasn't resolved.

🔴 CROSS-PR BLOCKING — the …004 collision became a …005 collision (still unresolved)

The migration was re-prefixed 1818000000004 → 1818000000005 (class + name both → AddInvoicePaymentStatus1818000000005) — but #1341 also independently re-prefixed to 1818000000005. So both now share …005 (and …004 is an unused gap). origin/main contains neither, so it's invisible in each isolated diff, but the second-to-merge still fails pnpm lint. Fix (needs coordination with #1341): exactly one PR owns …005; the other takes a distinct slot — …004 is now free and is the correct next step after main's …003 tail. Flagged the same on #1341.

✅ Non-migration delta — approve-quality

• Adapter (+27): a behavior-preserving refactor, not a correctness change — status === 'paid' → INFAKT_PAID_TOKENS.includes(status) (tokens ['paid'], identical), the partial branch → INFAKT_PARTIAL_TOKENS.some(...) (logically identical to the prior includes('partial')||includes('partly')), paid_date fallback + unpaid default unchanged. Now driven by named constants + a doc comment citing the verified live inFakt dictionaries. Neutrality holds — inFakt tokens stay in the adapter; only the neutral PaymentStatus crosses to core; getPaymentStatus still returns unknown when providerInvoiceId is absent and never trusts a webhook body.
• New payment-status-refresh.handler.spec.ts (+91): real handler test, colocated, AAA — 7 it.each payload-validation branches all assert SyncJobExecutionError and the service is never called; success asserts delegation to refreshByExternalId('conn-1','inv-1') + {outcome:'ok'}; error asserts OL-shaped wrapping with jobId/jobType/connectionId. Correctly leaves the unsupported/not-found branching to the refresh service's own spec (the handler is a thin delegate). Closes the worker-handler test gap.
• Adapter spec (+5): exercises the new token constants (partial_payment → partially-paid, draft/printed → unpaid).

🟢 Minor (non-blocking, pre-existing)

The doc comment attributes partial_payment to inFakt's payment_statuses dictionary, but toPaymentStatus reads invoice.status (dictionary draft|sent|printed|paid, no partial token), and InfaktInvoice carries only a single status field — so the partial branch may be unreachable against real invoice.status data. It's fiscally conservative (partial safer than a false "paid") and unchanged from the prior head — worth a note, not a gate.

✅ No regressions

Authoritative re-read still never trusts the webhook body; write-on-change owned by the (untouched) service; sub-capability PaymentStatusReader + isPaymentStatusReader guard barrel-exported; fiscal-safe unknown default preserved; no any. Migration body still idempotent (ADD COLUMN IF NOT EXISTS … DEFAULT 'unknown') with a symmetric down().

Verdict: 🔄 Approve with changes — resolve the 1818000000005 collision with #1341 (one PR takes a distinct slot, e.g. …004); everything else is merge-ready.

[norbert-kulus-blockydevs]

Migration timestamp collision with #1341 — resolved on the #1341 side

Heads-up on the 1818000000005 migration collision @piotrswierzy flagged (both this PR and #1341 added a 1818000000005-* migration on invoice_records).

I've re-prefixed #1341's backfill down to the free 1818000000004 slot. That leaves 1818000000005-add-invoice-payment-status.ts uniquely to this PR — so #1361 needs no change, just keep …005 as-is. Once both land, ordering is …003 (main) → …004 (#1341 backfill) → …005 (#1361 payment status), all distinct. check-migration-timestamps.mjs passes.

[norbert-kulus-blockydevs]

🟢 Minor doc finding addressed (4f309826 → 43406b8e)

Also actioned the last-pass 🟢 Minor: the token doc attributed partial_payment to the field toPaymentStatus reads, but that function reads InfaktInvoice.status (the invoice_statuses dictionary — draft|sent|printed|paid, no partial token), so the partial/partly branch is unreachable against real status data.

Fix is comment-only (behavior unchanged, still fiscally conservative): the doc now states plainly that the partial match is an intentional forward-looking guard against future Infakt vocabulary drift, not a currently-reachable classification — kept so a part-settled document would classify as partially-paid rather than silently unpaid if Infakt ever surfaces a settlement token on status.

All findings on this PR (blocker + minor) are now addressed.

[piotrswierzy]

/pr-review — delta re-review (4f309826 → 5aac870c)

The migration collision is resolved — the sole remaining blocker. Approving.

Verified locally: this PR keeps 1818000000005-add-invoice-payment-status.ts and #1341 moved to 1818000000004 — two distinct, sequential prefixes above main's 1818000000003 tail. No more collision.

The only author change since my last review (cutting through the merge-of-main noise from #1284/#1342/#1335 landing on main) is one commit — docs(infakt): clarify partial-payment branch is a forward-looking guard — which addresses my minor note that the partial mapping branch may be unreachable against real invoice.status data by documenting it as an intentional forward-looking guard. Good.

Everything else was already verified clean at the prior head: authoritative re-read (never trusts the webhook body), write-on-change only, PaymentStatusReader sub-capability + barrel-exported guard, routing gated on Invoicing capability, fiscal-safe unknown default, the 91-line handler spec, idempotent ADD COLUMN IF NOT EXISTS … DEFAULT 'unknown' migration with a symmetric down(), no any.

Verdict: ✅ Approve.

[piotrswierzy]

@norbert-kulus-blockydevs resolve conflicts