Re: [Django] #15779: admin cannot edit records with value 'add' as primary key
Django
-------------------------------------+-------------------------------------
Reporter: Marwan | Owner: nobody
Alsabbagh <marwan.alsabbagh@…> | Status: closed
Type: Bug | Component: contrib.admin
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: fixed | Has patch: 0
Triage Stage: Accepted | Needs tests: 0
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by ramiro):
* status: new => closed
* ui_ux: => 0
* resolution: => fixed
* easy: => 0
Comment:
I'm going to call this ticket fixed in [16857]. After that revision, it is
possible to implement a custom URL scheme in the admin app because it uses
named URLs when resolving hthe URLs in the model CRUD workflow. There is
an example of a model with a CharField PK and with an instance with an
`'add'` value for it in the test case added in such commit:
https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/admin_custom_urls/models.py?rev=16857
The customziation process isn't completely straightforward because the
named URL entry one is replacing needs to be removed from the !ModelAdmin
URL map to force the admin app to take in account the one we are
installing (see the `remove_url()` method in the example linked above).
Patches to make this easier are welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Marwan | Owner: nobody
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Needs documentation: 0 | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
* status: closed => reopened
* resolution: fixed =>
Comment:
After discussion with Ramiro and Julien in IRC, reopening this because it
really ought to "just work" and the right way to make it "just work" is to
reconsider the admin URL structure to remove ambiguity, as discussed
above.
Since this likely won't happen immediately, the workaround enabled by
r16857 can be a useful stopgap for those who need it.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:5>
Django
-------------------------------------+-------------------------------------
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by claudep):
I've done the first step of the work, by using `reverse` as much as
possible in the various admin tests.
https://github.com/django/django/pull/4076
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz <claude@…>):
In [changeset:"32e6a7d3a57b2287d55e8b8efa4e8cb7643b1720"]:
{{{
#!CommitTicketReference repository=""
revision="32e6a7d3a57b2287d55e8b8efa4e8cb7643b1720"
Replaced hardcoded URLs in admin_* tests
Refs #15779. This will allow easier admin URL changes, when needed.
Thanks Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/4088
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"cd260d03bd7c43b78ad3a394e1b62f526f40f4ce"]:
{{{
#!CommitTicketReference repository=""
revision="cd260d03bd7c43b78ad3a394e1b62f526f40f4ce"
[1.8.x] Replaced hardcoded URLs in admin_* tests
Refs #15779. This will allow easier admin URL changes, when needed.
Thanks Simon Charette for the review.
Backport of 32e6a7d3a57b2287d55e8b8efa4e8cb7643b1720 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
I am afraid that some users will have bookmarked existing edit URLs and
are going to be annoyed when those now 404. I wonder if we should issue
redirects for the old patterns and then have an option to turn them off
for users affected by this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: charettes (added)
Comment:
I think we could safely add a temporary redirect raising a deprecation
warning from the `r'^(.+)/$'` pattern as long as it appears after the
`r'^(.+)/edit/$'` one.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
Site users won't learn of that warning though. Is there a downside to
keeping that redirect indefinitely?
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
Replying to [comment:13 timgraham]:
> Site users won't learn of that warning though.
Right.
> Is there a downside to keeping that redirect indefinitely?
I don't see one but I would avoid using a permanent redirect here.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by claudep):
I've just added a complementary commit for the redirect. However, I think
we should get rid of it after "some" releases...
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: new
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Component: contrib.admin | Version: 1.3
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1791a7e75af8c9e7ca54425592379fd1f840fe88"]:
{{{
#!CommitTicketReference repository=""
revision="1791a7e75af8c9e7ca54425592379fd1f840fe88"
Fixed #15779 -- Allowed 'add' primary key in admin edition
Thanks Marwan Alsabbagh for the report, and Simon Charette and
Tim Graham for the reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Type: Bug | Status: closed
Component: contrib.admin | Version: 1.3
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
* severity: Normal => Release blocker
* stage: Ready for checkin => Accepted
Comment:
This broke the password change link at `/admin/auth/user/1/change/`.
We could change the URL in `UserAdmin.get_urls()` or update the link in
the form:
{{{
diff --git a/django/contrib/auth/forms.py b/django/contrib/auth/forms.py
index 928c4c7..84fc757 100644
--- a/django/contrib/auth/forms.py
+++ b/django/contrib/auth/forms.py
@@ -98,7 +98,7 @@ class UserChangeForm(forms.ModelForm):
password = ReadOnlyPasswordHashField(label=_("Password"),
help_text=_("Raw passwords are not stored, so there is no way to
see "
"this user's password, but you can change the
password "
- "using <a href=\"password/\">this form</a>."))
+ "using <a href=\"../password/\">this form</a>."))
class Meta:
model = User
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: nobody
<marwan.alsabbagh@…> |
Component: contrib.admin | Version: 1.3
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* status: closed => new
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:19>
Django
-------------------------------------+-------------------------------------
<marwan.alsabbagh@…> |
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => claudep
* status: new => assigned
* version: 1.3 => master
Comment:
Having this relative link into `help_text` is doomed to fail. However, I
don't see any simple workaround for now, so Tim's patch just needs a test.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: claudep
<marwan.alsabbagh@…> |
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by gertsteyn):
The first paragraph of the Django URL dispatcher documentation says:
"[http://www.w3.org/Provider/Style/URI Cool URIs don’t change]"...
Do we really want to change the URL structure of every Django site out
there for an extreme edge case like this?
If I use a CharField as primary key nothing stops me from using "1/change"
as key and we'll have exactly the same problem. If you insist on using a
CharField as primary key adding some validation to exclude some "reserved"
words is not to much of an ask.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: claudep
<marwan.alsabbagh@…> |
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by claudep):
> If I use a CharField as primary key nothing stops me from using
"1/change" as key and we'll have exactly the same problem.
No, "1/change" would be converted to "change_2F1" in the URL.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: claudep
<marwan.alsabbagh@…> |
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by claudep):
Err... "1_2Fchange", of course.
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: claudep
<marwan.alsabbagh@…> |
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/4400
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: claudep
<marwan.alsabbagh@…> |
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: claudep
<marwan.alsabbagh@…> |
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz <claude@…>):
In [changeset:"c2bfd76ec3678cc28fb6fce6be0ae217b46bcd9e" c2bfd76e]:
{{{
#!CommitTicketReference repository=""
revision="c2bfd76ec3678cc28fb6fce6be0ae217b46bcd9e"
Refs #15779 -- Fixed UserChangeForm regression introduced by 1791a7e75
Thanks Tim Graham for reporting the regression.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Marwan Alsabbagh | Owner: claudep
<marwan.alsabbagh@…> |
Component: contrib.admin | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/15779#comment:27>