From 0ec89c8a8a2f19e9c0af883176794ca58a20c703 Mon Sep 17 00:00:00 2001 From: Matthew McKenzie Date: Mon, 9 Dec 2024 14:55:44 +0000 Subject: [PATCH 1/6] Change composite key to random unique string --- common/models/transactions.py | 1 + taric_parsers/importer.py | 5 +++-- workbaskets/models.py | 6 +++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/common/models/transactions.py b/common/models/transactions.py index 537524c44..34c141e19 100644 --- a/common/models/transactions.py +++ b/common/models/transactions.py @@ -279,6 +279,7 @@ class Meta: # The default order ("partition", "order") gives global ordering. order = models.IntegerField() + # Originally containing order, partition and workbasket pk / envelope id. Now a random unique string to avoid duplicate values when reordering / deleting transactions. composite_key = models.CharField(max_length=16, unique=True) objects = TransactionManager.from_queryset(TransactionQueryset)() diff --git a/taric_parsers/importer.py b/taric_parsers/importer.py index 5d6553273..f15999f28 100644 --- a/taric_parsers/importer.py +++ b/taric_parsers/importer.py @@ -1,3 +1,4 @@ +from os import urandom from typing import Generator from typing import List from typing import Optional @@ -344,14 +345,14 @@ def commit_data(self): None """ - envelope = Envelope.new_envelope() + Envelope.new_envelope() transaction_order = 1 for parsed_transaction in self.parsed_transactions: # create transaction transaction_inst = Transaction.objects.create( - composite_key=f"{envelope.envelope_id}{transaction_order}", + composite_key=urandom(8).hex(), workbasket=self.workbasket, order=transaction_order, ) diff --git a/workbaskets/models.py b/workbaskets/models.py index b7424e348..6d584a975 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,8 @@ def new_transaction(self, **kwargs): ) if "composite_key" not in kwargs: - kwargs["composite_key"] = ( - f"{self.pk}-{kwargs['order']}-{kwargs['partition']}" - ) + # Generating a random 16 digit hexadecimal composite key to avoid duplicates + kwargs["composite_key"] = urandom(8).hex() # Get Transaction model via transactions.model to avoid circular import. return self.transactions.model.objects.create(workbasket=self, **kwargs) From ec624c0b3d1aa63d1d83ecbad3e4306a62300ed5 Mon Sep 17 00:00:00 2001 From: Matthew McKenzie Date: Tue, 10 Dec 2024 10:05:58 +0000 Subject: [PATCH 2/6] Undo importer change --- taric_parsers/importer.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/taric_parsers/importer.py b/taric_parsers/importer.py index f15999f28..5d6553273 100644 --- a/taric_parsers/importer.py +++ b/taric_parsers/importer.py @@ -1,4 +1,3 @@ -from os import urandom from typing import Generator from typing import List from typing import Optional @@ -345,14 +344,14 @@ def commit_data(self): None """ - Envelope.new_envelope() + envelope = Envelope.new_envelope() transaction_order = 1 for parsed_transaction in self.parsed_transactions: # create transaction transaction_inst = Transaction.objects.create( - composite_key=urandom(8).hex(), + composite_key=f"{envelope.envelope_id}{transaction_order}", workbasket=self.workbasket, order=transaction_order, ) From 938ecaf52ef45d2f350b0252ffb12d97eebab6b3 Mon Sep 17 00:00:00 2001 From: Matthew McKenzie Date: Tue, 10 Dec 2024 11:12:12 +0000 Subject: [PATCH 3/6] Add test to confirm bug fix --- workbaskets/tests/test_views.py | 41 +++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) 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.", + ) From 5187209592d38995d93e4d61c658b31009d984b9 Mon Sep 17 00:00:00 2001 From: Matthew McKenzie Date: Tue, 10 Dec 2024 12:52:10 +0000 Subject: [PATCH 4/6] Include wb pk in composite key --- common/models/transactions.py | 12 +++++++++++- workbaskets/models.py | 8 ++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/common/models/transactions.py b/common/models/transactions.py index 34c141e19..2fd56d20a 100644 --- a/common/models/transactions.py +++ b/common/models/transactions.py @@ -279,8 +279,18 @@ class Meta: # The default order ("partition", "order") gives global ordering. order = models.IntegerField() - # Originally containing order, partition and workbasket pk / envelope id. Now a random unique string to avoid duplicate values when reordering / deleting transactions. 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 6d584a975..18a40a4e4 100644 --- a/workbaskets/models.py +++ b/workbaskets/models.py @@ -558,8 +558,12 @@ def new_transaction(self, **kwargs): ) if "composite_key" not in kwargs: - # Generating a random 16 digit hexadecimal composite key to avoid duplicates - kwargs["composite_key"] = urandom(8).hex() + # 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) From f9f4365c7a880b7c112a459cda72c19719c3e263 Mon Sep 17 00:00:00 2001 From: Matthew McKenzie Date: Wed, 11 Dec 2024 13:23:08 +0000 Subject: [PATCH 5/6] Update to zero padded wb pk --- workbaskets/models.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/workbaskets/models.py b/workbaskets/models.py index 18a40a4e4..5de32fe94 100644 --- a/workbaskets/models.py +++ b/workbaskets/models.py @@ -558,12 +558,15 @@ def new_transaction(self, **kwargs): ) if "composite_key" not in kwargs: - # 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 + # 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. + workbasket_pk = str(self.pk).zfill(5) + if len(workbasket_pk) > 5: + raise ValueError( + "Workbasket PK cannot be bigger than 5 digits for composite_key generation.", + ) + 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) From 51ee064965b2d871e31881bdc30afaa223ff4169 Mon Sep 17 00:00:00 2001 From: Matthew McKenzie Date: Wed, 11 Dec 2024 17:11:12 +0000 Subject: [PATCH 6/6] PR comment --- workbaskets/models.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/workbaskets/models.py b/workbaskets/models.py index 5de32fe94..674265df9 100644 --- a/workbaskets/models.py +++ b/workbaskets/models.py @@ -560,11 +560,12 @@ def new_transaction(self, **kwargs): if "composite_key" not in kwargs: # 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. - workbasket_pk = str(self.pk).zfill(5) - if len(workbasket_pk) > 5: + 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}"