Code

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#20642 closed Cleanup/optimization (fixed)

Deprecate `Option.get_(add|change|delete)_permission`.

Reported by: charettes Owned by: nobody
Component: Database layer (models, ORM) Version: master
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

In the whole Clean Model Options process we should deprecate those methods in favor of contrib.auth alternatives.

Even if those methods are undocumented it's no secret they are used in the wild. Django exclusively use them in contrib.admin:

(django)simon@simon-laptop:~/workspace/django$ grep -E "get_(add|change|delete)_permission" django/ -Rn --include="*.py"
django/db/models/options.py:416:    def get_add_permission(self):
django/db/models/options.py:419:    def get_change_permission(self):
django/db/models/options.py:422:    def get_delete_permission(self):
django/contrib/admin/options.py:355:        return request.user.has_perm(opts.app_label + '.' + opts.get_add_permission())
django/contrib/admin/options.py:369:        return request.user.has_perm(opts.app_label + '.' + opts.get_change_permission())
django/contrib/admin/options.py:383:        return request.user.has_perm(opts.app_label + '.' + opts.get_delete_permission())
django/contrib/admin/options.py:1671:            self.opts.app_label + '.' + self.opts.get_add_permission())
django/contrib/admin/options.py:1683:            opts.app_label + '.' + opts.get_change_permission())
django/contrib/admin/options.py:1693:            self.opts.app_label + '.' + self.opts.get_delete_permission())
django/contrib/admin/util.py:122:                           opts.get_delete_permission())

Attachments (2)

0001-Fixed-20642-Deprecated-Option.get_-add-change-delete.patch (18.5 KB) - added by charettes 10 months ago.
Patch with doc
20642-tests.diff (3.9 KB) - added by timo 10 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 10 months ago by charettes

  • Has patch set

Added a patch that deprecates option permission methods and moves contrib.auth.management._get_permission_codename to contrib.auth.get_permission_codename and use it in contrib.admin.

Wondering if we should document get_permission_codename, it feels a bit odd to deprecate those methods while specifying no alternatives in the release notes.

I still pointed to it in the raised deprecation warning.

comment:2 Changed 10 months ago by loic84

  • Triage Stage changed from Unreviewed to Ready for checkin

+1 much needed cleanup.

Tested it on a project that uses permissions and ModelAdmin.has_x_permission overrides extensively and didn't notice any issue.

I'd go either ways for documenting get_permission_codename, the methods it replaces weren't documented, so people that used them know where to look.

Last edited 10 months ago by loic84 (previous) (diff)

Changed 10 months ago by charettes

Patch with doc

comment:3 Changed 10 months ago by charettes

Updated the RFC patch to use super in InlineAdmin instead of duplicating permission codename creation.

comment:4 Changed 10 months ago by Simon Charette <charette.s@…>

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

In b91787910c9d5a036674d46a73d1b48ca33123a3:

Fixed #20642 -- Deprecated Option.get_(add|change|delete)_permission.

Those methods were only used by contrib.admin internally and exclusively
related to contrib.auth. Since they were undocumented but used
in the wild the raised deprecation warning point to an also undocumented
alternative that lives in contrib.auth.

Also did some PEP8 and other cleanups in the affected modules.

comment:5 Changed 10 months ago by Simon Charette <charette.s@…>

In e1dd24d2f6dd3464ab50593320a7eb2325d6c196:

Added missing deprecation note for model permission methods.

refs #20642.

comment:6 Changed 10 months ago by Simon Charette <charette.s@…>

In e628753e7dd1611fb4cf770a78126e96a02ab1d7:

[1.6.x] Added missing deprecation note for model permission methods.

refs #20642.

Backport of e1dd24d2f from master.

Changed 10 months ago by timo

comment:7 Changed 10 months ago by Tim Graham <timograham@…>

In a6a905c619e48bb3db4a5fbb09e5e03abb7ed0f6:

Updated tests for deprecation of Option.get_(add|change|delete)_permission.

refs #20642.

comment:8 Changed 10 months ago by Tim Graham <timograham@…>

In 3c51962cabc9537221b86c667aac5ffaa1469660:

[1.6.x] Updated tests for deprecation of Option.get_(add|change|delete)_permission.

refs #20642.

Backport of a6a905c619 from master.

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.