Opened 4 weeks ago
Last modified 13 days ago
#37327 assigned Cleanup/optimization
Improve IncorrectLookupParameters admin error message
| Reported by: | Tim Schilling | Owned by: | Md. Saikat Islam |
|---|---|---|---|
| Component: | contrib.admin | Version: | dev |
| Severity: | Normal | Keywords: | messaging |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | yes | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description
This is based on leunga1000's PR. The ModelAdmin.changelist_view's handling of IncorrectLookupParameters presents a page title of "Database error" for the particular exception of IncorrectLookupParameters. Since the user is already past authentication and authorization, we should give them more information that the querystring had an error. I would suggest "Incorrect lookup" or "Incorrect lookup in query string" depending on how technical we want to get.
I agree with leunga1000's assessment that this message is a bit cryptic and think we should make a change.
Change History (8)
comment:1 by , 4 weeks ago
| Type: | Uncategorized → Cleanup/optimization |
|---|
comment:2 by , 4 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
| Triage Stage: | Unreviewed → Accepted |
comment:3 by , 4 weeks ago
| Has patch: | set |
|---|
comment:4 by , 4 weeks ago
PR is ready for this ticket: https://github.com/django/django/pull/21899
comment:5 by , 3 weeks ago
| Patch needs improvement: | set |
|---|
comment:6 by , 3 weeks ago
I'm having a a Chesterton's Fence doubt here. The code comment says "the 'invalid=1' parameter was already in the query string", so does anyone know why this was regarded as a database error before?
comment:7 by , 3 weeks ago
the changelist_view uses creates a get_changelist_instance and that method calls many methods inside ChangeList class in view/main.py. I inspected that, inside these two files most of the times we have raised IncorrectLookupParameters error, for e.g FieldDoesNotExist, ValueError.
Heres my anaylisis on this, when we first catch the IncorrectLookupParameters we consider it the users fault, and pass a flag (value is actually 'e=' in code, the comment is stale to have 'invalid='. And after a fresh redirect, when we again has IncorrectLookupParameters error, we are sure that, its not problem in request params, its maybe our raised errors.
So, I am now confidence that the "database error" is not correct. Best, we use "Server error" in when we have error after the flag set.
comment:8 by , 13 days ago
| Easy pickings: | unset |
|---|---|
| Needs tests: | set |
Saikat has done a great job being responsive to feedback. On their PR, I just requested a test case that shows the error condition that would actually render this 20-year old template. If we can't find one, we should close as needsinfo or possibly repurpose this ticket to remove the template (which is untested).
Without knowing if the error condition is realistic we can't take a decision about how careful to be about invalidating translations.
Seems valid issue. The error message should be improved. I am taking it.