From a74e531c04c24dde2a15171f3d5968cd9fdd9153 Mon Sep 17 00:00:00 2001 From: Andy Dingley Date: Thu, 22 Jan 2026 18:53:11 +0000 Subject: [PATCH 1/5] feat: admin can update LTFT WTE and Start Date To avoid slow un-submit -> re-submit processes for minor changes which have been agreed between admins and trainees, a PATCH endpoint should be added to allow admins to make limited changes to LTFT applications. Currently admins should only be able to perform a `replace` on `/change/wte` and `/change/startDate`. Any other patch attempts will fail. An admin triggered patch is considered a shortcut for unsubmit and re-submit and as such will trigger a revision increment and history update. The form will stay in the `SUBMITTED` state but will have the revision incremented, last modified information updated and a snapshot taken for the `LtftSubmissionHistory`. TIS21-8201 TIS21-8219 TIS21-8212 --- build.gradle.kts | 3 +- .../api/AdminLtftResourceIntegrationTest.java | 214 ++++++++++++++++-- .../trainee/forms/api/AdminLtftResource.java | 62 ++++- .../tis/trainee/forms/dto/FormPatchDto.java | 42 ++++ .../trainee/forms/service/LtftService.java | 89 +++++++- .../forms/api/AdminLtftResourceTest.java | 154 ++++++++++++- .../forms/service/LtftServiceTest.java | 188 ++++++++++++++- 7 files changed, 721 insertions(+), 31 deletions(-) create mode 100644 src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchDto.java diff --git a/build.gradle.kts b/build.gradle.kts index 417ece85..6a33b2e8 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -10,7 +10,7 @@ plugins { } group = "uk.nhs.hee.tis.trainee" -version = "0.59.3" +version = "0.60.0" configurations { compileOnly { @@ -57,6 +57,7 @@ dependencies { implementation(libs.bundles.aws.xray) implementation("commons-beanutils:commons-beanutils:1.11.0") + implementation("com.github.java-json-tools:json-patch:1.13") // PDF implementation(libs.bundles.pdf.publishing) diff --git a/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java b/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java index 13c2a1e7..6a16bd16 100644 --- a/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java +++ b/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java @@ -33,6 +33,7 @@ import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.params.provider.EnumSource.Mode.EXCLUDE; import static org.junit.jupiter.params.provider.EnumSource.Mode.INCLUDE; +import static org.springframework.http.HttpMethod.PATCH; import static org.springframework.http.MediaType.APPLICATION_JSON; import static org.springframework.http.MediaType.APPLICATION_PDF; import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.jwt; @@ -153,14 +154,23 @@ void shouldReturnForbiddenWhenNoToken(HttpMethod method, URI uri) throws Excepti .andExpect(jsonPath("$").doesNotExist()); } + @Test + void shouldReturnForbiddenPatchingWhenNoToken() throws Exception { + mockMvc.perform(request(PATCH, "/api/admin/ltft/123") + .contentType(APPLICATION_JSON)) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$").doesNotExist()); + } + @ParameterizedTest @CsvSource(delimiter = '|', textBlock = """ - GET | /api/admin/ltft - GET | /api/admin/ltft/123 - PUT | /api/admin/ltft/123/approve - PUT | /api/admin/ltft/123/reject - PUT | /api/admin/ltft/123/unsubmit - GET | /api/admin/ltft/count + GET | /api/admin/ltft + GET | /api/admin/ltft/123 + PATCH | /api/admin/ltft/123 + PUT | /api/admin/ltft/123/approve + PUT | /api/admin/ltft/123/reject + PUT | /api/admin/ltft/123/unsubmit + GET | /api/admin/ltft/count """) void shouldReturnForbiddenWhenEmptyToken(HttpMethod method, URI uri) throws Exception { Jwt token = TestJwtUtil.createToken("{}"); @@ -172,12 +182,13 @@ void shouldReturnForbiddenWhenEmptyToken(HttpMethod method, URI uri) throws Exce @ParameterizedTest @CsvSource(delimiter = '|', textBlock = """ - GET | /api/admin/ltft - GET | /api/admin/ltft/123 - PUT | /api/admin/ltft/123/approve - PUT | /api/admin/ltft/123/reject - PUT | /api/admin/ltft/123/unsubmit - GET | /api/admin/ltft/count + GET | /api/admin/ltft + GET | /api/admin/ltft/123 + PATCH | /api/admin/ltft/123 + PUT | /api/admin/ltft/123/approve + PUT | /api/admin/ltft/123/reject + PUT | /api/admin/ltft/123/unsubmit + GET | /api/admin/ltft/count """) void shouldReturnForbiddenWhenNoGroupsInToken(HttpMethod method, URI uri) throws Exception { mockMvc.perform(request(method, uri) @@ -188,10 +199,11 @@ void shouldReturnForbiddenWhenNoGroupsInToken(HttpMethod method, URI uri) throws @ParameterizedTest @CsvSource(delimiter = '|', textBlock = """ - GET | /api/admin/ltft/123 - PUT | /api/admin/ltft/123/approve - PUT | /api/admin/ltft/123/reject - PUT | /api/admin/ltft/123/unsubmit + GET | /api/admin/ltft/123 + PATCH | /api/admin/ltft/123 + PUT | /api/admin/ltft/123/approve + PUT | /api/admin/ltft/123/reject + PUT | /api/admin/ltft/123/unsubmit """) void shouldReturnBadRequestWhenInvalidFormId(HttpMethod method, URI uri) throws Exception { mockMvc.perform(request(method, uri) @@ -211,6 +223,101 @@ void shouldReturnBadRequestWhenRequiredReasonMissing(HttpMethod method, String u .andExpect(status().isBadRequest()); } + @Test + void shouldReturnBadRequestPatchingWhenPatchDtoNullValues() throws Exception { + UUID formId = UUID.randomUUID(); + + mockMvc.perform(request(PATCH, "/api/admin/ltft/{id}", formId) + .with(TestJwtUtil.createAdminToken(List.of(DBC_1), REQUIRED_ROLES)) + .contentType(APPLICATION_JSON) + .content("{}")) + .andExpect(status().isBadRequest()) + .andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON)) + .andExpect(jsonPath("$.type", is("about:blank"))) + .andExpect(jsonPath("$.title", is("Validation failure"))) + .andExpect(jsonPath("$.status", is(HttpStatus.BAD_REQUEST.value()))) + .andExpect(jsonPath("$.instance", is("/api/admin/ltft/%s".formatted(formId)))) + .andExpect(jsonPath("$.properties.errors").isArray()) + .andExpect(jsonPath("$.properties.errors", hasSize(3))) + .andExpect(jsonPath("$.properties.errors[0].pointer", is("#/message"))) + .andExpect(jsonPath("$.properties.errors[0].detail", is("must not be null"))) + .andExpect(jsonPath("$.properties.errors[1].pointer", is("#/patch"))) + .andExpect(jsonPath("$.properties.errors[1].detail", is("must not be null"))) + .andExpect(jsonPath("$.properties.errors[2].pointer", is("#/reason"))) + .andExpect(jsonPath("$.properties.errors[2].detail", is("must not be null"))); + } + + @Test + void shouldReturnBadRequestPatchingWhenPatchEmpty() throws Exception { + UUID formId = UUID.randomUUID(); + String formPatch = """ + { + "patch": [], + "reason": "reason1", + "message": "message1" + } + """; + + mockMvc.perform(request(PATCH, "/api/admin/ltft/{id}", formId) + .with(TestJwtUtil.createAdminToken(List.of(DBC_1), REQUIRED_ROLES)) + .contentType(APPLICATION_JSON) + .content(formPatch)) + .andExpect(status().isBadRequest()) + .andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON)) + .andExpect(jsonPath("$.type", is("about:blank"))) + .andExpect(jsonPath("$.title", is("Validation failure"))) + .andExpect(jsonPath("$.status", is(HttpStatus.BAD_REQUEST.value()))) + .andExpect(jsonPath("$.instance", is("/api/admin/ltft/%s".formatted(formId)))) + .andExpect(jsonPath("$.properties.errors").isArray()) + .andExpect(jsonPath("$.properties.errors", hasSize(1))) + .andExpect(jsonPath("$.properties.errors[0].pointer", is("#/patch"))) + .andExpect(jsonPath("$.properties.errors[0].detail", is("must not be empty"))); + } + + @Test + void shouldReturnBadRequestPatchingWhenPatchOpsNotAllowed() throws Exception { + UUID formId = UUID.randomUUID(); + String formPatch = """ + { + "patch": [ + { + "op": "replace", "path": "/formRef", "value": "newref_12345_001" + }, + { + "op": "add", "path": "/change/wte", "value": 0.5 + }, + { + "op": "add", "path": "/change/endDate", "value": "1970-01-01" + } + ], + "reason": "reason1", + "message": "message1" + } + """; + mockMvc.perform(request(PATCH, "/api/admin/ltft/{id}", formId) + .with(TestJwtUtil.createAdminToken(List.of(DBC_1), REQUIRED_ROLES)) + .contentType(APPLICATION_JSON) + .content(formPatch)) + .andExpect(status().isBadRequest()) + .andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON)) + .andExpect(jsonPath("$.type", is("about:blank"))) + .andExpect(jsonPath("$.title", is("Validation failure"))) + .andExpect(jsonPath("$.status", is(HttpStatus.BAD_REQUEST.value()))) + .andExpect(jsonPath("$.instance", is("/api/admin/ltft/%s".formatted(formId)))) + .andExpect(jsonPath("$.properties.errors").isArray()) + .andExpect(jsonPath("$.properties.errors", hasSize(4))) + .andExpect(jsonPath("$.properties.errors[0].pointer", is("#/patch/0/path"))) + .andExpect(jsonPath("$.properties.errors[0].detail", + is("user not authorized to update '/formRef'"))) + .andExpect(jsonPath("$.properties.errors[1].pointer", is("#/patch/1/op"))) + .andExpect(jsonPath("$.properties.errors[1].detail", is("only 'replace' supported"))) + .andExpect(jsonPath("$.properties.errors[2].pointer", is("#/patch/2/op"))) + .andExpect(jsonPath("$.properties.errors[2].detail", is("only 'replace' supported"))) + .andExpect(jsonPath("$.properties.errors[3].pointer", is("#/patch/2/path"))) + .andExpect(jsonPath("$.properties.errors[3].detail", + is("user not authorized to update '/change/endDate'"))); + } + @ParameterizedTest @CsvSource(delimiter = '|', textBlock = """ GET | /api/admin/ltft/{id} @@ -227,6 +334,26 @@ void shouldReturnNotFoundWhenFormIdNotFound(HttpMethod method, String uriTemplat .andExpect(status().isNotFound()); } + @Test + void shouldReturnNotFoundPatchingWhenFormIdNotFound() throws Exception { + String formPatch = """ + { + "patch": [ + { + "op": "replace", "path": "/change/wte", "value": 0.5 + } + ], + "reason": "reason1", + "message": "message1" + } + """; + mockMvc.perform(request(PATCH, "/api/admin/ltft/{id}", UUID.randomUUID()) + .with(TestJwtUtil.createAdminToken(List.of(DBC_1), REQUIRED_ROLES)) + .contentType(APPLICATION_JSON) + .content(formPatch)) + .andExpect(status().isNotFound()); + } + @ParameterizedTest @CsvSource(delimiter = '|', textBlock = """ GET | /api/admin/ltft/{id} @@ -246,6 +373,60 @@ void shouldReturnNotFoundWhenLtftDoesNotMatchDbc(HttpMethod method, String uriTe .andExpect(status().isNotFound()); } + @Test + void shouldReturnNotFoundPatchingWhenLtftDoesNotMatchDbc() throws Exception { + LtftForm form = createLtftForm(SUBMITTED, DBC_2, null); + form = template.save(form); + + String formPatch = """ + { + "patch": [ + { + "op": "replace", "path": "/change/wte", "value": 0.5 + } + ], + "reason": "reason1", + "message": "message1" + } + """; + mockMvc.perform(request(PATCH, "/api/admin/ltft/{id}", form.getId()) + .with(TestJwtUtil.createAdminToken(List.of(DBC_1), REQUIRED_ROLES)) + .contentType(APPLICATION_JSON) + .content(formPatch)) + .andExpect(status().isNotFound()); + } + + @Test + void shouldReturnPatchedFormWhenLtftDoesMatchDbc() throws Exception { + LtftForm form = createLtftForm(SUBMITTED, DBC_1, null); + form = template.save(form); + + String formPatch = """ + { + "patch": [ + { + "op": "replace", "path": "/change/startDate", "value": "1970-01-01" + }, + { + "op": "replace", "path": "/change/wte", "value": 0.5 + } + ], + "reason": "reason1", + "message": "message1" + } + """; + mockMvc.perform(request(PATCH, "/api/admin/ltft/{id}", form.getId()) + .with(TestJwtUtil.createAdminToken(List.of(DBC_1), REQUIRED_ROLES)) + .contentType(APPLICATION_JSON) + .content(formPatch)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.change.startDate", is("1970-01-01"))) + .andExpect(jsonPath("$.change.wte", is(0.5))) + .andExpect(jsonPath("$.status.current.state", is(SUBMITTED.toString()))) + .andExpect(jsonPath("$.status.current.detail.reason", is("reason1"))) + .andExpect(jsonPath("$.status.current.detail.message", is("message1"))); + } + @ParameterizedTest @NullAndEmptySource void shouldCountZeroWhenNoLtfts(String statusFilter) throws Exception { @@ -1581,6 +1762,7 @@ private LtftForm createLtftForm(LifecycleState state, String dbc, LocalDate chan LtftContent content = LtftContent.builder() .change(CctChange.builder() .startDate(changeStartDate) + .wte(0.8) .build()) .programmeMembership(ProgrammeMembership.builder() .designatedBodyCode(dbc) diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java index 8d568822..66b20fc9 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java @@ -26,11 +26,21 @@ import static uk.nhs.hee.tis.trainee.forms.dto.enumeration.LifecycleState.UNSUBMITTED; import com.amazonaws.xray.spring.aop.XRayEnabled; +import com.fasterxml.jackson.annotation.JsonView; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ArrayNode; +import jakarta.validation.Valid; import java.io.IOException; +import java.lang.reflect.Method; +import java.util.Iterator; import java.util.Map; +import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.UUID; import lombok.extern.slf4j.Slf4j; +import org.springframework.core.MethodParameter; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort.Direction; @@ -39,18 +49,25 @@ import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.validation.BeanPropertyBindingResult; +import org.springframework.validation.FieldError; +import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PatchMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PutMapping; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +import uk.nhs.hee.tis.trainee.forms.dto.FormPatchDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftAdminSummaryDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto.StatusDto.LftfStatusInfoDetailDto; import uk.nhs.hee.tis.trainee.forms.dto.PersonDto; +import uk.nhs.hee.tis.trainee.forms.dto.validation.Update; +import uk.nhs.hee.tis.trainee.forms.dto.views.Trainee; import uk.nhs.hee.tis.trainee.forms.service.LtftService; import uk.nhs.hee.tis.trainee.forms.service.PdfService; @@ -66,10 +83,12 @@ public class AdminLtftResource { private final LtftService service; private final PdfService pdfService; + private final ObjectMapper objectMapper; - public AdminLtftResource(LtftService service, PdfService pdfService) { + public AdminLtftResource(LtftService service, PdfService pdfService, ObjectMapper objectMapper) { this.service = service; this.pdfService = pdfService; + this.objectMapper = objectMapper; } /** @@ -139,6 +158,47 @@ ResponseEntity getLtftAdminDetailPdf(@PathVariable UUID id) { return ResponseEntity.notFound().build(); } + @PatchMapping("/{id}") + public ResponseEntity patchLtft(@PathVariable UUID id, + @Valid @RequestBody FormPatchDto formPatch) + throws MethodArgumentNotValidException { + ArrayNode patchJson = objectMapper.convertValue(formPatch.patch(), ArrayNode.class); + BeanPropertyBindingResult validationResult = new BeanPropertyBindingResult(formPatch, + "formPatch"); + + if (patchJson.isEmpty()) { + validationResult.addError(new FieldError("FormPatchDto", "patch", "must not be empty")); + } + + // TODO: validate patchability generically in a central place (e.g. annotations). + Set allowedPaths = Set.of("/change/startDate", "/change/wte"); + int opIndex = 0; + + for (Iterator opIterator = patchJson.elements(); opIterator.hasNext(); opIndex++) { + JsonNode operation = opIterator.next(); + String op = operation.get("op").asText(); + String path = operation.get("path").asText(); + + if (!Objects.equals(op, "replace")) { + validationResult.addError(new FieldError("FormPatchDto", "patch.%s.op".formatted(opIndex), + "only 'replace' supported")); + } + + if (!allowedPaths.contains(path)) { + validationResult.addError(new FieldError("FormPatchDto", "patch.%s.path".formatted(opIndex), + "user not authorized to update '%s'".formatted(path))); + } + } + + if (validationResult.hasErrors()) { + Method method = this.getClass().getMethods()[0]; + throw new MethodArgumentNotValidException(new MethodParameter(method, 0), validationResult); + } + + Optional patchedForm = service.applyAdminPatch(id, formPatch); + return ResponseEntity.of(patchedForm); + } + @PutMapping("/{id}/assign") ResponseEntity assignAdmin(@PathVariable UUID id, @RequestBody PersonDto admin) { Optional form = service.assignAdmin(id, admin); diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchDto.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchDto.java new file mode 100644 index 00000000..97e9cb77 --- /dev/null +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchDto.java @@ -0,0 +1,42 @@ +/* + * The MIT License (MIT) + * + * Copyright 2026 Crown Copyright (Health Education England) + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and + * associated documentation files (the "Software"), to deal in the Software without restriction, + * including without limitation the rights to use, copy, modify, merge, publish, distribute, + * sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or + * substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT + * NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + * + */ + +package uk.nhs.hee.tis.trainee.forms.dto; + +import com.github.fge.jsonpatch.JsonPatch; +import jakarta.validation.constraints.NotNull; + +/** + * A wrapper for {@link com.github.fge.jsonpatch.JsonPatch} with additional metadata for updating + * forms. + */ +public record FormPatchDto( + @NotNull + JsonPatch patch, + + @NotNull + String reason, + + @NotNull + String message) { + +} diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java index 03995098..be2d4de7 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java @@ -28,6 +28,11 @@ import static uk.nhs.hee.tis.trainee.forms.dto.enumeration.LifecycleState.WITHDRAWN; import com.amazonaws.xray.spring.aop.XRayEnabled; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.github.fge.jsonpatch.JsonPatch; +import com.github.fge.jsonpatch.JsonPatchException; import jakarta.annotation.Nullable; import java.util.AbstractMap.SimpleEntry; import java.util.List; @@ -53,6 +58,7 @@ import org.springframework.validation.FieldError; import org.springframework.web.bind.MethodArgumentNotValidException; import uk.nhs.hee.tis.trainee.forms.dto.FeaturesDto.FormFeatures.LtftFeatures; +import uk.nhs.hee.tis.trainee.forms.dto.FormPatchDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftAdminSummaryDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto.StatusDto.LftfStatusInfoDetailDto; @@ -87,6 +93,7 @@ public class LtftService { private final LtftFormRepository ltftFormRepository; private final MongoTemplate mongoTemplate; + private final ObjectMapper objectMapper; private final LtftMapper mapper; private final EventBroadcastService eventBroadcastService; @@ -104,6 +111,7 @@ public class LtftService { * @param traineeIdentity The logged-in trainee, for trainee features. * @param ltftFormRepository The LTFT repository. * @param mongoTemplate The Mongo template. + * @param objectMapper The JSON mapper. * @param mapper The LTFT mapper. * @param eventBroadcastService The service for broadcasting events. * @param ltftAssignmentUpdateTopic The SNS topic for LTFT assignment updates. @@ -111,8 +119,8 @@ public class LtftService { * @param ltftSubmissionHistoryService The service for LTFT submission history. */ public LtftService(AdminIdentity adminIdentity, TraineeIdentity traineeIdentity, - LtftFormRepository ltftFormRepository, MongoTemplate mongoTemplate, LtftMapper mapper, - EventBroadcastService eventBroadcastService, + LtftFormRepository ltftFormRepository, MongoTemplate mongoTemplate, ObjectMapper objectMapper, + LtftMapper mapper, EventBroadcastService eventBroadcastService, @Value("${application.aws.sns.ltft-assignment-updated}") String ltftAssignmentUpdateTopic, @Value("${application.aws.sns.ltft-status-updated}") String ltftStatusUpdateTopic, LtftSubmissionHistoryService ltftSubmissionHistoryService) { @@ -120,6 +128,7 @@ public LtftService(AdminIdentity adminIdentity, TraineeIdentity traineeIdentity, this.traineeIdentity = traineeIdentity; this.ltftFormRepository = ltftFormRepository; this.mongoTemplate = mongoTemplate; + this.objectMapper = objectMapper; this.mapper = mapper; this.ltftAssignmentUpdateTopic = ltftAssignmentUpdateTopic; this.ltftStatusUpdateTopic = ltftStatusUpdateTopic; @@ -189,13 +198,81 @@ public Page getAdminLtftSummaries(Map filte * @return The found form, empty if the form does not exist or does not match the admin's DBCs. */ public Optional getAdminLtftDetail(UUID formId) { + return getLtftForAdmin(formId).map(mapper::toDto); + } + + /** + * Find an LTFT form associated with the local offices of the calling admin. + * + * @param formId The ID of the form. + * @return The found form, empty if the form does not exist or does not match the admin's DBCs. + */ + private Optional getLtftForAdmin(UUID formId) { Set groups = adminIdentity.getGroups(); log.info("Getting LTFT form {} for admin {} with DBCs [{}]", formId, adminIdentity.getEmail(), groups); - Optional form = ltftFormRepository + return ltftFormRepository .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( formId, Set.of(DRAFT), adminIdentity.getGroups()); - return form.map(mapper::toDto); + } + + /** + * Apply a patch update to the LTFT with the given ID. + * + * @param formId The ID of the form. + * @param formPatch The patch and metadata to update the form with. + * @return The patched form, empty if the form does not exist or does not match the admin's DBCs. + */ + public Optional applyAdminPatch(UUID formId, FormPatchDto formPatch) { + return getLtftForAdmin(formId) + // Will result in NOT FOUND when not submitted, which mirrors GET behaviour. + .filter(ltft -> ltft.getLifecycleState().equals(SUBMITTED)) + + .map(ltft -> { + try { + ltft = patchLtft(ltft, formPatch.patch()); + + StatusDetail statusDetail = StatusDetail.builder() + .reason(formPatch.reason()) + .message(formPatch.message()) + .build(); + + // An admin patch is considered a shortcut of the un-submit -> re-submit revision flow. + Person modifiedBy = Person.builder() + .name(adminIdentity.getName()) + .email(adminIdentity.getEmail()) + .role(adminIdentity.getRole()) + .build(); + ltft.setLifecycleState(ltft.getLifecycleState(), statusDetail, modifiedBy, + ltft.getRevision() + 1); + ltft = ltftFormRepository.save(ltft); + ltftSubmissionHistoryService.takeSnapshot(ltft); + + return mapper.toDto(ltft); + } catch (JsonPatchException | JsonProcessingException e) { + // Should not happen given validation and known JSON structures, inform caller anyway. + throw new RuntimeException(e); + } + }); + } + + /** + * Update an {@link LtftFormDto} with the given patch. + * + * @param ltft The LTFT form to apply the patch to. + * @param patch The patch to apply to the form. + * @return The patched form entity. + * @throws JsonPatchException If the patch could not be applied. + * @throws JsonProcessingException If the patch creates an invalid form. + */ + private LtftForm patchLtft(LtftForm ltft, JsonPatch patch) + throws JsonPatchException, JsonProcessingException { + // Convert the entity to DTO for patching, as the client will base paths on the public DTO. + LtftFormDto dto = mapper.toDto(ltft); + JsonNode patchedNode = patch.apply(objectMapper.convertValue(dto, JsonNode.class)); + LtftFormDto patchedDto = objectMapper.treeToValue(patchedNode, LtftFormDto.class); + // TODO: validate patched DTO before persisting. + return mapper.toEntity(patchedDto); } /** @@ -672,8 +749,8 @@ public void publishUpdateNotification(LtftForm form, String messageAttribute, St } /** - * Move all LTFT forms from one trainee to another. Assumes that fromTraineeId and toTraineeId - * are valid. The updated LTFTs are broadcast as events. Also moves LTFT submission history. + * Move all LTFT forms from one trainee to another. Assumes that fromTraineeId and toTraineeId are + * valid. The updated LTFTs are broadcast as events. Also moves LTFT submission history. * * @param fromTraineeId The trainee ID to move LTFTs from. * @param toTraineeId The trainee ID to move LTFTs to. diff --git a/src/test/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceTest.java b/src/test/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceTest.java index bcce5dc9..c53b8f00 100644 --- a/src/test/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceTest.java +++ b/src/test/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceTest.java @@ -41,6 +41,9 @@ import static uk.nhs.hee.tis.trainee.forms.dto.enumeration.LifecycleState.SUBMITTED; import static uk.nhs.hee.tis.trainee.forms.dto.enumeration.LifecycleState.UNSUBMITTED; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import com.github.fge.jsonpatch.JsonPatch; import java.io.IOException; import java.util.List; import java.util.Map; @@ -48,6 +51,8 @@ import java.util.UUID; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.mockito.ArgumentCaptor; import org.springframework.data.domain.Page; import org.springframework.data.domain.PageImpl; @@ -56,6 +61,7 @@ import org.springframework.data.web.PagedModel; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.MethodArgumentNotValidException; +import uk.nhs.hee.tis.trainee.forms.dto.FormPatchDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftAdminSummaryDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto.StatusDto.LftfStatusInfoDetailDto; @@ -68,12 +74,14 @@ class AdminLtftResourceTest { private AdminLtftResource controller; private LtftService service; private PdfService pdfService; + private JsonMapper jsonMapper; @BeforeEach void setUp() { service = mock(LtftService.class); pdfService = mock(PdfService.class); - controller = new AdminLtftResource(service, pdfService); + jsonMapper = (JsonMapper) new JsonMapper().registerModule(new JavaTimeModule()); + controller = new AdminLtftResource(service, pdfService, jsonMapper); } @Test @@ -219,6 +227,150 @@ void shouldGetDetailPdfWhenFormFound() throws IOException { assertThat("Unexpected response body.", response.getBody(), sameInstance(body)); } + @Test + void shouldThrowValidationExceptionWhenPatchIsEmpty() throws IOException { + UUID id = UUID.randomUUID(); + JsonPatch patch = JsonPatch.fromJson(jsonMapper.readTree("[]")); + FormPatchDto formPatch = new FormPatchDto(patch, "reason1", "message1"); + + when(service.applyAdminPatch(id, formPatch)).thenReturn(Optional.empty()); + + assertThrows(MethodArgumentNotValidException.class, () -> controller.patchLtft(id, formPatch)); + + verifyNoInteractions(service); + } + + @Test + void shouldReturnBadRequestWhenPatchOpNotAllowed() throws IOException { + UUID id = UUID.randomUUID(); + JsonPatch patch = JsonPatch.fromJson(jsonMapper.readTree(""" + [ + { + "op": "add", + "path": "/change/wte", + "value": 0.5 + } + ] + """)); + FormPatchDto formPatch = new FormPatchDto(patch, "reason1", "message1"); + + when(service.applyAdminPatch(id, formPatch)).thenReturn(Optional.empty()); + + assertThrows(MethodArgumentNotValidException.class, () -> controller.patchLtft(id, formPatch)); + + verifyNoInteractions(service); + } + + @Test + void shouldReturnBadRequestWhenPatchPathNotAllowed() throws IOException { + UUID id = UUID.randomUUID(); + JsonPatch patch = JsonPatch.fromJson(jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/change/startDate", + "value": "1970-01-01" + }, + { + "op": "replace", + "path": "/formRef", + "value": "new-ref_12345_001" + } + ] + """)); + FormPatchDto formPatch = new FormPatchDto(patch, "reason1", "message1"); + + when(service.applyAdminPatch(id, formPatch)).thenReturn(Optional.empty()); + + assertThrows(MethodArgumentNotValidException.class, () -> controller.patchLtft(id, formPatch)); + + verifyNoInteractions(service); + } + + @ParameterizedTest + @CsvSource(delimiter = '|', textBlock = """ + /change/startDate | "1970-01-01" + /change/wte | 0.5 + """) + void shouldReturnNotFoundWhenPatchFormNotFound(String path, String value) + throws IOException, MethodArgumentNotValidException { + UUID id = UUID.randomUUID(); + JsonPatch patch = JsonPatch.fromJson(jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "%s", + "value": %s + } + ] + """.formatted(path, value))); + FormPatchDto formPatch = new FormPatchDto(patch, "reason1", "message1"); + + when(service.applyAdminPatch(id, formPatch)).thenReturn(Optional.empty()); + + ResponseEntity response = controller.patchLtft(id, formPatch); + + assertThat("Unexpected response code.", response.getStatusCode(), is(NOT_FOUND)); + assertThat("Unexpected response body.", response.getBody(), nullValue()); + } + + @ParameterizedTest + @CsvSource(delimiter = '|', textBlock = """ + /change/startDate | "1970-01-01" + /change/wte | 0.5 + """) + void shouldReturnPatchedFormWhenFormFoundAndSingleOpValid(String path, String value) + throws IOException, MethodArgumentNotValidException { + UUID id = UUID.randomUUID(); + JsonPatch patch = JsonPatch.fromJson(jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "%s", + "value": %s + } + ] + """.formatted(path, value))); + FormPatchDto formPatch = new FormPatchDto(patch, "reason1", "message1"); + + LtftFormDto form = LtftFormDto.builder().id(UUID.randomUUID()).build(); + when(service.applyAdminPatch(id, formPatch)).thenReturn(Optional.of(form)); + + ResponseEntity response = controller.patchLtft(id, formPatch); + + assertThat("Unexpected response code.", response.getStatusCode(), is(OK)); + assertThat("Unexpected response body.", response.getBody(), sameInstance(form)); + } + + @Test + void shouldReturnPatchedFormWhenFormFoundAndMultipleOpsValid() + throws IOException, MethodArgumentNotValidException { + UUID id = UUID.randomUUID(); + JsonPatch patch = JsonPatch.fromJson(jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/change/startDate", + "value": "1970-01-01" + }, + { + "op": "replace", + "path": "/change/wte", + "value": 0.5 + } + ] + """)); + FormPatchDto formPatch = new FormPatchDto(patch, "reason1", "message1"); + + LtftFormDto form = LtftFormDto.builder().id(UUID.randomUUID()).build(); + when(service.applyAdminPatch(id, formPatch)).thenReturn(Optional.of(form)); + + ResponseEntity response = controller.patchLtft(id, formPatch); + + assertThat("Unexpected response code.", response.getStatusCode(), is(OK)); + assertThat("Unexpected response body.", response.getBody(), sameInstance(form)); + } + @Test void shouldReturnNotFoundWhenAssignAdminFormNotFound() { UUID id = UUID.randomUUID(); diff --git a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java index 00c7c716..724ff6a2 100644 --- a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java +++ b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java @@ -26,6 +26,7 @@ import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.in; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; @@ -55,6 +56,13 @@ import static uk.nhs.hee.tis.trainee.forms.service.LtftService.FORM_ATTRIBUTE_FORM_STATUS; import static uk.nhs.hee.tis.trainee.forms.service.LtftService.FORM_ATTRIBUTE_TPD_STATUS; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import com.github.fge.jsonpatch.JsonPatch; +import com.github.fge.jsonpatch.JsonPatchException; +import java.io.IOException; import java.time.Duration; import java.time.Instant; import java.time.LocalDate; @@ -92,6 +100,7 @@ import uk.nhs.hee.tis.trainee.forms.dto.FeaturesDto; import uk.nhs.hee.tis.trainee.forms.dto.FeaturesDto.FormFeatures; import uk.nhs.hee.tis.trainee.forms.dto.FeaturesDto.FormFeatures.LtftFeatures; +import uk.nhs.hee.tis.trainee.forms.dto.FormPatchDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftAdminSummaryDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto.CctChangeDto; @@ -149,6 +158,7 @@ class LtftServiceTest { private LtftService service; private LtftFormRepository repository; private MongoTemplate mongoTemplate; + private JsonMapper jsonMapper; private LtftMapper mapper; private EventBroadcastService eventBroadcastService; private LtftSubmissionHistoryService ltftSubmissionHistoryService; @@ -178,9 +188,10 @@ void setUp() { eventBroadcastService = mock(EventBroadcastService.class); ltftSubmissionHistoryService = mock(LtftSubmissionHistoryService.class); + jsonMapper = (JsonMapper) new JsonMapper().registerModule(new JavaTimeModule()); mapper = new LtftMapperImpl(new TemporalMapperImpl()); - service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, mapper, - eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, + service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, + mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); } @@ -1426,6 +1437,171 @@ void shouldGetAdminLtftStatusDetailWithDefaultValuesWhenFormFoundWithNullValues( assertThat("Unexpected history count.", status.history(), nullValue()); } + @Test + void shouldReturnEmptyPatchedFormWhenFormNotFound() throws IOException { + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.empty()); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/path", + "value": "newValue" + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + Optional patchedForm = service.applyAdminPatch(ID, formPatch); + + assertThat("Unexpected form.", patchedForm, is(Optional.empty())); + verify(repository, never()).save(any()); + } + + @ParameterizedTest + @EnumSource(value = LifecycleState.class, mode = EXCLUDE, names = "SUBMITTED") + void shouldReturnEmptyPatchedFormWhenFormNotSubmitted(LifecycleState lifecycleState) + throws IOException { + LtftForm entity = new LtftForm(); + entity.setId(ID); + entity.setContent(LtftContent.builder().build()); + entity.setLifecycleState(lifecycleState); + + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.of(entity)); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/path", + "value": "newValue" + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + Optional patchedForm = service.applyAdminPatch(ID, formPatch); + + assertThat("Unexpected form.", patchedForm, is(Optional.empty())); + verify(repository, never()).save(any()); + } + + @Test + void shouldThrowExceptionPatchingFormWhenTargetPathNotExists() throws IOException { + LtftForm entity = new LtftForm(); + entity.setId(ID); + entity.setContent(LtftContent.builder().build()); + entity.setLifecycleState(SUBMITTED); + + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.of(entity)); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/path", + "value": "newValue" + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + RuntimeException exception = assertThrows(RuntimeException.class, + () -> service.applyAdminPatch(ID, formPatch)); + + assertThat("Unexpected exception.", exception.getCause(), instanceOf(JsonPatchException.class)); + verify(repository, never()).save(any()); + } + + @Test + void shouldThrowExceptionPatchingFormWhenNewPathNotExists() throws IOException { + LtftForm entity = new LtftForm(); + entity.setId(ID); + entity.setContent(LtftContent.builder().build()); + entity.setLifecycleState(SUBMITTED); + + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.of(entity)); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "add", + "path": "/path", + "value": "newValue" + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + RuntimeException exception = assertThrows(RuntimeException.class, + () -> service.applyAdminPatch(ID, formPatch)); + + assertThat("Unexpected exception.", exception.getCause(), + instanceOf(JsonProcessingException.class)); + verify(repository, never()).save(any()); + } + + @Test + void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { + LtftForm entity = new LtftForm(); + entity.setId(ID); + entity.setContent(LtftContent.builder().build()); + entity.setRevision(1); + entity.setLifecycleState(SUBMITTED); + + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.of(entity)); + when(repository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/formRef", + "value": "ref_12345_001" + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + Optional optionalForm = service.applyAdminPatch(ID, formPatch); + + assertThat("Unexpected form optional.", optionalForm.isPresent(), is(true)); + + LtftFormDto patchedForm = optionalForm.get(); + assertThat("Unexpected form ref.", patchedForm.formRef(), is("ref_12345_001")); + assertThat("Unexpected status history count.", patchedForm.status().history(), hasSize(2)); + + StatusInfoDto current = patchedForm.status().current(); + assertThat("Unexpected current revision.", current.revision(), is(2)); + assertThat("Unexpected current state.", current.state(), is(SUBMITTED)); + assertThat("Unexpected current status reason.", current.detail().reason(), is("reason1")); + assertThat("Unexpected current status message.", current.detail().message(), is("message1")); + + StatusInfoDto history = patchedForm.status().history().get(0); + assertThat("Unexpected original revision.", history.revision(), is(1)); + assertThat("Unexpected original state.", history.state(), is(SUBMITTED)); + assertThat("Unexpected original status reason.", history.detail().reason(), nullValue()); + assertThat("Unexpected original status message.", history.detail().message(), nullValue()); + + history = patchedForm.status().history().get(1); + assertThat("Unexpected latest revision.", history.revision(), is(2)); + assertThat("Unexpected latest state.", history.state(), is(SUBMITTED)); + assertThat("Unexpected latest status reason.", history.detail().reason(), is("reason1")); + assertThat("Unexpected latest status message.", history.detail().message(), is("message1")); + + verify(repository).save(any()); + } + @Test void shouldReturnEmptyAssigningAdminWhenFormNotFound() { when(repository.findByIdAndContent_ProgrammeMembership_DesignatedBodyCodeIn( @@ -1949,7 +2125,7 @@ void shouldNotSaveIfNewLtftFormForTraineeIfNoFeaturesSet() { TraineeIdentity traineeIdentity = new TraineeIdentity(); traineeIdentity.setTraineeId(TRAINEE_ID); - service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, + service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); @@ -1986,7 +2162,7 @@ void shouldNotSaveIfNewLtftFormForTraineeIfFeaturesLtftNotTrue() { .build()) .build()); - service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, + service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); @@ -2022,8 +2198,8 @@ void shouldNotSaveIfNewLtftFormForTraineeIfNoFeatureLtftProgrammes() { .build()) .build()); - service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, mapper, - eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, + service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, + mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); LtftFormDto dtoToSave = LtftFormDto.builder() From 81bb0f20eb46add26a6d12a23ed79689a87d5511 Mon Sep 17 00:00:00 2001 From: Andy Dingley Date: Fri, 23 Jan 2026 16:59:45 +0000 Subject: [PATCH 2/5] feat: validate patched LTFT content Add additional validation steps to ensure that applied LTFT patches do not make the application invalid. Due to the entity <-> DTO conversion the `Update` validation group can not be used without further changes, as the revision will default to `0` instead of `null` and always fail. So the default validation group is used. The default validation group is likely fine for the LTFT content, however LTFT content does not actually have any validation rules. So a simple validation of the WTE maximum value of `1.0` has been added, allowing testing of patched data validation. TIS21-8201 TIS21-8219 --- .../api/AdminLtftResourceIntegrationTest.java | 36 +++++ .../RestResponseEntityExceptionHandler.java | 34 +++++ .../tis/trainee/forms/dto/LtftFormDto.java | 4 + .../trainee/forms/service/LtftService.java | 31 ++++- ...estResponseEntityExceptionHandlerTest.java | 129 ++++++++++++++++++ .../forms/service/LtftServiceTest.java | 86 ++++++++++-- 6 files changed, 302 insertions(+), 18 deletions(-) diff --git a/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java b/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java index 6a16bd16..90271cd3 100644 --- a/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java +++ b/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java @@ -396,6 +396,40 @@ void shouldReturnNotFoundPatchingWhenLtftDoesNotMatchDbc() throws Exception { .andExpect(status().isNotFound()); } + @Test + void shouldReturnBadRequestPatchingWhenPatchedFormFailsValidation() throws Exception { + LtftForm form = createLtftForm(SUBMITTED, DBC_1, null); + form = template.save(form); + + String formPatch = """ + { + "patch": [ + { + "op": "replace", "path": "/change/wte", "value": 1.1 + } + ], + "reason": "reason1", + "message": "message1" + } + """; + mockMvc.perform(request(PATCH, "/api/admin/ltft/{id}", form.getId()) + .with(TestJwtUtil.createAdminToken(List.of(DBC_1), REQUIRED_ROLES)) + .contentType(APPLICATION_JSON) + .content(formPatch)) + .andExpect(status().isBadRequest()) + .andExpect(content().contentType(MediaType.APPLICATION_PROBLEM_JSON)) + .andExpect(jsonPath("$.type", is("about:blank"))) + .andExpect(jsonPath("$.title", is("Validation failure"))) + .andExpect(jsonPath("$.status", is(HttpStatus.BAD_REQUEST.value()))) + .andExpect(jsonPath("$.instance", is("/api/admin/ltft/%s".formatted(form.getId())))) + .andExpect(jsonPath("$.properties.errors").isArray()) + .andExpect(jsonPath("$.properties.errors", hasSize(1))) + // The pointer is not correct to spec, as the validation error comes from the patched form. + .andExpect(jsonPath("$.properties.errors[0].pointer", is("#/change/wte"))) + .andExpect( + jsonPath("$.properties.errors[0].detail", is("must be less than or equal to 1.0"))); + } + @Test void shouldReturnPatchedFormWhenLtftDoesMatchDbc() throws Exception { LtftForm form = createLtftForm(SUBMITTED, DBC_1, null); @@ -420,6 +454,8 @@ void shouldReturnPatchedFormWhenLtftDoesMatchDbc() throws Exception { .contentType(APPLICATION_JSON) .content(formPatch)) .andExpect(status().isOk()) + .andExpect(jsonPath("$.id", is(form.getId()))) + .andExpect(jsonPath("$.formRef", is(form.getFormRef()))) .andExpect(jsonPath("$.change.startDate", is("1970-01-01"))) .andExpect(jsonPath("$.change.wte", is(0.5))) .andExpect(jsonPath("$.status.current.state", is(SUBMITTED.toString()))) diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandler.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandler.java index 4ffc96dd..3cb3132b 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandler.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandler.java @@ -21,7 +21,10 @@ package uk.nhs.hee.tis.trainee.forms.api; +import static org.springframework.http.HttpStatus.BAD_REQUEST; + import com.fasterxml.jackson.core.JsonPointer; +import jakarta.validation.ConstraintViolationException; import java.util.Comparator; import java.util.List; import org.springframework.http.HttpHeaders; @@ -30,6 +33,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.validation.BindingResult; import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; import org.springframework.web.context.request.WebRequest; import org.springframework.web.method.annotation.HandlerMethodValidationException; @@ -89,6 +93,36 @@ protected ResponseEntity handleHandlerMethodValidationException( return handleExceptionInternal(ex, problemDetail, headers, status, request); } + /** + * Handle {@link ConstraintViolationException} and convert to Problem Detail. + * + * @param ex The contstraint violations. + * @param request The web request that triggered the exception. + * @return A Problem Detail response. + */ + @ExceptionHandler(ConstraintViolationException.class) + protected ResponseEntity handleConstraintViolation(ConstraintViolationException ex, + WebRequest request) { + List errors = ex.getConstraintViolations().stream() + .map(v -> { + String pointer = + "#/" + v.getPropertyPath().toString() + .replace('.', JsonPointer.SEPARATOR); + return new BodyValidationError(pointer, v.getMessage()); + }) + .sorted( + Comparator.comparing(BodyValidationError::pointer) + .thenComparing(BodyValidationError::detail) + ) + .toList(); + + ProblemDetail problemDetail = ProblemDetail.forStatus(BAD_REQUEST); + problemDetail.setTitle(TITLE_VALIDATION_FAILURE); + problemDetail.setProperty("errors", errors); + + return handleExceptionInternal(ex, problemDetail, new HttpHeaders(), BAD_REQUEST, request); + } + /** * A detailed parameter validation error. * diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/LtftFormDto.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/LtftFormDto.java index dc9cb64f..25226e5f 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/LtftFormDto.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/LtftFormDto.java @@ -22,6 +22,8 @@ package uk.nhs.hee.tis.trainee.forms.dto; import com.fasterxml.jackson.annotation.JsonView; +import jakarta.validation.Valid; +import jakarta.validation.constraints.DecimalMax; import jakarta.validation.constraints.Null; import java.time.Instant; import java.time.LocalDate; @@ -72,6 +74,7 @@ public record LtftFormDto( ProgrammeMembershipDto programmeMembership, DeclarationsDto declarations, DiscussionsDto discussions, + @Valid CctChangeDto change, ReasonsDto reasons, @@ -107,6 +110,7 @@ public record CctChangeDto( UUID id, UUID calculationId, CctChangeType type, + @DecimalMax("1.0") Double wte, LocalDate startDate, LocalDate endDate, diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java index be2d4de7..31b9f51e 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java @@ -34,6 +34,9 @@ import com.github.fge.jsonpatch.JsonPatch; import com.github.fge.jsonpatch.JsonPatchException; import jakarta.annotation.Nullable; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Validator; import java.util.AbstractMap.SimpleEntry; import java.util.List; import java.util.Map; @@ -95,6 +98,7 @@ public class LtftService { private final ObjectMapper objectMapper; private final LtftMapper mapper; + private final Validator validator; private final EventBroadcastService eventBroadcastService; @@ -113,6 +117,7 @@ public class LtftService { * @param mongoTemplate The Mongo template. * @param objectMapper The JSON mapper. * @param mapper The LTFT mapper. + * @param validator The validator to use for validating LTFTs. * @param eventBroadcastService The service for broadcasting events. * @param ltftAssignmentUpdateTopic The SNS topic for LTFT assignment updates. * @param ltftStatusUpdateTopic The SNS topic for LTFT status updates. @@ -120,7 +125,7 @@ public class LtftService { */ public LtftService(AdminIdentity adminIdentity, TraineeIdentity traineeIdentity, LtftFormRepository ltftFormRepository, MongoTemplate mongoTemplate, ObjectMapper objectMapper, - LtftMapper mapper, EventBroadcastService eventBroadcastService, + LtftMapper mapper, Validator validator, EventBroadcastService eventBroadcastService, @Value("${application.aws.sns.ltft-assignment-updated}") String ltftAssignmentUpdateTopic, @Value("${application.aws.sns.ltft-status-updated}") String ltftStatusUpdateTopic, LtftSubmissionHistoryService ltftSubmissionHistoryService) { @@ -130,6 +135,7 @@ public LtftService(AdminIdentity adminIdentity, TraineeIdentity traineeIdentity, this.mongoTemplate = mongoTemplate; this.objectMapper = objectMapper; this.mapper = mapper; + this.validator = validator; this.ltftAssignmentUpdateTopic = ltftAssignmentUpdateTopic; this.ltftStatusUpdateTopic = ltftStatusUpdateTopic; this.ltftSubmissionHistoryService = ltftSubmissionHistoryService; @@ -230,7 +236,9 @@ public Optional applyAdminPatch(UUID formId, FormPatchDto formPatch .map(ltft -> { try { - ltft = patchLtft(ltft, formPatch.patch()); + // Patch the content and copy it back in to avoid changes to read-only fields. + LtftContent ltftContent = patchLtftContent(ltft, formPatch.patch()); + ltft.setContent(ltftContent); StatusDetail statusDetail = StatusDetail.builder() .reason(formPatch.reason()) @@ -257,22 +265,31 @@ public Optional applyAdminPatch(UUID formId, FormPatchDto formPatch } /** - * Update an {@link LtftFormDto} with the given patch. + * Update an {@link LtftForm} content with the given patch. * * @param ltft The LTFT form to apply the patch to. * @param patch The patch to apply to the form. - * @return The patched form entity. + * @return The patched form content. * @throws JsonPatchException If the patch could not be applied. * @throws JsonProcessingException If the patch creates an invalid form. */ - private LtftForm patchLtft(LtftForm ltft, JsonPatch patch) + private LtftContent patchLtftContent(LtftForm ltft, JsonPatch patch) throws JsonPatchException, JsonProcessingException { // Convert the entity to DTO for patching, as the client will base paths on the public DTO. LtftFormDto dto = mapper.toDto(ltft); JsonNode patchedNode = patch.apply(objectMapper.convertValue(dto, JsonNode.class)); LtftFormDto patchedDto = objectMapper.treeToValue(patchedNode, LtftFormDto.class); - // TODO: validate patched DTO before persisting. - return mapper.toEntity(patchedDto); + + // Read-only fields can not be reliably ignored when using the Update validation group so the + // default group is used, which may miss some otherwise expected validations. + Set> violations = validator.validate(patchedDto); + + if (!violations.isEmpty()) { + throw new ConstraintViolationException(violations); + } + + // The content is returned to avoid any unexpected changes to managed/read-only fields. + return mapper.toEntity(patchedDto).getContent(); } /** diff --git a/src/test/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandlerTest.java b/src/test/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandlerTest.java index ba791628..6a37abdb 100644 --- a/src/test/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandlerTest.java +++ b/src/test/java/uk/nhs/hee/tis/trainee/forms/api/RestResponseEntityExceptionHandlerTest.java @@ -28,14 +28,21 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.springframework.http.HttpStatus.BAD_REQUEST; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Path; import jakarta.validation.constraints.NotNull; +import jakarta.validation.metadata.ConstraintDescriptor; import java.lang.reflect.Method; import java.net.URI; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Random; +import java.util.Set; +import org.hibernate.validator.internal.engine.path.PathImpl; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.context.support.DefaultMessageSourceResolvable; @@ -177,6 +184,63 @@ void shouldHandleMethodValidationException() { assertThat("Unexpected error detail.", error.detail(), is("detail3")); } + @Test + void shouldHandleConstraintViolationException() { + Set> violations = Set.of( + new ConstraintViolationStub("field2", "detail3"), + new ConstraintViolationStub("field1", "detail2"), + new ConstraintViolationStub("field1", "detail1"), + new ConstraintViolationStub("field1.subField", "detail4") + ); + + ConstraintViolationException exception = + new ConstraintViolationException(violations); + + HttpHeaders headers = HttpHeaders.EMPTY; + WebRequest request = new ServletWebRequest(new MockHttpServletRequest()); + + ResponseEntity response = handler.handleConstraintViolation(exception, request); + + assertThat("Unexpected response.", response, notNullValue()); + assertThat("Unexpected headers.", response.getHeaders(), is(headers)); + assertThat("Unexpected response code.", response.getStatusCode(), is(BAD_REQUEST)); + assertThat("Unexpected response type.", response.getBody(), instanceOf(ProblemDetail.class)); + + ProblemDetail problem = (ProblemDetail) response.getBody(); + assertThat("Unexpected problem.", problem, notNullValue()); + assertThat("Unexpected problem title.", problem.getTitle(), is("Validation failure")); + assertThat("Unexpected problem status.", problem.getStatus(), is(BAD_REQUEST.value())); + assertThat("Unexpected problem instance.", problem.getInstance(), nullValue()); + assertThat("Unexpected problem type.", problem.getType(), is(URI.create("about:blank"))); + assertThat("Unexpected problem detail.", problem.getDetail(), nullValue()); + + Map problemProperties = problem.getProperties(); + assertThat("Unexpected problem properties.", problemProperties, notNullValue()); + assertThat("Unexpected property count.", problemProperties.size(), is(1)); + + Object errors = problemProperties.get("errors"); + assertThat("Unexpected errors type.", errors, instanceOf(List.class)); + + List errorsList = (List) errors; + assertThat("Unexpected errors count.", errorsList.size(), is(4)); + + BodyValidationError error = errorsList.get(0); + assertThat("Unexpected error pointer.", error.pointer(), is("#/field1")); + assertThat("Unexpected error detail.", error.detail(), is("detail1")); + + error = errorsList.get(1); + assertThat("Unexpected error pointer.", error.pointer(), is("#/field1")); + assertThat("Unexpected error detail.", error.detail(), is("detail2")); + + error = errorsList.get(2); + assertThat("Unexpected error pointer.", error.pointer(), is("#/field1/subField")); + assertThat("Unexpected error detail.", error.detail(), is("detail4")); + + error = errorsList.get(3); + assertThat("Unexpected error pointer.", error.pointer(), is("#/field2")); + assertThat("Unexpected error detail.", error.detail(), is("detail3")); + } + /** * A test stub for {@link MethodValidationResult}. * @@ -218,4 +282,69 @@ public boolean isForReturnValue() { .toList(); } } + + /** + * A test stub for {@link ConstraintViolation}. + * + * @param propertyPath The property path of the violation. + * @param message The validation message. + */ + private record ConstraintViolationStub(String propertyPath, String message) implements + ConstraintViolation { + + @Override + public String getMessage() { + return message; + } + + @Override + public Path getPropertyPath() { + return PathImpl.createPathFromString(propertyPath); + } + + @Override + public String getMessageTemplate() { + return ""; + } + + @Override + public Object getRootBean() { + return null; + } + + @Override + public Class getRootBeanClass() { + return Object.class; + } + + @Override + public Object getLeafBean() { + return null; + } + + @Override + public Object[] getExecutableParameters() { + return null; + } + + @Override + public Object getExecutableReturnValue() { + return null; + } + + @Override + public Object getInvalidValue() { + return null; + } + + @Override + public ConstraintDescriptor getConstraintDescriptor() { + return null; + } + + @Override + public U unwrap(Class type) { + throw new UnsupportedOperationException(); + } + } } diff --git a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java index 724ff6a2..fb5df92b 100644 --- a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java +++ b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java @@ -62,6 +62,9 @@ import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; import com.github.fge.jsonpatch.JsonPatch; import com.github.fge.jsonpatch.JsonPatchException; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Validator; import java.io.IOException; import java.time.Duration; import java.time.Instant; @@ -160,6 +163,7 @@ class LtftServiceTest { private MongoTemplate mongoTemplate; private JsonMapper jsonMapper; private LtftMapper mapper; + private Validator validator; private EventBroadcastService eventBroadcastService; private LtftSubmissionHistoryService ltftSubmissionHistoryService; @@ -190,9 +194,10 @@ void setUp() { jsonMapper = (JsonMapper) new JsonMapper().registerModule(new JavaTimeModule()); mapper = new LtftMapperImpl(new TemporalMapperImpl()); + validator = mock(Validator.class); service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, - mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, - ltftSubmissionHistoryService); + mapper, validator, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, + LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); } @Test @@ -1549,6 +1554,35 @@ void shouldThrowExceptionPatchingFormWhenNewPathNotExists() throws IOException { verify(repository, never()).save(any()); } + @Test + void shouldThrowExceptionPatchingFormWhenPatchMakesFormInvalid() throws IOException { + LtftForm entity = new LtftForm(); + entity.setId(ID); + entity.setContent(LtftContent.builder().build()); + entity.setLifecycleState(SUBMITTED); + + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.of(entity)); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/formRef", + "value": null + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + when(validator.validate(any())).thenReturn(Set.of(mock(ConstraintViolation.class))); + + assertThrows(ConstraintViolationException.class, () -> service.applyAdminPatch(ID, formPatch)); + + verify(repository, never()).save(any()); + } + @Test void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { LtftForm entity = new LtftForm(); @@ -1566,8 +1600,8 @@ void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { [ { "op": "replace", - "path": "/formRef", - "value": "ref_12345_001" + "path": "/personalDetails/email", + "value": "new@example.com" } ] """); @@ -1578,7 +1612,7 @@ void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { assertThat("Unexpected form optional.", optionalForm.isPresent(), is(true)); LtftFormDto patchedForm = optionalForm.get(); - assertThat("Unexpected form ref.", patchedForm.formRef(), is("ref_12345_001")); + assertThat("Unexpected form ref.", patchedForm.personalDetails().email(), is("new@example.com")); assertThat("Unexpected status history count.", patchedForm.status().history(), hasSize(2)); StatusInfoDto current = patchedForm.status().current(); @@ -1602,6 +1636,36 @@ void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { verify(repository).save(any()); } + @Test + void shouldSnapshotPatchedFormWhenFormSubmitted() throws IOException { + LtftForm entity = new LtftForm(); + entity.setId(ID); + entity.setContent(LtftContent.builder().build()); + entity.setRevision(1); + entity.setLifecycleState(SUBMITTED); + + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.of(entity)); + when(repository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/personalDetails/email", + "value": "new@example.com" + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + Optional optionalForm = service.applyAdminPatch(ID, formPatch); + + assertThat("Unexpected form optional.", optionalForm.isPresent(), is(true)); + verify(ltftSubmissionHistoryService).takeSnapshot(any()); + } + @Test void shouldReturnEmptyAssigningAdminWhenFormNotFound() { when(repository.findByIdAndContent_ProgrammeMembership_DesignatedBodyCodeIn( @@ -2126,8 +2190,8 @@ void shouldNotSaveIfNewLtftFormForTraineeIfNoFeaturesSet() { traineeIdentity.setTraineeId(TRAINEE_ID); service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, - mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, - ltftSubmissionHistoryService); + mapper, validator, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, + LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); LtftFormDto dtoToSave = LtftFormDto.builder() .traineeTisId(TRAINEE_ID) @@ -2163,8 +2227,8 @@ void shouldNotSaveIfNewLtftFormForTraineeIfFeaturesLtftNotTrue() { .build()); service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, - mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, - ltftSubmissionHistoryService); + mapper, validator, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, + LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); LtftFormDto dtoToSave = LtftFormDto.builder() .traineeTisId(TRAINEE_ID) @@ -2199,8 +2263,8 @@ void shouldNotSaveIfNewLtftFormForTraineeIfNoFeatureLtftProgrammes() { .build()); service = new LtftService(adminIdentity, traineeIdentity, repository, mongoTemplate, jsonMapper, - mapper, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, LTFT_STATUS_UPDATE_TOPIC, - ltftSubmissionHistoryService); + mapper, validator, eventBroadcastService, LTFT_ASSIGNMENT_UPDATE_TOPIC, + LTFT_STATUS_UPDATE_TOPIC, ltftSubmissionHistoryService); LtftFormDto dtoToSave = LtftFormDto.builder() .traineeTisId(TRAINEE_ID) From 1424551822e077b211e4558afb927f252e2ac9fe Mon Sep 17 00:00:00 2001 From: Andy Dingley Date: Fri, 23 Jan 2026 17:36:18 +0000 Subject: [PATCH 3/5] fix: revision increment after LTFT patch The form revision is not correctly incremented when applying an LTFT patch, both the status and form root must have the incremented revision set. TIS21-8201 --- .../forms/api/AdminLtftResourceIntegrationTest.java | 3 ++- .../hee/tis/trainee/forms/service/LtftService.java | 13 +++++++------ .../tis/trainee/forms/service/LtftServiceTest.java | 1 + 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java b/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java index 90271cd3..922aee7f 100644 --- a/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java +++ b/src/integrationTest/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResourceIntegrationTest.java @@ -454,8 +454,9 @@ void shouldReturnPatchedFormWhenLtftDoesMatchDbc() throws Exception { .contentType(APPLICATION_JSON) .content(formPatch)) .andExpect(status().isOk()) - .andExpect(jsonPath("$.id", is(form.getId()))) + .andExpect(jsonPath("$.id", is(form.getId().toString()))) .andExpect(jsonPath("$.formRef", is(form.getFormRef()))) + .andExpect(jsonPath("$.revision", is(1))) .andExpect(jsonPath("$.change.startDate", is("1970-01-01"))) .andExpect(jsonPath("$.change.wte", is(0.5))) .andExpect(jsonPath("$.status.current.state", is(SUBMITTED.toString()))) diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java index 31b9f51e..72f79343 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java @@ -251,8 +251,9 @@ public Optional applyAdminPatch(UUID formId, FormPatchDto formPatch .email(adminIdentity.getEmail()) .role(adminIdentity.getRole()) .build(); + ltft.setRevision(ltft.getRevision() + 1); ltft.setLifecycleState(ltft.getLifecycleState(), statusDetail, modifiedBy, - ltft.getRevision() + 1); + ltft.getRevision()); ltft = ltftFormRepository.save(ltft); ltftSubmissionHistoryService.takeSnapshot(ltft); @@ -401,7 +402,7 @@ public Optional updateLtftForm(UUID formId, LtftFormDto dto) { * * @param formId The id of the LTFT form to delete. * @return Optional empty if the form was not found, true if the form was deleted, or false if it - * was not in a permitted state to delete. + * was not in a permitted state to delete. */ public Optional deleteLtftForm(UUID formId) { String traineeId = traineeIdentity.getTraineeId(); @@ -441,7 +442,7 @@ public Optional submitLtftForm(UUID formId, LftfStatusInfoDetailDto * @param formId The id of the LTFT form to unsubmit. * @param detail The status detail for the unsubmission. * @return The DTO of the unsubmitted form, or empty if form not found or could not be - * unsubmitted. + * unsubmitted. */ public Optional unsubmitLtftForm(UUID formId, LftfStatusInfoDetailDto detail) { return changeLtftFormState(formId, detail, UNSUBMITTED); @@ -465,7 +466,7 @@ public Optional withdrawLtftForm(UUID formId, LftfStatusInfoDetailD * @param detail The status detail for the change. * @param targetState The state to change to. * @return The DTO of the form after the state change, or empty if form not found or could not be - * changed to the target state. + * changed to the target state. */ protected Optional changeLtftFormState(UUID formId, LftfStatusInfoDetailDto detail, LifecycleState targetState) { @@ -495,7 +496,7 @@ protected Optional changeLtftFormState(UUID formId, LftfStatusInfoD * @param formId The ID of the LTFT application. * @param admin The admin to assign to the application. * @return The updated LTFT, empty if the form did not exist or did not belong to the admin's - * local office. + * local office. */ public Optional assignAdmin(UUID formId, PersonDto admin) { log.info("Assigning admin {} to LTFT form {}", admin.email(), formId); @@ -583,7 +584,7 @@ public Optional updateTpdNotificationStatus(UUID formId, St * @param state The new state. * @param detail A detailed reason for the change, may be null. * @return The updated LTFT application, empty if the form did not exist or did not belong to the - * admin's local office. + * admin's local office. * @throws MethodArgumentNotValidException If the state transition is not allowed. */ public Optional updateStatusAsAdmin(UUID formId, LifecycleState state, diff --git a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java index fb5df92b..eb0dd352 100644 --- a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java +++ b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java @@ -1612,6 +1612,7 @@ void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { assertThat("Unexpected form optional.", optionalForm.isPresent(), is(true)); LtftFormDto patchedForm = optionalForm.get(); + assertThat("Unexpected revision.", patchedForm.revision(), is(2)); assertThat("Unexpected form ref.", patchedForm.personalDetails().email(), is("new@example.com")); assertThat("Unexpected status history count.", patchedForm.status().history(), hasSize(2)); From aa49317f9f5a01404ed0b4966c2046d4df445dba Mon Sep 17 00:00:00 2001 From: Andy Dingley Date: Fri, 23 Jan 2026 18:05:03 +0000 Subject: [PATCH 4/5] fix: skip modify LTFT if patched content unchanged When the applied patch does not actually change the content e.g. it sets the same values, or the patch is received multiple times, then the application should not be revised or snapshotted. TIS21-8201 TIS21-8219 TIS21-8212 --- .../trainee/forms/dto/FormPatchResultDto.java | 36 ++++++++++++ .../trainee/forms/service/LtftService.java | 48 +++++++++------- .../forms/service/LtftServiceTest.java | 56 ++++++++++++++++++- 3 files changed, 118 insertions(+), 22 deletions(-) create mode 100644 src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchResultDto.java diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchResultDto.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchResultDto.java new file mode 100644 index 00000000..ef6f1ff3 --- /dev/null +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/dto/FormPatchResultDto.java @@ -0,0 +1,36 @@ +/* + * The MIT License (MIT) + * + * Copyright 2026 Crown Copyright (Health Education England) + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and + * associated documentation files (the "Software"), to deal in the Software without restriction, + * including without limitation the rights to use, copy, modify, merge, publish, distribute, + * sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or + * substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT + * NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + * + */ + +package uk.nhs.hee.tis.trainee.forms.dto; + +import uk.nhs.hee.tis.trainee.forms.model.content.FormContent; + +/** + * A result from a {@link FormPatchDto} triggered update. + * + * @param patchedContent The content after patching. + * @param changed Whether the patch actually modified the content. + * @param The type of {@link FormContent} being patched. + */ +public record FormPatchResultDto(T patchedContent, boolean changed) { + +} diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java index 72f79343..98b955f2 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java @@ -62,6 +62,7 @@ import org.springframework.web.bind.MethodArgumentNotValidException; import uk.nhs.hee.tis.trainee.forms.dto.FeaturesDto.FormFeatures.LtftFeatures; import uk.nhs.hee.tis.trainee.forms.dto.FormPatchDto; +import uk.nhs.hee.tis.trainee.forms.dto.FormPatchResultDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftAdminSummaryDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto.StatusDto.LftfStatusInfoDetailDto; @@ -237,25 +238,29 @@ public Optional applyAdminPatch(UUID formId, FormPatchDto formPatch .map(ltft -> { try { // Patch the content and copy it back in to avoid changes to read-only fields. - LtftContent ltftContent = patchLtftContent(ltft, formPatch.patch()); - ltft.setContent(ltftContent); - - StatusDetail statusDetail = StatusDetail.builder() - .reason(formPatch.reason()) - .message(formPatch.message()) - .build(); - - // An admin patch is considered a shortcut of the un-submit -> re-submit revision flow. - Person modifiedBy = Person.builder() - .name(adminIdentity.getName()) - .email(adminIdentity.getEmail()) - .role(adminIdentity.getRole()) - .build(); - ltft.setRevision(ltft.getRevision() + 1); - ltft.setLifecycleState(ltft.getLifecycleState(), statusDetail, modifiedBy, - ltft.getRevision()); - ltft = ltftFormRepository.save(ltft); - ltftSubmissionHistoryService.takeSnapshot(ltft); + FormPatchResultDto patchResult = patchLtftContent(ltft, formPatch.patch()); + + // Do not increment revision or snapshot if no changes made to the content. + if (patchResult.changed()) { + ltft.setContent(patchResult.patchedContent()); + + StatusDetail statusDetail = StatusDetail.builder() + .reason(formPatch.reason()) + .message(formPatch.message()) + .build(); + + // An admin patch is considered a shortcut of the un-submit -> re-submit revision flow. + Person modifiedBy = Person.builder() + .name(adminIdentity.getName()) + .email(adminIdentity.getEmail()) + .role(adminIdentity.getRole()) + .build(); + ltft.setRevision(ltft.getRevision() + 1); + ltft.setLifecycleState(ltft.getLifecycleState(), statusDetail, modifiedBy, + ltft.getRevision()); + ltft = ltftFormRepository.save(ltft); + ltftSubmissionHistoryService.takeSnapshot(ltft); + } return mapper.toDto(ltft); } catch (JsonPatchException | JsonProcessingException e) { @@ -274,7 +279,7 @@ public Optional applyAdminPatch(UUID formId, FormPatchDto formPatch * @throws JsonPatchException If the patch could not be applied. * @throws JsonProcessingException If the patch creates an invalid form. */ - private LtftContent patchLtftContent(LtftForm ltft, JsonPatch patch) + private FormPatchResultDto patchLtftContent(LtftForm ltft, JsonPatch patch) throws JsonPatchException, JsonProcessingException { // Convert the entity to DTO for patching, as the client will base paths on the public DTO. LtftFormDto dto = mapper.toDto(ltft); @@ -290,7 +295,8 @@ private LtftContent patchLtftContent(LtftForm ltft, JsonPatch patch) } // The content is returned to avoid any unexpected changes to managed/read-only fields. - return mapper.toEntity(patchedDto).getContent(); + LtftContent patchedContent = mapper.toEntity(patchedDto).getContent(); + return new FormPatchResultDto<>(patchedContent, !dto.equals(patchedDto)); } /** diff --git a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java index eb0dd352..3448f518 100644 --- a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java +++ b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java @@ -1612,8 +1612,10 @@ void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { assertThat("Unexpected form optional.", optionalForm.isPresent(), is(true)); LtftFormDto patchedForm = optionalForm.get(); + assertThat("Unexpected form ID.", patchedForm.id(), is(ID)); assertThat("Unexpected revision.", patchedForm.revision(), is(2)); - assertThat("Unexpected form ref.", patchedForm.personalDetails().email(), is("new@example.com")); + assertThat("Unexpected form ref.", patchedForm.personalDetails().email(), + is("new@example.com")); assertThat("Unexpected status history count.", patchedForm.status().history(), hasSize(2)); StatusInfoDto current = patchedForm.status().current(); @@ -1667,6 +1669,58 @@ void shouldSnapshotPatchedFormWhenFormSubmitted() throws IOException { verify(ltftSubmissionHistoryService).takeSnapshot(any()); } + @Test + void shouldReturnUnpatchedFormWhenPatchMakesNoChanges() throws IOException { + LtftForm entity = new LtftForm(); + entity.setId(ID); + entity.setContent(LtftContent.builder() + .change(CctChange.builder() + .wte(0.8) + .build()) + .build()); + entity.setRevision(1); + entity.setLifecycleState(SUBMITTED); + + when(repository + .findByIdAndStatus_Current_StateNotInAndContent_ProgrammeMembership_DesignatedBodyCodeIn( + any(), any(), any())).thenReturn(Optional.of(entity)); + + JsonNode patchNode = jsonMapper.readTree(""" + [ + { + "op": "replace", + "path": "/change/wte", + "value": 0.8 + } + ] + """); + FormPatchDto formPatch = new FormPatchDto(JsonPatch.fromJson(patchNode), "reason1", "message1"); + + Optional optionalForm = service.applyAdminPatch(ID, formPatch); + + assertThat("Unexpected form optional.", optionalForm.isPresent(), is(true)); + + LtftFormDto patchedForm = optionalForm.get(); + assertThat("Unexpected form ID.", patchedForm.id(), is(ID)); + assertThat("Unexpected revision.", patchedForm.revision(), is(1)); + assertThat("Unexpected status history count.", patchedForm.status().history(), hasSize(1)); + + StatusInfoDto current = patchedForm.status().current(); + assertThat("Unexpected current revision.", current.revision(), is(1)); + assertThat("Unexpected current state.", current.state(), is(SUBMITTED)); + assertThat("Unexpected current status reason.", current.detail().reason(), nullValue()); + assertThat("Unexpected current status message.", current.detail().message(), nullValue()); + + StatusInfoDto history = patchedForm.status().history().get(0); + assertThat("Unexpected original revision.", history.revision(), is(1)); + assertThat("Unexpected original state.", history.state(), is(SUBMITTED)); + assertThat("Unexpected original status reason.", history.detail().reason(), nullValue()); + assertThat("Unexpected original status message.", history.detail().message(), nullValue()); + + verify(repository, never()).save(any()); + verifyNoInteractions(ltftSubmissionHistoryService); + } + @Test void shouldReturnEmptyAssigningAdminWhenFormNotFound() { when(repository.findByIdAndContent_ProgrammeMembership_DesignatedBodyCodeIn( From 6f2b3938d0ce01e3d82003bf35609e618f145390 Mon Sep 17 00:00:00 2001 From: Andy Dingley Date: Fri, 23 Jan 2026 18:15:44 +0000 Subject: [PATCH 5/5] docs: add missing javadoc Add some missing javadoc and format some code which flagged up code smells. TIS21-8201 --- .../trainee/forms/api/AdminLtftResource.java | 32 +++++++++++++------ .../trainee/forms/service/LtftService.java | 10 +++--- .../forms/service/LtftServiceTest.java | 2 +- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java index 66b20fc9..1a8523f3 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/api/AdminLtftResource.java @@ -26,7 +26,6 @@ import static uk.nhs.hee.tis.trainee.forms.dto.enumeration.LifecycleState.UNSUBMITTED; import com.amazonaws.xray.spring.aop.XRayEnabled; -import com.fasterxml.jackson.annotation.JsonView; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; @@ -51,7 +50,6 @@ import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.validation.BeanPropertyBindingResult; import org.springframework.validation.FieldError; -import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PatchMapping; @@ -66,8 +64,6 @@ import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto; import uk.nhs.hee.tis.trainee.forms.dto.LtftFormDto.StatusDto.LftfStatusInfoDetailDto; import uk.nhs.hee.tis.trainee.forms.dto.PersonDto; -import uk.nhs.hee.tis.trainee.forms.dto.validation.Update; -import uk.nhs.hee.tis.trainee.forms.dto.views.Trainee; import uk.nhs.hee.tis.trainee.forms.service.LtftService; import uk.nhs.hee.tis.trainee.forms.service.PdfService; @@ -85,6 +81,13 @@ public class AdminLtftResource { private final PdfService pdfService; private final ObjectMapper objectMapper; + /** + * Construct the controller. + * + * @param service The LTFT service for accessing LTFT functionality. + * @param pdfService The PDF service for generating PDFs. + * @param objectMapper The mapper for handling JSON conversion. + */ public AdminLtftResource(LtftService service, PdfService pdfService, ObjectMapper objectMapper) { this.service = service; this.pdfService = pdfService; @@ -158,6 +161,14 @@ ResponseEntity getLtftAdminDetailPdf(@PathVariable UUID id) { return ResponseEntity.notFound().build(); } + /** + * Patch the contents of an LTFT with a particular ID associated with the admin's local office. + * + * @param id The ID of the form. + * @param formPatch The patch to apply. + * @return The updated form DTO. + * @throws MethodArgumentNotValidException If the patch was not valid. + */ @PatchMapping("/{id}") public ResponseEntity patchLtft(@PathVariable UUID id, @Valid @RequestBody FormPatchDto formPatch) @@ -167,7 +178,8 @@ public ResponseEntity patchLtft(@PathVariable UUID id, "formPatch"); if (patchJson.isEmpty()) { - validationResult.addError(new FieldError("FormPatchDto", "patch", "must not be empty")); + validationResult.addError( + new FieldError(FormPatchDto.class.getSimpleName(), "patch", "must not be empty")); } // TODO: validate patchability generically in a central place (e.g. annotations). @@ -180,13 +192,15 @@ public ResponseEntity patchLtft(@PathVariable UUID id, String path = operation.get("path").asText(); if (!Objects.equals(op, "replace")) { - validationResult.addError(new FieldError("FormPatchDto", "patch.%s.op".formatted(opIndex), - "only 'replace' supported")); + validationResult.addError( + new FieldError(FormPatchDto.class.getSimpleName(), "patch.%s.op".formatted(opIndex), + "only 'replace' supported")); } if (!allowedPaths.contains(path)) { - validationResult.addError(new FieldError("FormPatchDto", "patch.%s.path".formatted(opIndex), - "user not authorized to update '%s'".formatted(path))); + validationResult.addError( + new FieldError(FormPatchDto.class.getSimpleName(), "patch.%s.path".formatted(opIndex), + "user not authorized to update '%s'".formatted(path))); } } diff --git a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java index 98b955f2..c9d218bd 100644 --- a/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java +++ b/src/main/java/uk/nhs/hee/tis/trainee/forms/service/LtftService.java @@ -408,7 +408,7 @@ public Optional updateLtftForm(UUID formId, LtftFormDto dto) { * * @param formId The id of the LTFT form to delete. * @return Optional empty if the form was not found, true if the form was deleted, or false if it - * was not in a permitted state to delete. + * was not in a permitted state to delete. */ public Optional deleteLtftForm(UUID formId) { String traineeId = traineeIdentity.getTraineeId(); @@ -448,7 +448,7 @@ public Optional submitLtftForm(UUID formId, LftfStatusInfoDetailDto * @param formId The id of the LTFT form to unsubmit. * @param detail The status detail for the unsubmission. * @return The DTO of the unsubmitted form, or empty if form not found or could not be - * unsubmitted. + * unsubmitted. */ public Optional unsubmitLtftForm(UUID formId, LftfStatusInfoDetailDto detail) { return changeLtftFormState(formId, detail, UNSUBMITTED); @@ -472,7 +472,7 @@ public Optional withdrawLtftForm(UUID formId, LftfStatusInfoDetailD * @param detail The status detail for the change. * @param targetState The state to change to. * @return The DTO of the form after the state change, or empty if form not found or could not be - * changed to the target state. + * changed to the target state. */ protected Optional changeLtftFormState(UUID formId, LftfStatusInfoDetailDto detail, LifecycleState targetState) { @@ -502,7 +502,7 @@ protected Optional changeLtftFormState(UUID formId, LftfStatusInfoD * @param formId The ID of the LTFT application. * @param admin The admin to assign to the application. * @return The updated LTFT, empty if the form did not exist or did not belong to the admin's - * local office. + * local office. */ public Optional assignAdmin(UUID formId, PersonDto admin) { log.info("Assigning admin {} to LTFT form {}", admin.email(), formId); @@ -590,7 +590,7 @@ public Optional updateTpdNotificationStatus(UUID formId, St * @param state The new state. * @param detail A detailed reason for the change, may be null. * @return The updated LTFT application, empty if the form did not exist or did not belong to the - * admin's local office. + * admin's local office. * @throws MethodArgumentNotValidException If the state transition is not allowed. */ public Optional updateStatusAsAdmin(UUID formId, LifecycleState state, diff --git a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java index 3448f518..89a3e077 100644 --- a/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java +++ b/src/test/java/uk/nhs/hee/tis/trainee/forms/service/LtftServiceTest.java @@ -1614,7 +1614,7 @@ void shouldReturnPatchedFormWhenFormSubmitted() throws IOException { LtftFormDto patchedForm = optionalForm.get(); assertThat("Unexpected form ID.", patchedForm.id(), is(ID)); assertThat("Unexpected revision.", patchedForm.revision(), is(2)); - assertThat("Unexpected form ref.", patchedForm.personalDetails().email(), + assertThat("Unexpected email.", patchedForm.personalDetails().email(), is("new@example.com")); assertThat("Unexpected status history count.", patchedForm.status().history(), hasSize(2));