[Django] #36708: `formset` not a static attribute of `ChangeList`

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben | Owner: Ben Gregory
Gregory |
Type: | Status: assigned
Cleanup/optimization |
Component: | Version: 5.2
contrib.admin |
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 |
-------------------------------------+-------------------------------------
The `ChangeList` class is used in Django Admin to determine and configure
the functionality of a model's `change_list` admin page.

I have been experimenting with using `ChangeList` for my own purposes, to
create a custom page not dissimilar to `change_list.html`.

I have noted that in doing so, I have needed to set the attribute `formset
= None` on my `ChangeList` instance, to satisfy an expectation of the
`admin_list.result_list`
[https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/templatetags/admin_list.py#L328
template tag]. This is because `formset` is not a static attribute of the
class.

Looking at the source code, the
[https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/options.py#L2056
`django.contrib.admin.options`] module sets `formset` on the `ChangeList`
instance.

Outside of tests, `ChangeList` objects are only instantiated in the above
module, and as said above, will always be assigned a `formset` attribute.
See
[https://github.com/search?q=repo%3Adjango%2Fdjango+%2F%28%3F-i%29ChangeList%2F+language%3APython&type=code&l=Python
here]. So, it appears to me that there is an assumption that instances of
this class should _always_ have a `formset` attribute.

I propose we simply make `formset` an attribute of `ChangeList`, rather
than assign it dynamically in the aforementioned function. This will mean
developers working low-level in the admin needn't write a line of code to
be able to use their custom `ChangeList` objects with the relevant
template tags, and will serve to better document the expected attributes
of `ChangeList` objects more generally.

Proposal:

https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/views/main.py#L67:

{{{
+ formset = None
}}}

https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/options.py#L2056:

{{{
- formset = cl.formset = None
+ formset = None
}}}

One could also argue that this is an opportunity to remove this notation
`formset = cl.formset = *`, and use `cl.formset` directly.
--
Ticket URL: <https://code.djangoproject.com/ticket/36708>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------

Reporter: Ben Gregory | Owner: Ben
Type: | Gregory
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

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

Changes (by Antoliny):

* cc: Antoliny (added)
* keywords: => ChangeList

Comment:

Thank you for suggesting the improvements Ben!

You're absolutely right, when the `result_list` template tag renders the
template, the changelist must have a `formset` attribute.
{{{
def results(cl):
if cl.formset:
for res, form in zip(cl.result_list, cl.formset.forms):
yield ResultList(form, items_for_result(cl, res, form))
else:
for res in cl.result_list:
yield ResultList(None, items_for_result(cl, res, None))

def result_list(cl):
"""
Display the headers and data list together.
"""
headers = list(result_headers(cl))
num_sorted_fields = 0
for h in headers:
if h["sortable"] and h["sorted"]:
num_sorted_fields += 1
return {
"cl": cl,
"result_hidden_fields": list(result_hidden_fields(cl)),
"result_headers": headers,
"num_sorted_fields": num_sorted_fields,
"results": list(results(cl)),
}
}}}
And as you mentioned, this attribute is currently being dynamically
assigned in the `changelist_view` through the options.py.

I also agree that defining the formset attribute directly in the
`ChangeList` class would be more appropriate.
I agree with this improvement, but there might be aspects I haven’t
considered, so I’ll wait for another person’s triage :)
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:1>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Ben
Type: | Gregory
Cleanup/optimization | Status: assigned

Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

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

* easy: 0 => 1
* stage: Unreviewed => Accepted
* version: 5.2 => dev

Comment:

Thanks for the report. And thanks Antoliny for the triage, I trust your
view on admin issues :-)

Looks like this may be the issue from #13039, but I'm happy to keep this
issue open instead of reopening there.

Ben, would you like to submit a PR?
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:2>