PRMP-306 - ARF upload with the same file names is not handled correct…

…ly (#397) * [PRMP-306] Try use uuid instead of filename for matching up files to upload * add client id to test data * remove unused import * improve coverage * fix cypress test for upload path * remove unused const
nhsconnect · Jul 22, 2024 · 21493ef · 21493ef
1 parent bb2b830
commit 21493ef
Show file tree

Hide file tree

Showing 10 changed files with 89 additions and 132 deletions.
diff --git a/...cypress/e2e/0-ndr-core-tests/gp_user_workflows/upload_lloyd_george_is_bsol_gp_admin.cy.js b/...cypress/e2e/0-ndr-core-tests/gp_user_workflows/upload_lloyd_george_is_bsol_gp_admin.cy.js
@@ -54,35 +54,30 @@ const uploadedFileNames = {
 const bucketUrlIdentifer = 'document-store.s3.amazonaws.com';
 const singleFileUsecaseIndex = 0;
 const multiFileUsecaseIndex = 1;
-const fileNames = uploadedFileNames.LG[multiFileUsecaseIndex];
 
-const stubbedResponseMulti = {
-    statusCode: 200,
-    body: {
-        [fileNames[0]]: {
+const mockCreateDocRefHandler = (req) => {
+    const uploadPayload = req.body.content[0].attachment;
+    const clientIds = uploadPayload.map((document) => document.clientId);
+    const responseBody = clientIds.reduce((body, id, currentIndex) => {
+        body[id] = {
             url: 'http://' + bucketUrlIdentifer,
             fields: {
-                key: 'test key',
+                key: `test key ${currentIndex}`,
                 'x-amz-algorithm': 'xxxx-xxxx-SHA256',
                 'x-amz-credential': 'xxxxxxxxxxx/20230904/eu-west-2/s3/aws4_request',
                 'x-amz-date': '20230904T125954Z',
                 'x-amz-security-token': 'xxxxxxxxx',
                 'x-amz-signature': '9xxxxxxxx',
             },
-        },
-        [fileNames[1]]: {
-            url: 'http://' + bucketUrlIdentifer,
-            fields: {
-                key: 'test key',
-                'x-amz-algorithm': 'xxxx-xxxx-SHA256',
-                'x-amz-credential': 'xxxxxxxxxxx/20230904/eu-west-2/s3/aws4_request',
-                'x-amz-date': '20230904T125954Z',
-                'x-amz-security-token': 'xxxxxxxxx',
-                'x-amz-signature': '9xxxxxxxx',
-            },
-        },
-    },
+        };
+        return body;
+    }, {});
+
+    const response = { statusCode: 200, body: responseBody };
+
+    req.reply(response);
 };
+
 describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and patient has no record', () => {
     const beforeEachConfiguration = () => {
         cy.login(Roles.GP_ADMIN);
@@ -120,27 +115,7 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
             `User can upload a single LG file using the "Select files" button and can then view LG record`,
             { tags: 'regression' },
             () => {
-                const fileName = uploadedFileNames.LG[singleFileUsecaseIndex];
-
-                const stubbedResponse = {
-                    statusCode: 200,
-                    body: {
-                        [fileName]: {
-                            url: 'http://' + bucketUrlIdentifer,
-                            fields: {
-                                key: 'test key',
-                                'x-amz-algorithm': 'xxxx-xxxx-SHA256',
-                                'x-amz-credential':
-                                    'xxxxxxxxxxx/20230904/eu-west-2/s3/aws4_request',
-                                'x-amz-date': '20230904T125954Z',
-                                'x-amz-security-token': 'xxxxxxxxx',
-                                'x-amz-signature': '9xxxxxxxx',
-                            },
-                        },
-                    },
-                };
-
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponse);
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler);
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
                     req.reply({
                         statusCode: 204,
@@ -188,10 +163,10 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
         );
 
         it(
-            `User can upload a multiple LG file using the "Select files" button and can then view LG record`,
+            `User can upload multiple LG files using the "Select files" button and can then view LG record`,
             { tags: 'regression' },
             () => {
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponseMulti);
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler);
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
                     req.reply({
                         statusCode: 204,
@@ -235,7 +210,7 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
             `User can upload a multiple LG file using drag and drop and can then view LG record`,
             { tags: 'regression' },
             () => {
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponseMulti);
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler);
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
                     req.reply({
                         statusCode: 204,
@@ -280,27 +255,9 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
             `User can retry failed upload with a single LG file using the "Retry upload" button and can then view LG record`,
             { tags: 'regression' },
             () => {
-                const fileName = uploadedFileNames.LG[singleFileUsecaseIndex];
-
-                const stubbedResponse = {
-                    statusCode: 200,
-                    body: {
-                        [fileName]: {
-                            url: 'http://' + bucketUrlIdentifer,
-                            fields: {
-                                key: 'test key',
-                                'x-amz-algorithm': 'xxxx-xxxx-SHA256',
-                                'x-amz-credential':
-                                    'xxxxxxxxxxx/20230904/eu-west-2/s3/aws4_request',
-                                'x-amz-date': '20230904T125954Z',
-                                'x-amz-security-token': 'xxxxxxxxx',
-                                'x-amz-signature': '9xxxxxxxx',
-                            },
-                        },
-                    },
-                };
-
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponse).as('doc_upload');
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler).as(
+                    'doc_upload',
+                );
 
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
                     req.reply({
@@ -364,7 +321,7 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
             `User can retry a multiple failed LG files using the "Retry all uploads" warning button and can then view LG record`,
             { tags: 'regression' },
             () => {
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponseMulti).as(
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler).as(
                     'doc_upload',
                 );
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
@@ -428,7 +385,7 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
             `User can restart upload LG files journey when document upload fails more than once`,
             { tags: 'regression' },
             () => {
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponseMulti).as(
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler).as(
                     'doc_upload',
                 );
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
@@ -509,27 +466,9 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
             `User's upload journey is stopped if an infected file is detected`,
             { tags: 'regression' },
             () => {
-                const fileName = uploadedFileNames.LG[singleFileUsecaseIndex];
-
-                const stubbedResponse = {
-                    statusCode: 200,
-                    body: {
-                        [fileName]: {
-                            url: 'http://' + bucketUrlIdentifer,
-                            fields: {
-                                key: 'test key',
-                                'x-amz-algorithm': 'xxxx-xxxx-SHA256',
-                                'x-amz-credential':
-                                    'xxxxxxxxxxx/20230904/eu-west-2/s3/aws4_request',
-                                'x-amz-date': '20230904T125954Z',
-                                'x-amz-security-token': 'xxxxxxxxx',
-                                'x-amz-signature': '9xxxxxxxx',
-                            },
-                        },
-                    },
-                };
-
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponse).as('doc_upload');
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler).as(
+                    'doc_upload',
+                );
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
                     req.reply({
                         statusCode: 200,
@@ -566,27 +505,9 @@ describe('GP Workflow: Upload Lloyd George record when user is GP admin BSOL and
             `User is shown an error screen when the upload complete endpoint fails to complete`,
             { tags: 'regression' },
             () => {
-                const fileName = uploadedFileNames.LG[singleFileUsecaseIndex];
-
-                const stubbedResponse = {
-                    statusCode: 200,
-                    body: {
-                        [fileName]: {
-                            url: 'http://' + bucketUrlIdentifer,
-                            fields: {
-                                key: 'test key',
-                                'x-amz-algorithm': 'xxxx-xxxx-SHA256',
-                                'x-amz-credential':
-                                    'xxxxxxxxxxx/20230904/eu-west-2/s3/aws4_request',
-                                'x-amz-date': '20230904T125954Z',
-                                'x-amz-security-token': 'xxxxxxxxx',
-                                'x-amz-signature': '9xxxxxxxx',
-                            },
-                        },
-                    },
-                };
-
-                cy.intercept('POST', '**/DocumentReference**', stubbedResponse).as('doc_upload');
+                cy.intercept('POST', '**/DocumentReference**', mockCreateDocRefHandler).as(
+                    'doc_upload',
+                );
 
                 cy.intercept('POST', '**/' + bucketUrlIdentifer + '**', (req) => {
                     req.reply({

diff --git a/app/src/helpers/requests/uploadDocument.test.ts b/app/src/helpers/requests/uploadDocument.test.ts
@@ -1,5 +1,10 @@
 import axios, { AxiosError } from 'axios';
-import { buildDocument, buildTextFile, buildUploadSession } from '../test/testBuilders';
+import {
+    buildDocument,
+    buildLgFile,
+    buildTextFile,
+    buildUploadSession,
+} from '../test/testBuilders';
 import {
     DOCUMENT_TYPE,
     DOCUMENT_UPLOAD_STATE,
@@ -10,6 +15,7 @@ import {
     updateDocumentState,
     virusScan,
     uploadConfirmation,
+    uploadDocumentToS3,
 } from './uploadDocuments';
 import waitForSeconds from '../utils/waitForSeconds';
 
@@ -37,6 +43,27 @@ describe('[POST] updateDocumentState', () => {
     });
 });
 
+describe('uploadDocumentToS3', () => {
+    const testFile = buildLgFile(1, 3, 'John Doe');
+    const testDocument = buildDocument(
+        testFile,
+        DOCUMENT_UPLOAD_STATE.SELECTED,
+        DOCUMENT_TYPE.LLOYD_GEORGE,
+    );
+    const mockUploadSession = buildUploadSession([testDocument]);
+    const mockSetDocuments = jest.fn();
+
+    it('make POST request to s3 bucket', async () => {
+        await uploadDocumentToS3({
+            setDocuments: mockSetDocuments,
+            uploadSession: mockUploadSession,
+            document: testDocument,
+        });
+
+        expect(mockedAxios.post).toHaveBeenCalledTimes(1);
+    });
+});
+
 describe('virusScanResult', () => {
     const virusScanArgs = {
         documentReference: 'mock_doc_id',

diff --git a/app/src/helpers/requests/uploadDocuments.ts b/app/src/helpers/requests/uploadDocuments.ts
@@ -101,7 +101,7 @@ export const uploadConfirmation = async ({
     uploadSession,
 }: UploadConfirmationArgs) => {
     const fileKeyBuilder = documents.reduce((acc, doc) => {
-        const documentMetadata = uploadSession[doc.file.name];
+        const documentMetadata = uploadSession[doc.id];
         const fileReferenceUUID = getLastURLPath(documentMetadata.fields.key);
         const previousKeys = acc[doc.docType] ?? [];
 
@@ -134,7 +134,7 @@ export const uploadDocumentToS3 = async ({
     uploadSession,
     document,
 }: UploadDocumentsToS3Args) => {
-    const documentMetadata: S3Upload = uploadSession[document.file.name];
+    const documentMetadata: S3Upload = uploadSession[document.id];
     const formData = new FormData();
     const docFields: S3UploadFields = documentMetadata.fields;
     Object.entries(docFields).forEach(([key, value]) => {
@@ -189,6 +189,7 @@ const uploadDocuments = async ({
                     fileName: doc.file.name,
                     contentType: doc.file.type,
                     docType: doc.docType,
+                    clientId: doc.id,
                 })),
             },
         ],

diff --git a/app/src/helpers/test/testBuilders.ts b/app/src/helpers/test/testBuilders.ts
@@ -100,7 +100,7 @@ const buildUploadSession = (documents: Array<UploadDocument>) => {
     return documents.reduce(
         (acc, doc) => ({
             ...acc,
-            [doc.file.name]: {
+            [doc.id]: {
                 fields: {
                     key: `bucket/sub_folder/uuid_for_file(${doc.file.name})`,
                     'x-amz-algorithm': 'string',

diff --git a/app/src/helpers/utils/uploadAndScanDocumentHelpers.ts b/app/src/helpers/utils/uploadAndScanDocumentHelpers.ts
@@ -43,7 +43,7 @@ export const markDocumentsAsUploading = (
     uploadSession: UploadSession,
 ) => {
     return documents.map((doc) => {
-        const documentMetadata = uploadSession[doc.file.name];
+        const documentMetadata = uploadSession[doc.id];
         const documentReference = documentMetadata.fields.key;
         return {
             ...doc,

diff --git a/app/src/pages/lloydGeorgeUploadPage/LloydGeorgeUploadPage.test.tsx b/app/src/pages/lloydGeorgeUploadPage/LloydGeorgeUploadPage.test.tsx
@@ -29,6 +29,7 @@ jest.mock('../../helpers/requests/uploadDocuments');
 jest.mock('../../helpers/hooks/useBaseAPIHeaders');
 jest.mock('../../helpers/hooks/useBaseAPIUrl');
 jest.mock('../../helpers/hooks/usePatient');
+jest.mock('../../helpers/utils/waitForSeconds');
 jest.mock('react-router-dom', () => ({
     ...jest.requireActual('react-router-dom'),
     useNavigate: () => mockNavigate,
@@ -41,7 +42,6 @@ jest.mock('moment', () => {
         return jest.requireActual('moment')(arg);
     };
 });
-jest.mock('../../helpers/utils/waitForSeconds');
 
 const mockedUsePatient = usePatient as jest.Mock;
 const mockUploadDocuments = uploadDocuments as jest.Mock;
@@ -53,14 +53,6 @@ const mockNavigate = jest.fn();
 const mockPatient = buildPatientDetails();
 
 const lgFile = buildLgFile(1, 1, 'John Doe');
-const uploadDocument = {
-    file: lgFile,
-    state: DOCUMENT_UPLOAD_STATE.SELECTED,
-    id: '1',
-    progress: 50,
-    docType: DOCUMENT_TYPE.LLOYD_GEORGE,
-    attempts: 0,
-};
 
 /**
  * Update in other tests
@@ -82,7 +74,7 @@ describe('LloydGeorgeUploadPage', () => {
 
         process.env.REACT_APP_ENVIRONMENT = 'jest';
         mockedUsePatient.mockReturnValue(mockPatient);
-        mockUploadDocuments.mockReturnValue(buildUploadSession([uploadDocument]));
+        mockUploadDocuments.mockImplementation(({ documents }) => buildUploadSession(documents));
     });
     afterEach(() => {
         jest.clearAllMocks();

diff --git a/app/src/pages/uploadDocumentsPage/UploadDocumentsPage.test.tsx b/app/src/pages/uploadDocumentsPage/UploadDocumentsPage.test.tsx
@@ -163,13 +163,9 @@ describe('UploadDocumentsPage', () => {
 
             beforeEach(() => {
                 mockedUseNavigate.mockImplementation((path) => history.push(path));
-
-                const uploadDocs = arfDocuments.map((doc) =>
-                    buildDocument(doc, DOCUMENT_UPLOAD_STATE.SELECTED, DOCUMENT_TYPE.ARF),
-                );
-                const uploadSession = buildUploadSession(uploadDocs);
-
-                mockUploadDocuments.mockResolvedValue(uploadSession);
+                mockUploadDocuments.mockImplementation(({ documents }) => {
+                    return buildUploadSession(documents);
+                });
                 mockS3Upload.mockResolvedValue(successResponse);
             });
 

diff --git a/lambdas/models/nhs_document_reference.py b/lambdas/models/nhs_document_reference.py
@@ -14,6 +14,7 @@ class UploadRequestDocument(BaseModel):
     fileName: str
     contentType: str
     docType: SupportedDocumentTypes
+    clientId: str
 
 
 class NHSDocumentReference:

diff --git a/lambdas/services/create_document_reference_service.py b/lambdas/services/create_document_reference_service.py
@@ -90,8 +90,8 @@ def create_document_reference_request(
                             400, LambdaError.CreateDocInvalidType
                         )
 
-                url_responses[document_reference.file_name] = (
-                    self.prepare_pre_signed_url(document_reference)
+                url_responses[validated_doc.clientId] = self.prepare_pre_signed_url(
+                    document_reference
                 )
 
             if lg_documents: