Added logic for creation, mangemnet and display of case links#1527
Added logic for creation, mangemnet and display of case links#1527aschalew-at wants to merge 27 commits intomasterfrom
Conversation
CCD diff summary👉 Full report: https://github.com/hmcts/pcs-api/actions/runs/23739028491?check_suite_focus=true AuthorisationCaseState.json
CaseField.json
AuthorisationCaseEvent/AuthorisationCaseEvent.json
AuthorisationCaseField/caseworker-pcs.json
AuthorisationCaseField/caseworker-pcs-solicitor.json
CaseEvent/maintainCaseLink.json
CaseEvent/createCaseLink.json
CaseEventToFields/maintainCaseLink.json
CaseEventToFields/createCaseLink.json
CaseTypeTab/6_caseLinkscaseworker-pcs-solicitor.json
|
| defendantResponse.setPcsCase(this); | ||
| } | ||
|
|
||
| public void mergeCaseLinks(List<ListValue<CaseLink>> incomingLinkedCases) { |
| .decentralisedEvent(EventId.maintainCaseLink.name(), this::submit) | ||
| .forAllStates() | ||
| .name("Manage case links") | ||
| .description("To Manage link related cases") |
There was a problem hiding this comment.
Minor - small M on "manage". Also it might read better as "To manage linked cases" or something like that. (Is that actually displayed anywhere btw?)
There was a problem hiding this comment.
"To manage linked cases" will be the tool tip on the event "Manage case links".
| .orElseThrow(() -> new CaseNotFoundException(caseReference)); | ||
| } | ||
|
|
||
| public void patchCase(long caseReference, PCSCase pcsCase) { |
There was a problem hiding this comment.
This method name isn't clear on what it is doing. We used to have a single patchCase method that would handle all updates if they weren't null but that has since been changed to have specific methods to only update specific fields. So can you rename this to something like patchCaseLinks ?
| pcsCaseEntity.mergeCaseLinks(pcsCase.getCaseLinks()); | ||
| } | ||
|
|
||
| pcsCaseRepository.save(pcsCaseEntity); |
There was a problem hiding this comment.
You shouldn't need to save the pcsCaseEntity as it is already managed in this scenario.
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| private List<ListValue<CaseLink>> mapAndWrapCaseLinks(PcsCaseEntity pcsCaseEntity) { |
There was a problem hiding this comment.
Can you move this logic into a separate class please? Otherwise this PCSCaseView will get quite large. See ClaimView as an example - you can implement a class with the same setCaseFields(...) method signature and call it in the same way as the other *View instance are called, (https://github.com/hmcts/pcs-api/blob/master/src/main/java/uk/gov/hmcts/reform/pcs/ccd/PCSCaseView.java#L117-L127)
| -- ============================================ | ||
| -- ============================================ | ||
| -- PCS CASE LINK + LINK REASONS | ||
| -- ============================================ |
There was a problem hiding this comment.
This comment is repeated 🙂
| DROP TABLE IF EXISTS case_link_reason CASCADE; | ||
|
|
||
| -- Drop case_link table | ||
| DROP TABLE IF EXISTS case_link CASCADE; |
There was a problem hiding this comment.
Are these DROP statements actually needed, given that Flyway will be applying the migrations in the correct order so the tables and indexes shouldn't exist when this migration script is run.
|
|
||
| -- Table: case_link | ||
| CREATE TABLE case_link ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), |
There was a problem hiding this comment.
Minor - we rely on the ORM to assign these, so for consistency with the other table definitions could you remove the default?
| CREATE TABLE case_link ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| case_id UUID NOT NULL REFERENCES pcs_case(id) ON DELETE CASCADE, -- Source case | ||
| linked_case_id BIGINT NOT NULL, -- Linked case number (BIGINT) |
There was a problem hiding this comment.
I would argue that these comments are not needed - the column definition is clear enough.
build.gradle
Outdated
| "**/uk/gov/hmcts/reform/pcs/ccd/page/**/*.java," + | ||
| "**/uk/gov/hmcts/reform/pcs/ccd/entity/**/*.java," + | ||
| "**/uk/gov/hmcts/reform/pcs/ccd/event/**/*.java," + | ||
| "**/uk/gov/hmcts/reform/pcs/ccd/service/**/*.java," + |
There was a problem hiding this comment.
Can you revert these changes please? We should have code coverage on those packages.
| -- ============================================ | ||
| CREATE TABLE case_link ( | ||
| id UUID PRIMARY KEY, | ||
| case_link_reference UUID NOT NULL REFERENCES pcs_case(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
This FK is called case_id in all other tables that have it, so it would be better to be consistent with the existing naming convention.
There was a problem hiding this comment.
Renamed case_link_reference to case_id and linked_case_id to linked_case_reference.
| if (incomingLinkedCases == null) { | ||
| this.caseLinks.clear(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why should an empty incoming list remove the existing ones, if this is a merge operation?
There was a problem hiding this comment.
The whole if block can be removed, as incomingLinkedCases can never be null, as this is being checked in the calling method (patchCaseLinks())
| caseLinkReasonEntities.add(caseLinkReasonEntity); | ||
| } | ||
| caseLinkEntity.getReasons().clear(); | ||
| caseLinkEntity.getReasons().addAll(caseLinkReasonEntities); |
There was a problem hiding this comment.
Better to have a setReasons method on the caseLinkEntity rather than fetching the collection and manipulating it, since you don't know whether it is immutable or not.
There was a problem hiding this comment.
As the reasons field in in CaseLinkEntity is annotated with @onetomany with orphanRemoval set to true, Hibernate tracks the original collection instance and expects the same collection to be mutated and not swapped out.
| } | ||
|
|
||
| this.caseLinks.clear(); | ||
| this.caseLinks.addAll(result); |
There was a problem hiding this comment.
Is there a reason for not just assigning result, (which could have a better name btw), to this.caseLinks ?
There was a problem hiding this comment.
Same as above. The caseLinks field in PcsCaseEntity is annotated with @onetomany with orphanRemoval set to true, Hibernate tracks the original collection instance and expects the same collection to be mutated and not swapped out. result renamed to mergedCaseLinkEntities.
| @Override | ||
| public SetMultimap<HasRole, Permission> getGrants() { | ||
| SetMultimap<HasRole, Permission> grants = HashMultimap.create(); | ||
| grants.putAll(PCS_SOLICITOR, Permission.CRU); |
There was a problem hiding this comment.
Is the Delete missing for permissions ? In the event - CreateCaseLink class, solicitor has full CRUD access.
There was a problem hiding this comment.
The access profile in CaseLinkinAccess is applied to the fields in PCSCase (for updating cases) do not need the Delete permission in association with the events CreateCaseLink and MaintainLinkCase. In the event class CreateCaseLink, we only need the permissions CRU, whereas in the event class MaintainLinkCase, we need the Delete permission as well. Hence I will remove the Delete permission in CreateCaseLink and keep it in MaintainLinkCase. I will keep CRU in CaseLinkinAccess class
…n for solicitor in CreateCaseLink
tvr-hmcts
left a comment
There was a problem hiding this comment.
Some comments for consideration
| .build(); | ||
| caseLinkReasonEntities.add(caseLinkReasonEntity); | ||
| } | ||
| caseLinkEntity.getReasons().clear(); |
There was a problem hiding this comment.
Is this call needed again here?
| } | ||
|
|
||
| @Test | ||
| void shouldAddCaseLinkReason() { |
There was a problem hiding this comment.
This is just testing a setter no? Guess I am not understanding the value.
|
|
||
| if (dto.getReasonForLink() != null) { | ||
| List<CaseLinkReasonEntity> caseLinkReasonEntities = new ArrayList<>(); | ||
| for (ListValue<LinkReason> incomingLinkReason : dto.getReasonForLink()) { |
There was a problem hiding this comment.
This can just be streamed into the reasons collection within the entity rather than create this new arraylist.
| @Component | ||
| public class CaseLinkView { | ||
|
|
||
| public void setCaseFields(PCSCase pcsCase, PcsCaseEntity pcsCaseEntity) { |
There was a problem hiding this comment.
Not understanding the point in this wrapper. The same check is made within the other method also. So the other can just be public
|
|
||
| private final PcsCaseService pcsCaseService; | ||
|
|
||
|
|
| } | ||
|
|
||
| @Test | ||
| void shouldPatchCaseData() { |
There was a problem hiding this comment.
Need another test for when the pcsCase.getCaseLinks() != null
| // Given | ||
| PcsCaseEntity pcsCaseEntity = mock(PcsCaseEntity.class); | ||
| PCSCase caseData = PCSCase.builder().build(); | ||
| when(pcsCaseRepository.findByCaseReference(CASE_REFERENCE)).thenReturn(java.util.Optional.of(pcsCaseEntity)); |
There was a problem hiding this comment.
import java.util.Optional
| underTest.patchCaseLinks(CASE_REFERENCE, caseData); | ||
|
|
||
| // Then | ||
| verify(caseLinkService, atLeastOnce()).mergeCaseLinks(caseData.getCaseLinks(), pcsCaseEntity); |
There was a problem hiding this comment.
The number of times can be asserted more strictly here.
| } | ||
|
|
||
| @Test | ||
| void shouldModifyCaseLinksInSubmitCallback() { |
There was a problem hiding this comment.
The logic in here is a duplication of the other test CreateCaseLinkTest
| // Then | ||
| List<ListValue<CaseLink>> mappedCaseLinks = pcsCase.getCaseLinks(); | ||
| assertThat(mappedCaseLinks).hasSize(1); | ||
| assertThat(mappedCaseLinks.getFirst().getValue().getCaseReference()).isEqualTo("1234"); |
There was a problem hiding this comment.
mappedCaseLinks.getFirst().getValue() can be extracted and the variable used in the other asserts rather than call the same chain each time.
Jira link
See [HDPI-4510](https://tools.hmcts.net/jira/browse/HDPI-4510)
[HDPI-4509](https://tools.hmcts.net/jira/browse/HDPI-4509)
[HDPI-4512](https://tools.hmcts.net/jira/browse/HDPI-4512)
Change description
This is about case linking which is linking of cases based on certain reasons.The PR includes changes for creation, managing and displaying of cases links.
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