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 Tim Schilling, 4 weeks ago

Type: Uncategorized → Cleanup/optimization

comment:2 by Md. Saikat Islam, 4 weeks ago

Owner: set to Md. Saikat Islam
Status: new → assigned
Triage Stage: Unreviewed → Accepted

Seems valid issue. The error message should be improved. I am taking it.

comment:3 by Md. Saikat Islam, 4 weeks ago

Has patch: set

comment:4 by Md. Saikat Islam, 4 weeks ago

PR is ready for this ticket: ​https://github.com/django/django/pull/21899

comment:5 by blighj, 3 weeks ago

Patch needs improvement: set

comment:6 by Jacob Walls, 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 Md. Saikat Islam, 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 Jacob Walls, 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.

Note: See TracTickets for help on using tickets.
Back to Top