TP2000-1154 Transaction reordering bug

common/models/transactions.py

@@ -280,6 +280,17 @@ class Meta:
     order = models.IntegerField()
 
     composite_key = models.CharField(max_length=16, unique=True)
+    """
+    Originally containing workbasket pk, order and partition for user created
+    transactions.
+
+    Now a random unique string to avoid duplicate values when reordering /
+    deleting transactions.
+
+    It is not clear what the purpose of composite_key is other than as an
+    application level unique ID for a transaction. It is not used anywhere in
+    the app.
+    """
 
     objects = TransactionManager.from_queryset(TransactionQueryset)()
 

workbaskets/models.py

@@ -4,6 +4,7 @@
 import logging
 from abc import ABCMeta
 from abc import abstractmethod
+from os import urandom
 from typing import Optional
 from typing import Tuple
 
@@ -557,9 +558,12 @@ def new_transaction(self, **kwargs):
             )
 
         if "composite_key" not in kwargs:
-            kwargs["composite_key"] = (
-                f"{self.pk}-{kwargs['order']}-{kwargs['partition']}"
-            )
+            # Forms a composite key of workbasket id and a randomly generated hexadecimal 12 character string.
+            # The probability of a collision is miniscule unless workbasket transactions are in the millions. It becomes likely (>50%) around 20 million.
+            composite_key = f"{self.pk}{urandom(6).hex()}"
+            kwargs["composite_key"] = composite_key[
+                :16
+            ]  # Ensure maximum 16 characters should workbasket ids go over 9999
 
         # Get Transaction model via transactions.model to avoid circular import.
         return self.transactions.model.objects.create(workbasket=self, **kwargs)

workbaskets/tests/test_views.py

@@ -9,6 +9,7 @@
 import pytest
 from bs4 import BeautifulSoup
 from django.contrib.auth.models import Permission
+from django.db import IntegrityError
 from django.test.client import RequestFactory
 from django.urls import reverse
 from django.utils.timezone import localtime
@@ -2608,7 +2609,9 @@ def test_current_tasks_is_called(valid_user_client):
 
 
 def test_remove_all_workbasket_changes_button_only_shown_to_superusers(
-    client, user_workbasket, superuser
+    client,
+    user_workbasket,
+    superuser,
 ):
     url = reverse(
         "workbaskets:workbasket-ui-changes",
@@ -2626,7 +2629,8 @@ def test_remove_all_workbasket_changes_button_only_shown_to_superusers(
 
 
 def test_remove_all_workbasket_changes_button_not_shown_to_users_without_permision(
-    valid_user_client, user_workbasket
+    valid_user_client,
+    user_workbasket,
 ):
     url = reverse(
         "workbaskets:workbasket-ui-changes",
@@ -2639,3 +2643,36 @@ def test_remove_all_workbasket_changes_button_not_shown_to_users_without_permisi
 
     remove_all_button = page.find("button", value="remove-all")
     assert not remove_all_button
+
+
+def test_reordering_transactions_bug(valid_user_client, user_workbasket):
+    """Test that a user can reorder transactions, delete one and still be able
+    to add new objects to the workbasket."""
+    additional_code1, additional_code2, additional_code3 = (
+        factories.AdditionalCodeFactory.create_batch(3)
+    )
+    new_add_code1 = additional_code1.new_version(workbasket=user_workbasket)
+    new_add_code2 = additional_code2.new_version(workbasket=user_workbasket)
+    new_add_code3 = additional_code3.new_version(workbasket=user_workbasket)
+
+    url = reverse(
+        "workbaskets:workbasket-ui-transaction-order",
+        kwargs={"pk": user_workbasket.pk},
+    )
+    # Move the last transaction to the top
+    valid_user_client.post(
+        url,
+        {"form-action": f"promote-transaction-top__{new_add_code3.transaction.pk}"},
+    )
+    # Delete the now last (by order) transaction
+    last_transaction = user_workbasket.transactions.last()
+    last_transaction.tracked_models.all().delete()
+    last_transaction.delete()
+
+    # Try and add something new to the workbasket
+    try:
+        additional_code1.new_version(workbasket=user_workbasket)
+    except IntegrityError:
+        pytest.fail(
+            "IntegrityError - New trackedmodel cannot be created after reordering then deleting transactions.",
+        )