PRMT-3439 - Multiple EHR requests for the same EHR from the same GP #23

joefong-nhs · 2023-06-19T10:49:50Z

Add an e2e test for multiple EHR request from the same GP
Amend the getAllMessageContaining method to account for that SQS may not return all messages in one go, and may return same message more than once on repeated polling
Add a private method to put a small ehr of given NHS number into EHR repo

…ot exist on queue

… test messages

…5 template file

…and the other for unknown NHS number

… sec, unify the way to generate trigger message in two tests

…obvious that it is for testing

…h unknown conversation id

…ystem printf

…st naming

…class)

…seString to TestUtils

…me GP

… send the same message to consumer more than once

…efore triggering EHR OUT

MohammadIqbalAD-NHS · 2023-06-26T10:50:15Z

src/test/java/uk/nhs/prm/deduction/e2e/tests/Patient.java

@@ -6,6 +6,7 @@ public enum Patient {
    SUSPENDED_WITH_EHR_AT_TPP("9693642937"),
    PATIENT_WITH_LARGE_EHR_AT_EMIS_WITH_MOF_SET_TO_REPO_DEV_ODS("9693643038"),
    PATIENT_WITH_LARGE_EHR_AT_EMIS_WITH_MOF_SET_TO_REPO_TEST_ODS("9694181372"),
+    PATIENT_WITH_SMALL_EHR_RECORD_IN_REPO_AND_MOF_SET_TO_TPP("9727018440"),


For the sake of consistency, I suggest removing the word "RECORD" from the enum.

Suggested change

PATIENT_WITH_SMALL_EHR_RECORD_IN_REPO_AND_MOF_SET_TO_TPP("9727018440"),

PATIENT_WITH_SMALL_EHR_IN_REPO_AND_MOF_SET_TO_TPP("9727018440"),

MohammadIqbalAD-NHS · 2023-06-26T10:53:18Z

src/test/java/uk/nhs/prm/deduction/e2e/tests/RepositoryE2ETests.java

+        String smallEhrMessageId = createUuidAsString();
+        String previousGpForTestPatient = "M85019";
+
+        // TODO: Consider to implement a better way of filling details into the EHR files


This 'TODO' needs addressing.

MohammadIqbalAD-NHS · 2023-06-26T10:58:20Z

src/test/java/uk/nhs/prm/deduction/e2e/tests/RepositoryE2ETests.java

+                .withEhrSourceAsRepo(config)
+                .withEhrDestinationGp(Gp2GpSystem.TPP_PTL_INT)
+                .build();
+        int numberOfDuplicatedMessage = 3;


Consider pluralising this variable to 'numberOfDuplicatedMessages'

Suggested change

int numberOfDuplicatedMessage = 3;

int numberOfDuplicatedMessages = 3;

MohammadIqbalAD-NHS · 2023-06-26T11:04:21Z

src/test/java/uk/nhs/prm/deduction/e2e/tests/RepositoryE2ETests.java

+        assertThat(outboundMessages.size()).isEqualTo(1);
+
+        String outboundEhrCore = outboundMessages.get(0).body();
+        assertThat(outboundEhrCore).contains(testPatient.nhsNumber());


I would reuse the variable 'nhsNumberForTest' that had already been assigned the method 'testPatient.nhsNumber()'.

Suggested change

assertThat(outboundEhrCore).contains(testPatient.nhsNumber());

assertThat(outboundEhrCore).contains(nhsNumberForTest);

Thank you! I have addressed the PR comments

…-e2e-tests/pull/21\#discussion_r1252154140

martin-nhs · 2023-07-05T09:26:43Z

src/test/java/uk/nhs/prm/deduction/e2e/utility/TestUtils.java

@@ -161,4 +162,12 @@ public static LargeEhrTestFiles prepareTestFilesForLargeEhr(

        return new LargeEhrTestFiles(largeEhrCore, largeEhrFragment1, largeEhrFragment2, ehrRequest, continueRequest);
    }
+
+    public static String createUuidAsString() {


Suggested change

public static String createUuidAsString() {

public static String createUUIDAsString() {

Could we capitalise UUID, please?

martin-nhs · 2023-07-05T09:28:48Z

src/test/java/uk/nhs/prm/deduction/e2e/TestConfiguration.java

@@ -181,6 +181,10 @@ public String getEhrRepoUrl() {
        return String.format("https://ehr-repo.%s.patient-deductions.nhs.uk/", getEnvSuffix());
    }

+    public String getEhrOutUrl() {
+        return String.format("https://ehr-out-service.%s.patient-deductions.nhs.uk/", getEnvSuffix());


Could this not just be a class-level member constant?

Suggested change

return String.format("https://ehr-out-service.%s.patient-deductions.nhs.uk/", getEnvSuffix());

public static String EHR_OUT_URL = String.format("https://ehr-out-service.%s.patient-deductions.nhs.uk/", getEnvSuffix());

Additionally, reflect throughout the URLs :)

Thank you!
I have addressed this by changing the urls into static constants and updating the references in the new tests that we written.

There are some references to those getXXXUrl methods in other test suites which is not in the scope of this ticket, so I added a TODO comment to address those after we finish moving all packages to main.

martin-nhs · 2023-07-05T09:36:42Z

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java

+    public String conversationId() {
+        return getUUIDAsUpperCaseString(conversationId);
+    }
+
+    public String messageId() {
+        return getUUIDAsUpperCaseString(messageId);
+    }


Are these really required? I would say that if all the function does is return a single UUID in uppercase - could we not just use:

conversationId.toString().toUpperCase();

messageId.toString().toUpperCase();

Yea, previously we used toString().toUpperCase() here, but we extracted that to a static function as suggested by a PR comment in PRMT-3413, so probably I am keeping it like this for now.

martin-nhs · 2023-07-05T09:43:19Z

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java

+    private UUID conversationId;
+    private UUID messageId;
+    private String sourceGpOds;
+    private String destinationGpOds;
+    private String sourceGpAsid;
+    private String destinationGpAsid;


These can be final :D

Suggested change

private UUID conversationId;

private UUID messageId;

private String sourceGpOds;

private String destinationGpOds;

private String sourceGpAsid;

private String destinationGpAsid;

private UUID conversationId;

private final UUID messageId;

private final String sourceGpOds;

private final String destinationGpOds;

private final String sourceGpAsid;

private final String destinationGpAsid;

Yes, comment addressed

martin-nhs · 2023-07-05T09:44:14Z

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java

+    public ContinueRequestMessage(UUID conversationId, UUID messageId, String sourceGpOds, String destinationGpOds, String sourceGpAsid, String destinationGpAsid) {
+        this.conversationId = conversationId;
+        this.messageId = messageId;
+        this.sourceGpOds = sourceGpOds;
+        this.destinationGpOds = destinationGpOds;
+        this.sourceGpAsid = sourceGpAsid;
+        this.destinationGpAsid = destinationGpAsid;
+    }


Please leverage Lombok here in favour of adding the following annotation to the class level, in favour of this constructor - just removes some boilerplate stuff :)

@AllArgsConstructor

Thanks! It's so good to know this useful annotation! Boilerplate gone now :D

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java

martin-nhs · 2023-07-05T09:55:19Z

src/test/java/uk/nhs/prm/deduction/e2e/models/EhrRequestMessage.java

+    public EhrRequestMessage(String nhsNumber, String sourceGpOds, String destinationGpOds, String sourceGpAsid, String destinationGpAsid, UUID conversationId, UUID messageId) {
+        this.nhsNumber = nhsNumber;
+        this.sourceGpOds = sourceGpOds;
+        this.destinationGpOds = destinationGpOds;
+        this.sourceGpAsid = sourceGpAsid;
+        this.destinationGpAsid = destinationGpAsid;
+        this.conversationId = conversationId;
+        this.messageId = messageId;
+    }


Same as previously mentioned, please use Lombok's @AllArgsConstructor in favour of the manually defined constructor!

Sure! Replaced boilerplate with @ AllArgsConstructor here as well

martin-nhs · 2023-07-05T09:55:39Z

src/test/java/uk/nhs/prm/deduction/e2e/models/EhrRequestMessage.java

+    public String conversationId() {
+        return getUUIDAsUpperCaseString(conversationId);
+    }
+
+    public String messageId() {
+        return getUUIDAsUpperCaseString(messageId);
+    }


Same as above, do we need these?

martin-nhs · 2023-07-05T09:55:55Z

src/test/java/uk/nhs/prm/deduction/e2e/models/EhrRequestMessage.java

+        this.messageId = messageId;
+    }
+
+    public String toJsonString() {


Same as above again! :D

Suggested change

public String toJsonString() {

public String getJsonString() {

martin-nhs · 2023-07-05T09:57:19Z

src/test/java/uk/nhs/prm/deduction/e2e/queue/QueueMessageHelper.java

@@ -29,7 +30,7 @@ public void deleteAllMessages() {
        log(String.format("Trying to delete all the messages on : %s", this.queueUri));
        try {
            thinlyWrappedSqsClient.deleteAllMessages(queueUri);
-        } catch (Exception e){
+        } catch (Exception e) {


Can we find what exception is being thrown here and be explicit, such as IllegalArgumentException :) Additionally, please rename e in favour of exception.

Suggested change

} catch (Exception e) {

} catch (Exception exception) {

martin-nhs · 2023-07-05T09:59:48Z

Brilliant! You've picked up Java fast 😄

…o-e2e-tests/pull/23\#discussion_r1252823748`

…o-e2e-tests/pull/23\#discussion_r1252843770`

…o-e2e-tests/pull/23\#discussion_r1252845074`

…o-e2e-tests/pull/23\#discussion_r1252861910`

joefong-nhs added 22 commits June 13, 2023 12:18

[PRMT-3413] Add method to queue helper to check that a message does n…

0ff49b1

…ot exist on queue

[PRMT-3413] Add url of ehr out service to test config

b746f75

[PRMT-3413] Add asid code to the Gp2gp system class

c72be7f

[PRMT-3413] Add an EhrRequestMessage class and its builder for making…

0264513

… test messages

[PRMT-3413] Fixed the incoherent conversation ids in RCMR_IN010000UK0…

83d6646

…5 template file

[PRMT-3413] Added two E2E tests, one for unrecognised interaction ID …

73221e1

…and the other for unknown NHS number

[PRMT-3413] Allow health check to wait for 2xx response for at most 5…

e548cd8

… sec, unify the way to generate trigger message in two tests

[PRMT-3413] Change the invalid interactionId used in test to make it …

7266c1c

…obvious that it is for testing

[PRMT-3413] Add test for Erroneous message: COPC continue request wit…

a2972cd

…h unknown conversation id

[PRMT-3413] Bundled test cases into one ParameterizedTest

ebaa402

[PRMT-3413] Change healthcheck class to use LOGGER.info rather than s…

c349de9

…ystem printf

[PRMT-3413] Remove duplicated tests, minor fix on log wordings and te…

e8c1fae

…st naming

[PRMT-3413] Minor fix on wordings

a2f073b

[PRMT-3413] Move the list of known asid codes to a static final field

6e37ee4

[PRMT-3413] Amended a misleading log message

956dfa7

[PRMT-3413] Amend incorrect LOGGER setting in new class

3f7a68d

[PRMT-3413] Address comments (change some final class to just public …

f3cc475

…class)

[PRMT-3413] Address comments and add a static method getUuidAsUpperCa…

8f6eaf9

…seString to TestUtils

[PRMT-3439] Added test for duplicated ehr request message from the sa…

2b3103d

…me GP

[PRMT-3439] Amend getAllMessageContaining to account for that SQS may…

225cdd4

… send the same message to consumer more than once

[PRMT-3439] Add a step before test to put a small EHR into EHR REPO b…

f33e576

…efore triggering EHR OUT

[PRMT-3439] Refactor and add comments

1ecb43b

joefong-nhs marked this pull request as ready for review June 20, 2023 10:26

MohammadIqbalAD-NHS reviewed Jun 26, 2023

View reviewed changes

joefong-nhs added 3 commits June 28, 2023 10:55

[PRMT-3413] Addressed PR comment (getUUIDAsUpperCaseString)

a34a5c9

Merge branch 'PRMT-3413' into PRMT-3439

699e43a

[PRMT-3439] Addressed PR comments

fbcaa75

joefong-nhs added 2 commits July 4, 2023 10:18

[PRMT-3413] Address PR comment

161bc2d

[PRMT-3413] Address PR comment https://github.com/nhsconnect/prm-repo…

ea8bd7c

…-e2e-tests/pull/21\#discussion_r1252154140

martin-nhs reviewed Jul 5, 2023

View reviewed changes

src/test/java/uk/nhs/prm/deduction/e2e/models/ContinueRequestMessage.java Show resolved Hide resolved

martin-nhs reviewed Jul 5, 2023

View reviewed changes

joefong-nhs added 5 commits July 5, 2023 15:15

[PRMT-3439] Address PR comment https://github.com/nhsconnect/prm-rep…

b0a4c3e

…o-e2e-tests/pull/23\#discussion_r1252823748`

[PRMT-3439] Address PR comment https://github.com/nhsconnect/prm-rep…

5cd81b1

…o-e2e-tests/pull/23\#discussion_r1252843770`

[PRMT-3439] Address PR comment https://github.com/nhsconnect/prm-rep…

9491c24

…o-e2e-tests/pull/23\#discussion_r1252845074`

Merge branch 'PRMT-3413' into PRMT-3439

6c7f962

[PRMT-3439] Address PR comment https://github.com/nhsconnect/prm-rep…

ca0349f

…o-e2e-tests/pull/23\#discussion_r1252861910`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRMT-3439 - Multiple EHR requests for the same EHR from the same GP #23

PRMT-3439 - Multiple EHR requests for the same EHR from the same GP #23

joefong-nhs commented Jun 19, 2023

MohammadIqbalAD-NHS Jun 26, 2023

MohammadIqbalAD-NHS Jun 26, 2023

MohammadIqbalAD-NHS Jun 26, 2023

MohammadIqbalAD-NHS Jun 26, 2023

joefong-nhs Jun 28, 2023

martin-nhs Jul 5, 2023

martin-nhs Jul 5, 2023 •

edited

Loading

martin-nhs Jul 5, 2023

joefong-nhs Jul 5, 2023

martin-nhs Jul 5, 2023 •

edited

Loading

joefong-nhs Jul 5, 2023

martin-nhs Jul 5, 2023

joefong-nhs Jul 5, 2023

martin-nhs Jul 5, 2023 •

edited

Loading

joefong-nhs Jul 5, 2023

martin-nhs Jul 5, 2023

joefong-nhs Jul 5, 2023

martin-nhs Jul 5, 2023

martin-nhs Jul 5, 2023

martin-nhs Jul 5, 2023

martin-nhs commented Jul 5, 2023

	PATIENT_WITH_SMALL_EHR_RECORD_IN_REPO_AND_MOF_SET_TO_TPP("9727018440"),
	PATIENT_WITH_SMALL_EHR_IN_REPO_AND_MOF_SET_TO_TPP("9727018440"),

	int numberOfDuplicatedMessage = 3;
	int numberOfDuplicatedMessages = 3;

	assertThat(outboundEhrCore).contains(testPatient.nhsNumber());
	assertThat(outboundEhrCore).contains(nhsNumberForTest);

	public static String createUuidAsString() {
	public static String createUUIDAsString() {

	return String.format("https://ehr-out-service.%s.patient-deductions.nhs.uk/", getEnvSuffix());
	public static String EHR_OUT_URL = String.format("https://ehr-out-service.%s.patient-deductions.nhs.uk/", getEnvSuffix());

	public String toJsonString() {
	public String getJsonString() {

PRMT-3439 - Multiple EHR requests for the same EHR from the same GP #23

Are you sure you want to change the base?

PRMT-3439 - Multiple EHR requests for the same EHR from the same GP #23

Conversation

joefong-nhs commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-nhs Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-nhs Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-nhs Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-nhs commented Jul 5, 2023

martin-nhs Jul 5, 2023 •

edited

Loading

martin-nhs Jul 5, 2023 •

edited

Loading

martin-nhs Jul 5, 2023 •

edited

Loading