Code

Opened 3 years ago

Closed 3 years ago

#16101 closed New feature (fixed)

SingleObjectMixin should accept parameters for overriding the URL keywords for pk and slug

Reported by: AndrewIngram Owned by: nobody
Component: Generic views Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by julien)

Currently, in SingleObjectMixin, Django looks for a 'pk' or 'slug' keyword argument to use for looking up an instance of a model. You can override slug_field when the field on the model is something other than "slug". What you can't do is designate different URL keywords to use for these lookup, and to override this in a subclass the whole get_object method needs to be reimplemented.

My proposition is to have a 'pk_keyword' and 'slug_keyword' field on SingleObjectMixin which default to 'pk' and 'slug' respectively, but can easily be overridden.

Use case:

I have a product app with a URL like this:

r'^(?P<item_class_slug>[\w-]+)/(?P<item_slug>[\w-]*)-(?P<pk>\d+)/$'

The view uses a subclass of DetailView with the pk keyword being used for fetching the product. But we also have optional product review URLs which are only included if the reviews app is installed, and are included like this:

url(r'^(?P<item_class_slug>[\w-]+)/(?P<item_slug>[\w-]*)-(?P<pk>\d+)/reviews', include(self.reviews_app.urls)),

Within the reviews URLs we have a review detail view which also uses DetailView, this means I end up with two pk keywords in the URLs. Now I could just use a different keyword for the product pk keyword for this include, but I prefer consistency in my URL patterns. I would rather use 'item_pk' and 'review_pk' in all cases, which I can't currently do.
I've had a look at the code and it seems like a straightforward change, I'm happy to submit the patch myself if this issue is accepted.

Attachments (4)

patch_commit_9b22091813e7.patch (5.7 KB) - added by AndrewIngram 3 years ago.
Added pk_url_kwarg and slug_url_kwarg to SingleObjectMixin so the defaults of 'pk' and 'slug' can be overridden.
patch_commit_9b22091813e7.diff (5.7 KB) - added by AndrewIngram 3 years ago.
Added pk_url_kwarg and slug_url_kwarg to SingleObjectMixin so the defaults of 'pk' and 'slug' can be overridden.
16101.customisable-detailview-pk-slug-url-kwargs.diff (5.9 KB) - added by julien 3 years ago.
16101.customisable-detailview-pk-slug-url-kwargs.2.diff (5.9 KB) - added by julien 3 years ago.
Patch updated to latest trunk

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by AndrewIngram

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I made an error in my initial ticket, aside from accidentally supertexting the URLs, 'reviews' should be at the end of the second URL definition, before the include.

comment:2 Changed 3 years ago by julien

  • Description modified (diff)
  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

(Fixed the ticket description's formatting).

Accepting as this seems like a simple and reasonable way of increasing SingleObjectMixin's flexibility. Like for any new feature, the patch will need to include tests and documentation.

comment:3 Changed 3 years ago by julien

Just a note. slug_keyword and pk_keyword don't feel very explicit to me. I wonder if they shouldn't be slug_url_kwarg and pk_url_kwarg instead. Not the prettiest but at least more explicit, and also consistent with django.views.generic.edit.FormMixin.get_form_kwargs().

Changed 3 years ago by AndrewIngram

Added pk_url_kwarg and slug_url_kwarg to SingleObjectMixin so the defaults of 'pk' and 'slug' can be overridden.

Changed 3 years ago by AndrewIngram

Added pk_url_kwarg and slug_url_kwarg to SingleObjectMixin so the defaults of 'pk' and 'slug' can be overridden.

comment:4 Changed 3 years ago by AndrewIngram

  • Has patch set
  • Needs documentation unset
  • Needs tests unset

comment:5 Changed 3 years ago by julien

Thanks for the patch! I've slightly tweaked the doc, added release notes and removed the get_slug_url_kwarg() and get_pk_url_kwarg() as those complicated things and didn't add real value. I'll just seek for external opinion on the naming ("pk_url_kwarg" and "slug_url_kwarg") before RFC'ing this ticket.

Changed 3 years ago by julien

Patch updated to latest trunk

comment:6 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin
  • UI/UX unset

I'm not thrilled about the names, slug_url_argument and pk_url_argument would be a bit more explicit, but that's not a show stopper, IMO.

comment:7 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [16569]:

Fixed #16101 -- Added parameters to SingleObjectMixin to override the name of the URL keyword arguments used for pk and slug. Thanks, Andrew Ingram and Julien Phalip.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.