feature/482 Use list types from database - #487
Conversation
🎭 Playwright E2E Test Results83 tests 50 ✅ 4m 48s ⏱️ Results for commit 35246fe. ♻️ This comment has been updated with latest results. |
|
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:
📝 WalkthroughWalkthroughMigrates list-type sourcing from in-repo mocks to runtime database queries (Prisma), removes the mock list-types module/exports, updates services to use DB lookups for PDF generator and notifications, updates system-admin pages to fetch list types, and adjusts provenance data and related validation/UI. ChangesList-types common module removal
Notification & Publication service DB migration
System-admin pages: replace mocks with DB queries
Provenance data, validation and UI updates
Test infra and minor test changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EventProducer as PublicationEvent
participant PublicationService as Publication Service
participant Prisma as Postgres/Prisma
participant PDFRegistry as PDF_GENERATOR_REGISTRY
participant NotificationService as Notification Service
participant EmailBuilder as Email Builder
participant Outbound as Email/SNS
EventProducer->>PublicationService: sendPublication(event)
PublicationService->>Prisma: findUnique(listTypeId)
Prisma-->>PublicationService: listType { name, friendlyName } or null
PublicationService->>PDFRegistry: lookup generator by name (or key fallback)
PDFRegistry-->>PublicationService: generator or undefined
PublicationService->>NotificationService: sendPublicationNotifications(event, listTypeName?)
NotificationService->>Prisma: findUnique(listTypeId) [if needed]
Prisma-->>NotificationService: listType { name } or null
NotificationService->>EmailBuilder: buildEmailTemplateData(event, user, listTypeName?)
EmailBuilder-->>NotificationService: email payload
NotificationService->>Outbound: send email
Outbound-->>NotificationService: delivery result
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
libs/system-admin-pages/src/pages/manage-list-types/index.test.ts (1)
8-55: Rename the top-level fixture constant to SCREAMING_SNAKE_CASE.
mockDbListTypesshould follow the repo constant naming rule for consistency.Suggested rename
-const mockDbListTypes = [ +const MOCK_DB_LIST_TYPES = [ @@ - vi.mocked(queries.findAllListTypes).mockResolvedValue(mockDbListTypes as any); + vi.mocked(queries.findAllListTypes).mockResolvedValue(MOCK_DB_LIST_TYPES as any); @@ - vi.mocked(queries.findAllListTypes).mockResolvedValue([{ ...mockDbListTypes[0], friendlyName: null } as any]); + vi.mocked(queries.findAllListTypes).mockResolvedValue([{ ...MOCK_DB_LIST_TYPES[0], friendlyName: null } as any]); @@ - expect(listTypes.length).toBe(mockDbListTypes.length); + expect(listTypes.length).toBe(MOCK_DB_LIST_TYPES.length);As per coding guidelines: "Constants should use SCREAMING_SNAKE_CASE (e.g., MAX_FILE_SIZE, DEFAULT_TIMEOUT)."
Also applies to: 71-72, 139-140, 155-156
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 786f6c1c-e35d-4abc-9211-a1e115a6b26f
📒 Files selected for processing (15)
e2e-tests/tests/care-standards-tribunal-upload.spec.tse2e-tests/tests/configure-list-type.spec.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/list-types/common/src/index.tslibs/list-types/common/src/list-type-ids.tslibs/list-types/common/src/mock-list-types.tslibs/notifications/src/notification/notification-service.test.tslibs/notifications/src/notification/notification-service.tslibs/publication/src/index.tslibs/publication/src/processing/service.test.tslibs/publication/src/processing/service.tslibs/system-admin-pages/src/pages/manage-list-types/index.test.tslibs/system-admin-pages/src/pages/manage-list-types/index.tslibs/system-admin-pages/src/pages/manage-third-party-subscriptions/index.tstsconfig.json
💤 Files with no reviewable changes (4)
- libs/publication/src/index.ts
- libs/list-types/common/src/list-type-ids.ts
- libs/list-types/common/src/index.ts
- libs/list-types/common/src/mock-list-types.ts
| @@ -1,8 +1,8 @@ | |||
| import AxeBuilder from "@axe-core/playwright"; | |||
| import { prisma } from "@hmcts/postgres"; | |||
There was a problem hiding this comment.
Use the database id in this flow.
The file now pulls in Prisma, but the upload helper still selects hard-coded list type "9" later on. That keeps this E2E coupled to seed ids and can make it fail across environments even though the app now reads list types from the database.
| const { prisma } = await import("@hmcts/postgres"); | ||
| vi.mocked(prisma.listType.findUnique).mockResolvedValue({ name: "CIVIL_AND_FAMILY_DAILY_CAUSE_LIST" } as any); |
There was a problem hiding this comment.
Make the Prisma mock depend on the requested id.
This always returns the Civil/Family list type, so the unsupported-list-type path is no longer really exercised. Returning null for non-enhanced ids would keep the tests aligned with the new lookup behaviour.
Suggested fix
const { prisma } = await import("@hmcts/postgres");
- vi.mocked(prisma.listType.findUnique).mockResolvedValue({ name: "CIVIL_AND_FAMILY_DAILY_CAUSE_LIST" } as any);
+ vi.mocked(prisma.listType.findUnique).mockImplementation(async ({ where: { id } }) => {
+ if (id === 8) {
+ return { name: "CIVIL_AND_FAMILY_DAILY_CAUSE_LIST" } as any;
+ }
+ return null;
+ });📝 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.
| const { prisma } = await import("@hmcts/postgres"); | |
| vi.mocked(prisma.listType.findUnique).mockResolvedValue({ name: "CIVIL_AND_FAMILY_DAILY_CAUSE_LIST" } as any); | |
| const { prisma } = await import("@hmcts/postgres"); | |
| vi.mocked(prisma.listType.findUnique).mockImplementation(async ({ where: { id } }) => { | |
| if (id === 8) { | |
| return { name: "CIVIL_AND_FAMILY_DAILY_CAUSE_LIST" } as any; | |
| } | |
| return null; | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/system-admin-pages/src/pages/manage-third-party-subscriptions/index.test.ts (1)
26-29: Broaden fixture shape to coverfriendlyName/namefallback.The fixture currently only validates the
friendlyNamepath. Addnameand include one entry withoutfriendlyNameso the fallback behaviour is exercised.♻️ Proposed fixture update
const mockListTypes = [ - { id: 1, friendlyName: "Civil Daily Cause List" }, - { id: 2, friendlyName: "Crown Daily List" } + { id: 1, name: "CIVIL_DAILY_CAUSE_LIST", friendlyName: "Civil Daily Cause List" }, + { id: 2, name: "CROWN_DAILY_LIST" } ];libs/notifications/src/notification/notification-service.ts (1)
100-103: Make the fallback path observable.Line 103’s
catch(() => null)quietly downgrades every affected notification to the standard template. A warning or metric here would make DB/schema issues visible instead of silently degrading behaviour.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfbeb5e5-f35a-4b25-944f-099fa1723a4b
📒 Files selected for processing (3)
libs/notifications/src/notification/notification-service.tslibs/publication/src/processing/service.tslibs/system-admin-pages/src/pages/manage-third-party-subscriptions/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/publication/src/processing/service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e-tests/utils/seed-list-types.ts (1)
14-105: Avoid keeping a second hard-coded list-type catalogue here.These provenance values now have to stay in sync with
libs/location/src/list-type-data.ts, and this PR is already updating both copies for the same changes. Please consider pulling the shared list-type data into one source and reusing it here to avoid future drift.libs/system-admin-pages/src/pages/configure-list-type-enter-details/index.ts (1)
42-46: Extract the provenance-to-checkbox mapping into one helper.The GET path and POST error path now duplicate the same projection logic. Please keep this in one place so the next provenance change only needs a single update.
Possible tidy-up
+const buildCheckedProvenance = (allowedProvenance?: string[]) => ({ + CFT_IDAM: allowedProvenance?.includes("CFT_IDAM") || false, + PI_AAD: allowedProvenance?.includes("PI_AAD") || false, + CRIME_IDAM: allowedProvenance?.includes("CRIME_IDAM") || false +}); + const getHandler = async (req: Request, res: Response) => { const session = req.session as ListTypeSession; const language = req.query.lng === "cy" ? "cy" : "en"; const content = language === "cy" ? cy : en; const editId = req.query.id ? Number.parseInt(req.query.id as string, 10) : undefined; @@ - const checkedProvenance = { - CFT_IDAM: formData.allowedProvenance?.includes("CFT_IDAM") || false, - PI_AAD: formData.allowedProvenance?.includes("PI_AAD") || false, - CRIME_IDAM: formData.allowedProvenance?.includes("CRIME_IDAM") || false - }; + const checkedProvenance = buildCheckedProvenance(formData.allowedProvenance); @@ - const checkedProvenance = { - CFT_IDAM: formData.allowedProvenance?.includes("CFT_IDAM") || false, - PI_AAD: formData.allowedProvenance?.includes("PI_AAD") || false, - CRIME_IDAM: formData.allowedProvenance?.includes("CRIME_IDAM") || false - }; + const checkedProvenance = buildCheckedProvenance(formData.allowedProvenance);Also applies to: 105-109
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a870dcfe-89e7-414e-9929-b4860dcae0e0
📒 Files selected for processing (6)
e2e-tests/utils/seed-list-types.tslibs/location/src/list-type-data.tslibs/system-admin-pages/src/list-type/validation.tslibs/system-admin-pages/src/pages/configure-list-type-enter-details/index.njklibs/system-admin-pages/src/pages/configure-list-type-enter-details/index.test.tslibs/system-admin-pages/src/pages/configure-list-type-enter-details/index.ts
✅ Files skipped from review due to trivial changes (1)
- libs/system-admin-pages/src/list-type/validation.ts
…pes-from-database
…pes-from-database # Conflicts: # e2e-tests/tests/care-standards-tribunal-upload.spec.ts # e2e-tests/tests/configure-list-type.spec.ts # e2e-tests/utils/seed-list-types.ts # libs/system-admin-pages/src/pages/manage-list-types/index.test.ts # libs/system-admin-pages/src/pages/manage-list-types/index.ts # tsconfig.json
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/system-admin-pages/src/pages/manage-third-party-subscriptions/index.test.ts (1)
26-29: Add one case fornamefallback coverage.The new handler logic supports
friendlyName || name, but the fixture only exercisesfriendlyName. Add one list type withnameonly to protect that branch.Suggested test fixture tweak
const mockListTypes = [ - { id: 1, friendlyName: "Civil Daily Cause List" }, - { id: 2, friendlyName: "Crown Daily List" } + { id: 1, friendlyName: "Civil Daily Cause List", name: "CIVIL_DAILY_CAUSE_LIST" }, + { id: 2, friendlyName: "Crown Daily List", name: "CROWN_DAILY_LIST" }, + { id: 3, name: "FALLBACK_NAME_ONLY_LIST_TYPE" } ];As per coding guidelines, "Aim for >80% test coverage on business logic."
libs/publication/src/processing/service.test.ts (1)
258-280: Add a rejection-path test for list-type lookup failures.You already test
nullfallback; add one case where lookup throws to lock in the catch-and-continue behaviour.Suggested additional test case
it("should use fallback list type name when not found", async () => { vi.mocked(prisma.listType.findUnique).mockResolvedValue(null); @@ expect(sendPublicationNotifications).toHaveBeenCalledWith( expect.objectContaining({ hearingListName: "LIST_TYPE_999" }) ); }); + + it("should use fallback list type name when lookup throws", async () => { + vi.mocked(prisma.listType.findUnique).mockRejectedValueOnce(new Error("DB unavailable")); + vi.mocked(getLocationById).mockResolvedValue({ + id: 123, + name: "Test Court", + welshName: "Llys Prawf" + }); + vi.mocked(sendPublicationNotifications).mockResolvedValue({ + totalSubscriptions: 0, + sent: 0, + failed: 0, + skipped: 0, + errors: [] + }); + + await sendPublicationNotificationsForArtefact({ ...baseParams, listTypeId: 999 }); + + expect(sendPublicationNotifications).toHaveBeenCalledWith( + expect.objectContaining({ hearingListName: "LIST_TYPE_999" }) + ); + });As per coding guidelines, "Aim for >80% test coverage on business logic."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3e2a83d-1882-4b29-8c68-25c46df7373c
📒 Files selected for processing (11)
e2e-tests/utils/seed-list-types.tslibs/admin-pages/src/pages/manual-upload-summary/index.test.tslibs/list-types/common/src/index.tslibs/notifications/src/notification/notification-service.test.tslibs/notifications/src/notification/notification-service.tslibs/publication/src/processing/service.test.tslibs/publication/src/processing/service.tslibs/system-admin-pages/src/pages/configure-list-type-enter-details/index.test.tslibs/system-admin-pages/src/pages/manage-list-types/index.njklibs/system-admin-pages/src/pages/manage-third-party-subscriptions/index.test.tslibs/system-admin-pages/src/pages/manage-third-party-subscriptions/index.ts
💤 Files with no reviewable changes (1)
- libs/list-types/common/src/index.ts
✅ Files skipped from review due to trivial changes (2)
- libs/system-admin-pages/src/pages/manage-list-types/index.njk
- e2e-tests/utils/seed-list-types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/notifications/src/notification/notification-service.test.ts
- 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. |
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. |
3 participants
Jira link
#482
Change description
Use list types from database
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores