Merge pull request #2186 from uktrade/uat

Prod Release

.circleci/config.yml

@@ -134,7 +134,7 @@ jobs:
       - run:
           name: Run tests
           command: |
-            pipenv run pytest --circleci-parallelize --cov=. --cov-report xml --cov-config=.coveragerc --ignore lite_routing --ignore api/anonymised_db_dumps -k "not seeding and not elasticsearch and not performance and not migration and not db_anonymiser"
+            pipenv run pytest --circleci-parallelize --cov=. --cov-report xml --cov-config=.coveragerc --ignore lite_routing --ignore api/anonymised_db_dumps -k "not seeding and not elasticsearch and not performance and not migration and not db_anonymiser and not requires_transactions"
       - upload_code_coverage:
           alias: tests
 
@@ -154,7 +154,7 @@ jobs:
       - run:
           name: Run tests on Postgres 13
           command: |
-            pipenv run pytest --circleci-parallelize --cov=. --cov-report xml --cov-config=.coveragerc --ignore lite_routing --ignore api/anonymised_db_dumps -k "not seeding and not elasticsearch and not performance and not migration and not db_anonymiser"
+            pipenv run pytest --circleci-parallelize --cov=. --cov-report xml --cov-config=.coveragerc --ignore lite_routing --ignore api/anonymised_db_dumps -k "not seeding and not elasticsearch and not performance and not migration and not db_anonymiser and not requires_transactions"
       - upload_code_coverage:
           alias: tests_dbt_platform
 
@@ -232,6 +232,44 @@ jobs:
       - upload_code_coverage:
           alias: anonymised_db_dump_tests_dbt_platform
 
+  requires_transactions_tests:
+    docker:
+      - <<: *image_python
+      - <<: *image_postgres
+      - <<: *image_opensearch_v1
+      - <<: *image_redis
+    working_directory: ~/lite-api
+    environment:
+      <<: *common_env_vars
+      LITE_API_ENABLE_ES: True
+    steps:
+      - setup
+      - run:
+          name: Run requiring transactions tests
+          command: |
+            pipenv run pytest --cov=. --cov-report xml --cov-config=.coveragerc -k requires_transactions
+      - upload_code_coverage:
+          alias: requires_transactions_tests
+
+  requires_transactions_tests_dbt_platform:
+    docker:
+      - <<: *image_python
+      - <<: *image_postgres13
+      - <<: *image_opensearch
+      - <<: *image_redis
+    working_directory: ~/lite-api
+    environment:
+      <<: *common_env_vars
+      LITE_API_ENABLE_ES: True
+    steps:
+      - setup
+      - run:
+          name: Run requiring transactions tests on Postgres 13
+          command: |
+            pipenv run pytest --cov=. --cov-report xml --cov-config=.coveragerc -k requires_transactions
+      - upload_code_coverage:
+          alias: requires_transactions_tests_dbt_platform
+
   migration_tests:
     docker:
       - <<: *image_python
@@ -615,7 +653,10 @@ workflows:
               - open_search_tests
               - migration_tests
               - lite_routing_tests
+              - requires_transactions_tests
       - check-lite-routing-sha
       - e2e_tests
       - anonymised_db_dump_tests
       - anonymised_db_dump_tests_dbt_platform
+      - requires_transactions_tests
+      - requires_transactions_tests_dbt_platform

api/applications/exceptions.py

@@ -0,0 +1,2 @@
+class AmendmentError(Exception):
+    pass

api/applications/libraries/application_helpers.py

@@ -59,5 +59,4 @@ def create_submitted_audit(user, application, old_status: str, additional_payloa
         target=application.get_case(),
         payload=payload,
         ignore_case_status=True,
-        send_notification=False,
     )

api/applications/models.py

@@ -1,3 +1,4 @@
+import logging
 import uuid
 
 from django.contrib.postgres.fields import ArrayField
@@ -14,6 +15,7 @@
     NSGListType,
 )
 from api.appeals.models import Appeal
+from api.applications.exceptions import AmendmentError
 from api.applications.managers import BaseApplicationManager
 from api.applications.libraries.application_helpers import create_submitted_audit
 from api.audit_trail.models import AuditType
@@ -49,6 +51,9 @@
 from lite_routing.routing_rules_internal.enums import QueuesEnum
 
 
+logger = logging.getLogger(__name__)
+
+
 class ApplicationException(APIException):
     def __init__(self, data):
         super().__init__(data)
@@ -375,6 +380,30 @@ def clone(self, exclusions=None, **overrides):
 
     @transaction.atomic
     def create_amendment(self, user):
+        # It's possible that we've arrived here multiple times simultaneously because multiple requests to create an
+        # amendment have been made at the same time.
+        # The views that call this may have already checked the status but it's possible that when they checked it was
+        # before the status has actually been updated on the application so we've got a potential race condition going.
+        # What we want to do here is lock the row to make sure that only one request can be trying to create an
+        # amendment at any one time and if we have multiple requests clashing we'll re-check the status once the lock
+        # has been removed from any other racing requests.
+        # To be helpful to the caller we'll return the amendment that did happen even if there are multiple requests.
+        original = StandardApplication.objects.select_for_update().get(pk=self.pk)
+        if not CaseStatusEnum.can_invoke_major_edit(original.status.status):
+            logger.warning(
+                "Attempted to create an amendment from an already amended application %s with status %s",
+                original.pk,
+                original.status,
+            )
+            if original.superseded_by:
+                logger.info(
+                    "Found an amendment already: %s for the application: %s",
+                    original.superseded_by.pk,
+                    original.pk,
+                )
+                return StandardApplication.objects.get(pk=original.superseded_by.pk)
+            raise AmendmentError(f"Failed to create an amendment from {original.pk}")
+
         amendment_application = self.clone(amendment_of=self)
         CaseQueue.objects.filter(case=self.case_ptr).delete()
         audit_trail_service.create(

api/applications/tests/test_models.py

@@ -1,9 +1,16 @@
+import concurrent.futures
+import pytest
+
 from django.forms import model_to_dict
+from django.test import TransactionTestCase
 from django.utils import timezone
 
+from parameterized import parameterized
+
 from test_helpers.clients import DataTestClient
 
 from api.appeals.tests.factories import AppealFactory
+from api.applications.exceptions import AmendmentError
 from api.audit_trail.models import Audit
 from api.cases.models import CaseType, Queue
 from api.flags.models import Flag
@@ -32,8 +39,12 @@
 from api.staticdata.control_list_entries.models import ControlListEntry
 from api.staticdata.report_summaries.models import ReportSummary, ReportSummaryPrefix, ReportSummarySubject
 from api.staticdata.statuses.models import CaseStatus, CaseSubStatus
-from api.staticdata.statuses.enums import CaseStatusEnum
+from api.staticdata.statuses.enums import (
+    CaseStatusEnum,
+)
+from api.users.enums import SystemUser
 from api.users.models import ExporterUser
+from api.users.tests.factories import BaseUserFactory
 
 
 class TestBaseApplication(DataTestClient):
@@ -79,9 +90,10 @@ def test_on_submit_amendment_application(self):
 
 class TestStandardApplication(DataTestClient):
 
-    def test_create_amendment(self):
+    @parameterized.expand(CaseStatusEnum.can_invoke_major_edit_statuses())
+    def test_create_amendment(self, major_editable_status):
         original_application = StandardApplicationFactory(
-            status=CaseStatus.objects.get(status="ogd_advice"),
+            status=CaseStatus.objects.get(status=major_editable_status),
         )
         original_application.queues.add(Queue.objects.first())
         original_application.save()
@@ -109,10 +121,22 @@ def test_create_amendment(self):
         assert amendment_audit_entry.actor == exporter_user
         status_change_audit_entry = audit_entries[0]
         assert status_change_audit_entry.payload == {
-            "status": {"new": "superseded_by_exporter_edit", "old": "ogd_advice"}
+            "status": {"new": "superseded_by_exporter_edit", "old": major_editable_status}
         }
         assert status_change_audit_entry.verb == "updated_status"
 
+    @parameterized.expand(CaseStatusEnum.can_not_invoke_major_edit_statuses())
+    def test_create_amendment_failure(self, non_major_editable_status):
+        original_application = StandardApplicationFactory(
+            status=CaseStatus.objects.get(status=non_major_editable_status),
+        )
+        original_application.queues.add(Queue.objects.first())
+        original_application.save()
+
+        exporter_user = ExporterUser.objects.first()
+        with self.assertRaises(AmendmentError):
+            amendment_application = original_application.create_amendment(exporter_user)
+
     def test_clone(self):
         original_application = StandardApplicationFactory(
             activity="Trade",
@@ -574,3 +598,34 @@ def test_clone_with_party_override(self):
         cloned by default or not and adjust PartyOnApplication.clone_*
         attributes accordingly.
         """
+
+
+@pytest.mark.requires_transactions
+class TestStandardApplicationRaceConditions(TransactionTestCase):
+    def test_create_amendment_race_condition_success(self):
+        BaseUserFactory(id=SystemUser.id)
+
+        original_application = StandardApplicationFactory()
+
+        original_application = StandardApplication.objects.get(pk=original_application.pk)
+        same_application = StandardApplication.objects.get(pk=original_application.pk)
+
+        exporter_user = ExporterUser.objects.first()
+
+        def _create_amendment(application):
+            return application.create_amendment(exporter_user)
+
+        with concurrent.futures.ThreadPoolExecutor() as executor:
+            future_1 = executor.submit(_create_amendment, original_application)
+            future_2 = executor.submit(_create_amendment, same_application)
+
+            amendment_1 = future_1.result()
+            amendment_2 = future_2.result()
+
+        self.assertEqual(
+            StandardApplication.objects.count(),
+            2,
+        )
+        self.assertEqual(amendment_1, amendment_2)
+        self.assertEqual(amendment_1.amendment_of.get_case(), original_application.get_case())
+        self.assertEqual(amendment_2.amendment_of.get_case(), original_application.get_case())

api/applications/views/goods.py

@@ -363,7 +363,6 @@ def put(self, request, pk, good_on_application_pk):
                 "good_name": good_on_application.good.name,
             },
             ignore_case_status=True,
-            send_notification=False,
         )
 
         return JsonResponse(

api/audit_trail/managers.py

@@ -5,8 +5,6 @@
 
 from api.cases.models import Case
 from api.staticdata.statuses.libraries.case_status_validate import is_case_status_draft
-from api.users.models import ExporterUser
-from api.users.models import GovUser
 
 
 class AuditQuerySet(GFKQuerySet):
@@ -26,28 +24,16 @@ def create(self, *args, **kwargs):
         """
         Create an audit entry for a model
         target: the target object (such as a case)
-        actor: the object causing the audit entry (such as a user)
-        send_notification: certain scenarios alert internal users, default is True
         ignore_case_status: draft cases become audited, default is False
         """
-        # TODO: decouple notifications and audit (signals?)
         target = kwargs.get("target")
-        actor = kwargs.get("actor")
-        send_notification = kwargs.pop("send_notification", True)
         ignore_case_status = kwargs.pop("ignore_case_status", False)
 
         if isinstance(target, Case):
             # Only audit cases if their status is not draft
             if not is_case_status_draft(target.status.status) or ignore_case_status:
                 audit = super(AuditManager, self).create(*args, **kwargs)
-
-                # Notify gov users when an exporter updates a case
-                if isinstance(actor, ExporterUser) and send_notification:
-                    for gov_user in GovUser.objects.all():
-                        gov_user.send_notification(content_object=audit, case=target)
-
                 return audit
-
             return None
 
         return super(AuditManager, self).create(*args, **kwargs)

api/audit_trail/models.py

@@ -1,6 +1,6 @@
 import uuid
 
-from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation
+from django.contrib.contenttypes.fields import GenericForeignKey
 from django.contrib.contenttypes.models import ContentType
 from django.db import models
 from django.utils import timesince
@@ -9,7 +9,6 @@
 from api.audit_trail.managers import AuditManager
 from api.audit_trail.enums import AuditType
 from api.common.models import TimestampableModel
-from api.users.models import GovNotification
 
 
 class Audit(TimestampableModel):
@@ -51,8 +50,6 @@ class Audit(TimestampableModel):
 
     objects = AuditManager()
 
-    notifications = GenericRelation(GovNotification, related_query_name="audit")
-
     class Meta:
         ordering = ("-created_at",)
 

api/audit_trail/service.py

@@ -28,7 +28,6 @@ def create(
     target: Optional[Case] = None,
     payload=None,
     ignore_case_status: bool = False,
-    send_notification: bool = True,
 ) -> Optional[Audit]:
     if not payload:
         payload = {}
@@ -50,7 +49,6 @@ def create(
         target=target,
         payload=payload,
         ignore_case_status=ignore_case_status,
-        send_notification=send_notification,
     )
 
 

api/cases/helpers.py

@@ -2,7 +2,7 @@
 
 from api.audit_trail.enums import AuditType
 from api.common.dates import is_bank_holiday, is_weekend
-from api.users.models import BaseUser, GovUser, GovNotification
+from api.users.models import BaseUser, GovUser
 from api.users.enums import SystemUser
 
 
@@ -22,17 +22,6 @@ def get_assigned_as_case_officer_case_ids(user: GovUser):
     return Case.objects.filter(case_officer=user).values_list("id", flat=True)
 
 
-def get_updated_case_ids(user: GovUser):
-    """
-    Get the cases that have raised notifications when updated by an exporter
-    """
-    assigned_to_user_case_ids = get_assigned_to_user_case_ids(user)
-    assigned_as_case_officer_case_ids = get_assigned_as_case_officer_case_ids(user)
-    cases = assigned_to_user_case_ids.union(assigned_as_case_officer_case_ids)
-
-    return GovNotification.objects.filter(user_id=user.pk, case__id__in=cases).values_list("case__id", flat=True)
-
-
 def working_days_in_range(start_date, end_date):
     dates_in_range = [start_date + timedelta(n) for n in range((end_date - start_date).days)]
     return len([date for date in dates_in_range if (not is_bank_holiday(date) and not is_weekend(date))])

api/cases/libraries/delete_notifications.py

@@ -1,12 +1,8 @@
-from api.users.models import ExporterNotification, ExporterUser, GovNotification, GovUser
+from api.users.models import ExporterNotification, ExporterUser
 
 
 def delete_exporter_notifications(user: ExporterUser, organisation_id, objects: list):
     id_list = [obj.id for obj in objects]
     ExporterNotification.objects.filter(
         user=user.baseuser_ptr, organisation_id=organisation_id, object_id__in=id_list
     ).delete()
-
-
-def delete_gov_user_notifications(user: GovUser, id_list: list):
-    GovNotification.objects.filter(user=user.baseuser_ptr, object_id__in=id_list).delete()