DTSPB-4486 Improve Notify error reporting

src/integrationTest/java/uk/gov/hmcts/probate/service/NotificationServiceIT.java

@@ -16,6 +16,7 @@
 import uk.gov.hmcts.probate.config.properties.registries.Registry;
 import uk.gov.hmcts.probate.exception.BadRequestException;
 import uk.gov.hmcts.probate.exception.InvalidEmailException;
+import uk.gov.hmcts.probate.exception.RequestInformationParameterException;
 import uk.gov.hmcts.probate.insights.AppInsights;
 import uk.gov.hmcts.probate.model.ApplicationType;
 import uk.gov.hmcts.probate.model.CaseType;
@@ -2124,7 +2125,7 @@ void verifySendCaveatNocEmail()
 
     @Test
     void throwExceptionSendEmailWhenInvalidPersonalisationExists() {
-        NotificationClientException expectException =  assertThrows(NotificationClientException.class,
+        RequestInformationParameterException expectException =  assertThrows(RequestInformationParameterException.class,
                 () -> notificationService.sendEmail(CASE_STOPPED, markdownLinkCaseData));
         assertEquals(MARKDOWN_ERROR_MESSAGE, expectException.getMessage());
     }
@@ -2140,14 +2141,14 @@ void throwExceptionSendExecutorEmailWhenInvalidPersonalisationExists() {
                         .build())
                 .email("[email protected]")
                 .notification("Yes").build();
-        NotificationClientException expectException =  assertThrows(NotificationClientException.class,
+        RequestInformationParameterException expectException =  assertThrows(RequestInformationParameterException.class,
                 () -> notificationService.sendEmail(CASE_STOPPED_REQUEST_INFORMATION, markdownLinkCaseData));
         assertEquals(MARKDOWN_ERROR_MESSAGE, expectException.getMessage());
     }
 
     @Test
     void throwExceptionSendCaveatEmailWhenInvalidPersonalisationExists() {
-        NotificationClientException expectException =  assertThrows(NotificationClientException.class,
+        RequestInformationParameterException expectException =  assertThrows(RequestInformationParameterException.class,
                 () -> notificationService.sendCaveatEmail(GENERAL_CAVEAT_MESSAGE, markdownLinkCaveatData));
         assertEquals(MARKDOWN_ERROR_MESSAGE, expectException.getMessage());
     }
@@ -2166,7 +2167,7 @@ void throwExceptionSendEmailWithDocumentAttachedWhenInvalidPersonalisationExists
                         .build())
                 .email("[email protected]")
                 .notification("Yes").build();
-        NotificationClientException expectException =  assertThrows(NotificationClientException.class,
+        RequestInformationParameterException expectException =  assertThrows(RequestInformationParameterException.class,
                 () -> notificationService.sendEmailWithDocumentAttached(markdownLinkCaseData,
                         executorsApplyingNotification, REDECLARATION_SOT));
         assertEquals(MARKDOWN_ERROR_MESSAGE, expectException.getMessage());

src/main/java/uk/gov/hmcts/probate/controller/NotificationController.java

@@ -199,7 +199,8 @@ public ResponseEntity<CallbackResponse> sendDocumentReceivedNotification(
     }
 
     @PostMapping(path = "/stopped-information-request")
-    public ResponseEntity<CallbackResponse> informationRequest(@RequestBody CallbackRequest callbackRequest) {
+    public ResponseEntity<CallbackResponse> informationRequest(
+            @RequestBody final CallbackRequest callbackRequest) throws NotificationClientException {
         Optional<UserInfo> caseworkerInfo = userInfoService.getCaseworkerInfo();
         return ResponseEntity.ok(informationRequestService.handleInformationRequest(callbackRequest, caseworkerInfo));
     }

src/main/java/uk/gov/hmcts/probate/exception/RequestInformationParameterException.java

@@ -0,0 +1,10 @@
+package uk.gov.hmcts.probate.exception;
+
+public class RequestInformationParameterException extends BusinessValidationException {
+    private static final String INVALID_PERSONALISATION_ERROR_MESSAGE =
+            "Markdown Link detected in case data, stop sending notification email.";
+
+    public RequestInformationParameterException() {
+        super(INVALID_PERSONALISATION_ERROR_MESSAGE, INVALID_PERSONALISATION_ERROR_MESSAGE);
+    }
+}

src/main/java/uk/gov/hmcts/probate/exception/handler/DefaultExceptionHandler.java

@@ -19,7 +19,6 @@
 import uk.gov.hmcts.probate.model.ccd.ocr.ValidationResponse;
 import uk.gov.hmcts.probate.model.ccd.ocr.ValidationResponseStatus;
 import uk.gov.hmcts.probate.model.ccd.raw.response.CallbackResponse;
-import uk.gov.service.notify.NotificationClientException;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -41,7 +40,6 @@ class DefaultExceptionHandler extends ResponseEntityExceptionHandler {
     public static final String CONNECTION_ERROR = "Connection error";
     public static final String UNAUTHORISED_DATA_EXTRACT_ERROR = "Unauthorised access to Data-Extract error";
 
-
     @ExceptionHandler(BadRequestException.class)
     public ResponseEntity<ErrorResponse> handle(BadRequestException exception) {
 
@@ -93,16 +91,6 @@ public ResponseEntity<ErrorResponse> handle(ConnectionException exception) {
         return new ResponseEntity<>(errorResponse, headers, SERVICE_UNAVAILABLE);
     }
 
-    @ExceptionHandler(value = NotificationClientException.class)
-    public ResponseEntity<ErrorResponse> handle(NotificationClientException exception) {
-        log.warn("Notification service exception", exception);
-        ErrorResponse errorResponse =
-            new ErrorResponse(SERVICE_UNAVAILABLE.value(), CLIENT_ERROR, exception.getMessage());
-        HttpHeaders headers = new HttpHeaders();
-        headers.setContentType(MediaType.APPLICATION_JSON);
-        return new ResponseEntity<>(errorResponse, headers, SERVICE_UNAVAILABLE);
-    }
-
     @ExceptionHandler(value = NotFoundException.class)
     public ResponseEntity<ErrorResponse> handle(NotFoundException exception) {
         log.warn("Not found exception", exception);

src/main/java/uk/gov/hmcts/probate/exception/handler/NotificationClientExceptionHandler.java

@@ -0,0 +1,81 @@
+package uk.gov.hmcts.probate.exception.handler;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import lombok.extern.slf4j.Slf4j;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.http.HttpHeaders;
+import org.springframework.http.MediaType;
+import org.springframework.http.ResponseEntity;
+import org.springframework.web.bind.annotation.ControllerAdvice;
+import org.springframework.web.bind.annotation.ExceptionHandler;
+import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;
+import uk.gov.hmcts.probate.model.ccd.raw.response.CallbackResponse;
+import uk.gov.service.notify.NotificationClientException;
+
+import java.util.ArrayList;
+import java.util.List;
+
+@Slf4j
+@ControllerAdvice
+public class NotificationClientExceptionHandler extends ResponseEntityExceptionHandler {
+    public static final String UNABLE_TO_SEND_EMAIL = "Unable to send email";
+
+    private final ObjectMapper objectMapper;
+
+    @Autowired
+    public NotificationClientExceptionHandler(ObjectMapper objectMapper) {
+        this.objectMapper = objectMapper;
+    }
+
+    @ExceptionHandler(value = NotificationClientException.class)
+    public ResponseEntity<CallbackResponse> handle(NotificationClientException exception) {
+        log.warn("Notification service exception", exception);
+        final List<String> errors = List.copyOf(getNotifyErrors(exception.getMessage()));
+
+        final CallbackResponse errorResponse = CallbackResponse.builder()
+                .errors(errors)
+                .build();
+        HttpHeaders headers = new HttpHeaders();
+        headers.setContentType(MediaType.APPLICATION_JSON);
+
+        return ResponseEntity.ok()
+                .headers(headers)
+                .body(errorResponse);
+    }
+
+    /**
+     * This relies on the behaviour of NotifyClient#performPostRequest passing the connection's error stream as a json
+     * string into the exception message, so is liable to break.
+     */
+    private List<String> getNotifyErrors(final String exMessage) {
+        final String exJson = exMessage.replaceFirst("[^{]*", "");
+
+        final List<String> errors = new ArrayList<>();
+        errors.add(UNABLE_TO_SEND_EMAIL);
+
+        final JsonNode outerJson;
+        try {
+            outerJson = objectMapper.readTree(exJson);
+        } catch (JsonProcessingException e) {
+            return errors;
+        }
+
+        if (!outerJson.isObject()) {
+            return errors;
+        }
+        if (!outerJson.has("errors") || !outerJson.get("errors").isArray()) {
+            return errors;
+        }
+
+        final JsonNode errorsJson = outerJson.get("errors");
+        for (final JsonNode errorJson : errorsJson) {
+            if (errorJson.isObject() && errorJson.has("message") && errorJson.get("message").isTextual()) {
+                final String message = errorJson.get("message").asText();
+                errors.add(message);
+            }
+        }
+        return errors;
+    }
+}

src/main/java/uk/gov/hmcts/probate/service/InformationRequestCorrespondenceService.java

@@ -18,15 +18,9 @@ public class InformationRequestCorrespondenceService {
 
     private final NotificationService notificationService;
 
-    public List<Document> emailInformationRequest(CaseDetails caseDetails) {
-        try {
-            final Document notification = notificationService.sendEmail(CASE_STOPPED_REQUEST_INFORMATION, caseDetails);
-            log.info("Successful response for request for information email for case id {} ", caseDetails.getId());
-            return List.of(notification);
-        } catch (NotificationClientException e) {
-            log.error(e.getMessage());
-            // this feels wrong - do we not want to alert the caller that the email sending has failed?
-            return List.of();
-        }
+    public List<Document> emailInformationRequest(CaseDetails caseDetails) throws NotificationClientException {
+        final Document notification = notificationService.sendEmail(CASE_STOPPED_REQUEST_INFORMATION, caseDetails);
+        log.info("Successful response for request for information email for case id {} ", caseDetails.getId());
+        return List.of(notification);
     }
 }

src/main/java/uk/gov/hmcts/probate/service/InformationRequestService.java

@@ -26,8 +26,9 @@ public class InformationRequestService {
     private final CallbackResponseTransformer callbackResponseTransformer;
     private final EmailAddressNotifyApplicantValidationRule emailAddressNotifyApplicantValidationRule;
 
-    public CallbackResponse handleInformationRequest(CallbackRequest callbackRequest,
-                                                     Optional<UserInfo> caseworkerInfo) {
+    public CallbackResponse handleInformationRequest(
+            final CallbackRequest callbackRequest,
+            final Optional<UserInfo> caseworkerInfo) throws NotificationClientException {
         CaseData caseData = callbackRequest.getCaseDetails().getData();
         CCDData dataForEmailAddress = CCDData.builder()
                 .applicationType(caseData.getApplicationType().name())

src/main/java/uk/gov/hmcts/probate/service/NotificationService.java

@@ -11,6 +11,7 @@
 import uk.gov.hmcts.probate.config.properties.registries.Registry;
 import uk.gov.hmcts.probate.exception.BadRequestException;
 import uk.gov.hmcts.probate.exception.InvalidEmailException;
+import uk.gov.hmcts.probate.exception.RequestInformationParameterException;
 import uk.gov.hmcts.probate.model.ApplicationType;
 import uk.gov.hmcts.probate.model.CaseOrigin;
 import uk.gov.hmcts.probate.model.Constants;
@@ -71,8 +72,6 @@ public class NotificationService {
     private static final String PERSONALISATION_APPLICANT_NAME = "applicant_name";
     private static final String PERSONALISATION_SOT_LINK = "sot_link";
     private static final DateTimeFormatter RELEASE_DATE_FORMAT = DateTimeFormatter.ofPattern("yyyy-MM-dd");
-    private static final String INVALID_PERSONALISATION_ERROR_MESSAGE =
-            "Markdown Link detected in case data, stop sending notification email.";
 
     private final EmailAddresses emailAddresses;
     private final NotificationTemplates notificationTemplates;
@@ -555,7 +554,7 @@ private String removedSolicitorNameForPersonalisation(CaseData caseData) {
 
     CommonNotificationResult doCommonNotificationServiceHandling(
             final Map<String, ?> personalisation,
-            final Long caseId) throws NotificationClientException {
+            final Long caseId) throws RequestInformationParameterException {
         final PersonalisationValidationRule.PersonalisationValidationResult validationResult =
                 personalisationValidationRule.validatePersonalisation(personalisation);
         final Map<String, String> invalidFields = validationResult.invalidFields();
@@ -564,7 +563,7 @@ CommonNotificationResult doCommonNotificationServiceHandling(
         if (!invalidFields.isEmpty()) {
             log.error("Personalisation validation failed for case: {} fields: {}",
                     caseId, invalidFields);
-            throw new NotificationClientException(INVALID_PERSONALISATION_ERROR_MESSAGE);
+            throw new RequestInformationParameterException();
         } else if (!htmlFields.isEmpty()) {
             log.info("Personalisation validation found HTML for case: {} fields: {}",
                     caseId, validationResult.htmlFields());

src/test/java/uk/gov/hmcts/probate/exception/handler/DefaultExceptionHandlerTest.java

@@ -11,16 +11,17 @@
 import uk.gov.hmcts.probate.exception.ConnectionException;
 import uk.gov.hmcts.probate.exception.NotFoundException;
 import uk.gov.hmcts.probate.exception.OCRMappingException;
+import uk.gov.hmcts.probate.exception.RequestInformationParameterException;
 import uk.gov.hmcts.probate.exception.SocketException;
 import uk.gov.hmcts.probate.exception.model.ErrorResponse;
 import uk.gov.hmcts.probate.exception.model.FieldErrorResponse;
 import uk.gov.hmcts.probate.model.ccd.ocr.ValidationResponse;
 import uk.gov.hmcts.probate.model.ccd.raw.response.CallbackResponse;
-import uk.gov.service.notify.NotificationClientException;
 
 import java.util.Arrays;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static org.mockito.MockitoAnnotations.openMocks;
 import static org.springframework.http.HttpStatus.BAD_REQUEST;
@@ -41,9 +42,6 @@ class DefaultExceptionHandlerTest {
     @Mock
     private BadRequestException badRequestException;
 
-    @Mock
-    private NotificationClientException notificationClientException;
-
     @Mock
     private BusinessValidationException businessValidationException;
 
@@ -111,17 +109,6 @@ void shouldHandleMissingPDFDataAsStatusUN() {
         assertEquals(bve2Mock, response.getBody().getFieldErrors().get(1));
     }
 
-    @Test
-    void shouldReturnNotificationClientException() {
-        when(notificationClientException.getMessage()).thenReturn(EXCEPTION_MESSAGE);
-
-        ResponseEntity<ErrorResponse> response = underTest.handle(notificationClientException);
-
-        assertEquals(SERVICE_UNAVAILABLE, response.getStatusCode());
-        assertEquals(DefaultExceptionHandler.CLIENT_ERROR, response.getBody().getError());
-        assertEquals(EXCEPTION_MESSAGE, response.getBody().getMessage());
-    }
-
     @Test
     void shouldReturnBusinessValidationException() {
         when(businessValidationException.getUserMessage()).thenReturn(EXCEPTION_MESSAGE);
@@ -178,4 +165,19 @@ void shouldReturnOCRMappingException() {
         assertEquals(1, response.getBody().getErrors().size());
         assertEquals("Message", response.getBody().getErrors().get(0));
     }
+
+    @Test
+    void shouldReturnMarkdownError() {
+        RequestInformationParameterException ex = mock(RequestInformationParameterException.class);
+        when(ex.getMessage()).thenReturn("");
+        when(ex.getUserMessage()).thenReturn(EXCEPTION_MESSAGE);
+
+        ResponseEntity<CallbackResponse> response = underTest.handle(ex);
+
+        assertEquals(OK, response.getStatusCode(),
+                "Expected HTTP OK from RequestInformationParameterException handler");
+        assertEquals(1, response.getBody().getErrors().size(), "Expected one error");
+        assertEquals(EXCEPTION_MESSAGE, response.getBody().getErrors().get(0),
+                "Expected error to be extracted from exception");
+    }
 }