Strip courtCentre.roomId/roomName for unallocated CROWN hearings#307
Closed
ozturkmenb wants to merge 6 commits into
Closed
Strip courtCentre.roomId/roomName for unallocated CROWN hearings#307ozturkmenb wants to merge 6 commits into
ozturkmenb wants to merge 6 commits into
Conversation
For an unallocated CROWN hearing (DRAFT court-schedule session), the CCP2 UI sends progression.command.list-new-hearing with courtCentre.roomId and courtCentre.roomName populated (denormalised from bookedSlots for display purposes). Progression today copies that courtCentre verbatim into progression.hearing.payload via ProgressionService.transform- HearingListingNeeds, and ListHearingRequestedProcessor.sendHearing- NotificationsToDefenceAndProsecutor reads courtCentre.getRoomId() into the new-hearing notification email. The result: the email and the persisted hearing snapshot both name a phantom courtroom the hearing is not actually allocated to. Listing already strips this on its side in HearingEnrichmentOrchestra- tor.stripRoomInfoIfAnyDraft (SPRDT-858), but progression observes the payload first and would otherwise leak it before listing sees it. This change adds defence-in-depth at the top of ListHearingRequested- Processor.handle: if jurisdiction is CROWN, bookedSlots is non-empty, and courtCentre.roomId is set, we call listing-query-api's new listing.query.court.schedule.draft.status endpoint via Requester to ask whether any of the booked sessions is DRAFT. If yes, we replace listHearingRequested with a sanitised clone whose courtCentre has null roomId and null roomName. All three downstream operations (listing forward, hearing.payload write, notification) then operate on the sanitised view. Cross-context boundary: progression talks to listing-query-api over the JJ messaging bus via Requester, never directly to listing-court- scheduler. CourtScheduleQueryAdapter fails-safe by reporting anyDraft=true if the remote call throws or returns a malformed payload - leaking a phantom courtroom is worse than dropping room info for what may be a confirmed-allocated hearing. NB: requires listing to publish the listing.query.court.schedule. draft.status endpoint at runtime. The corresponding listing PR adds that endpoint to listing-query-api on team/ccsph2n; this progression PR can merge independently because the call is dispatched by string action name at runtime - compile-time RAML knowledge isn't required. At deploy time, the listing service must be running the post-PR listing version.
ozturkmenb
requested review from
VenkataNRChalla,
newmaldenite and
sarathdrhmcts
and removed request for
a team
|
|
The Weld CDI container needs a component qualifier on the Requester
field to pick the right RequesterProducer-produced instance. Without
it the deploy fails with:
java.lang.IllegalStateException: No annotation found to define
component for class uk.gov.moj.cpp.progression.service.CourtScheduleQueryAdapter
at ComponentNameExtractor.componentFromComponentAnnotation
at RequesterProducer.produceRequester
Matches the same pattern used by ProgressionService.requester and
ListingService.sender — qualifier annotation on the field, no class
level @servicecomponent needed (CourtScheduleQueryAdapter is a plain
helper, not a JAX-RS resource or @Handles processor).
Unit tests bypass Weld entirely (Mockito @Injectmocks uses field
injection without container validation), which is why this only
surfaced at deploy time in ns-ste-ccm-72.
|
|
… error)
The original fail-safe returned anyDraft=true when the listing-query call
threw, came back null, or returned a malformed payload. That makes the
"treat as draft → strip room info" path the default during any listing-side
outage - which strips the courtroom from ALLOCATED CROWN hearings too.
Observed in ns-ste-ccm-72: every CROWN-with-bookedSlots hearing lost its
courtCentre.roomId in the notification email because the listing PR
(#854) endpoint was not yet deployed there, so every Requester call hit
the catch-block and triggered the strip.
Flip the direction: on error / null / malformed response, return false
(fail-open). The reasoning is asymmetric:
- Allocated CROWN (the common case): preserve room info always,
even when listing-query is unreachable. Notification email and
hearing.payload keep the correct courtroom.
- Unallocated CROWN (the case we are guarding against): strip room
info only when we can prove the session is draft. During a
listing-query outage the original unallocated-leak resumes - which
is exactly the same behaviour as before the strip was added, so a
regression to a known-bad-but-recoverable state rather than a new
silent corruption of allocated hearings.
The WARN logs still fire so persistent failures are visible.
Test changes:
- Renamed two adapter tests from failsSafeToTrue* to failsOpenToFalse*
and flipped assertions.
- Added a third adapter test for the requestAsAdmin-returns-null path
(the Requester contract permits null on some dispatch failures).
- Tightened the processor no-strip test to assert exact roomId/roomName
preservation rather than the toString().isEmpty() check, which was
trivially true for any UUID and would have passed even if the strip
leaked into the allocated path.
Black-box IT for the unallocated-CROWN courtCentre.roomId/roomName strip
behaviour. Exercises the full chain from
progression.list-new-hearing through to the body forwarded to
listing-command:
- shouldStripCourtCentreRoomWhenListingDraftStatusReportsDraft
Stubs listing-query-api's court-schedule-draft-status to return
{"anyDraft":true} and verifies the body sent on to
listing-command has courtCentre with roomId/roomName stripped.
This is the unallocated CROWN scenario the user hit on
ns-ste-ccm-72.
- shouldPreserveCourtCentreRoomWhenListingDraftStatusReportsNonDraft
Stubs listing-query-api to return {"anyDraft":false} and verifies
the body sent on to listing-command keeps courtCentre.roomId
intact. This is the allocated CROWN regression we hit when the
adapter was failing-closed to anyDraft=true.
Verification approach: we inspect the body of the outgoing
listing.command.list-court-hearing request captured by WireMock,
rather than polling progression's hearing query API. The
ListNewHearingHandler picks a random hearingId for new (non-related)
hearings, so polling by ID would require coupling to that random
value. Inspecting the captured downstream request body is direct.
Stub additions in ListingStub:
- stubCourtScheduleDraftStatusReturnsDraft / ReturnsNonDraft
stub the new /listing-service/query/api/rest/listing/courtScheduleDraftStatus
endpoint to return {"anyDraft":true} or {"anyDraft":false} respectively.
|
|
Request shape sent to listing.query.court.schedule.draft.status:
Before: {"courtScheduleIdList": [{"courtScheduleId": "<uuid>"}, ...]}
After: {"courtScheduleIdList": ["<uuid>", "<uuid>", ...]}
Mirrors the listing-side schema simplification - every element is a UUID
so the per-entry object wrapper added no information. The adapter now
appends each id as a JSON string directly.
Two new unit tests added that capture the JSON sent to the enveloper and
assert (a) the array contains UUID strings (not wrapper objects) and
(b) the order is preserved. These pin the wire contract so a regression
to the wrapped form would fail loudly here rather than at runtime in
ns-ste-ccm-72.
Removes the now-unused COURT_SCHEDULE_ID constant.
|
|
Contributor
Author
|
Superseded by source-side filtering in cpp-context-listing-courtscheduler#828: courtscheduler now strips courtRoomId/Name/Number from draft sessions, and the cpp-ui-prosecution-casefile crown-scheduling flow reads room straight from the slot response — so the list-new-hearing payload is cleaned end-to-end at source. This progression-side event-processor strip is therefore redundant. See ADR-005. Strip commits live only on this unmerged branch; team/ccsph2 is unaffected (nothing to revert). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
For unallocated CROWN hearings, strips the UI-denormalised
courtCentre.roomId/roomNamefrom the incomingprogression.command.list-new-hearingpayload before any downstream operation inListHearingRequestedProcessor.handlereads it. This prevents the phantom courtroom from being written toprogression.hearing.payloadand from appearing in the new-hearing notification email.Asks the new
listing.query.court.schedule.draft.statusendpoint oncpp-context-listing(see companion PR https://github.com/hmcts/cpp-context-listing/pull/854) whether any of the booked court-schedule sessions is DRAFT; if yes, the strip fires.Background
team/ccsph2nlisting +team/ccsph2progression, every CROWNSENT_FOR_LISTING/HEARING_INITIALISEDhearing inprogressionviewstore.hearing.payloadhascourtCentre.roomId/roomNamepopulated, even though listing's ownlisting.events.hearing-listedhas top-levelcourtRoomId: nullfor the same hearing. The CCP2 UI denormalises the slot's courtroom intocourtCentrefor display; progression copies it verbatim intohearing.payloadviaProgressionService.transformHearingListingNeeds:285and reads it into the notification viasendHearingNotificationsToDefenceAndProsecutor.HearingEnrichmentOrchestrator.stripRoomInfoIfAnyDraft(SPRDT-858) already strips room info on listing's side — but listing sees the payload after progression has already done its writes.ListHearingRequestedProcessor.handleso all three downstream operations (listing forward,hearing.payloadwrite, notification email) see the sanitised payload.Files
CourtScheduleQueryAdapter.java— callslisting.query.court.schedule.draft.statusviaRequester.requestAsAdmin. Goes through listing-context (not directly to listingcourtscheduler) so the cross-context boundary stays clean. Fails-safe by returninganyDraft=trueon remote-call exception or malformed response — leaking a phantom courtroom is worse than dropping room info for what may actually be a confirmed-allocated hearing.ListHearingRequestedProcessor.java— addsstripCourtCentreRoomIfAnyDraftSession(...)invoked at the top ofhandle(...). Filters: jurisdiction=CROWN, bookedSlots non-empty, courtCentre.roomId set. SanitisedListHearingRequestedthen flows to all three downstream calls.ListHearingRequestedProcessorTest.java—@Mock CourtScheduleQueryAdapter+ 4 new strip tests covering: any-draft → strip happens, all-non-draft → no strip, MAGS → adapter not called, CROWN without bookedSlots → adapter not called.CourtScheduleQueryAdapterTest.java— 6 tests covering null/empty input, any-draft, all-non-draft, missing anyDraft key (fail-safe true), exception (fail-safe true).What is NOT changed
hearing.payloadrows on ste are not retroactively scrubbed. The fix is prevention-only; the user explicitly chose this trade-off.cpp-ui-prosecution-casefile/.../manual-case-details.effects.ts:148-149is unchanged. Progression now acts as the boundary that catches the bad data.progression.event.list-hearing-requestedevent payload sanitisation — events are an immutable log of what arrived from the user; we never rewrite history. Only downstream reads of it are sanitised.Test plan
mvn install -DskipTests -Denforcer.skipMoJLatestInterfacesRule=trueonprogression-event/progression-event-processor -am— BUILD SUCCESSmvn test -Dtest='CourtScheduleQueryAdapterTest,ListHearingRequestedProcessorTest'— 26 run, 0 failures (20 processor + 6 adapter)courtCentre: { id, name }only inprogression.hearing.payload(noroomId/roomName), and the new-hearing notification email has no courtroom field.Deploy ordering
listing.query.court.schedule.draft.statuswill fail (and adapter will fail-safe to strip — degraded but not incorrect behaviour).listing.versionbump is needed.