Merge branch 'master' into VED-779-refactor-mns

Author: JamesW1-NHS

.github/dependabot.yml

@@ -58,7 +58,7 @@ updates:
       - "/filenameprocessor"
       - "/mesh_processor"
       - "/recordprocessor"
-      - "/redis_sync"
+      - "/lambdas/redis_sync"
       - "/lambdas/id_sync"
       - "/lambdas/mns_subscription"
       - "/lambdas/shared"

.github/workflows/sonarcloud.yml

@@ -135,15 +135,15 @@ jobs:
             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 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

Makefile

@@ -1,6 +1,6 @@
 SHELL=/usr/bin/env bash -euo pipefail
 
-PYTHON_PROJECT_DIRS_WITH_UNIT_TESTS = ack_backend backend batch_processor_filter delta_backend filenameprocessor mesh_processor recordprocessor redis_sync lambdas/id_sync lambdas/mns_subscription lambdas/shared
+PYTHON_PROJECT_DIRS_WITH_UNIT_TESTS = ack_backend backend batch_processor_filter delta_backend filenameprocessor mesh_processor recordprocessor 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"

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."""
 

filenameprocessor/src/file_name_processor.py

@@ -9,7 +9,7 @@
 import argparse
 from uuid import uuid4
 from utils_for_filenameprocessor import get_creation_and_expiry_times, move_file
-from file_validation import validate_file_key, is_file_in_directory_root, validate_file_not_empty
+from file_validation import validate_file_key, is_file_in_directory_root
 from send_sqs_message import make_and_send_sqs_message
 from make_and_upload_ack_file import make_and_upload_the_ack_file
 from audit_table import upsert_audit_table
@@ -20,8 +20,7 @@
     VaccineTypePermissionsError,
     InvalidFileKeyError,
     UnhandledAuditTableError,
-    UnhandledSqsError,
-    EmptyFileError
+    UnhandledSqsError
 )
 from constants import FileNotProcessedReason, FileStatus, ERROR_TYPE_TO_STATUS_CODE_MAP, SOURCE_BUCKET_NAME
 
@@ -68,8 +67,6 @@ def handle_record(record) -> dict:
         created_at_formatted_string, expiry_timestamp = get_creation_and_expiry_times(s3_response)
 
         vaccine_type, supplier = validate_file_key(file_key)
-        # VED-757: Known issue with suppliers sometimes sending empty files
-        validate_file_not_empty(s3_response)
         permissions = validate_vaccine_type_permissions(vaccine_type=vaccine_type, supplier=supplier)
 
         queue_name = f"{supplier}_{vaccine_type}"
@@ -94,17 +91,12 @@ def handle_record(record) -> dict:
 
     except (  # pylint: disable=broad-exception-caught
         VaccineTypePermissionsError,
-        EmptyFileError,
         InvalidFileKeyError,
         UnhandledAuditTableError,
         UnhandledSqsError,
         Exception,
     ) as error:
-        if isinstance(error, EmptyFileError):
-            # Avoid error log noise for accepted scenario in which supplier provides a batch file with no records
-            logger.warning("Error processing file '%s': %s", file_key, str(error))
-        else:
-            logger.error("Error processing file '%s': %s", file_key, str(error))
+        logger.error("Error processing file '%s': %s", file_key, str(error))
 
         queue_name = f"{supplier}_{vaccine_type}"
         file_status = get_file_status_for_error(error)
@@ -137,8 +129,6 @@ def get_file_status_for_error(error: Exception) -> str:
     """Creates a file status based on the type of error that was thrown"""
     if isinstance(error, VaccineTypePermissionsError):
         return f"{FileStatus.NOT_PROCESSED} - {FileNotProcessedReason.UNAUTHORISED}"
-    elif isinstance(error, EmptyFileError):
-        return f"{FileStatus.NOT_PROCESSED} - {FileNotProcessedReason.EMPTY}"
 
     return FileStatus.FAILED
 

filenameprocessor/src/file_validation.py

@@ -2,9 +2,9 @@
 
 from re import match
 from datetime import datetime
-from constants import VALID_VERSIONS, EMPTY_BATCH_FILE_SIZE_IN_BYTES
+from constants import VALID_VERSIONS
 from elasticache import get_valid_vaccine_types_from_cache, get_supplier_system_from_cache
-from errors import InvalidFileKeyError, EmptyFileError
+from errors import InvalidFileKeyError
 
 
 def is_file_in_directory_root(file_key: str) -> bool:
@@ -72,9 +72,3 @@ def validate_file_key(file_key: str) -> tuple[str, str]:
         raise InvalidFileKeyError("Initial file validation failed: invalid file key")
 
     return vaccine_type, supplier
-
-
-def validate_file_not_empty(s3_response: dict) -> None:
-    """Checks that the batch file from S3 is not empty or containing only the header row"""
-    if s3_response.get("ContentLength", 0) <= EMPTY_BATCH_FILE_SIZE_IN_BYTES:
-        raise EmptyFileError("Initial file validation failed: batch file was empty")