#33095 closed Cleanup/optimization (wontfix)
Admin actions shown with zero results
Reported by: | Richard Laager | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | 3.2 |
Severity: | Normal | Keywords: | |
Cc: | Carlton Gibson | Triage Stage: | Unreviewed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description (last modified by )
contrib/admin/views/main.py has this:
# Admin actions are shown if there is at least one entry # or if entries are not counted because show_full_result_count is disabled self.show_admin_actions = not self.show_full_result_count or bool(full_result_count)
This was changed in 17557d068c43bd61cdc6c18caf250ffa469414a1 for #8408.
Is it intentional that the admin actions be shown if there are any rows in the table as opposed to any results on the page? Put differently, if there are no results on the page, I can't do anything with the actions, right?
It seems like this should use result_count (which is the number of filtered results) rather than full_result_count (which is the number of rows in the table):
# Admin actions are shown if there is at least one entry self.show_admin_actions = bool(result_count)
Additionally, if the above is accepted, then the only use of full_result_count is on the search page, which opens up another opportunity to optimize. As discussed further in #8408, there are two different SELECT COUNT(*) calls. One is for the paginator, which operates on the filtered results and one is for full_result_count which is unfiltered. The latter is only used once (if the above suggestion is adopted), in contrib/admin/templates/admin/search_form.html.
So we could skip calculating full_result_count if there is no search being done. Note that this is subtly different than if there are no filters; see #22810 for why that is an issue.
Alternatively, if you want to be safe and not make assumptions about what people might be doing with full_result_count, this also works: full_result_count = lazy(self.root_queryset.count()). Then if it is never evaluated, the query never runs. I've tested both approaches.
Change History (6)
comment:1 by , 3 years ago
Description: | modified (diff) |
---|
follow-up: 3 comment:2 by , 3 years ago
Cc: | added |
---|---|
UI/UX: | set |
follow-up: 4 comment:3 by , 3 years ago
Has patch: | set |
---|
Replying to Mariusz Felisiak:
Yes I believe that's intentional. It's true that you cannot do anything when there are no results on the page, however you can change filters dynamically and toggling actions every time would make it annoying. "wontfix" as for me, but I'm not a UX expert, so let's wait for the second opinion.
Fair enough, but let me ask a couple follow-ups then... Is this important enough to justify a SELECT COUNT(*), which there are a bunch of complaints about in #8408? But more to the point, if this is going to show the actions if there are any rows in the table, then why not just always show the actions? Surely it's not common to have a database table with 0 rows. And the same argument about the UI changing applies there too... it's going to change as soon as you add the first object.
comment:4 by , 3 years ago
Replying to Richard Laager:
And the same argument about the UI changing applies there too... it's going to change as soon as you add the first object.
IMO, no, because you're adding new objects in a different view. So the layout is not changing when you're on it.
comment:5 by , 3 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Hi Richard, thanks yes... — I'm think I'm with Mariusz here, but it was an intentional change so I'd like to see some consensus to change it if we were going to after all this time. An approach to the DevelopersMailingList (with a fuller proposal, to guide folks along) would be appropriate.
I hope that makes sense. Thanks.
comment:6 by , 7 weeks ago
I doubt this would get accepted into Django itself (as there would probably be a concern about queryset edge cases), but for anyone finding this bug... There is another way to eliminate the duplicate query in the common case where no filters have been applied:
--- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -280,7 +280,11 @@ class ChangeList: # Get the total number of objects, with no admin filters applied. if self.model_admin.show_full_result_count: - full_result_count = self.root_queryset.count() + root_queryset = self.root_queryset + if root_queryset.query.where == self.queryset.query.where: + full_result_count = paginator.count + else: + full_result_count = root_queryset.count() else: full_result_count = None can_show_all = result_count <= self.list_max_show_all
That checks to see if any conditions have been applied to the where clause. If not, then there is no point in doing the root_queryset query, as it will return the same thing that the paginator's count method will.
Yes I believe that's intentional. It's true that you cannot do anything when there are no results on the page, however you can change filters dynamically and toggling actions every time would make it annoying. "wontfix" as for me, but I'm not a UX expert, so let's wait for the second opinion.