CCM-14199 - Foundational: Infrastructure as Code and Core Types#31
CCM-14199 - Foundational: Infrastructure as Code and Core Types#31mjewildnhs wants to merge 25 commits intomainfrom
Conversation
mjewildnhs
force-pushed
the
feature/CCM-14199-foundation
branch
from
80e7546 to
fd5a645
Compare
mjewildnhs
force-pushed
the
feature/CCM-14199-foundation
branch
4 times, most recently
from
5fdffbf to
2165b0e
Compare
|
mjewildnhs
force-pushed
the
feature/CCM-14199-foundation
branch
from
d97e7b1 to
a965084
Compare
…ds which aren't needed
mjewildnhs
force-pushed
the
feature/CCM-14199-foundation
branch
from
5e03c91 to
7ca4971
Compare
lambdas/client-transform-filter-lambda/src/models/status-transition-event.ts
Outdated
Show resolved
Hide resolved
| describe("EventTypes", () => { | ||
| it("should match the expected event type values", () => { | ||
| expect(EventTypes).toEqual({ | ||
| MESSAGE_STATUS_TRANSITIONED: | ||
| "uk.nhs.notify.client-callbacks.message.status.transitioned.v1", | ||
| CHANNEL_STATUS_TRANSITIONED: | ||
| "uk.nhs.notify.client-callbacks.channel.status.transitioned.v1", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
No hence the "// coverage purposes" but sonar will fail coverage otherwise and i don't want to start adding exclusions
| | "TECHNICAL_FAILURE" | ||
| | "ACCEPTED" | ||
| | "CANCELLED" | ||
| | "PENDING_VIRUS_CHECK" |
There was a problem hiding this comment.
This also need a matching VIRUS_SCAN_FAILED? The API claims it can emit that, apparently.
What's odd though, is that is doesn't appear in SUPPLIER_STATUSES (commsTypes.ts) or getSupplierStatus.ts (which maps statuses to the API statuses), or in comms-mgr at all, as far as I can see. It doesn't look like we ever emit this supplier status in old callback mechanism. So maybe taking our PENDING_VIRUS_CHECK is the way to go?
There was a problem hiding this comment.
Yeah think we want to go off communications-manager-api/specification/schemas/enums/SupplierStatusEnum.yaml
I'll double check things would explode if VIRUS_SCAN_FAILED bever appeared
There was a problem hiding this comment.
Hmm actually...
https://github.com/NHSDigital/comms-mgr/blob/3382123d6c47b4f229a3919dd49c5ac8e7e277ee/packages/libs/utils/src/types/commsTypes.ts#L301
vs
https://github.com/NHSDigital/communications-manager-api/blob/release/specification/schemas/enums/SupplierStatusEnum.yaml
These lists vary quite a bit.
I gave it the OpenAPI spec schema as the source.
There was a problem hiding this comment.
I think the supplier status is normalized and mapped from the raw form in getSupplierStatus here:
https://github.com/NHSDigital/comms-mgr/blob/3382123d6c47b4f229a3919dd49c5ac8e7e277ee/packages/libs/routing-orchestrator/src/supplier-status/getSupplierStatus.ts#L78
This seems to make its way to the DB (in the lowercase form - which is inconsistent with the other statuses)
The current callback code passes this straight through as the callback payload.
The list of the website matches the list in the schema on the website here:
https://digital.nhs.uk/developer/api-catalogue/nhs-notify#post-/%3Cclient-provided-channel-status-URI%3E
Which is different to the list here:
https://notify.nhs.uk/using-nhs-notify/message-channel-supplier-status
I'm pretty sure we want the 23 lowercase options from here:
https://github.com/NHSDigital/comms-mgr/blob/3382123d6c47b4f229a3919dd49c5ac8e7e277ee/packages/libs/utils/src/types/commsTypes.ts#L301
I'll try and clear it up with someone
| * Message-level statuses | ||
| */ | ||
| export type MessageStatus = | ||
| | "CREATED" |
There was a problem hiding this comment.
Where did it get CREATED from? It's not in REQUEST_ITEM_STATUSES (commsType.ts), nor in the API...?
There was a problem hiding this comment.
There was a problem hiding this comment.
I think this is the open api spec being wrong again
lambdas/client-transform-filter-lambda/src/models/status-transition-event.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * Channel-level statuses | ||
| */ | ||
| export type ChannelStatus = |
There was a problem hiding this comment.
Missing:
- assigning_batch
- retry
- stale_pds
There was a problem hiding this comment.
The subscription types use the enum which includes the missing values
https://github.com/NHSDigital/comms-mgr/blob/0e19549a2794d0320d5fd0ca0925781520cc4ee5/packages/libs/utils/src/client-subscriptions/types.ts#L18
So presumably you could subscribe to them and its not just a case of a callback for those statuses not being possible
so again i think the spec is wrong
3 participants
Description
This PR establishes the foundational infrastructure and core type definitions for the NHS Notify Client Callbacks system. It includes:
Context
This work is part of CCM-14199 and lays the groundwork for externalizing client callbacks from the main Communications Manager system. The foundational infrastructure enables:
This foundation will be built upon in subsequent phases to add full transformation logic, error handling, DLQs, and monitoring.
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.