From 8ab614ea4419e65a7268f0f076e5be4fc70be7c9 Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Mon, 11 Jan 2016 13:55:39 +0000 Subject: [PATCH 1/9] Add failing test --- orderable/tests/models.py | 6 ++++++ orderable/tests/test_models.py | 12 ++++++++++++ requirements.txt | 2 +- 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/orderable/tests/models.py b/orderable/tests/models.py index 1cdc12f..3d8d497 100644 --- a/orderable/tests/models.py +++ b/orderable/tests/models.py @@ -6,6 +6,9 @@ class Task(Orderable): """A basic orderable model for tests.""" + def __str__(self): + return 'Task {}'.format(self.pk) + class SubTask(Orderable): """An orderable model with unique_together.""" @@ -13,3 +16,6 @@ class SubTask(Orderable): class Meta: unique_together = ('task', 'sort_order') + + def __str__(self): + return 'SubTask {}'.format(self.pk) diff --git a/orderable/tests/test_models.py b/orderable/tests/test_models.py index 07afe02..038533e 100644 --- a/orderable/tests/test_models.py +++ b/orderable/tests/test_models.py @@ -207,3 +207,15 @@ def test_changing_parent(self): subtask_2.task = task subtask_2.save() self.assertSequenceEqual(task.subtask_set.all(), [subtask, subtask_2]) + + def test_save_subtask(self): + task = Task.objects.create() + + subtasks = [ + SubTask.objects.create(task=task, sort_order=order) + for order in [1, 7, 14, 20, 22, 23, 24] + ] + + subtask = subtasks[-1] + subtask.sort_order = 2 + subtask.save() diff --git a/requirements.txt b/requirements.txt index 23bfd50..126a4d5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -3,4 +3,4 @@ django-discover-runner==1.0 flake8==2.2.5 flake8-docstrings==0.2.1 flake8-import-order==0.5.3 -psycopg2==2.5.1 +psycopg2==2.6.1 From 7515b69c3e5a48de1cfa913efa690b772a3ca6d3 Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Mon, 11 Jan 2016 14:09:03 +0000 Subject: [PATCH 2/9] Use hypothesis to find a minimal failing example --- .gitignore | 1 + orderable/tests/test_models.py | 9 ++++++--- requirements.txt | 1 + 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index d9437c3..3d35263 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ *.pot *.pyc local_settings.py +.hypothesis/ diff --git a/orderable/tests/test_models.py b/orderable/tests/test_models.py index 038533e..7c65d1a 100644 --- a/orderable/tests/test_models.py +++ b/orderable/tests/test_models.py @@ -1,4 +1,6 @@ -from django.test import TestCase +from hypothesis import given +from hypothesis.extra.django import TestCase +from hypothesis.strategies import integers, lists from .models import SubTask, Task @@ -208,12 +210,13 @@ def test_changing_parent(self): subtask_2.save() self.assertSequenceEqual(task.subtask_set.all(), [subtask, subtask_2]) - def test_save_subtask(self): + @given(lists(integers(min_value=1), min_size=1, unique=True)) + def test_save_subtask(self, sort_orders): task = Task.objects.create() subtasks = [ SubTask.objects.create(task=task, sort_order=order) - for order in [1, 7, 14, 20, 22, 23, 24] + for order in sort_orders ] subtask = subtasks[-1] diff --git a/requirements.txt b/requirements.txt index 126a4d5..eae49c6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -3,4 +3,5 @@ django-discover-runner==1.0 flake8==2.2.5 flake8-docstrings==0.2.1 flake8-import-order==0.5.3 +hypothesis==2.0.0 psycopg2==2.6.1 From e61ed80d95a1c08be6c6e1bb7378002bbf554980 Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Mon, 11 Jan 2016 14:41:46 +0000 Subject: [PATCH 3/9] Simplify hypothesis test further --- orderable/tests/test_models.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/orderable/tests/test_models.py b/orderable/tests/test_models.py index 7c65d1a..21374a0 100644 --- a/orderable/tests/test_models.py +++ b/orderable/tests/test_models.py @@ -214,11 +214,5 @@ def test_changing_parent(self): def test_save_subtask(self, sort_orders): task = Task.objects.create() - subtasks = [ + for order in sort_orders: SubTask.objects.create(task=task, sort_order=order) - for order in sort_orders - ] - - subtask = subtasks[-1] - subtask.sort_order = 2 - subtask.save() From 5f7ccabea732552cb141a9924862edc5aed0ed5e Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Tue, 12 Jan 2016 09:25:58 +0000 Subject: [PATCH 4/9] Fix Orderable._save for new instances --- orderable/models.py | 9 ++++++++- orderable/tests/test_models.py | 6 ++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/orderable/models.py b/orderable/models.py index 2568ccc..a2c6a84 100644 --- a/orderable/models.py +++ b/orderable/models.py @@ -80,7 +80,14 @@ def _save(self, objects, old_pos, new_pos): # Increment `sort_order` on objects with: # sort_order > new_pos. to_shift = to_shift.filter(sort_order__gte=self.sort_order) - to_shift.update(sort_order=models.F('sort_order') + 1) + try: + with transaction.atomic(): + to_shift.update(sort_order=models.F('sort_order') + 1) + except IntegrityError: + for obj in to_shift.order_by('-sort_order'): + to_shift.filter(pk=obj.pk).update( + sort_order=models.F('sort_order') + 1, + ) self.sort_order = new_pos # self.sort_order decreased. diff --git a/orderable/tests/test_models.py b/orderable/tests/test_models.py index 21374a0..73f9a6f 100644 --- a/orderable/tests/test_models.py +++ b/orderable/tests/test_models.py @@ -52,12 +52,14 @@ def test_insert_on_create(self): old_3 = Task.objects.create(sort_order=3) # Insert between old_1 and old_2 - with self.assertNumQueries(4): + with self.assertNumQueries(6): # Queries: # Savepoint + # Savepoint # Bump old_2 to position 3 + # Release savepoint + # Release savepoint # Save new in position 2 - # Commit new = Task.objects.create(sort_order=old_2.sort_order) tasks = Task.objects.all() From 0a4257828ddbc9e129c6fce889e958cad114da48 Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Tue, 12 Jan 2016 09:34:20 +0000 Subject: [PATCH 5/9] Test moving an existing Orderable instance --- orderable/models.py | 9 ++++++++- orderable/tests/test_models.py | 11 ++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/orderable/models.py b/orderable/models.py index a2c6a84..5900cd2 100644 --- a/orderable/models.py +++ b/orderable/models.py @@ -97,7 +97,14 @@ def _save(self, objects, old_pos, new_pos): # Increment `sort_order` on objects with: # sort_order >= new_pos and sort_order < old_pos to_shift = to_shift.filter(sort_order__gte=new_pos, sort_order__lt=old_pos) - to_shift.update(sort_order=models.F('sort_order') + 1) + try: + with transaction.atomic(): + to_shift.update(sort_order=models.F('sort_order') + 1) + except IntegrityError: + for obj in to_shift.order_by('-sort_order'): + to_shift.filter(pk=obj.pk).update( + sort_order=models.F('sort_order') + 1, + ) self.sort_order = new_pos # self.sort_order increased. diff --git a/orderable/tests/test_models.py b/orderable/tests/test_models.py index 73f9a6f..0a7c498 100644 --- a/orderable/tests/test_models.py +++ b/orderable/tests/test_models.py @@ -118,14 +118,16 @@ def test_decrease_order(self): item5 = Task.objects.create(sort_order=5) # Move item4 to position 2 - with self.assertNumQueries(6): + with self.assertNumQueries(8): # Queries: # Savepoint # Find end of list # Move item4 to end of list + # Savepoint # Bump item2 and item3 on by one + # Release savepoint + # Release savepoint # Save item4 to new desired position - # Commit item4.sort_order = item2.sort_order item4.save() @@ -217,4 +219,7 @@ def test_save_subtask(self, sort_orders): task = Task.objects.create() for order in sort_orders: - SubTask.objects.create(task=task, sort_order=order) + subtask = SubTask.objects.create(task=task, sort_order=order) + + subtask.sort_order = 2 + subtask.save() From c6cea48f993b52593e509774fef43809cf9352a8 Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Tue, 12 Jan 2016 09:38:49 +0000 Subject: [PATCH 6/9] Add Orderable._update helper method --- orderable/models.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/orderable/models.py b/orderable/models.py index 5900cd2..88f56fd 100644 --- a/orderable/models.py +++ b/orderable/models.py @@ -67,6 +67,20 @@ def prev(self): sort_order__lt=self.sort_order ).order_by('-sort_order').first() + @staticmethod + def _update(qs): + """ + Increment the sort_order in a queryset. + + Handle IntegrityErrors caused by unique constraints. + """ + try: + with transaction.atomic(): + qs.update(sort_order=models.F('sort_order') + 1) + except IntegrityError: + for obj in qs.order_by('-sort_order'): + qs.filter(pk=obj.pk).update(sort_order=models.F('sort_order') + 1) + def _save(self, objects, old_pos, new_pos): """WARNING: Intensive giggery-pokery zone.""" to_shift = objects.exclude(pk=self.pk) if self.pk else objects @@ -80,14 +94,7 @@ def _save(self, objects, old_pos, new_pos): # Increment `sort_order` on objects with: # sort_order > new_pos. to_shift = to_shift.filter(sort_order__gte=self.sort_order) - try: - with transaction.atomic(): - to_shift.update(sort_order=models.F('sort_order') + 1) - except IntegrityError: - for obj in to_shift.order_by('-sort_order'): - to_shift.filter(pk=obj.pk).update( - sort_order=models.F('sort_order') + 1, - ) + self._update(to_shift) self.sort_order = new_pos # self.sort_order decreased. @@ -97,14 +104,7 @@ def _save(self, objects, old_pos, new_pos): # Increment `sort_order` on objects with: # sort_order >= new_pos and sort_order < old_pos to_shift = to_shift.filter(sort_order__gte=new_pos, sort_order__lt=old_pos) - try: - with transaction.atomic(): - to_shift.update(sort_order=models.F('sort_order') + 1) - except IntegrityError: - for obj in to_shift.order_by('-sort_order'): - to_shift.filter(pk=obj.pk).update( - sort_order=models.F('sort_order') + 1, - ) + self._update(to_shift) self.sort_order = new_pos # self.sort_order increased. From 9f864ba718196fb7400b70a247f3b893eabaa10b Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Tue, 12 Jan 2016 09:50:10 +0000 Subject: [PATCH 7/9] Ignore more flake8 docstring warnings --- setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index 15901ef..006930e 100644 --- a/setup.cfg +++ b/setup.cfg @@ -2,7 +2,7 @@ universal = 1 [flake8] application-import-names = orderable -ignore = D100,D101,D102,D203,D204 +ignore = D100,D101,D102,D104,D105,D203,D204 import-order-style = google max-complexity = 10 max-line-length = 90 From 6e69fc3df6d2dfd537df651972dd1f87ee59ec3b Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Tue, 12 Jan 2016 11:52:27 +0000 Subject: [PATCH 8/9] Add docstring to SubTask.save test --- orderable/tests/test_models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/orderable/tests/test_models.py b/orderable/tests/test_models.py index 0a7c498..a90672d 100644 --- a/orderable/tests/test_models.py +++ b/orderable/tests/test_models.py @@ -215,7 +215,8 @@ def test_changing_parent(self): self.assertSequenceEqual(task.subtask_set.all(), [subtask, subtask_2]) @given(lists(integers(min_value=1), min_size=1, unique=True)) - def test_save_subtask(self, sort_orders): + def test_save_subtask_no_errors(self, sort_orders): + """Ensure Orderable.save does not raise IntegrityError.""" task = Task.objects.create() for order in sort_orders: From 497679a94323121d7516e9723fd0b89da97a722e Mon Sep 17 00:00:00 2001 From: Ian Foote Date: Tue, 12 Jan 2016 12:00:17 +0000 Subject: [PATCH 9/9] Add two explict examples to hypothesis test --- orderable/tests/test_models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/orderable/tests/test_models.py b/orderable/tests/test_models.py index a90672d..9c918b7 100644 --- a/orderable/tests/test_models.py +++ b/orderable/tests/test_models.py @@ -1,4 +1,4 @@ -from hypothesis import given +from hypothesis import example, given from hypothesis.extra.django import TestCase from hypothesis.strategies import integers, lists @@ -215,6 +215,8 @@ def test_changing_parent(self): self.assertSequenceEqual(task.subtask_set.all(), [subtask, subtask_2]) @given(lists(integers(min_value=1), min_size=1, unique=True)) + @example([2, 3, 1]) + @example([2, 3, 4]) def test_save_subtask_no_errors(self, sort_orders): """Ensure Orderable.save does not raise IntegrityError.""" task = Task.objects.create()