Conversation
src/app/flows/fines/fines-con/services/utils/fines-con-payload-map-defendant-accounts.utils.ts
Show resolved
Hide resolved
...ws/fines/fines-con/services/utils/fines-con-payload-build-defendant-accounts-search.utils.ts
Show resolved
Hide resolved
...fines-con/consolidate-acc/fines-con-consolidate-acc/fines-con-consolidate-acc.component.html
Outdated
Show resolved
Hide resolved
...fines-con/consolidate-acc/fines-con-consolidate-acc/fines-con-consolidate-acc.component.html
Outdated
Show resolved
Hide resolved
src/app/flows/fines/fines-con/services/utils/fines-con-payload-map-defendant-accounts.utils.ts
Outdated
Show resolved
Hide resolved
src/app/flows/fines/fines-con/services/utils/fines-con-payload-map-defendant-accounts.utils.ts
Outdated
Show resolved
Hide resolved
iamfrankiemoran
left a comment
There was a problem hiding this comment.
Using the opal-frontend-review-guidelines skill because this is a PR-style code review.
<< Code review finished >>
• The new results flow has a broken primary action, and its state handling can resurface stale or unexpectedly cleared search results. Those regressions make the patch unsafe to treat as functionally correct.
Full review comments:
- [P1] Handle the Add to list action before exposing it — /Users/iamfrankiemoran/Documents/GitHub/opal-frontend/src/app/flows/fines/fines-con/consolidate-acc/fines-con-search-result/fines-con-search-result.component.html:4-8
Selecting accounts and pressing Add to list is currently a no-op because app-fines-con-search-result-defendant-table-wrapper emits addToList, but this parent never listens for it or updates any state. In the current Results flow that means users can
make selections but cannot move anything into the "For consolidation" step, so the primary action on this screen is broken.- [P2] Clear cached results when a search request fails — /Users/iamfrankiemoran/Documents/GitHub/opal-frontend/src/app/flows/fines/fines-con/consolidate-acc/fines-con-search-result/fines-con-search-result.component.ts:124-130
If a user already has results in the store and the next defendant-account search fails, this catchError path only clears the local view model. individualResults/companyResults keep the previous successful response, so switching away from Results and
back with searchPayload = null rehydrates stale accounts from the store and shows the wrong defendants for the latest search.- [P2] Do not wipe stored results when resetting the search form — /Users/iamfrankiemoran/Documents/GitHub/opal-frontend/src/app/flows/fines/fines-con/stores/fines-con.store.ts:94-99
resetSearchAccountForm() now clears both result buckets as well as the form state. That makes the Search tab's Clear search action erase the previously fetched Results: after a successful search, clearing the form and returning to the Results tab
will show the empty state because this store reset removed the cached accounts that the Results view rehydrates from.- [P3] Preserve alias line breaks in the table cell — /Users/iamfrankiemoran/Documents/GitHub/opal-frontend/src/app/flows/fines/fines-con/consolidate-acc/fines-con-search-result/fines-con-search-result-defendant-table-wrapper/fines-con-search-result-
defendant-table-wrapper.component.html:192-196
The mapper formats multiple aliases by joining them with \n, but this cell renders that string as normal text. In a standard table cell those newline characters collapse to spaces, so rows with more than one alias display as a single run of text
instead of one alias per line, and the formatting added in mapDefendantAccounts() is never visible to users.
P2's are fixed. P3 and P1 are not valid. P3 is how we have done it in fines-sa and then P1 is introduced in another ticket later down. |
iamfrankiemoran
left a comment
There was a problem hiding this comment.
[P1]: Handle the Add to list action before exposing it
src/app/flows/fines/fines-con/consolidate-acc/fines-con-search-result/fines-con-search-result.component.html:4
src/app/flows/fines/fines-con/consolidate-acc/fines-con-search-result/fines-con-search-result-defendant-table-wrapper/fines-con-search-result-defendant-table-wrapper.component.ts:337
Problem: The results parent still does not bind the child component’s addToList output, so clicking Add to list only emits an event that nobody handles.
Why: Users can select accounts on Results, press the primary action, and nothing moves into the “For consolidation” step, so the main flow is still broken.
Fix: Wire (addToList) to a container handler that persists the selected accounts and advances or hydrates the “For consolidation” state.
Example: (addToList)="handleAddToList($event)"[P3]: Preserve alias line breaks in the table cell
src/app/flows/fines/fines-con/consolidate-acc/fines-con-search-result/fines-con-search-result-defendant-table-wrapper/fines-con-search-result-defendant-table-wrapper.component.html:192
Problem: The aliases cell still renders the \n-joined alias string with normal table-cell whitespace.
Why: Newline characters collapse to spaces in this markup, so multi-alias defendants still appear as one run of text instead of one alias per line.
Fix: Preserve whitespace in the rendered aliases content, or render each alias as its own block element.
Example: wrap the value in an element styled with white-space: pre-line.The two state-handling comments look fixed: failed searches now clear cached results, and resetting the search form no longer wipes stored results.
Jira link
See https://tools.hmcts.net/jira/browse/PO-2415
Change description
Testing done
Security Vulnerability Assessment
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist