Skip to content

Create endpoint for resending an EhrExtract#990

Merged
adrianclay merged 28 commits intomainfrom
NIAD-3234-create-endpoint
Nov 26, 2024
Merged

Create endpoint for resending an EhrExtract#990
adrianclay merged 28 commits intomainfrom
NIAD-3234-create-endpoint

Conversation

@adrianclay
Copy link
Copy Markdown
Contributor

@adrianclay adrianclay commented Nov 12, 2024

What

Please include a summary of the changes and the related issue

Why

Please include details of the reasoning for these changes

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Internal change (non-breaking change with no effect on the functionality affecting end users)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the Changelog with details of my change in the UNRELEASED section if this change will affect end users

@adrianclay adrianclay force-pushed the NIAD-3234-create-endpoint branch from f2d4dd5 to 24da3bb Compare November 12, 2024 13:56
@ORybak5 ORybak5 marked this pull request as ready for review November 21, 2024 15:59
Comment thread service/src/main/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendController.java Outdated
Comment thread service/src/main/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendController.java Outdated
Comment thread service/src/main/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendController.java Outdated
Comment thread service/src/test/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendControllerTest.java Outdated
Comment thread service/src/test/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendControllerTest.java Outdated
String operationOutcomeUrl = rootNode.path("meta").path("profile").get(0).asText();

assertEquals(INVALID_IDENTIFIER_VALUE, code);
assertEquals(OPERATION_OUTCOME_URL, operationOutcomeUrl);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really we should be using assertJ here for readability (assertThat(code).isEqualTo(INVALID_IDENTIFIER_VALUE))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it is the other way around, I find "assertEquals" to be short, clean and readable.

Comment thread service/src/test/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendControllerTest.java Outdated
Comment thread service/src/test/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendControllerTest.java Outdated
Comment thread service/src/test/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendControllerTest.java Outdated

var response = ehrResendController.scheduleEhrExtractResend(CONVERSATION_ID);

JsonNode rootNode = objectMapper.readTree(response.getBody());
Copy link
Copy Markdown
Collaborator

@MartinWheelerMT MartinWheelerMT Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really not sure what is going on with this? Why are we using a JsonTree and navigating it, rather than just reading the value into an object? The rootNode makes it a whole less readable

var operationOutcome = objectMapper.readValue(response.getBody(), OperationOutcome.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to work with JSON and assess via the prism of JSON as it is what we get in the end when "scheduleEhrExtractResend" returns a result.
They are all comparable technique (assessing JSON or operationOutcome object), just thought it would be more concise to do it that way via JSON because before I had to create an expected boilerplate operationOutcome objects etc.

Comment thread service/src/test/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendControllerTest.java Outdated
Comment thread service/src/test/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendControllerTest.java Outdated

var response = ehrResendController.scheduleEhrExtractResend(CONVERSATION_ID);

JsonNode rootNode = objectMapper.readTree(response.getBody());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, not sure why it is being done this way as not as a concrete type

Copy link
Copy Markdown
Collaborator

@MartinWheelerMT MartinWheelerMT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comments left

Comment thread service/src/main/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendController.java Outdated
Comment thread service/src/main/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendController.java Outdated
Comment thread service/src/main/java/uk/nhs/adaptors/gp2gp/ehr/EhrResendController.java Outdated
new Coding("http://fhir.nhs.net/ValueSet/gpconnect-error-or-warning-code-1", codeableConceptCode, null));
}

private static boolean hasNoErrorsInEhrReceivedAcknowledgement(EhrExtractStatus ehrExtractStatus) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, but this doesn't really feel like it belongs in the Controller. Can refactor this in a later PR though.

@adrianclay adrianclay enabled auto-merge (squash) November 26, 2024 11:47
@adrianclay
Copy link
Copy Markdown
Contributor Author

Paired with @ole4ryb reviewing this change, and we're both happy with this change.

@sonarqubecloud
Copy link
Copy Markdown

@NHSDigital NHSDigital deleted a comment from github-actions Bot Nov 26, 2024
@adrianclay adrianclay merged commit bde8dcc into main Nov 26, 2024
@adrianclay adrianclay deleted the NIAD-3234-create-endpoint branch November 26, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants