Prmdr 180 #83

AlexHerbertNHS · 2023-10-10T10:05:26Z

No description provided.

Adding explicit import for proper compiling Fixing test so it can be run by Windows users

finished on the code; need to rework a few tests due to refactoring

Adding more tests, some renaming, some refactoring, some import cleanup

Finished testing the Manifest Dynamo Service

Small cleanup, removing unnecessary comments and imports

Fix for existing tests

More test fixes

Adding final bits for comprehensive unit tests

lambdas/handlers/document_manifest_by_nhs_number_handler.py

lambdas/services/manifest_dynamo_service.py

lambdas/utils/lambda_response.py

Making changes and refactors as per code review

fixes for tests based on code review changes

lambdas/handlers/document_manifest_by_nhs_number_handler.py

lambdas/services/manifest_dynamo_service.py

Changing doctype validation logic and adding test as per code review

joefong-nhs

Look good to me, gave some minor suggestions

joefong-nhs · 2023-10-11T12:41:45Z

lambdas/handlers/document_manifest_by_nhs_number_handler.py

-
-        documents = lg_documents + ds_documents
+        dynamo_service = ManifestDynamoService()
+        documents = dynamo_service.discover_uploaded_documents(nhs_number, doc_type)


do we want this doc_type to be a string or an array?
Because in Readme it says For all docs ["LG", "ARF"], but the variable we pass into discover_uploaded_documents is a str

It's going to be a string from the UI side; I'll update the readme. Apologies for the confusion

joefong-nhs · 2023-10-11T12:44:30Z

lambdas/README.md

+
+PatientID is an NHS Number
+
+This is an array and can be set to the following values:


would it be docType is an array / a string and can be set to the following values: ?

joefong-nhs · 2023-10-11T12:44:48Z

lambdas/README.md

+
+For all docs ["LG", "ARF"]
+
+If the value is not one of the above speified then a 204 will be returned


Would it be:

Suggested change

If the value is not one of the above speified then a 204 will be returned

If the value is not one of the above specified then a 400 Bad Request response will be returned

joefong-nhs · 2023-10-11T12:50:49Z

lambdas/tests/unit/services/test_document_manifest_service.py

+    if is_windows():
+        expected_download_path = (
+            f"{service.temp_downloads_dir}\\{MOCK_DOCUMENTS[0].file_name}"
+        )
+    else:
+        expected_download_path = (
+            f"{service.temp_downloads_dir}/{MOCK_DOCUMENTS[0].file_name}"
+        )


would suggest to use os.path.join(service.temp_downloads_dir, MOCK_DOCUMENTS[0].file_name) here

Smart! Thank you, that check for Windows file system was a bit rubbish :D

joefong-nhs · 2023-10-11T12:58:37Z

lambdas/README.md

+For just Lloyd George docs ["LG"]
+
+For just ARF docs ["ARF"]
+
+For all docs ["LG", "ARF"]


from the implementation and tests, seems to me that options of docType that we support are LG, ARF or both of them connected by a comma
would it be good to give example so that it is clear on what the lambda expect?

For just Lloyd George docs: `/DocumentManifest?docType=LG` For just ARF docs: `/DocumentManifest?docType=ARF` For both LG and ARF docs: `/DocumentManifest?docType=LG,ARF`

Documentation has been updated :)

Changes as per code review

thisusernameisnowtaken and others added 16 commits October 5, 2023 14:52

lloyd george document manifest lambda and test

8360dfe

lloyd george manifest code tidy up

bbcd5c0

Separate dynamoDB repo code into service

b88e24e

Merge branch 'main' into PRMDR-180

5e86907

Merge branch 'main' into prmdr-180

b424aa2

Filter documents by type in the manifest handler

969c008

[PRMDR-270]

e61405f

Adding explicit import for proper compiling Fixing test so it can be run by Windows users

[PRMDR-180]

6ed5c43

finished on the code; need to rework a few tests due to refactoring

PRMDR-180

b62546a

PRMDR-180

ce71612

Adding more tests, some renaming, some refactoring, some import cleanup

PRMDR-180

36d5b1b

Finished testing the Manifest Dynamo Service

PRMDR-180

892eb09

Small cleanup, removing unnecessary comments and imports

Tests

76eab37

PRMDR-180

7b93ad7

Fix for existing tests

PRMDR-180

cbed6c6

More test fixes

PRMDR-180

bda7459

Adding final bits for comprehensive unit tests

AlexHerbertNHS temporarily deployed to development October 10, 2023 10:05 — with GitHub Actions Inactive

NogaNHS reviewed Oct 10, 2023

View reviewed changes

lambdas/handlers/document_manifest_by_nhs_number_handler.py Outdated Show resolved Hide resolved

NogaNHS reviewed Oct 10, 2023

View reviewed changes

lambdas/handlers/document_manifest_by_nhs_number_handler.py Show resolved Hide resolved

NogaNHS reviewed Oct 10, 2023

View reviewed changes

lambdas/services/manifest_dynamo_service.py Outdated Show resolved Hide resolved

NogaNHS reviewed Oct 10, 2023

View reviewed changes

lambdas/utils/lambda_response.py Outdated Show resolved Hide resolved

PRMDR-180

67bedf0

Making changes and refactors as per code review

AlexHerbertNHS temporarily deployed to development October 10, 2023 14:47 — with GitHub Actions Inactive

AlexHerbertNHS had a problem deploying to development October 10, 2023 14:47 — with GitHub Actions Failure

AlexHerbertNHS temporarily deployed to development October 10, 2023 14:47 — with GitHub Actions Inactive

PRMDR-180

a0d0197

fixes for tests based on code review changes

AlexHerbertNHS temporarily deployed to development October 10, 2023 15:19 — with GitHub Actions Inactive

AlexHerbertNHS temporarily deployed to development October 11, 2023 08:35 — with GitHub Actions Inactive

AlexHerbertNHS temporarily deployed to development October 11, 2023 10:01 — with GitHub Actions Inactive

AlexHerbertNHS temporarily deployed to test October 11, 2023 10:02 — with GitHub Actions Inactive

AlexHerbertNHS temporarily deployed to development October 11, 2023 10:02 — with GitHub Actions Inactive

AlexHerbertNHS temporarily deployed to test October 11, 2023 10:02 — with GitHub Actions Inactive

AlexHerbertNHS temporarily deployed to development October 11, 2023 10:02 — with GitHub Actions Inactive

AlexHerbertNHS temporarily deployed to test October 11, 2023 10:02 — with GitHub Actions Inactive

NogaNHS reviewed Oct 11, 2023

View reviewed changes

lambdas/handlers/document_manifest_by_nhs_number_handler.py Show resolved Hide resolved

NogaNHS reviewed Oct 11, 2023

View reviewed changes

lambdas/services/manifest_dynamo_service.py Show resolved Hide resolved

PRMDR-180

0f4882d

Changing doctype validation logic and adding test as per code review

AlexHerbertNHS temporarily deployed to development October 11, 2023 10:59 — with GitHub Actions Inactive

joefong-nhs reviewed Oct 11, 2023

View reviewed changes

AlexHerbertNHS temporarily deployed to development October 12, 2023 09:12 — with GitHub Actions Inactive

PRMDR-180

e16ba88

Changes as per code review

AlexHerbertNHS temporarily deployed to development October 16, 2023 08:54 — with GitHub Actions Inactive

joefong-nhs approved these changes Oct 16, 2023

View reviewed changes

AlexHerbertNHS merged commit d2a43ae into main Oct 16, 2023
16 checks passed

richbyrne-nhs deleted the PRMDR-180 branch November 20, 2023 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prmdr 180 #83

Prmdr 180 #83

AlexHerbertNHS commented Oct 10, 2023

joefong-nhs left a comment

joefong-nhs Oct 11, 2023

AlexHerbertNHS Oct 13, 2023

joefong-nhs Oct 11, 2023 •

edited

Loading

AlexHerbertNHS Oct 16, 2023

joefong-nhs Oct 11, 2023

AlexHerbertNHS Oct 16, 2023

joefong-nhs Oct 11, 2023

AlexHerbertNHS Oct 16, 2023

joefong-nhs Oct 11, 2023

AlexHerbertNHS Oct 16, 2023


		PatientID is an NHS Number

		This is an array and can be set to the following values:


		For all docs ["LG", "ARF"]

		If the value is not one of the above speified then a 204 will be returned

Prmdr 180 #83

Prmdr 180 #83

Conversation

AlexHerbertNHS commented Oct 10, 2023

joefong-nhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joefong-nhs Oct 11, 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

Choose a reason for hiding this comment

joefong-nhs Oct 11, 2023 •

edited

Loading