diff --git a/common/models/transactions.py b/common/models/transactions.py index 537524c44..2fd56d20a 100644 --- a/common/models/transactions.py +++ b/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)() diff --git a/workbaskets/models.py b/workbaskets/models.py index b7424e348..674265df9 100644 --- a/workbaskets/models.py +++ b/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,16 @@ 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 a zero-padded 5 digit workbasket id + a randomly generated hexadecimal 11 character string. + # The probability of a collision is miniscule. It becomes 1% around 600,000 transactions and likely (>50%) at around 5 million. + if len(str(self.pk)) > 5: + raise ValueError( + "Workbasket PK cannot be bigger than 5 digits for composite_key generation.", + ) + workbasket_pk = str(self.pk).zfill(5) + + hex_string_11_digit = f"{urandom(6).hex()}"[:11] + kwargs["composite_key"] = f"{workbasket_pk}{hex_string_11_digit}" # Get Transaction model via transactions.model to avoid circular import. return self.transactions.model.objects.create(workbasket=self, **kwargs) diff --git a/workbaskets/tests/test_views.py b/workbaskets/tests/test_views.py index 73e7d6635..4bdf04e6e 100644 --- a/workbaskets/tests/test_views.py +++ b/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.", + )