From c4630dca2bdafd6bac1bd2328081f524953872cf Mon Sep 17 00:00:00 2001 From: ibaguio Date: Sun, 12 Mar 2023 01:39:29 +0800 Subject: [PATCH 01/39] fix: utilize order_wrt _id instead of object the newly introduced OrderedModelBase._wrt_map method gets the value of the fields defined in order_with_respect_to. this forces django to query the object from the database, but only to be used for comparison. It is optimal to just compare the _id instead of the actual model object. --- ordered_model/models.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ordered_model/models.py b/ordered_model/models.py index d3314854..8bfa7c44 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -127,8 +127,11 @@ def __init__(self, *args, **kwargs): def _wrt_map(self): d = {} - for a in self.get_order_with_respect_to(): - d[a] = get_lookup_value(self, a) + for order_field_name in self.get_order_with_respect_to(): + if not order_field_name.endswith("_id"): + order_field_name = order_field_name + "_id" + d[order_field_name] = get_lookup_value(self, order_field_name) + return d @classmethod From 34ab3716b992b517a37ea756e2fc91e8c0c1380c Mon Sep 17 00:00:00 2001 From: ibaguio Date: Sun, 12 Mar 2023 02:10:33 +0800 Subject: [PATCH 02/39] Add query count tests --- tests/tests.py | 15 +++++++++++++-- tests/utils.py | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 tests/utils.py diff --git a/tests/tests.py b/tests/tests.py index 00bcbf24..9823f000 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -16,6 +16,7 @@ from rest_framework.test import APIRequestFactory, APITestCase from rest_framework import status from tests.drf import ItemViewSet, router +from tests.utils import assertNumQueries from ordered_model.models import OrderedModel from ordered_model.signals import on_ordered_model_delete @@ -164,6 +165,9 @@ def setUp(self): self.q1_a2 = q1.answers.create(user=u0) self.q2_a2 = q2.answers.create(user=u0) + with assertNumQueries(self, 1): + Answer.objects.get(id=self.q1_a1.id) + def test_saved_order(self): self.assertSequenceEqual( Answer.objects.values_list("pk", "order"), @@ -303,9 +307,14 @@ def setUp(self): self.u1_a2 = q1.answers.create(user=self.u1) self.u1_a3 = q1.answers.create(user=self.u1) + with assertNumQueries(self, 1): + Answer.objects.get(id=self.u0_a1.id) + def test_reorder_when_field_value_changed(self): self.u0_a2.user = self.u1 - self.u0_a2.save() + with assertNumQueries(self, 3): + self.u0_a2.save() + self.assertSequenceEqual( Answer.objects.values_list("pk", "order"), [ @@ -685,7 +694,9 @@ def setUp(self): ) # tomatoe, mozarella, mushrooms, ham # Now put the toppings on the pizza self.p1_t1 = PizzaToppingsThroughModel(pizza=self.p1, topping=self.t1) - self.p1_t1.save() + with assertNumQueries(self, 2): + self.p1_t1.save() + self.p1_t2 = PizzaToppingsThroughModel(pizza=self.p1, topping=self.t2) self.p1_t2.save() self.p1_t3 = PizzaToppingsThroughModel(pizza=self.p1, topping=self.t3) diff --git a/tests/utils.py b/tests/utils.py new file mode 100644 index 00000000..a23caebb --- /dev/null +++ b/tests/utils.py @@ -0,0 +1,37 @@ +# Query count helpers, copied from django source +# Added here for compatibility (django<3.1) +from django.db import DEFAULT_DB_ALIAS, connections +from django.test.utils import CaptureQueriesContext + + +class _AssertNumQueriesContext(CaptureQueriesContext): + def __init__(self, test_case, num, connection): + self.test_case = test_case + self.num = num + super().__init__(connection) + + def __exit__(self, exc_type, exc_value, traceback): + super().__exit__(exc_type, exc_value, traceback) + if exc_type is not None: + return + executed = len(self) + self.test_case.assertEqual( + executed, self.num, + "%d queries executed, %d expected\nCaptured queries were:\n%s" % ( + executed, self.num, + '\n'.join( + '%d. %s' % (i, query['sql']) for i, query in enumerate(self.captured_queries, start=1) + ) + ) + ) + + +def assertNumQueries(self, num, func=None, *args, using=DEFAULT_DB_ALIAS, **kwargs): + conn = connections[using] + + context = _AssertNumQueriesContext(self, num, conn) + if func is None: + return context + + with context: + func(*args, **kwargs) \ No newline at end of file From 2878689e1454beca98e64cad0f1942da177bfa1f Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 13 Mar 2023 16:10:22 +0000 Subject: [PATCH 03/39] run black tests/utils.py --- tests/utils.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/utils.py b/tests/utils.py index a23caebb..67e92b93 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -16,13 +16,17 @@ def __exit__(self, exc_type, exc_value, traceback): return executed = len(self) self.test_case.assertEqual( - executed, self.num, - "%d queries executed, %d expected\nCaptured queries were:\n%s" % ( - executed, self.num, - '\n'.join( - '%d. %s' % (i, query['sql']) for i, query in enumerate(self.captured_queries, start=1) - ) - ) + executed, + self.num, + "%d queries executed, %d expected\nCaptured queries were:\n%s" + % ( + executed, + self.num, + "\n".join( + "%d. %s" % (i, query["sql"]) + for i, query in enumerate(self.captured_queries, start=1) + ), + ), ) @@ -34,4 +38,4 @@ def assertNumQueries(self, num, func=None, *args, using=DEFAULT_DB_ALIAS, **kwar return context with context: - func(*args, **kwargs) \ No newline at end of file + func(*args, **kwargs) From 275091d5af70ce57069b73ed4ea1c5a914fa5c23 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Tue, 14 Mar 2023 11:43:40 +0000 Subject: [PATCH 04/39] add system Check that OrderedQuerySet and OrderedModelManager subclasses are returned --- ordered_model/models.py | 24 ++++++++++++++--- tests/tests.py | 57 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/ordered_model/models.py b/ordered_model/models.py index 8bfa7c44..c0cb3b32 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -315,20 +315,36 @@ def check(cls, **kwargs): ordering = getattr(cls._meta, "ordering", None) if ordering is None or len(ordering) < 1: errors.append( - checks.Warning( + checks.Error( "OrderedModelBase subclass needs Meta.ordering specified.", hint="If you have overwritten Meta, try inheriting with Meta(OrderedModel.Meta).", obj=str(cls.__qualname__), - id="ordered_model.W001", + id="ordered_model.E001", ) ) owrt = getattr(cls, "order_with_respect_to") if not (type(owrt) is tuple or type(owrt) is str or owrt is None): errors.append( checks.Error( - "OrderedModelBase subclass order_with_respect_to value invalid. Expected tuple, str or None", + "OrderedModelBase subclass order_with_respect_to value invalid. Expected tuple, str or None.", obj=str(cls.__qualname__), - id="ordered_model.E001", + id="ordered_model.E002", + ) + ) + if not issubclass(cls.objects.__class__, OrderedModelManager): + errors.append( + checks.Error( + "OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager.", + obj=str(cls.__qualname__), + id="ordered_model.E003", + ) + ) + if not issubclass(cls.objects.none().__class__, OrderedModelQuerySet): + errors.append( + checks.Error( + "OrderedModelBase subclass ModelManager did not return a QuerySet inheriting from OrderedModelQuerySet.", + obj=str(cls.__qualname__), + id="ordered_model.E004", ) ) return errors diff --git a/tests/tests.py b/tests/tests.py index 9823f000..a10727ba 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -4,6 +4,7 @@ from django.contrib.auth.models import User from django.core.management import call_command from django.core import checks +from django.db import models from django.db.models.signals import post_delete from django.dispatch import Signal from django.utils.timezone import now @@ -18,7 +19,7 @@ from tests.drf import ItemViewSet, router from tests.utils import assertNumQueries -from ordered_model.models import OrderedModel +from ordered_model.models import OrderedModel, OrderedModelManager, OrderedModelQuerySet from ordered_model.signals import on_ordered_model_delete from tests.models import ( @@ -1280,11 +1281,11 @@ class Meta: self.assertEqual( checks.run_checks(app_configs=self.apps.get_app_configs()), [ - checks.Warning( + checks.Error( msg="OrderedModelBase subclass needs Meta.ordering specified.", hint="If you have overwritten Meta, try inheriting with Meta(OrderedModel.Meta).", obj="ChecksTest.test_no_inherited_ordering..TestModel", - id="ordered_model.W001", + id="ordered_model.E001", ) ], ) @@ -1304,6 +1305,56 @@ class Meta(OrderedModel.Meta): self.assertEqual(checks.run_checks(app_configs=self.apps.get_app_configs()), []) + def test_bad_ort(self): + class TestModel(OrderedModel): + order_with_respect_to = 7 + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [ + checks.Error( + msg="OrderedModelBase subclass order_with_respect_to value invalid. Expected tuple, str or None.", + obj="ChecksTest.test_bad_ort..TestModel", + id="ordered_model.E002", + ) + ], + ) + + def test_bad_manager(self): + class BadModelManager(models.Manager.from_queryset(OrderedModelQuerySet)): + pass + class TestModel(OrderedModel): + objects = BadModelManager() + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [ + checks.Error( + msg="OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager.", + obj="ChecksTest.test_bad_manager..TestModel", + id="ordered_model.E003", + ) + ], + ) + + def test_bad_queryset(self): + # I've swapped the inheritance order here so that the models.QuerySet is returned + class BadQSModelManager(models.Manager.from_queryset(models.QuerySet), OrderedModelManager): + pass + class TestModel(OrderedModel): + objects = BadQSModelManager() + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [ + checks.Error( + msg="OrderedModelBase subclass ModelManager did not return a QuerySet inheriting from OrderedModelQuerySet.", + obj="ChecksTest.test_bad_queryset..TestModel", + id="ordered_model.E004", + ) + ], + ) + class TestCascadedDelete(TestCase): def test_that_model_when_deleted_by_cascade_still_maintains_ordering(self): From 34f267813ffedc226bfefe54a2efe62956a81fef Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Tue, 14 Mar 2023 12:57:49 +0000 Subject: [PATCH 05/39] check that order_with_respect_to values are ForeignKey fields --- ordered_model/models.py | 38 ++++++++++++++++++++++++++++++++-- tests/tests.py | 45 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/ordered_model/models.py b/ordered_model/models.py index c0cb3b32..cc76e091 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -1,9 +1,10 @@ from functools import partial, reduce from django.core import checks -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, FieldDoesNotExist from django.db import models from django.db.models import Max, Min, F +from django.db.models.fields.related import ForeignKey from django.db.models.constants import LOOKUP_SEP from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ @@ -143,7 +144,7 @@ def get_order_with_respect_to(cls): elif cls.order_with_respect_to is None: return tuple() else: - raise AssertionError("Invalid value for model.order_with_respect_to") + raise ValueError("Invalid value for model.order_with_respect_to") def _validate_ordering_reference(self, ref): if self._wrt_map() != ref._wrt_map(): @@ -347,6 +348,39 @@ def check(cls, **kwargs): id="ordered_model.E004", ) ) + + # each field may be an FK, or recursively an FK ref to an FK + try: + for wrt_field in cls.get_order_with_respect_to(): + mc = cls + for p in wrt_field.split(LOOKUP_SEP): + try: + f = mc._meta.get_field(p) + if not isinstance(f, ForeignKey): + errors.append( + checks.Error( + "OrderedModel order_with_respect_to specifies field '{0}' (within '{1}') which is not a ForeignKey. This is unsupported.".format( + p, wrt_field + ), + obj=str(cls.__qualname__), + id="ordered_model.E005", + ) + ) + break + mc = f.remote_field.model + except FieldDoesNotExist: + errors.append( + checks.Error( + "OrderedModel order_with_respect_to specifies field '{0}' (within '{1}') which does not exist.".format( + p, wrt_field + ), + obj=str(cls.__qualname__), + id="ordered_model.E006", + ) + ) + except ValueError: + # already handled by type checks for E002 + pass return errors diff --git a/tests/tests.py b/tests/tests.py index a10727ba..d10e85bb 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1305,7 +1305,7 @@ class Meta(OrderedModel.Meta): self.assertEqual(checks.run_checks(app_configs=self.apps.get_app_configs()), []) - def test_bad_ort(self): + def test_bad_owrt(self): class TestModel(OrderedModel): order_with_respect_to = 7 @@ -1314,7 +1314,7 @@ class TestModel(OrderedModel): [ checks.Error( msg="OrderedModelBase subclass order_with_respect_to value invalid. Expected tuple, str or None.", - obj="ChecksTest.test_bad_ort..TestModel", + obj="ChecksTest.test_bad_owrt..TestModel", id="ordered_model.E002", ) ], @@ -1323,6 +1323,7 @@ class TestModel(OrderedModel): def test_bad_manager(self): class BadModelManager(models.Manager.from_queryset(OrderedModelQuerySet)): pass + class TestModel(OrderedModel): objects = BadModelManager() @@ -1339,8 +1340,11 @@ class TestModel(OrderedModel): def test_bad_queryset(self): # I've swapped the inheritance order here so that the models.QuerySet is returned - class BadQSModelManager(models.Manager.from_queryset(models.QuerySet), OrderedModelManager): + class BadQSModelManager( + models.Manager.from_queryset(models.QuerySet), OrderedModelManager + ): pass + class TestModel(OrderedModel): objects = BadQSModelManager() @@ -1355,6 +1359,41 @@ class TestModel(OrderedModel): ], ) + def test_owrt_not_foreign_key(self): + class TestModel(OrderedModel): + name = models.CharField(max_length=100) + order_with_respect_to = "name" + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [ + checks.Error( + msg="OrderedModel order_with_respect_to specifies field 'name' (within 'name') which is not a ForeignKey. This is unsupported.", + obj="ChecksTest.test_owrt_not_foreign_key..TestModel", + id="ordered_model.E005", + ) + ], + ) + + def test_owrt_not_immediate_foreign_key(self): + class TestTargetModel(OrderedModel): + name = models.CharField(max_length=100) + + class TestModel(OrderedModel): + target = models.ForeignKey(to=TestTargetModel, on_delete=models.CASCADE) + order_with_respect_to = "target__name" + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [ + checks.Error( + msg="OrderedModel order_with_respect_to specifies field 'name' (within 'target__name') which is not a ForeignKey. This is unsupported.", + obj="ChecksTest.test_owrt_not_immediate_foreign_key..TestModel", + id="ordered_model.E005", + ) + ], + ) + class TestCascadedDelete(TestCase): def test_that_model_when_deleted_by_cascade_still_maintains_ordering(self): From f43e93f0623c2fa01cf2353a8fa861060f6440a1 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Tue, 14 Mar 2023 14:35:34 +0000 Subject: [PATCH 06/39] add slowpath for admin that queries FK objects --- ordered_model/admin.py | 18 +++++++----------- ordered_model/models.py | 16 +++++++++++----- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/ordered_model/admin.py b/ordered_model/admin.py index 0cc360c0..2536339e 100644 --- a/ordered_model/admin.py +++ b/ordered_model/admin.py @@ -209,17 +209,13 @@ def move_up_down_links(self, obj): # Find the fields which refer to the parent model of this inline, and # use one of them if they aren't None. - order_with_respect_to = obj._wrt_map() or [] - fields = [ - str(value.pk) - for value in order_with_respect_to.values() - if ( - type(value) == self.parent_model - or issubclass(self.parent_model, type(value)) - ) - and value is not None - and value.pk is not None - ] + fields = [] + for value in obj._get_related_objects(): + # Note 'a class is considered a subclass of itself' pydocs + if issubclass(self.parent_model, type(value)): + if value is not None and value.pk is not None: + fields.append(str(value.pk)) + order_obj_name = fields[0] if len(fields) > 0 else None model_info = self._get_model_info() diff --git a/ordered_model/models.py b/ordered_model/models.py index cc76e091..c508581e 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -128,13 +128,19 @@ def __init__(self, *args, **kwargs): def _wrt_map(self): d = {} - for order_field_name in self.get_order_with_respect_to(): - if not order_field_name.endswith("_id"): - order_field_name = order_field_name + "_id" - d[order_field_name] = get_lookup_value(self, order_field_name) - + for order_wrt_name in self.get_order_with_respect_to(): + # we know order_wrt_name is a ForeignKey, so use a cheaper _id lookup + field_path = order_wrt_name + "_id" + d[order_wrt_name] = get_lookup_value(self, field_path) return d + def _get_related_objects(self): + # slow path, for use in the admin which requires the objects + # expected to generate extra queries + return [ + get_lookup_value(self, name) for name in self.get_order_with_respect_to() + ] + @classmethod def get_order_with_respect_to(cls): if type(cls.order_with_respect_to) is tuple: From 71d6e074432e52ae972acf80c6015673c42eca07 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Tue, 14 Mar 2023 15:05:33 +0000 Subject: [PATCH 07/39] update docs prior to release --- CHANGES.md | 7 +++++++ README.md | 6 ++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 6b5b522a..dc6414d4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,13 @@ Change log Unreleased ---------- +3.7.2 - 2023-03-14 +---------- +- Fix a performance regression (unnecessary queries) in the WRT change detection (#286) +- Add a Check that `order_with_respect_to` specifies only ForeignKey fields +- Add a Check that our subclasses of ModelManager and QuerySet are used (#286) + + 3.7.1 - 2023-03-06 ---------- diff --git a/README.md b/README.md index f409be21..2be08965 100644 --- a/README.md +++ b/README.md @@ -211,6 +211,8 @@ class GroupedItem(OrderedModel): Here items are put into groups that have some general information used by its items, but the ordering of the items is independent of the group the item is in. +In all cases `order_with_respect_to` must specify a `ForeignKey` field on the model, or a Django Check `E002`, `E005` or `E006` error will be raised with further help. + When you want ordering on the baseclass instead of subclasses in an ordered list of objects of various classes, specify the full module path of the base class: ```python @@ -242,9 +244,9 @@ When your model your extends `OrderedModel`, it inherits a custom `ModelManager` * `above(index)`, * `below(index)` -If your `Model` uses a custom `ModelManager` (such as `ItemManager` below) please have it extend `OrderedModelManager`. +If your `Model` uses a custom `ModelManager` (such as `ItemManager` below) please have it extend `OrderedModelManager`, or else Django Check `E003` will be raised. -If your `ModelManager` returns a custom `QuerySet` (such as `ItemQuerySet` below) please have it extend `OrderedModelQuerySet`. +If your `ModelManager` returns a custom `QuerySet` (such as `ItemQuerySet` below) please have it extend `OrderedModelQuerySet`, or Django Check `E004` will be raised. ```python from ordered_model.models import OrderedModel, OrderedModelManager, OrderedModelQuerySet From ed8337bdd0abb1bb5d0d00a2f2f36b830cce9223 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Tue, 14 Mar 2023 15:25:24 +0000 Subject: [PATCH 08/39] update version --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 1e9dea50..76017d8f 100644 --- a/setup.py +++ b/setup.py @@ -12,7 +12,7 @@ name="django-ordered-model", long_description=long_description, long_description_content_type="text/markdown", - version="3.7.1", + version="3.7.2", description="Allows Django models to be ordered and provides a simple admin interface for reordering them.", author="Ben Firshman", author_email="ben@firshman.co.uk", From ab72d563f83bcec7d4a454c615b0c46c9e30d13f Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Tue, 14 Mar 2023 18:51:35 +0000 Subject: [PATCH 09/39] Fix `reorder_model` management command re-ordering with multiple `order_with_respect_to` values --- CHANGES.md | 2 + .../management/commands/reorder_model.py | 24 ++++-------- tests/tests.py | 39 +++++++++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index dc6414d4..583338bb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,8 @@ Change log Unreleased ---------- +- Fix `reorder_model` management command re-ordering with multiple `order_with_respect_to` values + 3.7.2 - 2023-03-14 ---------- - Fix a performance regression (unnecessary queries) in the WRT change detection (#286) diff --git a/ordered_model/management/commands/reorder_model.py b/ordered_model/management/commands/reorder_model.py index 8507dd95..45d74be5 100644 --- a/ordered_model/management/commands/reorder_model.py +++ b/ordered_model/management/commands/reorder_model.py @@ -5,17 +5,6 @@ from ordered_model.models import OrderedModelBase -def get_order_with_respect_to(model): - if not model.order_with_respect_to: - return None - if isinstance(model.order_with_respect_to, str): - return model.order_with_respect_to - assert ( - len(model.order_with_respect_to) <= 1 - ), "re-ordering with more than 1 fields in order_with_respect_to is not supported" - return model.order_with_respect_to[0] - - class Command(BaseCommand): help = "Re-do the ordering of a certain Model" @@ -54,17 +43,18 @@ def handle(self, *args, **options): self.reorder(model) def reorder(self, model): - if model.order_with_respect_to: - order_with_respect_to = get_order_with_respect_to(model) - rel_kwargs = {"{}__isnull".format(order_with_respect_to): False} + owrt = model.get_order_with_respect_to() + if owrt: + rel_kwargs = dict([("{}__isnull".format(k), False) for k in owrt]) relation_to_list = ( - model.objects.order_by(order_with_respect_to) - .values_list(order_with_respect_to, flat=True) + model.objects.order_by(*owrt) + .values_list(*owrt) .filter(**rel_kwargs) .distinct() ) for relation_to in relation_to_list: - kwargs = {order_with_respect_to: relation_to} + kwargs = dict([(k, v) for k, v in zip(owrt, relation_to)]) + # print('re-ordering: {}'.format(kwargs)) self.reorder_queryset(model.objects.filter(**kwargs)) else: self.reorder_queryset(model.objects.all()) diff --git a/tests/tests.py b/tests/tests.py index d10e85bb..444c4895 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1125,6 +1125,45 @@ def test_reorder_with_respect_to(self): "changing order of tests.GroupedItem (3) from 1 to 2\n", out.getvalue() ) + def test_reorder_with_respect_to_tuple(self): + u1 = TestUser.objects.create() + u2 = TestUser.objects.create() + q1 = Question.objects.create() + q2 = Question.objects.create() + + for q in (q1, q2): + for u in (u1, u2): + Answer.objects.create(user=u, question=q, order=0) + Answer.objects.create(user=u, question=q, order=0) + + self.assertSequenceEqual( + Answer.objects.filter(user=u2, question=q1).values_list("order", flat=True), + [0, 0], + ) + + out = StringIO() + call_command("reorder_model", "tests.Answer", verbosity=1, stdout=out) + + self.assertSequenceEqual( + Answer.objects.filter(user=u2, question=q1).values_list("order", flat=True), + [0, 1], + ) + + self.assertEqual( + ( + "changing order of tests.Answer (2) from 0 to 1\n" + + "changing order of tests.Answer (4) from 0 to 1\n" + + "changing order of tests.Answer (6) from 0 to 1\n" + + "changing order of tests.Answer (8) from 0 to 1\n" + ), + out.getvalue(), + ) + + out = StringIO() + call_command("reorder_model", "tests.Answer", verbosity=1, stdout=out) + + self.assertEqual("", out.getvalue()) + def test_reorder_with_custom_order_field(self): """ Test that 'reorder_model' changes the order of OpenQuestions From 743cb6d720a3fd31f53828193d352408e8f0d5a1 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Wed, 15 Mar 2023 23:37:44 +0000 Subject: [PATCH 10/39] rework cascaded/queryset delete detection to avoid global signal listener Restrict signal handler 'senders' to subclasses of `OrderedModelBase` to avoid query count regression due to `Collector.can_fast_delete` logic in `models/deletion.py fixes #288 --- CHANGES.md | 3 +++ ordered_model/apps.py | 13 +++++++++---- ordered_model/models.py | 19 +++++++++++++++++++ ordered_model/signals.py | 30 ------------------------------ setup.py | 2 +- tests/tests.py | 22 +++++++++++++++++++--- 6 files changed, 51 insertions(+), 38 deletions(-) delete mode 100644 ordered_model/signals.py diff --git a/CHANGES.md b/CHANGES.md index 583338bb..004a43c6 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,9 @@ Change log Unreleased ---------- +3.7.3 - 2023-03-15 +---------- +- Restrict signal handler 'senders' to subclasses of `OrderedModelBase` to avoid query count regression due to `Collector.can_fast_delete` logic in `models/deletion.py` (#288) - Fix `reorder_model` management command re-ordering with multiple `order_with_respect_to` values 3.7.2 - 2023-03-14 diff --git a/ordered_model/apps.py b/ordered_model/apps.py index 812a21e8..cd5ef707 100644 --- a/ordered_model/apps.py +++ b/ordered_model/apps.py @@ -1,4 +1,5 @@ -from django.apps import AppConfig +from django.apps import AppConfig, apps +from django.db.models.signals import post_delete class OrderedModelConfig(AppConfig): @@ -6,6 +7,10 @@ class OrderedModelConfig(AppConfig): label = "ordered_model" def ready(self): - # This import has side effects - # noinspection PyUnresolvedReferences - from .signals import on_ordered_model_delete + from .models import OrderedModelBase + + for cls in apps.get_models(): + if issubclass(cls, OrderedModelBase): + post_delete.connect( + cls._on_ordered_model_delete, sender=cls, dispatch_uid=cls.__name__ + ) diff --git a/ordered_model/models.py b/ordered_model/models.py index c508581e..04f10a8d 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -141,6 +141,25 @@ def _get_related_objects(self): get_lookup_value(self, name) for name in self.get_order_with_respect_to() ] + @classmethod + def _on_ordered_model_delete(cls, sender=None, instance=None, **kwargs): + """ + This signal handler makes sure that when an OrderedModelBase is deleted via + cascade database deletes, or queryset delete that the models keep order. + """ + + if getattr(instance, "_was_deleted_via_delete_method", False): + return + + extra_update = kwargs.get("extra_update", None) + + # Copy of upshuffle logic from OrderedModelBase.delete + qs = instance.get_ordering_queryset() + extra_update = {} if extra_update is None else extra_update + qs.above_instance(instance).decrease_order(**extra_update) + + setattr(instance, "_was_deleted_via_delete_method", True) + @classmethod def get_order_with_respect_to(cls): if type(cls.order_with_respect_to) is tuple: diff --git a/ordered_model/signals.py b/ordered_model/signals.py deleted file mode 100644 index 1b846312..00000000 --- a/ordered_model/signals.py +++ /dev/null @@ -1,30 +0,0 @@ -from django.db.models.signals import post_delete -from django.dispatch import receiver -from ordered_model.models import OrderedModelBase -from django.db.models import F - - -@receiver(post_delete, dispatch_uid="on_ordered_model_delete") -def on_ordered_model_delete(sender, instance, **kwargs): - """ - This signal makes sure that when an OrderedModelBase is deleted via cascade database deletes, the models - keep order. - """ - - """ - We're only interested in subclasses of OrderedModelBase. - We want to be able to support 'extra_kwargs' on the delete() - method, which we can't do if we do all our work in the signal. We add a property to signal whether or not - the model's .delete() method was called, because if so - we don't need to do any more work. - """ - if not issubclass(sender, OrderedModelBase): - return - if getattr(instance, "_was_deleted_via_delete_method", False): - return - - extra_update = kwargs.get("extra_update", None) - - # Copy of upshuffle logic from OrderedModelBase.delete - qs = instance.get_ordering_queryset() - extra_update = {} if extra_update is None else extra_update - qs.above_instance(instance).decrease_order(**extra_update) diff --git a/setup.py b/setup.py index 76017d8f..91a782b0 100644 --- a/setup.py +++ b/setup.py @@ -12,7 +12,7 @@ name="django-ordered-model", long_description=long_description, long_description_content_type="text/markdown", - version="3.7.2", + version="3.7.3", description="Allows Django models to be ordered and provides a simple admin interface for reordering them.", author="Ben Firshman", author_email="ben@firshman.co.uk", diff --git a/tests/tests.py b/tests/tests.py index 444c4895..f7e98376 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -20,7 +20,7 @@ from tests.utils import assertNumQueries from ordered_model.models import OrderedModel, OrderedModelManager, OrderedModelQuerySet -from ordered_model.signals import on_ordered_model_delete + from tests.models import ( Answer, @@ -33,6 +33,7 @@ Pizza, Topping, PizzaToppingsThroughModel, + BaseQuestion, OpenQuestion, MultipleChoiceQuestion, ItemGroup, @@ -1202,10 +1203,25 @@ def test_delete_bypass(self): OpenQuestion.objects.create(answer="4", order=3) # bypass our OrderedModel delete logic to leave a hole in ordering - self.assertTrue(post_delete.disconnect(dispatch_uid="on_ordered_model_delete")) + # remove signal handlers + # print(post_delete.receivers) + self.assertTrue( + post_delete.disconnect( + sender=OpenQuestion, dispatch_uid=OpenQuestion.__name__ + ) + ) + self.assertTrue( + post_delete.disconnect( + sender=BaseQuestion, dispatch_uid=BaseQuestion.__name__ + ) + ) + + # delete on the queryset fires post_delete, but does not call model.delete() OpenQuestion.objects.filter(answer="3").delete() post_delete.connect( - on_ordered_model_delete, dispatch_uid="on_ordered_model_delete" + OpenQuestion._on_ordered_model_delete, + sender=OpenQuestion, + dispatch_uid=OpenQuestion.__name__, ) self.assertEqual([0, 1, 3], [i.order for i in OpenQuestion.objects.all()]) From 7b9c89f75acc8216e45931e29ca7c1e360802ff7 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 17 Mar 2023 15:39:23 +0000 Subject: [PATCH 11/39] relax OrderedModelManager check to a Warning if it returns correct OrderedModelQuerySet --- CHANGES.md | 5 +++++ ordered_model/models.py | 25 ++++++++++++++++++------- setup.py | 2 +- tests/tests.py | 17 ++++++++++++++++- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 004a43c6..0ae0dd33 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,11 @@ Change log Unreleased ---------- +3.7.4 - 2023-03-17 +---------- +- Relax Check for `OrderedModelManager` to a `Warning`, if the manager returns the correct queryset (#290) + + 3.7.3 - 2023-03-15 ---------- - Restrict signal handler 'senders' to subclasses of `OrderedModelBase` to avoid query count regression due to `Collector.can_fast_delete` logic in `models/deletion.py` (#288) diff --git a/ordered_model/models.py b/ordered_model/models.py index 04f10a8d..c00754c8 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -358,14 +358,25 @@ def check(cls, **kwargs): ) ) if not issubclass(cls.objects.__class__, OrderedModelManager): - errors.append( - checks.Error( - "OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager.", - obj=str(cls.__qualname__), - id="ordered_model.E003", + # Not using our Manager. This is an Error if the queryset is also wrong, or + # a Warning if our own QuerySet is returned. + if issubclass(cls.objects.none().__class__, OrderedModelQuerySet): + errors.append( + checks.Warning( + "OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager. This is not ideal but will work.", + obj=str(cls.__qualname__), + id="ordered_model.W003", + ) ) - ) - if not issubclass(cls.objects.none().__class__, OrderedModelQuerySet): + else: + errors.append( + checks.Error( + "OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager.", + obj=str(cls.__qualname__), + id="ordered_model.E003", + ) + ) + elif not issubclass(cls.objects.none().__class__, OrderedModelQuerySet): errors.append( checks.Error( "OrderedModelBase subclass ModelManager did not return a QuerySet inheriting from OrderedModelQuerySet.", diff --git a/setup.py b/setup.py index 91a782b0..22ab5391 100644 --- a/setup.py +++ b/setup.py @@ -12,7 +12,7 @@ name="django-ordered-model", long_description=long_description, long_description_content_type="text/markdown", - version="3.7.3", + version="3.7.4", description="Allows Django models to be ordered and provides a simple admin interface for reordering them.", author="Ben Firshman", author_email="ben@firshman.co.uk", diff --git a/tests/tests.py b/tests/tests.py index f7e98376..b19644c3 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1376,7 +1376,7 @@ class TestModel(OrderedModel): ) def test_bad_manager(self): - class BadModelManager(models.Manager.from_queryset(OrderedModelQuerySet)): + class BadModelManager(models.Manager.from_queryset(models.QuerySet)): pass class TestModel(OrderedModel): @@ -1393,6 +1393,21 @@ class TestModel(OrderedModel): ], ) + def test_builtin_manager_to_queryset(self): + class TestModel(OrderedModel): + objects = OrderedModelQuerySet.as_manager() + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [ + checks.Warning( + msg="OrderedModelBase subclass has a ModelManager that does not inherit from OrderedModelManager. This is not ideal but will work.", + obj="ChecksTest.test_builtin_manager_to_queryset..TestModel", + id="ordered_model.W003", + ) + ], + ) + def test_bad_queryset(self): # I've swapped the inheritance order here so that the models.QuerySet is returned class BadQSModelManager( From 5e03a93823c4cded172d37dcfd4743d57b60b0e6 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 31 Mar 2023 11:02:29 +0100 Subject: [PATCH 12/39] rename duplicate test methods #296 --- tests/tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tests.py b/tests/tests.py index b19644c3..e0d9bcb5 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -738,10 +738,10 @@ def test_previous(self): def test_previous_first(self): self.assertEqual(self.p2_t1.previous(), None) - def test_down(self): + def test_next(self): self.assertEqual(self.p2_t1.next(), self.p2_t2) - def test_down_last(self): + def test_next_last(self): self.assertEqual(self.p1_t3.next(), None) def test_up(self): From 715217264735f668a1396e4efd5144d201aa75d7 Mon Sep 17 00:00:00 2001 From: Joris Jansen Date: Mon, 5 Jun 2023 15:56:47 +0200 Subject: [PATCH 13/39] Remove extra 'your' in README --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 2be08965..e643b3bc 100644 --- a/README.md +++ b/README.md @@ -211,7 +211,7 @@ class GroupedItem(OrderedModel): Here items are put into groups that have some general information used by its items, but the ordering of the items is independent of the group the item is in. -In all cases `order_with_respect_to` must specify a `ForeignKey` field on the model, or a Django Check `E002`, `E005` or `E006` error will be raised with further help. +In all cases `order_with_respect_to` must specify a `ForeignKey` field on the model, or a Django Check `E002`, `E005` or `E006` error will be raised with further help. When you want ordering on the baseclass instead of subclasses in an ordered list of objects of various classes, specify the full module path of the base class: @@ -235,7 +235,7 @@ class OpenQuestion(BaseQuestion): Custom Manager and QuerySet ----------------- -When your model your extends `OrderedModel`, it inherits a custom `ModelManager` instance which in turn provides additional operations on the resulting `QuerySet`. For example if `Item` is an `OrderedModel` subclass, the queryset `Item.objects.all()` has functions: +When your model extends `OrderedModel`, it inherits a custom `ModelManager` instance which in turn provides additional operations on the resulting `QuerySet`. For example if `Item` is an `OrderedModel` subclass, the queryset `Item.objects.all()` has functions: * `above_instance(object)`, * `below_instance(object)`, From 3e2c0c448049f94588d359c838032c5123fdca6c Mon Sep 17 00:00:00 2001 From: Solomon Hawk Date: Wed, 28 Jun 2023 11:15:13 -0400 Subject: [PATCH 14/39] Return super().delete() in OrderedModel#delete - this makes django-ordered-model play nicely with other libraries that rely on the return value of model#delete such as django-safedelete --- ordered_model/models.py | 2 +- tests/tests.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/ordered_model/models.py b/ordered_model/models.py index c00754c8..ef1a9cc5 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -230,7 +230,7 @@ def delete(self, *args, extra_update=None, **kwargs): qs = self.get_ordering_queryset() extra_update = {} if extra_update is None else extra_update qs.above_instance(self).decrease_order(**extra_update) - super().delete(*args, **kwargs) + return super().delete(*args, **kwargs) def swap(self, replacement): """ diff --git a/tests/tests.py b/tests/tests.py index e0d9bcb5..c295ef4b 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -151,7 +151,10 @@ def test_below_self(self): self.assertNames(["1", "2", "3", "4"]) def test_delete(self): - Item.objects.get(pk=2).delete() + deleted = Item.objects.get(pk=2).delete() + # the default return value of delete is (num_deleted, deleted_count_per_model) + # https://github.com/django/django/blob/main/django/db/models/deletion.py#L522 + self.assertEqual(deleted, (1, {"tests.Item": 1})) self.assertNames(["1", "3", "4"]) Item.objects.get(pk=3).up() self.assertNames(["3", "1", "4"]) From df489a0eb4193e99754778539bbd59c44e9ef206 Mon Sep 17 00:00:00 2001 From: mkuehne Date: Thu, 20 Jul 2023 17:43:12 +0200 Subject: [PATCH 15/39] #306 do not rely on upshuffle logic for potential out of order bulk operations --- ordered_model/models.py | 15 +++++++++------ tests/tests.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/ordered_model/models.py b/ordered_model/models.py index ef1a9cc5..29e8183f 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -151,12 +151,15 @@ def _on_ordered_model_delete(cls, sender=None, instance=None, **kwargs): if getattr(instance, "_was_deleted_via_delete_method", False): return - extra_update = kwargs.get("extra_update", None) - - # Copy of upshuffle logic from OrderedModelBase.delete - qs = instance.get_ordering_queryset() - extra_update = {} if extra_update is None else extra_update - qs.above_instance(instance).decrease_order(**extra_update) + # upshuffle logic from OrderedModelBase.delete can't be used here because signal + # handlers run per instance, but not necessarily in the right order + qs = instance.get_ordering_queryset().only("pk", instance.order_field_name) + to_update = set() + for i, item in enumerate(qs): + if getattr(item, instance.order_field_name) != i: + setattr(item, instance.order_field_name, i) + to_update.add(item) + qs.bulk_update(to_update, (instance.order_field_name,)) setattr(instance, "_was_deleted_via_delete_method", True) diff --git a/tests/tests.py b/tests/tests.py index c295ef4b..b0c1c8fd 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1495,3 +1495,35 @@ def test_that_model_when_deleted_by_cascade_still_maintains_ordering(self): # Assert the hole has been filled self.assertEqual(child_with_order_0.order, 0) self.assertEqual(child_with_order_2.order, 1) + + def test_that_model_when_multiple_unordered_deleted_by_cascade_still_maintain_ordering( + self, + ): + parent_for_order_1_and_0_child = CascadedParentModel.objects.create() + # reverse the order on the first two children + child_with_order_1 = CascadedOrderedModel.objects.create( + parent=parent_for_order_1_and_0_child, + order=1, + ) + child_with_order_0 = CascadedOrderedModel.objects.create( + parent=parent_for_order_1_and_0_child, + order=0, + ) + parent_for_order_2_and_3_child = CascadedParentModel.objects.create() + child_with_order_2 = CascadedOrderedModel.objects.create( + parent=parent_for_order_2_and_3_child + ) + child_with_order_3 = CascadedOrderedModel.objects.create( + parent=parent_for_order_2_and_3_child + ) + + # Delete positition 0 and 1 parent, now there's a hole of two, which child_with_order_2 and 3 should take + parent_for_order_1_and_0_child.delete() + + # Refresh children from db + child_with_order_2.refresh_from_db() + child_with_order_3.refresh_from_db() + + # Assert the hole has been filled + self.assertEqual(child_with_order_2.order, 0) + self.assertEqual(child_with_order_3.order, 1) From 3df23a82641d05737993063289c8204c9573de2a Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Wed, 18 Oct 2023 21:08:40 +0100 Subject: [PATCH 16/39] update CHANGES.md --- CHANGES.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 0ae0dd33..75484d3a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,6 +3,8 @@ Change log Unreleased ---------- +- Fix `post_delete` signal triggered upshuffles to do a potentially expensive full reordering of the owrt group (#307) + 3.7.4 - 2023-03-17 ---------- @@ -14,6 +16,7 @@ Unreleased - Restrict signal handler 'senders' to subclasses of `OrderedModelBase` to avoid query count regression due to `Collector.can_fast_delete` logic in `models/deletion.py` (#288) - Fix `reorder_model` management command re-ordering with multiple `order_with_respect_to` values + 3.7.2 - 2023-03-14 ---------- - Fix a performance regression (unnecessary queries) in the WRT change detection (#286) From 1df98a48858948a764459c7cdd9a5c16d72d72e9 Mon Sep 17 00:00:00 2001 From: Solomon Hawk Date: Wed, 28 Jun 2023 11:08:36 -0400 Subject: [PATCH 17/39] Add `--batch_size` arg to management command (for bulk_update) --- .../management/commands/reorder_model.py | 8 ++++- tests/tests.py | 36 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/ordered_model/management/commands/reorder_model.py b/ordered_model/management/commands/reorder_model.py index 45d74be5..1be77ba2 100644 --- a/ordered_model/management/commands/reorder_model.py +++ b/ordered_model/management/commands/reorder_model.py @@ -10,6 +10,7 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument("model_name", type=str, nargs="*") + parser.add_argument("--batch_size", type=int, nargs=1, default=1000) def handle(self, *args, **options): """ @@ -17,6 +18,8 @@ def handle(self, *args, **options): try re-ordering to a working state. """ self.verbosity = options["verbosity"] + self.batch_size = options["batch_size"] + orderedmodels = [ m._meta.label for m in apps.get_models() if issubclass(m, OrderedModelBase) ] @@ -78,4 +81,7 @@ def reorder_queryset(self, queryset): ) setattr(obj, order_field_name, order) bulk_update_list.append(obj) - model.objects.bulk_update(bulk_update_list, [order_field_name]) + + model.objects.bulk_update( + bulk_update_list, [order_field_name], batch_size=self.batch_size + ) diff --git a/tests/tests.py b/tests/tests.py index b0c1c8fd..06cf95fd 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1245,6 +1245,42 @@ def test_delete_bypass(self): "changing order of tests.OpenQuestion (4) from 3 to 2\n", out.getvalue() ) + def test_reorder_with_custom_batch_size(self): + """ + Test that 'reorder_model' can be called with a valid `batch_size` argument. + """ + OpenQuestion.objects.create(order=0) + OpenQuestion.objects.create(order=0) + out = StringIO() + call_command( + "reorder_model", "tests.OpenQuestion", verbosity=1, stdout=out, batch_size=2 + ) + + self.assertSequenceEqual( + OpenQuestion.objects.values_list("order", flat=True).order_by("order"), + [0, 1], + ) + self.assertIn( + "changing order of tests.OpenQuestion (2) from 0 to 1", out.getvalue() + ) + + def test_reorder_with_invalid_custom_batch_size(self): + """ + Test that 'reorder_model' raises a TypeError if a non-int value is passed + as the `batch_size` argument. + """ + OpenQuestion.objects.create(order=0) + OpenQuestion.objects.create(order=0) + + with self.assertRaises(TypeError): + call_command( + "reorder_model", + "tests.OpenQuestion", + verbosity=1, + stdout=StringIO(), + batch_size="2", + ) + class DRFTestCase(APITestCase): fixtures = ["test_items.json"] From a3294b9f7e5e9a5f81a7f26bc25f184295374edb Mon Sep 17 00:00:00 2001 From: Solomon Hawk Date: Wed, 18 Oct 2023 16:35:59 -0400 Subject: [PATCH 18/39] Set default batch_size to None --- ordered_model/management/commands/reorder_model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ordered_model/management/commands/reorder_model.py b/ordered_model/management/commands/reorder_model.py index 1be77ba2..bcbc5db4 100644 --- a/ordered_model/management/commands/reorder_model.py +++ b/ordered_model/management/commands/reorder_model.py @@ -10,7 +10,7 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument("model_name", type=str, nargs="*") - parser.add_argument("--batch_size", type=int, nargs=1, default=1000) + parser.add_argument("--batch_size", type=int, nargs=1, default=None) def handle(self, *args, **options): """ From 40414feb0b7b86b45d7ea65c75cf63d7b58ad50c Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Wed, 18 Oct 2023 21:52:13 +0100 Subject: [PATCH 19/39] update CHANGES.md --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index 75484d3a..edef9814 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,7 @@ Change log Unreleased ---------- - Fix `post_delete` signal triggered upshuffles to do a potentially expensive full reordering of the owrt group (#307) +- Support passing custom `--batch_size` to `reorder_model` management command (#303) 3.7.4 - 2023-03-17 From d4941cac2b0c5dc8d3f6da25cfa0f3b09cd35c43 Mon Sep 17 00:00:00 2001 From: Jonah George Date: Fri, 13 Oct 2023 13:32:33 -0700 Subject: [PATCH 20/39] Fix Django 4 deprecation warning --- ordered_model/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ordered_model/__init__.py b/ordered_model/__init__.py index 852967ac..c81b7b7c 100644 --- a/ordered_model/__init__.py +++ b/ordered_model/__init__.py @@ -1 +1,4 @@ -default_app_config = "ordered_model.apps.OrderedModelConfig" +import django + +if django.VERSION < (3, 2): + default_app_config = "ordered_model.apps.OrderedModelConfig" From c834f9ce2ab70f1e0e7a419147db5c8ccb090a9e Mon Sep 17 00:00:00 2001 From: Jonah George Date: Wed, 18 Oct 2023 12:09:38 -0700 Subject: [PATCH 21/39] Update ordered_model/__init__.py Co-authored-by: Chris Shucksmith --- ordered_model/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ordered_model/__init__.py b/ordered_model/__init__.py index c81b7b7c..6b2bca2c 100644 --- a/ordered_model/__init__.py +++ b/ordered_model/__init__.py @@ -1,4 +1,4 @@ import django if django.VERSION < (3, 2): - default_app_config = "ordered_model.apps.OrderedModelConfig" + default_app_config = "ordered_model.apps.OrderedModelConfig" From a970d684e3f15c0d68de5a5615a53416fa38821e Mon Sep 17 00:00:00 2001 From: Nik Nyby Date: Mon, 26 Jun 2023 11:05:28 -0400 Subject: [PATCH 22/39] Update github actions versions --- .github/workflows/test.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 127401ed..d04813c6 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,9 +13,9 @@ jobs: python-version: [3.5, 3.6, 3.7, 3.8, 3.9, '3.10'] steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v3 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} - name: Install dependencies From 4570e28607c79db4f02d01972a2bde3060fbbb19 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 29 Jan 2024 11:01:39 +0000 Subject: [PATCH 23/39] fix: upstream black change to formatting rules --- ordered_model/admin.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/ordered_model/admin.py b/ordered_model/admin.py index 2536339e..481ac27a 100644 --- a/ordered_model/admin.py +++ b/ordered_model/admin.py @@ -91,9 +91,11 @@ def move_view(self, request, object_id, direction): redir_path = "%s%s%s" % ( mangled, "/" if not mangled.endswith("/") else "", - ("?" + iri_to_uri(request.META.get("QUERY_STRING", ""))) - if request.META.get("QUERY_STRING", "") - else "", + ( + ("?" + iri_to_uri(request.META.get("QUERY_STRING", ""))) + if request.META.get("QUERY_STRING", "") + else "" + ), ) return HttpResponseRedirect(redir_path) @@ -196,9 +198,11 @@ def move_view(self, request, admin_id, object_id, direction): redir_path = "%s%s%s" % ( mangled, "/" if not mangled.endswith("/") else "", - ("?" + iri_to_uri(request.META.get("QUERY_STRING", ""))) - if request.META.get("QUERY_STRING", "") - else "", + ( + ("?" + iri_to_uri(request.META.get("QUERY_STRING", ""))) + if request.META.get("QUERY_STRING", "") + else "" + ), ) return HttpResponseRedirect(redir_path) From 48f74edb3e5b481d44a2340a93e870d79a2f91be Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 29 Jan 2024 21:17:09 +0000 Subject: [PATCH 24/39] fix build icon --- README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index e643b3bc..822c72ec 100644 --- a/README.md +++ b/README.md @@ -1,9 +1,8 @@ django-ordered-model ==================== -[![Build Status](https://secure.travis-ci.org/bfirsh/django-ordered-model.png?branch=master)](https://travis-ci.org/bfirsh/django-ordered-model) +[![Build Status](https://github.com/django-ordered-model/django-ordered-model/actions/workflows/test.yml/badge.svg)](https://github.com/django-ordered-model/django-ordered-model/actions/workflows/test.yml) [![PyPI version](https://badge.fury.io/py/django-ordered-model.svg)](https://badge.fury.io/py/django-ordered-model) -[![codecov](https://codecov.io/gh/bfirsh/django-ordered-model/branch/master/graph/badge.svg)](https://codecov.io/gh/bfirsh/django-ordered-model) [![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/python/black) django-ordered-model allows models to be ordered and provides a simple admin From fc9e23a7cf4a14b04b332856a626ca2d5d43d9cc Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 2 Feb 2024 15:11:00 +0000 Subject: [PATCH 25/39] disprove issue 196 --- tests/models.py | 12 ++++++++++++ tests/tests.py | 26 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/tests/models.py b/tests/models.py index 2113835d..e778bbed 100644 --- a/tests/models.py +++ b/tests/models.py @@ -1,6 +1,7 @@ from django.db import models from ordered_model.models import OrderedModel, OrderedModelBase +import uuid # test simple automatic ordering @@ -134,3 +135,14 @@ class CascadedParentModel(models.Model): class CascadedOrderedModel(OrderedModel): parent = models.ForeignKey(to=CascadedParentModel, on_delete=models.CASCADE) + + +class Flow(models.Model): + pass + + +class StateMachine(OrderedModel): + id = models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True) + name = models.CharField(max_length=32) + flow = models.ForeignKey(Flow, on_delete=models.PROTECT, null=True, blank=True) + order_with_respect_to = "flow" diff --git a/tests/tests.py b/tests/tests.py index 06cf95fd..c9d0e6ca 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -41,6 +41,8 @@ TestUser, CascadedParentModel, CascadedOrderedModel, + Flow, + StateMachine, ) @@ -857,6 +859,30 @@ def test_bottom(self): ) +class ConstructorTest(TestCase): + def test_constructors_issue196(self): + self.f1 = Flow.objects.create() + + self.sm1 = StateMachine(name="a", flow_id=self.f1.id) + self.sm1.save() + + self.sm2 = StateMachine() + self.sm2.name = "b" + self.sm2.flow = self.f1 + self.sm2.save() + + self.sm3 = StateMachine.objects.create(name="c", flow=self.f1) + + self.assertSequenceEqual( + StateMachine.objects.values_list("flow__pk", "order", "name"), + [ + (self.f1.pk, 0, "a"), + (self.f1.pk, 1, "b"), + (self.f1.pk, 2, "c"), + ], + ) + + class MultiOrderWithRespectToTests(TestCase): def setUp(self): q1 = Question.objects.create() From 9b3b021932340901bfe8bc0eadbab7efb9ed2e72 Mon Sep 17 00:00:00 2001 From: Nik Nyby Date: Mon, 26 Jun 2023 11:04:26 -0400 Subject: [PATCH 26/39] GitHub Actions: Test on python 3.11 --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d04813c6..34db1979 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,7 +10,7 @@ jobs: runs-on: ubuntu-20.04 strategy: matrix: - python-version: [3.5, 3.6, 3.7, 3.8, 3.9, '3.10'] + python-version: [3.5, 3.6, 3.7, 3.8, 3.9, '3.10', '3.11'] steps: - uses: actions/checkout@v3 From 071da018ac382f46af384936913f83ae898a3cca Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 2 Feb 2024 15:23:41 +0000 Subject: [PATCH 27/39] test django 4.1 on python 3.11 --- CHANGES.md | 1 + tox.ini | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index edef9814..e1b93cf4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,6 +5,7 @@ Unreleased ---------- - Fix `post_delete` signal triggered upshuffles to do a potentially expensive full reordering of the owrt group (#307) - Support passing custom `--batch_size` to `reorder_model` management command (#303) +- Add tox builder for python 3.11, Django 4.1 and above 3.7.4 - 2023-03-17 diff --git a/tox.ini b/tox.ini index 16bde43b..4c7c3cc0 100644 --- a/tox.ini +++ b/tox.ini @@ -5,8 +5,8 @@ envlist = py{36,37,38,39,310}-django31 py{36,37,38,39,310}-django32 py{38,39,310}-django40 - py{38,39,310}-django41 - py{310}-djangoupstream + py{38,39,310,311}-django41 + py{310,311}-djangoupstream py{310}-drfupstream black @@ -19,6 +19,7 @@ python = 3.8: py38 3.9: py39 3.10: py310 + 3.11: py311 [testenv] deps = From 8e5d6413fd6183ba82be7131e4d45e4dbb0dfc70 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 6 Mar 2023 17:07:30 +0000 Subject: [PATCH 28/39] add fields.OrderedManyToManyField to order related models by through model Meta Assuming a 'through' model is provided to the field, it will be queried while constructing QuerySets for related models and the through Model Meta ordering clauses will be applied. Use in place of fields.ManyToManyField. --- CHANGES.md | 2 +- README.md | 26 +++++++++++++++++++++ ordered_model/fields.py | 48 +++++++++++++++++++++++++++++++++++++ tests/models.py | 16 +++++++++++++ tests/tests.py | 52 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 ordered_model/fields.py diff --git a/CHANGES.md b/CHANGES.md index e1b93cf4..d0def4d3 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,7 +6,7 @@ Unreleased - Fix `post_delete` signal triggered upshuffles to do a potentially expensive full reordering of the owrt group (#307) - Support passing custom `--batch_size` to `reorder_model` management command (#303) - Add tox builder for python 3.11, Django 4.1 and above - +- Add `ordered_model.fields.OrderedManyToManyField` which respects `Meta.ordering` when following ManyToMany related fields. (#277) 3.7.4 - 2023-03-17 ---------- diff --git a/README.md b/README.md index 822c72ec..2c838130 100644 --- a/README.md +++ b/README.md @@ -232,6 +232,32 @@ class OpenQuestion(BaseQuestion): answer = models.TextField(max_length=100) ``` +Ordering of ManyToMany Relationship query results +----------------- + +Django ManyToMany relationships created by `ManyToManyField` [do not respect `Meta.ordering` on the intermediate model](https://code.djangoproject.com/ticket/30460) in results fetched from the 'members' queryset. For example with our usual `Pizza` example, getting the `Toppings` for a `hawaiian_pizza` instance using `PizzaToppingsThroughModel.objects.filter(pizza=hawaiian_pizza).all()` is correctly ordered (by the ThroughModel `Meta.ordering`). However `hawaiian_pizza.toppings.all()` is not, and returns the objects following the 'to' model ordering. + +To work around this, explicitly add an ordering clause, e.g. with `hawaiian_pizza.toppings.all().order_by('pizzatoppingsthroughmodel__order')` or use our `OrderedManyToManyField` which does this by default: + +```python +from ordered_model.fields import OrderedManyToManyField + +class Pizza(models.Model): + name = models.CharField(max_length=100) + toppings = OrderedManyToManyField(Topping, through="PizzaToppingsThroughModel") + + +class PizzaToppingsThroughModel(OrderedModel): + pizza = models.ForeignKey(Pizza, on_delete=models.CASCADE) + topping = models.ForeignKey(Topping, on_delete=models.CASCADE) + order_with_respect_to = "pizza" + + class Meta: + ordering = ("pizza", "order") +``` + +With this definition `hawaiian_pizza.toppings.all()` returns toppings in order. + Custom Manager and QuerySet ----------------- When your model extends `OrderedModel`, it inherits a custom `ModelManager` instance which in turn provides additional operations on the resulting `QuerySet`. For example if `Item` is an `OrderedModel` subclass, the queryset `Item.objects.all()` has functions: diff --git a/ordered_model/fields.py b/ordered_model/fields.py new file mode 100644 index 00000000..44eec833 --- /dev/null +++ b/ordered_model/fields.py @@ -0,0 +1,48 @@ +from django.db.models.fields.related_descriptors import ( + ManyToManyDescriptor, + create_forward_many_to_many_manager, +) +from django.utils.functional import cached_property +from django.db import models + +# OrderedManyToManyField can be used in place of ManyToManyField and will +# sort the returned data by the model Meta ordering when traversing child +# objects + + +def create_sorted_forward_many_to_many_manager(superclass, rel, reverse): + cls = create_forward_many_to_many_manager(superclass, rel, reverse) + + class SortedManyRelatedManager(cls): + def get_queryset(self): + qs = super().get_queryset() + m = rel.through._meta + if m.ordering: + # import pdb; pdb.set_trace() + ors = [m.model_name + "__" + field for field in m.ordering] + qs = qs.order_by(*ors) + return qs + + return SortedManyRelatedManager + + +class SortedManyToManyDescriptor(ManyToManyDescriptor): + def __init__(self, field): + super().__init__(field.remote_field) + + @cached_property + def related_manager_cls(self): + related_model = self.rel.related_model if self.reverse else self.rel.model + + return create_sorted_forward_many_to_many_manager( + related_model._default_manager.__class__, + self.rel, + reverse=self.reverse, + ) + + +class OrderedManyToManyField(models.ManyToManyField): + def contribute_to_class(self, cls, name, **kwargs): + super().contribute_to_class(cls, name, **kwargs) + # print(f"contributed to {cls} {name} remote_field={self.remote_field}") + setattr(cls, self.name, SortedManyToManyDescriptor(self)) diff --git a/tests/models.py b/tests/models.py index e778bbed..2ff0fa34 100644 --- a/tests/models.py +++ b/tests/models.py @@ -1,6 +1,7 @@ from django.db import models from ordered_model.models import OrderedModel, OrderedModelBase +from ordered_model.fields import OrderedManyToManyField import uuid @@ -146,3 +147,18 @@ class StateMachine(OrderedModel): name = models.CharField(max_length=32) flow = models.ForeignKey(Flow, on_delete=models.PROTECT, null=True, blank=True) order_with_respect_to = "flow" + + +# Duplicate Pizza models using OrderedManyToManyField +class PizzaOM2M(models.Model): + name = models.CharField(max_length=100) + toppings = OrderedManyToManyField(Topping, through="PizzaOM2MToppingsThroughModel") + + +class PizzaOM2MToppingsThroughModel(OrderedModel): + pizza = models.ForeignKey(PizzaOM2M, on_delete=models.CASCADE) + topping = models.ForeignKey(Topping, on_delete=models.CASCADE) + order_with_respect_to = "pizza" + + class Meta: + ordering = ("pizza", "order") diff --git a/tests/tests.py b/tests/tests.py index c9d0e6ca..66e58480 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -31,8 +31,10 @@ CustomPKGroupItem, CustomPKGroup, Pizza, + PizzaOM2M, Topping, PizzaToppingsThroughModel, + PizzaOM2MToppingsThroughModel, BaseQuestion, OpenQuestion, MultipleChoiceQuestion, @@ -733,6 +735,30 @@ def test_saved_order(self): ], ) + def test_members_order_issue277(self): + # make order differ from pk order + self.p1_t3.top() # anchovy, tomatoe, mozarella, + + # ManyToMany relationship iterates by 'to' model order, ie. PK of topping + l1 = self.p1.toppings.all().values_list("name", flat=True) + self.assertEqual(list(l1), ["tomatoe", "mozarella", "anchovy"]) # pk order + + # Through model ordering is ordered correctly + l2 = ( + PizzaToppingsThroughModel.objects.filter(pizza=self.p1) + .all() + .values_list("topping__name", flat=True) + ) + self.assertEqual(list(l2), ["anchovy", "tomatoe", "mozarella"]) # ordered + + # explicit ordering works + l3 = ( + self.p1.toppings.all() + .order_by("pizzatoppingsthroughmodel__order") + .values_list("name", flat=True) + ) + self.assertEqual(list(l3), ["anchovy", "tomatoe", "mozarella"]) # ordered + def test_swap(self): with self.assertRaises(ValueError): self.p1_t1.swap(self.p2_t1) @@ -883,6 +909,32 @@ def test_constructors_issue196(self): ) +class OrderWithRespectToTestsOrderedManyToManyField(TestCase): + def setUp(self): + self.t1 = Topping.objects.create(name="tomatoe") + self.t2 = Topping.objects.create(name="mozarella") + self.t3 = Topping.objects.create(name="anchovy") + self.p1 = PizzaOM2M.objects.create(name="Napoli") + # tomatoe, mozarella, anchovy + self.p1_t1 = PizzaOM2MToppingsThroughModel.objects.create( + pizza=self.p1, topping=self.t1 + ) + self.p1_t2 = PizzaOM2MToppingsThroughModel.objects.create( + pizza=self.p1, topping=self.t2 + ) + self.p1_t3 = PizzaOM2MToppingsThroughModel.objects.create( + pizza=self.p1, topping=self.t3 + ) + + def test_members_order_issue277(self): + # make order differ from pk order + self.p1_t3.top() # anchovy, tomatoe, mozarella, + + # OrderedManyToMany relationship iterates by ordered model order + l1 = self.p1.toppings.all().values_list("name", flat=True) + self.assertEqual(list(l1), ["anchovy", "tomatoe", "mozarella"]) + + class MultiOrderWithRespectToTests(TestCase): def setUp(self): q1 = Question.objects.create() From 94d91e56b04af4cf9add7f573f6468d14c0fd8e5 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 5 Feb 2024 16:16:26 +0000 Subject: [PATCH 29/39] Relax check that `order_with_respect_to` entries final element must be a `ForeignKey` - it can be any `Field` instance (#298) This required re-working the optimisation in c4630dca2bdafd6bac1bd2328081f524953872cf to handle OWRT paths ending on a non ForeignKey field. --- CHANGES.md | 2 + ordered_model/models.py | 41 ++++++++++++----- tests/models.py | 22 +++++++++ tests/tests.py | 98 ++++++++++++++++++++++++++++++++++++++--- tox.ini | 4 +- 5 files changed, 147 insertions(+), 20 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index d0def4d3..d7f7167a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -7,6 +7,8 @@ Unreleased - Support passing custom `--batch_size` to `reorder_model` management command (#303) - Add tox builder for python 3.11, Django 4.1 and above - Add `ordered_model.fields.OrderedManyToManyField` which respects `Meta.ordering` when following ManyToMany related fields. (#277) +- Relax check that `order_with_respect_to` entries final element must be a `ForeignKey` - it can be any `Field` instance (#298) + 3.7.4 - 2023-03-17 ---------- diff --git a/ordered_model/models.py b/ordered_model/models.py index 29e8183f..330301df 100644 --- a/ordered_model/models.py +++ b/ordered_model/models.py @@ -10,9 +10,24 @@ from django.utils.translation import gettext_lazy as _ -def get_lookup_value(obj, field): +def get_lookup_value(obj, wrt_field, use_fkid=True): + # starting with obj, traverse the wrt_field path and return the value of the + # final field. if field_path *ends* at a ForeignKey, and use_fkid=True, return the pk + # of the fk rather than build the object itself. try: - return reduce(lambda i, f: getattr(i, f), field.split(LOOKUP_SEP), obj) + mc = type(obj) + path = wrt_field.split(LOOKUP_SEP) + leafindex = len(path) - 1 + for depth, p in enumerate(path): + f = mc._meta.get_field(p) + if depth == leafindex and use_fkid and isinstance(f, ForeignKey): + return getattr(obj, p + "_id") + elif depth == leafindex: + return getattr(obj, p) + else: + mc = f.remote_field.model + obj = getattr(obj, p) + except ObjectDoesNotExist: return None @@ -129,16 +144,15 @@ def __init__(self, *args, **kwargs): def _wrt_map(self): d = {} for order_wrt_name in self.get_order_with_respect_to(): - # we know order_wrt_name is a ForeignKey, so use a cheaper _id lookup - field_path = order_wrt_name + "_id" - d[order_wrt_name] = get_lookup_value(self, field_path) + d[order_wrt_name] = get_lookup_value(self, order_wrt_name, use_fkid=True) return d def _get_related_objects(self): # slow path, for use in the admin which requires the objects # expected to generate extra queries return [ - get_lookup_value(self, name) for name in self.get_order_with_respect_to() + get_lookup_value(self, name, use_fkid=False) + for name in self.get_order_with_respect_to() ] @classmethod @@ -388,17 +402,19 @@ def check(cls, **kwargs): ) ) - # each field may be an FK, or recursively an FK ref to an FK + # each field may be an FK ref, until the leaf which may be an FK ref or another type of field try: for wrt_field in cls.get_order_with_respect_to(): mc = cls - for p in wrt_field.split(LOOKUP_SEP): + path = wrt_field.split(LOOKUP_SEP) + leafindex = len(path) - 1 + for depth, p in enumerate(path): try: f = mc._meta.get_field(p) - if not isinstance(f, ForeignKey): + if depth < leafindex and not isinstance(f, ForeignKey): errors.append( checks.Error( - "OrderedModel order_with_respect_to specifies field '{0}' (within '{1}') which is not a ForeignKey. This is unsupported.".format( + "OrderedModel order_with_respect_to specifies intermediate field '{0}' (within '{1}') which is not a ForeignKey. This is unsupported.".format( p, wrt_field ), obj=str(cls.__qualname__), @@ -406,7 +422,10 @@ def check(cls, **kwargs): ) ) break - mc = f.remote_field.model + + if isinstance(f, ForeignKey): + mc = f.remote_field.model + except FieldDoesNotExist: errors.append( checks.Error( diff --git a/tests/models.py b/tests/models.py index 2ff0fa34..4a265d17 100644 --- a/tests/models.py +++ b/tests/models.py @@ -162,3 +162,25 @@ class PizzaOM2MToppingsThroughModel(OrderedModel): class Meta: ordering = ("pizza", "order") + + +# issue 298 +class Training(models.Model): + pass + + +class TrainingExercise(OrderedModel, models.Model): + WARMUP = 1 + MAINPART = 2 + ENDPART = 3 + TrainingChoices = [ + (WARMUP, "WarmUp"), + (MAINPART, "MainPart"), + (ENDPART, "EndPart"), + ] + training = models.ForeignKey(Training, on_delete=models.CASCADE) + stage = models.PositiveSmallIntegerField(choices=TrainingChoices) + order_with_respect_to = ("training", "stage") + + class Meta: + ordering = ("training", "stage") diff --git a/tests/tests.py b/tests/tests.py index 66e58480..6c3f6e87 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -45,6 +45,8 @@ CascadedOrderedModel, Flow, StateMachine, + Training, + TrainingExercise, ) @@ -1020,6 +1022,47 @@ def test_above_between_groups(self): ) +class TestOrderWithRespectToNonFKFieldsTest(TestCase): + def setUp(self): + self.t1 = Training.objects.create() + self.t2 = Training.objects.create() + tc = TrainingExercise + self.tc = tc + self.te1_wu_0 = TrainingExercise.objects.create( + training=self.t1, stage=tc.WARMUP + ) + self.te1_wu_1 = TrainingExercise.objects.create( + training=self.t1, stage=tc.WARMUP + ) + self.te2_wu_0 = TrainingExercise.objects.create( + training=self.t2, stage=tc.WARMUP + ) + self.te2_mp_0 = TrainingExercise.objects.create( + training=self.t2, stage=tc.MAINPART + ) + self.te2_mp_1 = TrainingExercise.objects.create( + training=self.t2, stage=tc.MAINPART + ) + + def test_move_between_groups(self): + tc = self.tc + self.te2_mp_1.stage = TrainingExercise.WARMUP + self.te2_mp_1.save() + + self.assertSequenceEqual( + TrainingExercise.objects.all().values_list( + "pk", "training", "stage", "order" + ), + [ + (1, self.t1.pk, tc.WARMUP, 0), + (2, self.t1.pk, tc.WARMUP, 1), + (3, self.t2.pk, tc.WARMUP, 0), + (5, self.t2.pk, tc.WARMUP, 1), + (4, self.t2.pk, tc.MAINPART, 0), + ], + ) + + class PolymorphicOrderGenerationTests(TestCase): def test_order_of_baselist(self): o1 = OpenQuestion.objects.create() @@ -1551,20 +1594,29 @@ class TestModel(OrderedModel): name = models.CharField(max_length=100) order_with_respect_to = "name" + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [], + ) + + def test_owrt_not_exist(self): + class TestModel(OrderedModel): + order_with_respect_to = "name" + self.assertEqual( checks.run_checks(app_configs=self.apps.get_app_configs()), [ checks.Error( - msg="OrderedModel order_with_respect_to specifies field 'name' (within 'name') which is not a ForeignKey. This is unsupported.", - obj="ChecksTest.test_owrt_not_foreign_key..TestModel", - id="ordered_model.E005", + msg="OrderedModel order_with_respect_to specifies field 'name' (within 'name') which does not exist.", + obj="ChecksTest.test_owrt_not_exist..TestModel", + id="ordered_model.E006", ) ], ) - def test_owrt_not_immediate_foreign_key(self): + def test_owrt_leaf_not_exist(self): class TestTargetModel(OrderedModel): - name = models.CharField(max_length=100) + pass class TestModel(OrderedModel): target = models.ForeignKey(to=TestTargetModel, on_delete=models.CASCADE) @@ -1574,13 +1626,45 @@ class TestModel(OrderedModel): checks.run_checks(app_configs=self.apps.get_app_configs()), [ checks.Error( - msg="OrderedModel order_with_respect_to specifies field 'name' (within 'target__name') which is not a ForeignKey. This is unsupported.", - obj="ChecksTest.test_owrt_not_immediate_foreign_key..TestModel", + msg="OrderedModel order_with_respect_to specifies field 'name' (within 'target__name') which does not exist.", + obj="ChecksTest.test_owrt_leaf_not_exist..TestModel", + id="ordered_model.E006", + ) + ], + ) + + def test_owrt_intermediate_not_fk(self): + class TestModel(OrderedModel): + target = models.CharField(max_length=100) + order_with_respect_to = "target__name" + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [ + checks.Error( + msg="OrderedModel order_with_respect_to specifies intermediate field 'target' (within 'target__name') which is not a ForeignKey. This is unsupported.", + obj="ChecksTest.test_owrt_intermediate_not_fk..TestModel", id="ordered_model.E005", ) ], ) + def test_owrt_deep(self): + class TestTargetModel(OrderedModel): + name = models.CharField(max_length=100) + + class TestMiddleModel(OrderedModel): + target = models.ForeignKey(to=TestTargetModel, on_delete=models.CASCADE) + + class TestModel(OrderedModel): + middle = models.ForeignKey(to=TestMiddleModel, on_delete=models.CASCADE) + order_with_respect_to = "middle__target__name" + + self.assertEqual( + checks.run_checks(app_configs=self.apps.get_app_configs()), + [], + ) + class TestCascadedDelete(TestCase): def test_that_model_when_deleted_by_cascade_still_maintains_ordering(self): diff --git a/tox.ini b/tox.ini index 4c7c3cc0..91d43c98 100644 --- a/tox.ini +++ b/tox.ini @@ -38,9 +38,9 @@ deps = django40: djangorestframework~=3.13.0 django41: djangorestframework~=3.13.0 djangoupstream: https://github.com/encode/django-rest-framework/archive/master.tar.gz - coverage commands = - coverage run {envbindir}/django-admin test --pythonpath=. --settings=tests.settings {posargs} + {envbindir}/django-admin check --pythonpath=. --settings=tests.settings + {envbindir}/django-admin test --pythonpath=. --settings=tests.settings {posargs} [testenv:black] basepython = python3 From 8477a18496cf1b6dad91e8bb7ac053cc92fcf0b9 Mon Sep 17 00:00:00 2001 From: kylepollina Date: Sun, 3 Mar 2024 15:32:09 -0800 Subject: [PATCH 30/39] Update README.md --- README.md | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 2c838130..1cc7dea6 100644 --- a/README.md +++ b/README.md @@ -408,21 +408,23 @@ Django Rest Framework To support updating ordering fields by Django Rest Framework, we include a serializer `OrderedModelSerializer` that intercepts writes to the ordering field, and calls `OrderedModel.to()` method to effect a re-ordering: - from rest_framework import routers, serializers, viewsets - from ordered_model.serializers import OrderedModelSerializer - from tests.models import CustomItem +```python +from rest_framework import routers, serializers, viewsets +from ordered_model.serializers import OrderedModelSerializer +from tests.models import CustomItem - class ItemSerializer(serializers.HyperlinkedModelSerializer, OrderedModelSerializer): - class Meta: - model = CustomItem - fields = ['pkid', 'name', 'modified', 'order'] +class ItemSerializer(serializers.HyperlinkedModelSerializer, OrderedModelSerializer): + class Meta: + model = CustomItem + fields = ['pkid', 'name', 'modified', 'order'] - class ItemViewSet(viewsets.ModelViewSet): - queryset = CustomItem.objects.all() - serializer_class = ItemSerializer +class ItemViewSet(viewsets.ModelViewSet): + queryset = CustomItem.objects.all() + serializer_class = ItemSerializer - router = routers.DefaultRouter() - router.register(r'items', ItemViewSet) +router = routers.DefaultRouter() +router.register(r'items', ItemViewSet) +``` Note that you need to include the 'order' field (or your custom field name) in the `Serializer`'s `fields` list, either explicitly or using `__all__`. See [ordered_model/serializers.py](ordered_model/serializers.py) for the implementation. From a6dc605fd3f06ee13e62d27f109e9ccd3af3bfac Mon Sep 17 00:00:00 2001 From: Tony Narlock Date: Fri, 12 Jul 2024 07:20:00 -0500 Subject: [PATCH 31/39] chore(OrderedInlineModelAdminMixin): Fix typo in docstring --- ordered_model/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ordered_model/admin.py b/ordered_model/admin.py index 481ac27a..44738c1d 100644 --- a/ordered_model/admin.py +++ b/ordered_model/admin.py @@ -144,7 +144,7 @@ def move_up_down_links(self, obj): class OrderedInlineModelAdminMixin: """ - ModelAdminMixin for classes that contain OrderedInilines + ModelAdminMixin for classes that contain OrderedInlines. """ def get_urls(self): From fe04e8f27489eeded93e0d0fc531885981206c51 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 15 Jul 2024 20:04:08 +0100 Subject: [PATCH 32/39] update handle pip/pypi TLSv1 deprecation --- .github/workflows/test.yml | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 34db1979..de931f02 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,13 +11,20 @@ jobs: strategy: matrix: python-version: [3.5, 3.6, 3.7, 3.8, 3.9, '3.10', '3.11'] - + include: + - pip-trusted-host: '' + # Relax security checks for Python 3.5 only. (https://github.com/actions/setup-python/issues/866) + - python-version: '3.5' + pip-trusted-host: 'pypi.python.org pypi.org files.pythonhosted.org' steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} + env: + PIP_TRUSTED_HOST: ${{ matrix.pip-trusted-host }} + PIP_DISABLE_PIP_VERSION_CHECK: 1 - name: Install dependencies run: | python -m pip install --upgrade pip From 24ac19ad084a6bed24d333ed6296f282bb25fa51 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 15 Jul 2024 20:22:06 +0100 Subject: [PATCH 33/39] test Django 4.2, upgrade drfupstream to django>4.2 --- tox.ini | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 91d43c98..f91bd073 100644 --- a/tox.ini +++ b/tox.ini @@ -6,6 +6,7 @@ envlist = py{36,37,38,39,310}-django32 py{38,39,310}-django40 py{38,39,310,311}-django41 + py{38,39,310,311}-django42 py{310,311}-djangoupstream py{310}-drfupstream black @@ -29,14 +30,16 @@ deps = django32: Django>=3.2,<4.0 django40: Django>=4.0,<4.1 django41: Django>=4.1,<4.2 + django42: Django>=4.2,<4.3 djangoupstream: https://github.com/django/django/archive/main.tar.gz - drfupstream: Django~=3.2.0 + drfupstream: Django~=4.2.0 drfupstream: https://github.com/encode/django-rest-framework/archive/master.tar.gz django22: djangorestframework~=3.12.0 django30,django31,django32: djangorestframework~=3.12.0 django40: djangorestframework~=3.13.0 django41: djangorestframework~=3.13.0 + django42: djangorestframework~=3.15.0 djangoupstream: https://github.com/encode/django-rest-framework/archive/master.tar.gz commands = {envbindir}/django-admin check --pythonpath=. --settings=tests.settings From 36506ada7fc926475aa10903b608c31c49fc947e Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Mon, 15 Jul 2024 19:58:04 +0100 Subject: [PATCH 34/39] add test case proposed on #320 (already fixed) --- tests/models.py | 15 +++++++++++++++ tests/tests.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/tests/models.py b/tests/models.py index 4a265d17..f35b23f7 100644 --- a/tests/models.py +++ b/tests/models.py @@ -184,3 +184,18 @@ class TrainingExercise(OrderedModel, models.Model): class Meta: ordering = ("training", "stage") + + +# issue 320 parent/child models +class Foobar(models.Model): + name = models.CharField(max_length=100) + + +class ParentModel(OrderedModel): + name = models.CharField(max_length=100) + foobar = models.ForeignKey(Foobar, on_delete=models.CASCADE) + order_with_respect_to = "foobar" + + +class ChildModel(ParentModel): + age = models.IntegerField() diff --git a/tests/tests.py b/tests/tests.py index 6c3f6e87..14fad3b3 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -47,6 +47,9 @@ StateMachine, Training, TrainingExercise, + Foobar, + ChildModel, + ParentModel, ) @@ -1725,3 +1728,34 @@ def test_that_model_when_multiple_unordered_deleted_by_cascade_still_maintain_or # Assert the hole has been filled self.assertEqual(child_with_order_2.order, 0) self.assertEqual(child_with_order_3.order, 1) + + +## @pytest.mark.django_db +class ParentChildModelTests(TestCase): + def test_parent_child_order(self): + foobar = Foobar.objects.create(name="foobar") + child1 = ChildModel.objects.create(name="child1", foobar=foobar, age=1) + child2 = ChildModel.objects.create(name="child2", foobar=foobar, age=2) + child3 = ChildModel.objects.create(name="child3", foobar=foobar, age=3) + child4 = ChildModel.objects.create(name="child4", foobar=foobar, age=4) + + # This is the order of the children at the start + assert child1.order == 0 + assert child2.order == 1 + assert child3.order == 2 + assert child4.order == 3 + + # Delete the first child + # This causes the parent to be deleted as well + child1.delete() + + # Refresh the db + child2.refresh_from_db() + child3.refresh_from_db() + child4.refresh_from_db() + + # The order of the children should be updated + # The expected order + assert child2.order == 0 + assert child3.order == 1 + assert child4.order == 2 From 2a9d37bf17a3dca185833670570451c817e7e330 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 1 Nov 2024 10:55:04 +0000 Subject: [PATCH 35/39] version 3.8 alpha --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index d7f7167a..3a83499d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,7 +1,7 @@ Change log ========== -Unreleased +3.8.0 ---------- - Fix `post_delete` signal triggered upshuffles to do a potentially expensive full reordering of the owrt group (#307) - Support passing custom `--batch_size` to `reorder_model` management command (#303) From 0711da89e9860330ef0738d14502ea9d273dcf7f Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 1 Nov 2024 11:04:40 +0000 Subject: [PATCH 36/39] bump github actions due to deprecation --- .github/workflows/distribute.yml | 6 +++--- .github/workflows/lint.yml | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/distribute.yml b/.github/workflows/distribute.yml index 03fb4357..60f38049 100644 --- a/.github/workflows/distribute.yml +++ b/.github/workflows/distribute.yml @@ -8,9 +8,9 @@ jobs: build: runs-on: ubuntu-20.04 steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v2 + uses: actions/setup-python@v4 with: python-version: 3.8 - name: Install dependencies @@ -20,7 +20,7 @@ jobs: run: python setup.py sdist bdist_wheel - name: Run twine check run: twine check dist/* - - uses: actions/upload-artifact@v2 + - uses: actions/upload-artifact@v4.4.3 with: name: django-ordered-model-dist path: dist diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index c7d21b5e..9ab9f6b5 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -9,7 +9,7 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - uses: actions/setup-python@v2 + - uses: actions/checkout@v4 + - uses: actions/setup-python@v4 - uses: psf/black@stable From c0a1870a219daf1e854d348bfe15ea13e090a94d Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 1 Nov 2024 11:19:30 +0000 Subject: [PATCH 37/39] bump version number --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 22ab5391..1b3cccd0 100644 --- a/setup.py +++ b/setup.py @@ -12,7 +12,7 @@ name="django-ordered-model", long_description=long_description, long_description_content_type="text/markdown", - version="3.7.4", + version="3.8.0-alpha", description="Allows Django models to be ordered and provides a simple admin interface for reordering them.", author="Ben Firshman", author_email="ben@firshman.co.uk", From b3383ab58e3586d9fe7f0358ef1829fc8907b1b9 Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Wed, 6 Nov 2024 16:15:21 +0000 Subject: [PATCH 38/39] add version badge --- README.md | 2 +- setup.py | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 1cc7dea6..c6bd2838 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ django-ordered-model ==================== -[![Build Status](https://github.com/django-ordered-model/django-ordered-model/actions/workflows/test.yml/badge.svg)](https://github.com/django-ordered-model/django-ordered-model/actions/workflows/test.yml) +![Python versions](https://img.shields.io/pypi/pyversions/django-ordered-model.svg) [![Build Status](https://github.com/django-ordered-model/django-ordered-model/actions/workflows/test.yml/badge.svg)](https://github.com/django-ordered-model/django-ordered-model/actions/workflows/test.yml) [![PyPI version](https://badge.fury.io/py/django-ordered-model.svg)](https://badge.fury.io/py/django-ordered-model) [![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/python/black) diff --git a/setup.py b/setup.py index 1b3cccd0..1d59b95c 100644 --- a/setup.py +++ b/setup.py @@ -30,7 +30,14 @@ "License :: OSI Approved :: BSD License", "Operating System :: OS Independent", "Programming Language :: Python", - "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.5", + "Programming Language :: Python :: 3.6", + "Programming Language :: Python :: 3.7", + "Programming Language :: Python :: 3.8", + "Programming Language :: Python :: 3.9", + "Programming Language :: Python :: 3.10", + "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", ], zip_safe=False, package_data={ From f4a338ca4b9e1c7d9edb677fddc6c2743552232e Mon Sep 17 00:00:00 2001 From: Chris Shucksmith Date: Fri, 15 Nov 2024 15:06:39 +0000 Subject: [PATCH 39/39] add test run on Django 5.0, 5.1 --- .github/workflows/test.yml | 2 +- README.md | 2 ++ tox.ini | 9 ++++++++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index de931f02..5818e09d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,7 +10,7 @@ jobs: runs-on: ubuntu-20.04 strategy: matrix: - python-version: [3.5, 3.6, 3.7, 3.8, 3.9, '3.10', '3.11'] + python-version: [3.5, 3.6, 3.7, 3.8, 3.9, '3.10', '3.11', '3.12'] include: - pip-trusted-host: '' # Relax security checks for Python 3.5 only. (https://github.com/actions/setup-python/issues/866) diff --git a/README.md b/README.md index c6bd2838..f18e72d4 100644 --- a/README.md +++ b/README.md @@ -450,6 +450,8 @@ Compatibility with Django and Python |django-ordered-model version | Django version | Python version | DRF (optional) |-----------------------------|---------------------|-------------------|---------------- +| **3.8.x** | **3.x**, **4.x**, **5.x** | **3.10** to **3.12** | 3.15 and above +| **3.7.x** | **3.x**, **4.x** | **3.5** and above | 3.12 and above | **3.6.x** | **3.x**, **4.x** | **3.5** and above | 3.12 and above | **3.5.x** | **3.x**, **4.x** | **3.5** and above | - | **3.4.x** | **2.x**, **3.x** | **3.5** and above | - diff --git a/tox.ini b/tox.ini index f91bd073..6d7870c0 100644 --- a/tox.ini +++ b/tox.ini @@ -7,7 +7,9 @@ envlist = py{38,39,310}-django40 py{38,39,310,311}-django41 py{38,39,310,311}-django42 - py{310,311}-djangoupstream + py{310,311,312}-django50 + py{310,311,312}-django51 + py{310,311,312}-djangoupstream py{310}-drfupstream black @@ -21,6 +23,7 @@ python = 3.9: py39 3.10: py310 3.11: py311 + 3.12: py312 [testenv] deps = @@ -31,6 +34,8 @@ deps = django40: Django>=4.0,<4.1 django41: Django>=4.1,<4.2 django42: Django>=4.2,<4.3 + django50: Django>=5.0,<5.1 + django51: Django>=5.1,<5.2 djangoupstream: https://github.com/django/django/archive/main.tar.gz drfupstream: Django~=4.2.0 @@ -40,6 +45,8 @@ deps = django40: djangorestframework~=3.13.0 django41: djangorestframework~=3.13.0 django42: djangorestframework~=3.15.0 + django50: djangorestframework~=3.15.0 + django51: djangorestframework~=3.15.0 djangoupstream: https://github.com/encode/django-rest-framework/archive/master.tar.gz commands = {envbindir}/django-admin check --pythonpath=. --settings=tests.settings