Merge branch 'master' into VED-788-refactor-ack-backend

Author: JamesW1-NHS

.github/dependabot.yml

@@ -58,10 +58,10 @@ updates:
       - "/filenameprocessor"
       - "/mesh_processor"
       - "/recordprocessor"
-      - "/redis_sync"
+      - "/lambdas/redis_sync"
       - "/lambdas/id_sync"
+      - "/lambdas/mns_subscription"
       - "/lambdas/shared"
-      - "/mns_subscription"
     schedule:
       interval: "daily"
     open-pull-requests-limit: 1

.github/workflows/sonarcloud.yml

@@ -123,27 +123,27 @@ jobs:
           poetry run coverage xml -o ../mesh_processor-coverage.xml
 
       - name: Run unittest with coverage-mns-subscription
-        working-directory: mns_subscription
-        env:
-          PYTHONPATH: ${{ github.workspace }}/mns_subscription/src:${{ github.workspace }}/mns_subscription/tests
+        working-directory: lambdas/mns_subscription
         id: mns_subscription
+        env:
+          PYTHONPATH: ${{ env.LAMBDA_PATH }}/mns_subscription/src:${{ env.SHARED_PATH }}/src
         continue-on-error: true
         run: |
           poetry install
-          poetry run coverage run -m unittest discover || echo "mns_subscription tests failed" >> ../failed_tests.txt
+          poetry run coverage run -m unittest discover || echo "mns_subscription tests failed" >> ../../failed_tests.txt
           poetry run coverage report -m
-          poetry run coverage xml -o ../mns_subscription-coverage.xml
+          poetry run coverage xml -o ../../mns_subscription-coverage.xml
 
       - name: Run unittest with redis_sync
-        working-directory: redis_sync
+        working-directory: lambdas/redis_sync
         id: redis_sync
         env:
-          PYTHONPATH: ${{ github.workspace }}/redis_sync/src:${{ github.workspace }}/redis_sync/tests
+          PYTHONPATH: ${{ env.LAMBDA_PATH }}/redis_sync/src:${{ env.SHARED_PATH }}/src
         continue-on-error: true
         run: |
-            poetry install
-            poetry run coverage run -m unittest discover || echo "redis_sync tests failed" >> ../failed_tests.txt
-            poetry run coverage xml -o ../redis_sync-coverage.xml
+          poetry install
+          poetry run coverage run --source=src -m unittest discover || echo "redis_sync tests failed" >> ../../failed_tests.txt
+          poetry run coverage xml -o ../../redis_sync-coverage.xml
 
       - name: Run unittest with shared
         working-directory: lambdas/shared
@@ -152,9 +152,9 @@ jobs:
           PYTHONPATH: ${{ env.SHARED_PATH }}/src
         continue-on-error: true
         run: |
-            poetry install
-            poetry run coverage run --rcfile=.coveragerc --source=src -m unittest discover -s tests -p "test_*.py" -v || echo "shared tests failed" >> ../../failed_tests.txt
-            poetry run coverage xml -o ../../shared-coverage.xml
+          poetry install
+          poetry run coverage run --rcfile=.coveragerc --source=src -m unittest discover -s tests -p "test_*.py" -v || echo "shared tests failed" >> ../../failed_tests.txt
+          poetry run coverage xml -o ../../shared-coverage.xml
 
       - name: Run unittest with id_sync
         working-directory: lambdas/id_sync
@@ -163,9 +163,9 @@ jobs:
           PYTHONPATH: ${{ env.LAMBDA_PATH }}/id_sync/src:${{ env.SHARED_PATH }}/src
         continue-on-error: true
         run: |
-            poetry install
-            poetry run coverage run --rcfile=.coveragerc --source=src -m unittest discover || echo "id_sync tests failed" >> ../../failed_tests.txt
-            poetry run coverage xml -o ../../id_sync-coverage.xml
+          poetry install
+          poetry run coverage run --rcfile=.coveragerc --source=src -m unittest discover || echo "id_sync tests failed" >> ../../failed_tests.txt
+          poetry run coverage xml -o ../../id_sync-coverage.xml
 
       - name: Run Test Failure Summary
         id: check_failure

Makefile

@@ -1,6 +1,6 @@
 SHELL=/usr/bin/env bash -euo pipefail
 
-PYTHON_PROJECT_DIRS_WITH_UNIT_TESTS = lambdas/ack_backend backend batch_processor_filter delta_backend filenameprocessor mesh_processor recordprocessor redis_sync lambdas/id_sync lambdas/shared mns_subscription
+PYTHON_PROJECT_DIRS_WITH_UNIT_TESTS = backend batch_processor_filter delta_backend filenameprocessor mesh_processor recordprocessor lambdas/ack_backend lambdas/redis_sync lambdas/id_sync lambdas/mns_subscription lambdas/shared
 PYTHON_PROJECT_DIRS = e2e e2e_batch $(PYTHON_PROJECT_DIRS_WITH_UNIT_TESTS)
 
 #Installs dependencies using poetry.

backend/src/log_structure.py

@@ -42,7 +42,6 @@ def function_info(func):
     @wraps(func)
     def wrapper(*args, **kwargs):
         event = args[0] if args else {}
-        logger.info(f"Event: {event}")
         headers = event.get("headers", {})
         correlation_id = headers.get("X-Correlation-ID", "X-Correlation-ID not passed")
         request_id = headers.get("X-Request-ID", "X-Request-ID not passed")
@@ -64,7 +63,6 @@ def wrapper(*args, **kwargs):
         start = time.time()
         try:
             result = func(*args, **kwargs)
-            logger.info(f"Result:{result}")
             end = time.time()
             log_data["time_taken"] = f"{round(end - start, 5)}s"
             log_data.update(_log_data_from_body(event))

backend/tests/test_log_structure_wrapper.py

@@ -23,6 +23,14 @@ def mock_success_function(_event, _context):
     def mock_function_raises(_event, _context):
         raise ValueError("Test error")
 
+    def extract_all_call_args_for_logger(self, mock_logger) -> list:
+        """Extracts all arguments for logger.*."""
+        return (
+            [ args[0] for args, _ in mock_logger.info.call_args_list ]
+            + [ args[0] for args, _ in mock_logger.warning.call_args_list ]
+            + [ args[0] for args, _ in mock_logger.error.call_args_list ]
+        )
+
     def test_successful_execution(self, mock_logger, mock_firehose_logger):
         # Arrange
         test_correlation = "test_correlation"
@@ -65,6 +73,37 @@ def test_successful_execution(self, mock_logger, mock_firehose_logger):
         self.assertEqual(logged_info['local_id'], '12345^http://test')
         self.assertEqual(logged_info['vaccine_type'], 'FLU')
 
+    def test_successful_execution_pii(self, mock_logger, mock_firehose_logger):
+        """Pass personally identifiable information in an event, and ensure that it is not logged anywhere."""
+        # Arrange
+        test_correlation = "test_correlation"
+        test_request = "test_request"
+        test_supplier = "test_supplier"
+        test_actual_path = "/test"
+        test_resource_path = "/test"
+
+        self.mock_redis_client.hget.return_value = "FLU"
+        wrapped_function = function_info(self.mock_success_function)
+        event = {
+            'headers': {
+                'X-Correlation-ID': test_correlation,
+                'X-Request-ID': test_request,
+                'SupplierSystem': test_supplier
+            },
+            'path': test_actual_path,
+            'requestContext': {'resourcePath': test_resource_path},
+            'body': "{\"identifier\": [{\"system\": \"http://test\", \"value\": \"12345\"}], \"contained\": [{\"resourceType\": \"Patient\", \"id\": \"Pat1\", \"identifier\": [{\"system\": \"https://fhir.nhs.uk/Id/nhs-number\", \"value\": \"9693632109\"}]}], \"protocolApplied\": [{\"targetDisease\": [{\"coding\": [{\"system\": \"http://snomed.info/sct\", \"code\": \"840539006\", \"display\": \"Disease caused by severe acute respiratory syndrome coronavirus 2\"}]}]}]}"
+        }
+
+        # Act
+        result = wrapped_function(event, {})
+
+        # Assert
+        self.assertEqual(result, "Success")
+
+        for logger_info in self.extract_all_call_args_for_logger(mock_logger):
+            self.assertNotIn("9693632109", str(logger_info))
+
     def test_exception_handling(self, mock_logger, mock_firehose_logger):
         # Arrange
         test_correlation = "failed_test_correlation"

batch_processor_filter/src/batch_processor_filter_service.py

@@ -32,6 +32,8 @@ def apply_filter(self, batch_file_created_event: BatchFileCreatedEvent) -> None:
         supplier = batch_file_created_event["supplier"]
         vaccine_type = batch_file_created_event["vaccine_type"]
 
+        logger.info("Received batch file event for filename: %s with message id: %s", filename, message_id)
+
         if self._is_duplicate_file(filename):
             # Mark as processed and return without error so next event will be picked up from queue
             logger.error("A duplicate file has already been processed. Filename: %s", filename)
@@ -52,12 +54,12 @@ def apply_filter(self, batch_file_created_event: BatchFileCreatedEvent) -> None:
             raise EventAlreadyProcessingForSupplierAndVaccTypeError(f"Batch event already processing for supplier: "
                                                                     f"{supplier} and vacc type: {vaccine_type}")
 
+        self._batch_audit_repository.update_status(message_id, FileStatus.PROCESSING)
         self._queue_client.send_message(
             QueueUrl=QUEUE_URL,
             MessageBody=json.dumps(batch_file_created_event),
             MessageGroupId=f"{supplier}_{vaccine_type}"
         )
-        self._batch_audit_repository.update_status(message_id, FileStatus.PROCESSING)
 
         successful_log_message = f"File forwarded for processing by ECS. Filename: {filename}"
         logger.info(successful_log_message)

batch_processor_filter/tests/test_lambda_handler.py

@@ -4,7 +4,7 @@
 import boto3
 import copy
 from unittest import TestCase
-from unittest.mock import patch, Mock, ANY
+from unittest.mock import patch, Mock, ANY, call
 
 import botocore
 from moto import mock_aws
@@ -206,10 +206,17 @@ def test_lambda_handler_raises_error_when_event_already_processing_for_supplier_
                 sqs_messages = sqs_client.receive_message(QueueUrl=self.mock_queue_url)
                 self.assertEqual(sqs_messages.get("Messages", []), [])
 
-                self.mock_logger.info.assert_called_once_with(
-                    "Batch event already processing for supplier and vacc type. Filename: %s",
-                    "Menacwy_Vaccinations_v5_TEST_20250826T15003000.csv"
-                )
+                self.mock_logger.info.assert_has_calls([
+                    call(
+                        "Received batch file event for filename: %s with message id: %s",
+                        "Menacwy_Vaccinations_v5_TEST_20250826T15003000.csv",
+                        "3b60c4f7-ef67-43c7-8f0d-4faee04d7d0e"
+                    ),
+                    call(
+                        "Batch event already processing for supplier and vacc type. Filename: %s",
+                        "Menacwy_Vaccinations_v5_TEST_20250826T15003000.csv"
+                    )
+                ])
 
     def test_lambda_handler_processes_event_successfully(self):
         """Should update the audit entry status to Processing and forward to SQS"""
@@ -225,11 +232,20 @@ def test_lambda_handler_processes_event_successfully(self):
         self.assertEqual(len(sqs_messages.get("Messages", [])), 1)
         self.assertDictEqual(json.loads(sqs_messages["Messages"][0]["Body"]), dict(self.default_batch_file_event))
 
-        expected_log_message = (f"File forwarded for processing by ECS. Filename: "
-                                f"{self.default_batch_file_event['filename']}")
-        self.mock_logger.info.assert_called_once_with(expected_log_message)
+        expected_success_log_message = (f"File forwarded for processing by ECS. Filename: "
+                                        f"{self.default_batch_file_event['filename']}")
+        self.mock_logger.info.assert_has_calls([
+            call(
+                "Received batch file event for filename: %s with message id: %s",
+                "Menacwy_Vaccinations_v5_TEST_20250820T10210000.csv",
+                "df0b745c-b8cb-492c-ba84-8ea28d9f51d5"
+            ),
+            call(
+                expected_success_log_message
+            )
+        ])
         self.mock_firehose_send_log.assert_called_once_with(
-            {**self.default_batch_file_event, "message": expected_log_message}
+            {**self.default_batch_file_event, "message": expected_success_log_message}
         )
 
     def test_lambda_handler_processes_event_successfully_when_event_for_same_supplier_and_vacc_already_processed(self):
@@ -256,8 +272,17 @@ def test_lambda_handler_processes_event_successfully_when_event_for_same_supplie
         self.assertEqual(len(sqs_messages.get("Messages", [])), 1)
         self.assertDictEqual(json.loads(sqs_messages["Messages"][0]["Body"]), dict(test_event))
 
-        expected_log_message = f"File forwarded for processing by ECS. Filename: {test_event['filename']}"
-        self.mock_logger.info.assert_called_once_with(expected_log_message)
+        expected_success_log_message = f"File forwarded for processing by ECS. Filename: {test_event['filename']}"
+        self.mock_logger.info.assert_has_calls([
+            call(
+                "Received batch file event for filename: %s with message id: %s",
+                "Menacwy_Vaccinations_v5_TEST_20250826T15003000.csv",
+                "3b60c4f7-ef67-43c7-8f0d-4faee04d7d0e"
+            ),
+            call(
+                expected_success_log_message
+            )
+        ])
         self.mock_firehose_send_log.assert_called_once_with(
-            {**test_event, "message": expected_log_message}
+            {**test_event, "message": expected_success_log_message}
         )

config/prod/permissions_config.json

@@ -21,7 +21,10 @@
   },
   {
     "supplier": "RAVS",
-    "permissions": ["RSV.RS"],
+    "permissions": [
+      "MMR.CRUDS",
+      "RSV.RS"
+    ],
     "ods_codes": ["X26", "X8E5B"]
   },
   {

filenameprocessor/src/constants.py

@@ -7,7 +7,6 @@
     VaccineTypePermissionsError,
     InvalidFileKeyError,
     UnhandledAuditTableError,
-    EmptyFileError,
     UnhandledSqsError
 )
 
@@ -23,15 +22,11 @@
 ERROR_TYPE_TO_STATUS_CODE_MAP = {
     VaccineTypePermissionsError: 403,
     InvalidFileKeyError: 400,  # Includes invalid ODS code, therefore unable to identify supplier
-    EmptyFileError: 400,
     UnhandledAuditTableError: 500,
     UnhandledSqsError: 500,
     Exception: 500,
 }
 
-# The size in bytes of an empty batch file containing only the headers row
-EMPTY_BATCH_FILE_SIZE_IN_BYTES = 700
-
 
 class FileStatus(StrEnum):
     """File status constants"""

filenameprocessor/src/errors.py

@@ -1,10 +1,6 @@
 """Custom exceptions for the Filename Processor."""
 
 
-class EmptyFileError(Exception):
-    """A custom exception for when the batch file contains only the header row or is completely empty"""
-
-
 class UnhandledAuditTableError(Exception):
     """A custom exception for when an unexpected error occurs whilst adding the file to the audit table."""