Conversation
nhsd-david-wass
force-pushed
the
feature/CCM-13359-Volume-Group-Eligibility
branch
5 times, most recently
from
eed6694 to
fa5d2e1
Compare
nhsd-david-wass
force-pushed
the
feature/CCM-13359-Volume-Group-Eligibility
branch
10 times, most recently
from
16c74f1 to
4c7fbc2
Compare
nhsd-david-wass
force-pushed
the
feature/CCM-13359-Volume-Group-Eligibility
branch
from
fe90b52 to
7ec7e85
Compare
stevebux
left a comment
stevebux
left a comment
There was a problem hiding this comment.
Good work, just a few comments
| description: "No letter variant found for id", | ||
| variantId, | ||
| }); | ||
| throw new Error(`No letter variant details found for id ${variantId}`); |
There was a problem hiding this comment.
I suspect we probably don't need to both log and rethrow the exception - if we rethrow it it'll get logged further up the call stack
| function createVolumeGroupItem(groupId: string, status = "PROD") { | ||
| const startDate = new Date(Date.now() - 24 * 1000 * 60 * 60) | ||
| .toISOString() | ||
| .split("T")[0]; // Started an hour ago to ensure it's active based on start date. Tests can override this if needed. |
There was a problem hiding this comment.
This look like 24 hours ago, but the comment says 1 hour
| groupDetails && | ||
| (groupDetails.status !== "PROD" || | ||
| new Date(groupDetails.startDate) > new Date() || | ||
| (groupDetails.endDate && new Date(groupDetails.endDate) < new Date())) |
There was a problem hiding this comment.
Is the end date inclusive or exclusive? The unit tests suggest that it's going to be just a calendar date (rather than a date-time), so we probably need to consider whether a volume group expires at the start of the day on the end date or at the end of the day.
There's also a question of what time zone these dates represent - we're assuming UTC but is it Europe/London? That may be a lesser worry though as it would mean an error of no more than one hour, rather than a whole day.
| supplierId: string, | ||
| deps: Deps, | ||
| ): Promise<SupplierAllocation[]> { | ||
| const allocations = |
There was a problem hiding this comment.
It looks like you're using the empty string to represent "no supplierId". Would declaring the type as string? be clearer maybe?
| const supplierDetails: Supplier[] = | ||
| await deps.supplierConfigRepo.getSuppliersDetails(supplierIds); | ||
|
|
||
| if (Object.keys(supplierDetails).length === 0) { |
There was a problem hiding this comment.
Would it be an error if we found supplier allocations for only some of the suppliers, or is that OK?
2 participants
Description
Context
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.