Niad 2858: storing negative ack error code in DB #359
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adrianclay
reviewed
Nov 6, 2023
| @@ -94,7 +94,7 @@ public boolean handleMessage(Message message) { | |||
| // Current child try catch blocks do not detect this condition so no failed migration log is added... | |||
| // We are however unlikely to have a payload at this point so cannot send a NACK | |||
| if (conversationId != null && !conversationId.isEmpty()) { | |||
| migrationStatusLogService.addMigrationStatusLog(EHR_GENERAL_PROCESSING_ERROR, conversationId, null); | |||
| migrationStatusLogService.addMigrationStatusLog(EHR_GENERAL_PROCESSING_ERROR, conversationId, null, "99"); | |||
There was a problem hiding this comment.
Another 99 I'm curious about.
| @@ -171,7 +171,7 @@ private void handleMigrationTimeout(PatientMigrationRequest migrationRequest, NA | |||
| LOGGER.error("Error retrieving persist duration: [{}]", e.getMessage()); | |||
| } catch (JsonProcessingException | SAXException | DateTimeParseException | JAXBException e) { | |||
| LOGGER.error("Error parsing inbound message from database"); | |||
| migrationStatusLogService.addMigrationStatusLog(EHR_GENERAL_PROCESSING_ERROR, conversationId, null); | |||
| migrationStatusLogService.addMigrationStatusLog(EHR_GENERAL_PROCESSING_ERROR, conversationId, null, "99"); | |||
There was a problem hiding this comment.
What was the thinking behind "99" in this case. I'm not sure I have an alternative suggestion as to what we could do here, just curious as to the thinking.
| @@ -43,12 +43,12 @@ public boolean prepareAndSendRequest(TransferRequestMessage message) { | |||
| LOGGER.debug(response); | |||
| } catch (WebClientResponseException wcre) { | |||
| LOGGER.error("Received an ERROR response from MHS: [{}]", wcre.getMessage()); | |||
| migrationStatusLogService.addMigrationStatusLog(MigrationStatus.EHR_EXTRACT_REQUEST_ERROR, conversationId, null); | |||
| migrationStatusLogService.addMigrationStatusLog(MigrationStatus.EHR_EXTRACT_REQUEST_ERROR, conversationId, null, "2"); | |||
There was a problem hiding this comment.
"2" is mysterious. I thought GP2GP errors were all 2 digits.
Also, I wonder if it makes sense to refererence an enum of valid GP2GP error codes. That way this line of code would become more understandable.
Update: Looking at the NACKReason enum, I can't see a 2. 🤔
| @@ -41,7 +41,7 @@ public void prepareAndSendRequest(ContinueRequestData data) { | |||
| mhsClientService.send(request); | |||
| } catch (WebClientResponseException webClientResponseException) { | |||
| LOGGER.error("Received an ERROR response from MHS: [{}]", webClientResponseException.getMessage()); | |||
| migrationStatusLogService.addMigrationStatusLog(MigrationStatus.CONTINUE_REQUEST_ERROR, data.getConversationId(), null); | |||
| migrationStatusLogService.addMigrationStatusLog(MigrationStatus.CONTINUE_REQUEST_ERROR, data.getConversationId(), null, "8"); | |||
There was a problem hiding this comment.
Similar comment to the "2" comment below.
| @@ -1280,7 +1280,7 @@ public void When_HandleMessage_With_AttachmentProcessingErrorNotStorageCause_Exp | |||
| assertThat(nackMessageDataCaptor.getValue().getNackCode()).isEqualTo(LARGE_MESSAGE_ATTACHMENTS_NOT_RECEIVED.getCode()); | |||
|
|
|||
| verify(migrationStatusLogService, times(1)) | |||
| .addMigrationStatusLog(LARGE_MESSAGE_ATTACHMENTS_NOT_RECEIVED.getMigrationStatus(), CONVERSATION_ID, null); | |||
| .addMigrationStatusLog(LARGE_MESSAGE_ATTACHMENTS_NOT_RECEIVED.getMigrationStatus(), CONVERSATION_ID, null, "31"); | |||
There was a problem hiding this comment.
What's interesting here is that LARGE_MESSAGE_ATTACHMENTS_NOT_RECEIVED already has the code associated with it.
Perhaps this suggests we should have a new method called .addNackMigrationStatusLog which takes in a NACKReason. Thoughts?
|
|
||
| <changeSet id="14" author="ole4ryb"> | ||
| <addColumn schemaName="public" tableName="migration_status_log"> | ||
| <column name="error_code" type="varchar(255)"> |
There was a problem hiding this comment.
I'm wondering if we should be clear in the name that this is a GP2GP Error Code.
| @@ -150,7 +150,7 @@ public void handleMessage(InboundMessage inboundMessage, String conversationId) | |||
| } else { | |||
| failMigration(conversationId, LARGE_MESSAGE_ATTACHMENTS_NOT_RECEIVED); | |||
| migrationStatusLogService.addMigrationStatusLog( | |||
| LARGE_MESSAGE_ATTACHMENTS_NOT_RECEIVED.getMigrationStatus(), conversationId, null); | |||
| LARGE_MESSAGE_ATTACHMENTS_NOT_RECEIVED.getMigrationStatus(), conversationId, null, "31"); | |||
gpc-api-facade/src/test/java/uk/nhs/adaptors/pss/gpc/util/fhir/OperationOutcomeUtilsTest.java
Show resolved
Hide resolved
adrianclay
approved these changes
Nov 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Capturing negative ack error code in DB
Why
PS Adaptor doesn't record/store original error code sent by an incumbent in case of a failure. There is a requirement by NHS England to send failure details hence there should be some mechanism to capture it in PS Adaptor in order to give an access to such information when required.
Type of change
Please delete options that are not relevant.
Checklist: