`formset` not a static attribute of `ChangeList`

[Ben Gregory]

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 ​template tag. This is because formset is not a static attribute of the class.

Looking at the source code, the ​`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 ​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.

[Antoliny]

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 :)

[Jacob Walls]

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?

[Rudraksha Dwivedi]

PR submitted: ​https://github.com/django/django/pull/20184
I think it addresses the issue aptly, I shall set the Has patch to yes

[Rudraksha Dwivedi]

can someone shed some light on the flake8 test on the automated github testing flow ?
i think the issue is solved but some linting test fail
so i am not setting needs improvement to no currently
thank you!

[Antoliny]

Replying to Rudraksha Dwivedi:

can someone shed some light on the flake8 test on the automated github testing flow ?
i think the issue is solved but some linting test fail
so i am not setting needs improvement to no currently
thank you!

Would you like to refer to this ​document?

You can run the tests with tox as well, but if you use pre-commit, lint tests will run automatically when you make a commit!

[Rudraksha Dwivedi]

Replying to Antoliny:

Replying to Rudraksha Dwivedi:

can someone shed some light on the flake8 test on the automated github testing flow ?
i think the issue is solved but some linting test fail
so i am not setting needs improvement to no currently
thank you!

Would you like to refer to this ​document?

You can run the tests with tox as well, but if you use pre-commit, lint tests will run automatically when you make a commit!

hey thanks for your response!!
i read through the article and your suggestion about about precommit lint test was really helpful
I ran the pre-commit hooks locally, and all relevant checks passed
with relevant test log = (
black....................................................................Passed
blacken-docs.........................................(no files to check)Skipped
isort....................................................................Passed
flake8...................................................................Passed
eslint...............................................(no files to check)Skipped
[ticket-36708 e877e5a3be] Cleanup for flake8 check

1 file changed, 1 insertion(+), 1 deletion(-)

)

Since everything appeared clean locally, I expected the GitHub workflow to pass as well, but the same flake8 issue is still reported in CI. Could you provide any guidance on what else I should check or what might cause this discrepancy?
Also, on a related note: is it generally acceptable to make several small commits and pushes to a PR when iterating on CI failures, or is this discouraged?
Thanks again.

[Antoliny]

Running only flake8 locally through tox is also an option.

tox -e flake8

Replying to Rudraksha Dwivedi:

Since everything appeared clean locally, I expected the GitHub workflow to pass as well, but the same flake8 issue is still reported in CI. Could you provide any guidance on what else I should check or what might cause this discrepancy?
Also, on a related note: is it generally acceptable to make several small commits and pushes to a PR when iterating on CI failures, or is this discouraged?
Thanks again.

I’m also not entirely sure how to answer this part.
It probably won’t be a big issue, but once you get more familiar with Django’s CI, I think you won’t have to worry much about these CI-related concerns :)

[Rudraksha Dwivedi]

Replying to Antoliny:

Running only flake8 locally through tox is also an option.

tox -e flake8

Replying to Rudraksha Dwivedi:

Since everything appeared clean locally, I expected the GitHub workflow to pass as well, but the same flake8 issue is still reported in CI. Could you provide any guidance on what else I should check or what might cause this discrepancy?
Also, on a related note: is it generally acceptable to make several small commits and pushes to a PR when iterating on CI failures, or is this discouraged?
Thanks again.

I’m also not entirely sure how to answer this part.
It probably won’t be a big issue, but once you get more familiar with Django’s CI, I think you won’t have to worry much about these CI-related concerns :)

hey! Thanks for being patient ! i realised how i had duplicated the same like of code in two distinct places, also figured out how to setup and test locally properly
Thanks ^_
If the issues dont occur i will mark needs improvement as no
peace ✨