Feature 323 - Third party subscription fulfilment - current - #477
Conversation
Created technical specification and task list for implementing third-party subscription fulfilment with HTTPS push notifications. Plan includes: - New libs/third-party-fulfilment module - Database schema for subscriptions and push audit logs - Certificate-based authentication with Azure Key Vault - Retry logic with exponential backoff - Custom headers for P&I push API - Integration with existing upload flows Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…arty-subscription-fulfilment
…23-third-party-subscription-fulfilment
…23-third-party-subscription-fulfilment
…23-third-party-subscription-fulfilment
…23-third-party-subscription-fulfilment
…/323-third-party-subscription-fulfilment
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new third‑party fulfilment subsystem is added via Changes
Sequence Diagram(s)sequenceDiagram
participant User as Publisher/System
participant PubSvc as Publication Service
participant ThirdSvc as ThirdParty Fulfilment
participant SubQuery as Subscriber Query
participant LocSvc as Location Service
participant Header as Header Builder
participant HTTP as HTTP Client (retry)
participant DB as Push Log (Prisma)
participant External as Third‑party API
User->>PubSvc: processPublication(params...)
PubSvc->>PubSvc: generate PDF / prepare payload
PubSvc->>ThirdSvc: sendThirdPartyPublications(params) (async)
ThirdSvc->>ThirdSvc: resolve COURTEL_API_URL & certificate
ThirdSvc->>SubQuery: findSubscribersByListType(listTypeId, sensitivity)
SubQuery-->>ThirdSvc: [subscribers]
ThirdSvc->>LocSvc: getLocationWithDetails(locationId)
LocSvc-->>ThirdSvc: location details
ThirdSvc->>Header: buildPushHeaders(metadata)
Header-->>ThirdSvc: headers
ThirdSvc->>HTTP: pushWithRetry(url, cert, headers, body, pdfPath/flatFile)
HTTP->>External: HTTPS POST (with retries/backoff)
External-->>HTTP: statusCode
HTTP-->>ThirdSvc: {statusCode, success}
ThirdSvc->>DB: prisma.thirdPartyPushLog.create({...})
DB-->>ThirdSvc: ack
sequenceDiagram
participant User as Admin UI
participant Remove as RemoveListHandler
participant Query as Artefact Query
participant Delete as Delete DB
participant ThirdSvc as ThirdParty Fulfilment
participant HTTP as HTTP Client (retry)
participant External as Third‑party API
participant DB as Push Log
User->>Remove: confirm deletion([ids])
Remove->>Query: getArtefactsByIds(ids)
Query-->>Remove: artefact metadata
Remove->>Delete: deleteArtefacts(ids)
Delete-->>Remove: success
loop each artefact
Remove->>ThirdSvc: sendThirdPartyDeletion(artefact metadata) (async)
ThirdSvc->>ThirdSvc: resolve secrets & find subscribers
ThirdSvc->>HTTP: pushWithRetry(url, cert, headers, null)
HTTP->>External: HTTPS POST (null body)
External-->>HTTP: statusCode
HTTP-->>ThirdSvc: {statusCode, success}
ThirdSvc->>DB: create push log (DELETION)
end
Remove-->>User: deletion confirmed
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…arty-subscription-fulfilment # Conflicts: # apps/web/helm/values.dev.yaml # libs/admin-pages/src/pages/manual-upload-summary/index.test.ts # libs/admin-pages/src/pages/remove-list-confirmation/index.ts
🎭 Playwright E2E Test Results84 tests 51 ✅ 5m 39s ⏱️ Results for commit 81c1c8c. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
130-141:⚠️ Potential issue | 🟠 MajorVariable shadowing causes
jsonDatato always beundefinedinprocessPublication.Line 130 declares
let jsonData: unknown;but line 133 redeclaresconst jsonDatainside the try block, shadowing the outer variable. The outerjsonData(passed toprocessPublicationat line 150) will never be assigned.Proposed fix
let jsonData: unknown; if (!isFlatFile) { try { - const jsonData = JSON.parse(uploadData.file.toString("utf-8")); + jsonData = JSON.parse(uploadData.file.toString("utf-8")); await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData); } catch (error) {
🧹 Nitpick comments (10)
libs/api/src/blob-ingestion/repository/service.test.ts (1)
64-64: Add one update-path test forisUpdate: true.The revised tests only exercise
isUpdate: false. Please add a companion case assertingprocessPublicationreceivesisUpdate: truewhencreateArtefactreports an update.As per coding guidelines: "Aim for >80% test coverage on business logic."
Also applies to: 91-91, 159-159, 178-178, 199-199, 299-315, 327-327, 347-347, 376-376
libs/admin-pages/src/pages/non-strategic-upload-summary/index.test.ts (1)
76-77: Please cover theisUpdate: truebranch in POST tests.Current mocks pin
isUpdatetofalse; add a case that verifies the handler forwardsisUpdate: trueintoprocessPublication.As per coding guidelines: "Aim for >80% test coverage on business logic."
Also applies to: 405-405
docs/tickets/323/review.md (1)
52-52: Minor documentation inconsistency.The line reference
index.ts:118-137appears outdated. In the current code, the deletion integration spans lines 134-148. Consider updating for accuracy.📝 Suggested fix
-- **Deletion integration** correctly wired into `libs/admin-pages/src/pages/remove-list-confirmation/index.ts:118-137` — fetches artefact metadata before deletion, then calls `sendThirdPartyDeletion` fire-and-forget for each deleted artefact. +- **Deletion integration** correctly wired into `libs/admin-pages/src/pages/remove-list-confirmation/index.ts:134-148` — fetches artefact metadata before deletion, then calls `sendThirdPartyDeletion` fire-and-forget for each deleted artefact.libs/third-party-fulfilment/src/push/retry.ts (1)
5-5: Consider usingSetfor status code lookup.Per static analysis,
SUCCESS_STATUSES.has()is more idiomatic than array.includes()for membership checks.♻️ Suggested refactor
-const SUCCESS_STATUSES = [200, 201, 202, 204]; +const SUCCESS_STATUSES = new Set([200, 201, 202, 204]);And at line 43:
- if (SUCCESS_STATUSES.includes(lastResult.statusCode)) { + if (SUCCESS_STATUSES.has(lastResult.statusCode)) {Also applies to: 43-43
libs/third-party-fulfilment/src/queries.ts (1)
12-16: Consider removing unuseduserrelation include.Based on
libs/third-party-fulfilment/src/service.ts(lines 67 and 89), the subscriber query result is only checked for.length— individual subscriber or user data is never accessed. The push goes to a single centralised API endpoint.Removing
include: { user: true }would reduce query overhead.♻️ Suggested change
export async function findSubscribersByListType(listTypeId: number, sensitivity: string) { return prisma.legacyThirdPartySubscription.findMany({ - where: { listTypeId, sensitivity: { in: eligibleSensitivities(sensitivity) } }, - include: { user: true } + where: { listTypeId, sensitivity: { in: eligibleSensitivities(sensitivity) } } }); }docs/tickets/323/plan.md (1)
63-81: Minor: Add language specifier to fenced code block.The code block starting at line 63 lacks a language identifier, which affects syntax highlighting.
📝 Suggested fix
-``` +```text libs/third-party-fulfilment/ ├── package.jsonlibs/third-party-fulfilment/src/push/http-client.ts (1)
29-29: Consider destroying the HTTPS agent after use.The
https.Agentis created per request but never explicitly destroyed. While Node.js will clean up eventually, explicitly callingagent.destroy()after the request completes would release resources sooner, especially under high load.♻️ Proposed fix
return new Promise((resolve, reject) => { const req = https.request(options, (res) => { // Drain the response body to free the socket res.resume(); res.on("end", () => { const statusCode = res.statusCode ?? 0; + agent.destroy(); resolve({ statusCode, success: isSuccessStatus(statusCode) }); }); }); req.on("timeout", () => { req.destroy(new Error(`Request timed out after ${REQUEST_TIMEOUT_MS}ms`)); }); - req.on("error", reject); + req.on("error", (err) => { + agent.destroy(); + reject(err); + });libs/third-party-fulfilment/src/service.test.ts (1)
46-53: Consider cleaning up environment variables after tests.The
beforeEachsetsprocess.env.COURTEL_API_URLandprocess.env.COURTEL_CERTIFICATE, but these aren't cleaned up inafterEach. This could leak state to other test files.♻️ Proposed fix
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +const originalEnv = { ...process.env }; + beforeEach(() => { vi.resetAllMocks(); vi.mocked(pushWithRetry).mockResolvedValue({ statusCode: 200, success: true }); vi.mocked(findSubscribersByListType).mockResolvedValue([{ id: "sub-1", user: { id: "user-1", name: "courtel" } }] as never); mockPushLogCreate.mockResolvedValue({}); process.env.COURTEL_API_URL = "https://courtel.example.com/api"; process.env.COURTEL_CERTIFICATE = Buffer.from("-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----").toString("base64"); }); + +afterEach(() => { + process.env = { ...originalEnv }; +});docs/tickets/323/tasks.md (2)
1-1: Use consistent compound hyphenation in the title.Line 1 would read more cleanly as “Third-Party Subscription Fulfilment” for consistent compound-adjective style.
55-56: Add verifiable test evidence alongside pass/coverage claims.Line 55 and Line 56 state outcomes, but there is no pointer to artefacts (for example, command output path, coverage report location, or CI job link). Adding one makes the checklist auditable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d5b62bf-c095-4f0b-b987-f802112fb41b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
apps/api/helm/values.dev.yamlapps/api/helm/values.yamlapps/postgres/prisma/migrations/20260409150532_add_legacy_third_party_push_log/migration.sqlapps/postgres/src/schema-discovery.test.tsapps/postgres/src/schema-discovery.tsapps/web/helm/values.dev.yamlapps/web/helm/values.yamldocs/tickets/323/plan.mddocs/tickets/323/review.mddocs/tickets/323/tasks.mddocs/tickets/323/ticket.mdlibs/admin-pages/package.jsonlibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/admin-pages/src/pages/manual-upload-summary/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.test.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.tslibs/admin-pages/src/pages/remove-list-confirmation/index.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/api/src/blob-ingestion/repository/service.tslibs/publication/package.jsonlibs/publication/src/processing/service.test.tslibs/publication/src/processing/service.tslibs/publication/src/repository/model.tslibs/publication/src/repository/queries.test.tslibs/publication/src/repository/queries.tslibs/third-party-fulfilment/package.jsonlibs/third-party-fulfilment/prisma/schema.prismalibs/third-party-fulfilment/src/config.tslibs/third-party-fulfilment/src/index.tslibs/third-party-fulfilment/src/push/headers.test.tslibs/third-party-fulfilment/src/push/headers.tslibs/third-party-fulfilment/src/push/http-client.test.tslibs/third-party-fulfilment/src/push/http-client.tslibs/third-party-fulfilment/src/push/retry.test.tslibs/third-party-fulfilment/src/push/retry.tslibs/third-party-fulfilment/src/queries.test.tslibs/third-party-fulfilment/src/queries.tslibs/third-party-fulfilment/src/service.test.tslibs/third-party-fulfilment/src/service.tslibs/third-party-fulfilment/tsconfig.jsontsconfig.json
| **Pre-Condition** | ||
| * CaTH Third Party users can subscribe to receive specific hearing lists published in CaTH | ||
|
|
||
| **TECHNIAL SPECIFICATION** |
There was a problem hiding this comment.
Typo: "TECHNIAL" should be "TECHNICAL".
📝 Suggested fix
- **TECHNIAL SPECIFICATION**
+ **TECHNICAL SPECIFICATION**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **TECHNIAL SPECIFICATION** | |
| **TECHNICAL SPECIFICATION** |
| * Successful <`**POST**`>(https://hmcts.github.io/restful~~api~~standards/#post) requests will generate: | ||
| ** 200 (if resources have been updated) | ||
| ** 201 (if resources have been created) | ||
| ** 202 (if the request was accepted but has not been finished yet) | ||
| *** 204 with <`**Location*`>(https://tools.ietf.org/html/rfc7231#section-7.1.2) header (if the actual resource is not returned) | ||
| * Validation is established to ensure no publication is sent when the trigger has not been activated, to ensure that Non-subscribed JSON payload is not sent to the third party and to ensure that the system differentiates between newly uploaded and updated publication |
There was a problem hiding this comment.
Markdown formatting issues in acceptance criteria.
The bullet list formatting appears malformed with mixed link syntax. Consider reviewing the markdown structure for proper rendering.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 47-47: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
| "declarationMap": true | ||
| }, | ||
| "include": ["src/**/*"], | ||
| "exclude": ["**/*.test.ts", "**/*.spec.ts", "dist", "node_modules"] |
There was a problem hiding this comment.
Add assets to exclude to match the library tsconfig baseline.
assets is currently not excluded, so static assets can be pulled into compilation if present.
Suggested patch
- "exclude": ["**/*.test.ts", "**/*.spec.ts", "dist", "node_modules"]
+ "exclude": ["**/*.test.ts", "**/*.spec.ts", "dist", "node_modules", "assets"]Based on learnings: "Applies to libs/*/tsconfig.json : Library modules must have tsconfig.json extending root tsconfig with outDir, rootDir, declaration, and declarationMap compiler options. Exclude test files, dist, node_modules, and assets from compilation."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "exclude": ["**/*.test.ts", "**/*.spec.ts", "dist", "node_modules"] | |
| "exclude": ["**/*.test.ts", "**/*.spec.ts", "dist", "node_modules", "assets"] |
| - name: sso-sg-admin-local | ||
| alias: SSO_INTERNAL_ADMIN_LOCAL_GROUP_ID | ||
| # Third-party push secrets | ||
| - name: auto-pip-stg-courtel-api |
There was a problem hiding this comment.
COURTEL_API_URL and COURTEL_CERTIFICATE are in both web and api helm. Do they need to be in both helm charts?
| @@ -0,0 +1,22 @@ | |||
| generator client { | |||
There was a problem hiding this comment.
I wonder we should name the lib legacy_third_party_fulfilment. The tables all prefixed with the word legacy_. Also when we add the new third-party stuff later, we can avoid confusion.
| return { | ||
| "x-provenance": provenance, | ||
| "x-source-artefact-id": artefactId, | ||
| "x-type": listTypeName, |
There was a problem hiding this comment.
x-type can be LIST, GENERAL_PUBLICATIONS, etc. This normally comes from the Artefact table. I can see this field is currently missing in the AI Artefact table. For now, may be we can just hard code the value to be "LIST" because that is the only thing Courtel received
| let bodyBuffer: Buffer; | ||
| const requestHeaders: Record<string, string | number> = { ...headers }; | ||
|
|
||
| if (pdfPath) { |
There was a problem hiding this comment.
The current behaviour is:
- If it is a JSON publication, we will check if the PDF is generated or not. If yes, we will send them both the JSON publication, and the generated PDF. if no, we will just send them the JSON publication.
- If it is a flat file publication, we will just send them the flat file just the content type of multipart form data.
Not sure if that is what it is doing currently as I can't really see those logic.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
libs/legacy-third-party-fulfilment/src/push/retry.ts (1)
36-41: Swallowed error loses diagnostics.The bare
catch {}discards the underlying network error (e.g.ECONNREFUSED, TLS failure), making field diagnosis reliant onstatusCode 0alone. A one-liner log preserves the cause without changing retry behaviour.♻️ Suggested tweak
- try { - lastResult = await executePush(url, certPem, headers, body, pdfPath); - } catch { - // Network-level error — treat as statusCode 0 and retry - lastResult = { statusCode: 0, success: false }; - } + try { + lastResult = await executePush(url, certPem, headers, body, pdfPath); + } catch (error) { + console.warn(`${logPrefix} Push attempt ${attempt + 1}/${MAX_ATTEMPTS} network error:`, error); + lastResult = { statusCode: 0, success: false }; + }libs/legacy-third-party-fulfilment/src/service.ts (1)
32-40:console.errorfor missing env vars overstates the severity.A missing
COURTEL_API_URL/COURTEL_CERTIFICATEis a "feature disabled / skipped" condition, not a failure — logging aterrorlevel will trigger ops alerting in environments where the integration isn't configured.console.warn(orinfo) reads more accurately given the message already says "skipped".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0818c3af-4589-4d45-9922-0abe47c14cc8
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
apps/postgres/src/schema-discovery.test.tsapps/postgres/src/schema-discovery.tslibs/admin-pages/package.jsonlibs/admin-pages/src/pages/remove-list-confirmation/index.tslibs/legacy-third-party-fulfilment/package.jsonlibs/legacy-third-party-fulfilment/prisma/schema.prismalibs/legacy-third-party-fulfilment/src/config.tslibs/legacy-third-party-fulfilment/src/index.tslibs/legacy-third-party-fulfilment/src/push/headers.test.tslibs/legacy-third-party-fulfilment/src/push/headers.tslibs/legacy-third-party-fulfilment/src/push/http-client.test.tslibs/legacy-third-party-fulfilment/src/push/http-client.tslibs/legacy-third-party-fulfilment/src/push/retry.test.tslibs/legacy-third-party-fulfilment/src/push/retry.tslibs/legacy-third-party-fulfilment/src/queries.test.tslibs/legacy-third-party-fulfilment/src/queries.tslibs/legacy-third-party-fulfilment/src/service.test.tslibs/legacy-third-party-fulfilment/src/service.tslibs/legacy-third-party-fulfilment/tsconfig.jsonlibs/list-types/civil-and-family-daily-cause-list/src/pages/index.test.tslibs/location/src/seed-list-types.tslibs/publication/package.jsonlibs/publication/src/processing/service.test.tslibs/publication/src/processing/service.tslibs/publication/src/repository/queries.tstsconfig.json
✅ Files skipped from review due to trivial changes (8)
- libs/location/src/seed-list-types.ts
- libs/publication/package.json
- libs/legacy-third-party-fulfilment/tsconfig.json
- libs/legacy-third-party-fulfilment/src/config.ts
- libs/admin-pages/package.json
- libs/legacy-third-party-fulfilment/src/index.ts
- tsconfig.json
- libs/legacy-third-party-fulfilment/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/postgres/src/schema-discovery.ts
- apps/postgres/src/schema-discovery.test.ts
- libs/publication/src/processing/service.test.ts
- libs/publication/src/processing/service.ts
- libs/publication/src/repository/queries.ts
| for (const artefact of artefactsToDelete) { | ||
| sendThirdPartyDeletion({ | ||
| artefactId: artefact.artefactId, | ||
| locationId: artefact.locationId, | ||
| listTypeId: artefact.listTypeId, | ||
| contentDate: artefact.contentDate, | ||
| sensitivity: artefact.sensitivity, | ||
| language: artefact.language, | ||
| displayFrom: artefact.displayFrom, | ||
| displayTo: artefact.displayTo, | ||
| provenance: artefact.provenance | ||
| }).catch((error) => { | ||
| console.error("Third-party deletion push failed:", error); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fire-and-forget pushes risk being dropped after the response.
sendThirdPartyDeletion(...) is invoked without await, so the handler redirects (and the request/process may be torn down) before the HTTPS push, its retries, and the push-log write complete. In a containerised/serverless deployment this can silently lose deletion notifications and log rows. Consider awaiting the pushes (e.g. await Promise.allSettled(...)) before the redirect, or dispatching them to a durable queue/worker.
♻️ Suggested shape
- for (const artefact of artefactsToDelete) {
- sendThirdPartyDeletion({
- artefactId: artefact.artefactId,
- ...
- }).catch((error) => {
- console.error("Third-party deletion push failed:", error);
- });
- }
+ await Promise.allSettled(
+ artefactsToDelete.map((artefact) =>
+ sendThirdPartyDeletion({
+ artefactId: artefact.artefactId,
+ locationId: artefact.locationId,
+ listTypeId: artefact.listTypeId,
+ contentDate: artefact.contentDate,
+ sensitivity: artefact.sensitivity,
+ language: artefact.language,
+ displayFrom: artefact.displayFrom,
+ displayTo: artefact.displayTo,
+ provenance: artefact.provenance
+ }).catch((error) => {
+ console.error("Third-party deletion push failed:", error);
+ })
+ )
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const artefact of artefactsToDelete) { | |
| sendThirdPartyDeletion({ | |
| artefactId: artefact.artefactId, | |
| locationId: artefact.locationId, | |
| listTypeId: artefact.listTypeId, | |
| contentDate: artefact.contentDate, | |
| sensitivity: artefact.sensitivity, | |
| language: artefact.language, | |
| displayFrom: artefact.displayFrom, | |
| displayTo: artefact.displayTo, | |
| provenance: artefact.provenance | |
| }).catch((error) => { | |
| console.error("Third-party deletion push failed:", error); | |
| }); | |
| } | |
| await Promise.allSettled( | |
| artefactsToDelete.map((artefact) => | |
| sendThirdPartyDeletion({ | |
| artefactId: artefact.artefactId, | |
| locationId: artefact.locationId, | |
| listTypeId: artefact.listTypeId, | |
| contentDate: artefact.contentDate, | |
| sensitivity: artefact.sensitivity, | |
| language: artefact.language, | |
| displayFrom: artefact.displayFrom, | |
| displayTo: artefact.displayTo, | |
| provenance: artefact.provenance | |
| }).catch((error) => { | |
| console.error("Third-party deletion push failed:", error); | |
| }) | |
| ) | |
| ); |
| return new Promise((resolve, reject) => { | ||
| const req = https.request(options, (res) => { | ||
| // Drain the response body to free the socket | ||
| res.resume(); | ||
|
|
||
| res.on("end", () => { | ||
| const statusCode = res.statusCode ?? 0; | ||
| resolve({ statusCode, success: isSuccessStatus(statusCode) }); | ||
| }); | ||
| }); | ||
|
|
||
| req.on("timeout", () => { | ||
| req.destroy(new Error(`Request timed out after ${REQUEST_TIMEOUT_MS}ms`)); | ||
| }); | ||
|
|
||
| req.on("error", reject); | ||
|
|
||
| if (bodyBuffer.byteLength > 0) { | ||
| req.write(bodyBuffer); | ||
| } | ||
|
|
||
| req.end(); | ||
| }); |
There was a problem hiding this comment.
Missing res error handler can reject after resolve.
If the response stream emits error after headers are received (e.g. socket reset mid-drain), there's no res.on("error", ...) listener, so the promise may never settle (the end event won't fire) and the retry layer will hang until the outer request timeout fires — but req.destroy inside the timeout handler won't necessarily re-emit on req once the response has started. Adding res.on("error", reject) closes this gap cheaply.
🛡️ Suggested fix
const req = https.request(options, (res) => {
// Drain the response body to free the socket
res.resume();
+ res.on("error", reject);
res.on("end", () => {
const statusCode = res.statusCode ?? 0;
resolve({ statusCode, success: isSuccessStatus(statusCode) });
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new Promise((resolve, reject) => { | |
| const req = https.request(options, (res) => { | |
| // Drain the response body to free the socket | |
| res.resume(); | |
| res.on("end", () => { | |
| const statusCode = res.statusCode ?? 0; | |
| resolve({ statusCode, success: isSuccessStatus(statusCode) }); | |
| }); | |
| }); | |
| req.on("timeout", () => { | |
| req.destroy(new Error(`Request timed out after ${REQUEST_TIMEOUT_MS}ms`)); | |
| }); | |
| req.on("error", reject); | |
| if (bodyBuffer.byteLength > 0) { | |
| req.write(bodyBuffer); | |
| } | |
| req.end(); | |
| }); | |
| return new Promise((resolve, reject) => { | |
| const req = https.request(options, (res) => { | |
| // Drain the response body to free the socket | |
| res.resume(); | |
| res.on("error", reject); | |
| res.on("end", () => { | |
| const statusCode = res.statusCode ?? 0; | |
| resolve({ statusCode, success: isSuccessStatus(statusCode) }); | |
| }); | |
| }); | |
| req.on("timeout", () => { | |
| req.destroy(new Error(`Request timed out after ${REQUEST_TIMEOUT_MS}ms`)); | |
| }); | |
| req.on("error", reject); | |
| if (bodyBuffer.byteLength > 0) { | |
| req.write(bodyBuffer); | |
| } | |
| req.end(); | |
| }); |
| it("treats unknown sensitivity as PUBLIC and includes all levels", async () => { | ||
| await findSubscribersByListType(42, "UNKNOWN"); | ||
|
|
||
| expect(mockFindMany).toHaveBeenCalledWith({ | ||
| where: { listTypeId: 42, sensitivity: { in: ["PUBLIC", "PRIVATE", "CLASSIFIED"] } }, | ||
| include: { user: true } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test encodes the fail-open behaviour.
Paired with the eligibleSensitivities comment above: once unknown sensitivities fail closed, this case should assert an empty set (or an error) rather than broadcasting to all levels. Update alongside the production fix.
| function eligibleSensitivities(publicationSensitivity: string): SensitivityLevel[] { | ||
| const rank = SENSITIVITY_ORDER.indexOf(publicationSensitivity as SensitivityLevel); | ||
| const minRank = rank === -1 ? 0 : rank; | ||
| return SENSITIVITY_ORDER.filter((_, i) => i >= minRank); | ||
| } |
There was a problem hiding this comment.
Unknown sensitivity silently falls open to PUBLIC.
Defaulting an unrecognised publicationSensitivity to rank 0 means an artefact with a typo or a future-added level (e.g. "RESTRICTED") would be broadcast to every subscriber, including PUBLIC. For a classification-driven distribution this is a fail-open posture. Prefer to fail closed (return [] / throw) and log, so mis-tagged publications are not leaked.
🛡️ Suggested fix
function eligibleSensitivities(publicationSensitivity: string): SensitivityLevel[] {
const rank = SENSITIVITY_ORDER.indexOf(publicationSensitivity as SensitivityLevel);
- const minRank = rank === -1 ? 0 : rank;
- return SENSITIVITY_ORDER.filter((_, i) => i >= minRank);
+ if (rank === -1) {
+ console.error(`[ThirdParty] Unknown sensitivity "${publicationSensitivity}" — refusing to distribute`);
+ return [];
+ }
+ return SENSITIVITY_ORDER.slice(rank);
}| beforeEach(() => { | ||
| vi.resetAllMocks(); | ||
| vi.mocked(pushWithRetry).mockResolvedValue({ statusCode: 200, success: true }); | ||
| vi.mocked(findSubscribersByListType).mockResolvedValue([{ id: "sub-1", user: { id: "user-1", name: "courtel" } }] as never); | ||
| mockPushLogCreate.mockResolvedValue({}); | ||
| process.env.COURTEL_API_URL = "https://courtel.example.com/api"; | ||
| process.env.COURTEL_CERTIFICATE = Buffer.from("-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----").toString("base64"); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does vitest vi.resetAllMocks reset implementations set via vi.mock factory mockReturnValue?
💡 Result:
Yes, vi.resetAllMocks resets implementations set via mockReturnValue on mock functions created in a vi.mock factory. It calls .mockReset on all spies/mocks, which clears call history and resets the mock implementation to its default (empty function returning undefined for vi.fn without initial impl, or original impl for vi.fn(impl)). Any mockReturnValue calls are overrides on the mock and get reset. For example, in a vi.mock('./module', => ({ fn: vi.fn.mockReturnValue('value') })), calling vi.resetAllMocks will reset fn so it no longer returns 'value' but undefined. This is evidenced by Vitest docs stating vi.resetAllMocks "Calls .mockReset on all spies" (sources 2,3,7,9), .mockReset resets implementation (source 8), and real-world cases where mockReset: true config (auto resetAllMocks) resets factory-set implementations, requiring per-test re-setup (source 6). Note: Module-level vi.mock itself persists (hoisted, cached); resetAllMocks affects mock functions within, not the module mock. Use vi.doUnmock + reimport to fully reset module mocks.
Citations:
- 1: https://vitest.dev/guide/mocking
- 2: https://v3.vitest.dev/api/vi
- 3: https://v4.vitest.dev/api/vi
- 4: https://stackoverflow.com/questions/78128073/vitest-how-to-set-mock-implementation-in-vi-mock
- 5: https://vitest.dev/api/vi.html
- 6: https://vitest.dev/api/mock.html
- 7: https://vitest.dev/api/vi
- 8: Mocks are not restored using resetAllMocks() or restoreAllMocks() vitest-dev/vitest#2536
vi.resetAllMocks() clears factory-mocked implementations.
This call resets the mock implementations set inside vi.mock(...) factories, so buildPushHeaders and getLocationWithDetails (lines 7 and 19) return undefined after reset. The current tests don't depend on these values, but this is fragile—future tests inspecting the headers will silently get undefined. Consider using vi.clearAllMocks() instead and re-arm only the mocks you intentionally need per test.
| return; | ||
| } | ||
|
|
||
| const location = await getLocationWithDetails(Number.parseInt(locationId, 10)); |
There was a problem hiding this comment.
Guard against non-numeric locationId.
Number.parseInt(locationId, 10) will silently produce NaN for malformed inputs, which then flows into getLocationWithDetails and yields null — the push still goes out with empty location headers. Consider validating and logging/skipping when the parsed id is NaN, so bad upstream data is surfaced rather than masked.
| writePushLog(artefactId, listTypeId, isUpdate ? "UPDATE" : "CREATE", result).catch((err) => { | ||
| console.warn(`${logPrefix} Failed to write push log:`, err instanceof Error ? err.message : String(err)); | ||
| }); |
There was a problem hiding this comment.
Fire-and-forget writePushLog can lose log rows on short-lived processes.
The log write is intentionally not awaited, but callers that invoke sendThirdPartyPublications/sendThirdPartyDeletion in fire-and-forget style from request handlers or the deletion flow may return/exit before the Prisma write resolves, in which case the push-log row is dropped and only the .catch warning remains — and only if the process is still alive. Since the push itself is already awaited, awaiting the log write as well would be cheap and materially more reliable.
Proposed change
- const result = await pushWithRetry(secrets.url, secrets.certPem, headers, body, logPrefix, pdfPath);
- writePushLog(artefactId, listTypeId, isUpdate ? "UPDATE" : "CREATE", result).catch((err) => {
- console.warn(`${logPrefix} Failed to write push log:`, err instanceof Error ? err.message : String(err));
- });
+ const result = await pushWithRetry(secrets.url, secrets.certPem, headers, body, logPrefix, pdfPath);
+ try {
+ await writePushLog(artefactId, listTypeId, isUpdate ? "UPDATE" : "CREATE", result);
+ } catch (err) {
+ console.warn(`${logPrefix} Failed to write push log:`, err instanceof Error ? err.message : String(err));
+ }Also applies to: 100-102
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
libs/publication/src/processing/service.ts (1)
186-186: Rename the new boolean flag to anis/has/canform.
skipThirdPartyPushis readable, but it diverges from the repo’s boolean naming rule (for example,isThirdPartyPushSkipped).As per coding guidelines, "Booleans should use is/has/can prefixes (e.g.,
isActive,hasAccess,canEdit)."Also applies to: 214-214
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98a7e3f0-f985-4674-8c01-0702c68535a4
📒 Files selected for processing (11)
libs/admin-pages/src/manual-upload/file-storage.tslibs/admin-pages/src/pages/manual-upload-summary/index.tslibs/api/src/blob-ingestion/file-storage.tslibs/legacy-third-party-fulfilment/src/push/http-client.test.tslibs/legacy-third-party-fulfilment/src/push/http-client.tslibs/legacy-third-party-fulfilment/src/push/retry.test.tslibs/legacy-third-party-fulfilment/src/push/retry.tslibs/legacy-third-party-fulfilment/src/service.test.tslibs/legacy-third-party-fulfilment/src/service.tslibs/publication/src/processing/service.test.tslibs/publication/src/processing/service.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/admin-pages/src/pages/manual-upload-summary/index.ts
- libs/publication/src/processing/service.test.ts
- libs/legacy-third-party-fulfilment/src/push/http-client.ts
- libs/legacy-third-party-fulfilment/src/service.test.ts
- libs/legacy-third-party-fulfilment/src/push/http-client.test.ts
| it("returns success immediately without retrying on first attempt", async () => { | ||
| mockExecutePush.mockResolvedValue({ statusCode: 200, success: true }); | ||
|
|
||
| const result = await pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
|
|
||
| expect(result).toEqual({ statusCode: 200, success: true }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("retries on 5xx and returns success on the second attempt", async () => { | ||
| mockExecutePush.mockResolvedValueOnce({ statusCode: 503, success: false }).mockResolvedValueOnce({ statusCode: 200, success: true }); | ||
|
|
||
| const resultPromise = pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
| await vi.runAllTimersAsync(); | ||
| const result = await resultPromise; | ||
|
|
||
| expect(result).toEqual({ statusCode: 200, success: true }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("does not retry on 4xx (except 429) and returns the 4xx result", async () => { | ||
| mockExecutePush.mockResolvedValue({ statusCode: 400, success: false }); | ||
|
|
||
| const result = await pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
|
|
||
| expect(result).toEqual({ statusCode: 400, success: false }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("does not retry on 401", async () => { | ||
| mockExecutePush.mockResolvedValue({ statusCode: 401, success: false }); | ||
|
|
||
| const result = await pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
|
|
||
| expect(result).toEqual({ statusCode: 401, success: false }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("retries on 429 rate-limit response", async () => { | ||
| mockExecutePush.mockResolvedValueOnce({ statusCode: 429, success: false }).mockResolvedValueOnce({ statusCode: 200, success: true }); | ||
|
|
||
| const resultPromise = pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
| await vi.runAllTimersAsync(); | ||
| const result = await resultPromise; | ||
|
|
||
| expect(result).toEqual({ statusCode: 200, success: true }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("exhausts all 3 attempts on repeated 5xx and returns the last result", async () => { | ||
| mockExecutePush.mockResolvedValue({ statusCode: 502, success: false }); | ||
|
|
||
| const resultPromise = pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
| await vi.runAllTimersAsync(); | ||
| const result = await resultPromise; | ||
|
|
||
| expect(result).toEqual({ statusCode: 502, success: false }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(3); | ||
| }); | ||
|
|
||
| it("retries and succeeds on the third attempt", async () => { | ||
| mockExecutePush | ||
| .mockResolvedValueOnce({ statusCode: 500, success: false }) | ||
| .mockResolvedValueOnce({ statusCode: 500, success: false }) | ||
| .mockResolvedValueOnce({ statusCode: 201, success: true }); | ||
|
|
||
| const resultPromise = pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
| await vi.runAllTimersAsync(); | ||
| const result = await resultPromise; | ||
|
|
||
| expect(result).toEqual({ statusCode: 201, success: true }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(3); | ||
| }); | ||
|
|
||
| it("retries on network error (rejected promise) and returns success on second attempt", async () => { | ||
| mockExecutePush.mockRejectedValueOnce(new Error("ECONNREFUSED")).mockResolvedValueOnce({ statusCode: 200, success: true }); | ||
|
|
||
| const resultPromise = pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY); | ||
| await vi.runAllTimersAsync(); | ||
| const result = await resultPromise; | ||
|
|
||
| expect(result).toEqual({ statusCode: 200, success: true }); | ||
| expect(mockExecutePush).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("passes through null body correctly", async () => { | ||
| mockExecutePush.mockResolvedValue({ statusCode: 204, success: true }); | ||
|
|
||
| const result = await pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, null); | ||
|
|
||
| expect(result).toEqual({ statusCode: 204, success: true }); | ||
| expect(mockExecutePush).toHaveBeenCalledWith(TEST_URL, TEST_CERT, TEST_HEADERS, null, undefined, undefined); | ||
| }); | ||
|
|
||
| it("forwards pdfPath to executePush", async () => { | ||
| mockExecutePush.mockResolvedValue({ statusCode: 200, success: true }); | ||
| const pdfPath = "/tmp/publication.pdf"; | ||
|
|
||
| await pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY, "[ThirdParty]", pdfPath); | ||
|
|
||
| expect(mockExecutePush).toHaveBeenCalledWith(TEST_URL, TEST_CERT, TEST_HEADERS, TEST_BODY, pdfPath, undefined); | ||
| }); | ||
|
|
||
| it("forwards flatFilePath to executePush", async () => { | ||
| mockExecutePush.mockResolvedValue({ statusCode: 200, success: true }); | ||
| const flatFilePath = "/tmp/artefact-1.xlsx"; | ||
|
|
||
| await pushWithRetry(TEST_URL, TEST_CERT, TEST_HEADERS, null, "[ThirdParty]", undefined, flatFilePath); | ||
|
|
||
| expect(mockExecutePush).toHaveBeenCalledWith(TEST_URL, TEST_CERT, TEST_HEADERS, null, undefined, flatFilePath); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Please add a 204-without-Location negative test.
The suite currently proves 204 success paths, but it does not protect the required behaviour where a bare 204 must not be accepted as a valid acknowledgement.
| if (SUCCESS_STATUSES.includes(lastResult.statusCode)) { | ||
| return lastResult; |
There was a problem hiding this comment.
204 responses are accepted too broadly.
At Line 44, any 204 is treated as success, but the fulfilment contract requires 204 only when a Location header is present. Please gate 204 success on that header (which likely needs executePush to return header metadata).
| sensitivity = "", | ||
| language = "", |
There was a problem hiding this comment.
Avoid empty-string defaults for sensitivity and language.
If callers omit these fields, subscriber matching and outbound header values can silently degrade, causing missed pushes or invalid metadata.
Also applies to: 260-261
| displayFrom: displayFrom ?? new Date(), | ||
| displayTo: displayTo ?? new Date(), |
There was a problem hiding this comment.
displayFrom/displayTo should not default to new Date().
This produces incorrect x-display-from/x-display-to values when publication window dates are missing, which can misrepresent list validity.
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…lment # Conflicts: # apps/api/helm/values.yaml # libs/admin-pages/src/pages/manual-upload-summary/index.test.ts
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…lment # Conflicts: # libs/postgres-prisma/src/schema-discovery.test.ts # libs/postgres-prisma/src/schema-discovery.ts # libs/publication/src/repository/queries.ts
…arty-subscription-fulfilment
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…arty-subscription-fulfilment # Conflicts: # libs/api/src/blob-ingestion/repository/service.test.ts
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
5 participants
[VIBE-367] Third Party subscription Fulfilment - Current #323
Change description
Implement third party subscription fulfilment
Closes #323
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores