[Django] #37004: BaseModelFormSet could leverage totally_ordered property

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Sarah Boyce
Walls |
Type: | Status: assigned
Cleanup/optimization |
Component: Forms | Version: dev
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
#36857 added a new property `totally_ordered` on `QuerySet` to capture the
difference between a queryset having ''some'' ordering versus a "stable,
deterministic" ordering.

I noticed that we probably want to use that new property here in
`BaseModelFormSet` for the purpose described in #10163:

{{{#!diff
diff --git a/django/forms/models.py b/django/forms/models.py
index 104369c0b0..3843203a1d 100644
--- a/django/forms/models.py
+++ b/django/forms/models.py
@@ -776,7 +776,7 @@ class BaseModelFormSet(BaseFormSet, AltersData):
# If the queryset isn't already ordered we need to add an
# artificial ordering here to make sure that all formsets
# constructed from this queryset have the same form order.
- if not qs.ordered:
+ if not qs.totally_ordered:
qs = qs.order_by(self.model._meta.pk.name)

# Removed queryset limiting here. As per discussion re:
#13023
}}}

Tentatively assigning to Sarah to see if a good fit for any Djangonauts
this session. This would involve cooking up a test.
--
Ticket URL: <https://code.djangoproject.com/ticket/37004>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Sarah
Type: | Boyce
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by David Smith):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:1>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Sarah
Type: | Boyce
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rodrigo Vieira):

Hi, I'm a Djangonaut (Session 6) and if the author of #36857 or anyone
else is not available/interested I'd like to give it a go, seems to be
mostly making the change proposed and a few tests?
--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:2>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Sarah
Type: | Boyce
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Go for it Rodrigo!

We'll likely want to ''augment'' the ordering with `pk` here and not
entirely replace it
([https://github.com/django/django/blob/edd899786851f6285abbc3c272f4f5ec0e48a74c/django/core/serializers/python.py#L82-L87
just like serializers do]) otherwise it might break the natural ordering
expected by users (e.g inlines ordered by name or publish date) so the
proposed patch will need a bit of tweaking.

`order_by` also accepts `"pk"` so we can avoid model introspection here;
`self.model._meta.pk.name` -> `"pk"`.
--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:3>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Rodrigo
Type: | Vieira

Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* owner: Sarah Boyce => Rodrigo Vieira

--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:4>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Rodrigo
Type: | Vieira
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rodrigo Vieira):

Thanks for the pointers Simon!
[https://github.com/django/django/pull/21036 PR is here], following the
same pattern as the code you pointed out in serializers. I added one test
for each "branch" of the checks.
--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:5>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Rodrigo
Type: | Vieira
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Rodrigo Vieira):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:6>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Rodrigo
Type: | Vieira
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:7>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Rodrigo
Type: | Vieira
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:8>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Rodrigo
Type: | Vieira
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"08aa1b5ac8fd95d165aad749f3ce29de4aa1be0d" 08aa1b5a]:
{{{#!CommitTicketReference repository=""
revision="08aa1b5ac8fd95d165aad749f3ce29de4aa1be0d"
Refs #37004 -- Added coverage for BaseModelFormSet.get_queryset() fallback
ordering.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:9>

[Django]

#37004: BaseModelFormSet could leverage totally_ordered property
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Rodrigo
Type: | Vieira

Cleanup/optimization | Status: closed
Component: Forms | Version: dev
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls <jacobtylerwalls@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"856c915326768962806705ca7733e4abbb8f794f" 856c915]:
{{{#!CommitTicketReference repository=""
revision="856c915326768962806705ca7733e4abbb8f794f"
Fixed #37004 -- Used QuerySet.totally_ordered in
BaseModelFormSet.get_queryset() for stable ordering.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37004#comment:10>