Opened 4 weeks ago
Closed 3 weeks ago
#37004 closed Cleanup/optimization (fixed)
BaseModelFormSet could leverage totally_ordered property
| Reported by: | Jacob Walls | Owned by: | Rodrigo Vieira |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
#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:
-
django/forms/models.py
diff --git a/django/forms/models.py b/django/forms/models.py index 104369c0b0..3843203a1d 100644
a b class BaseModelFormSet(BaseFormSet, AltersData): 776 776 # If the queryset isn't already ordered we need to add an 777 777 # artificial ordering here to make sure that all formsets 778 778 # constructed from this queryset have the same form order. 779 if not qs. ordered:779 if not qs.totally_ordered: 780 780 qs = qs.order_by(self.model._meta.pk.name) 781 781 782 782 # 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.
Change History (10)
comment:1 by , 3 weeks ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:3 by , 3 weeks ago
Go for it Rodrigo!
We'll likely want to augment the ordering with pk here and not entirely replace it (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".
comment:4 by , 3 weeks ago
| Owner: | changed from to |
|---|
comment:5 by , 3 weeks ago
Thanks for the pointers Simon! 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.
comment:6 by , 3 weeks ago
| Has patch: | set |
|---|
comment:7 by , 3 weeks ago
| Patch needs improvement: | set |
|---|
comment:8 by , 3 weeks ago
| Patch needs improvement: | unset |
|---|---|
| Triage Stage: | Accepted → Ready for checkin |
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?