Opened 15 years ago

Last modified 22 months ago

#10686 assigned New feature

Add class name interpolation in Meta.permissions codenames — at Version 24

Reported by: faldridge Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: permissions inheritance
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by julian@pinabausch.org)


Change History (26)

by faldridge, 15 years ago

Attachment: permission_inheritance.diff added

comment:1 by Jacob, 15 years ago

Triage Stage: UnreviewedDesign decision needed

comment:2 by anonymous, 13 years ago

I think both of these improvements need to be implemented. I make heavy use of abstract classes and writing the permissions for each child model is a lot of redundant coding.

comment:3 by Seth Buntin, 13 years ago

Severity: Normal
Type: New feature

by Soby, 13 years ago

Updated patch that works on rev 16527. Also supports de-duping permissions and combining permissions from multiple inheritance on the model.

comment:4 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:5 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:6 by Anssi Kääriäinen, 11 years ago

Triage Stage: Design decision neededAccepted

Seems like good changes to me.

comment:7 by Ilkka Hakkari, 11 years ago

I managed to write tests for the new functionality described in this ticket.

https://github.com/derega/django/tree/ticket_10686

comment:8 by Tim Graham, 11 years ago

Needs documentation: set

These changes also need to be documented.

comment:9 by Sergei Maertens, 8 years ago

Owner: changed from nobody to Sergei Maertens
Status: newassigned

comment:10 by Sergei Maertens, 8 years ago

I'm going to apply the patch to current master and add documentation updates.

comment:11 by Sergei Maertens, 8 years ago

Needs documentation: unset

Applied the patch to current master and I've added documentation.

comment:12 by Tim Graham, 8 years ago

Patch needs improvement: set

django-developers discussion which suggests to avoid the proposed inherit_permissions Meta option.

comment:13 by Sergei Maertens, 8 years ago

I'm starting work on this again such that inherit_permissions is not needed.

Models inheriting from abstract models inherit the Meta, so unless permissions are being added in the concrete model, there is no boilerplate required. In the event that the concrete class also has custom permissions, the pythonic way is the way to go:

class Task(BaseTask):
    ...

    class Meta(BaseTask.Meta):
        permissions = BaseTask.Meta.permissions + (
            ('reassign_assignedtask', 'Can reassign assigned task')
        )

comment:14 by Sergei Maertens, 8 years ago

Addition: it's not possible to inherit a concrete class' Meta per https://docs.djangoproject.com/en/1.9/topics/db/models/#meta-and-multi-table-inheritance

comment:15 by Sergei Maertens, 8 years ago

Patch needs improvement: unset
Last edited 8 years ago by Tim Graham (previous) (diff)

comment:16 by Tim Graham, 8 years ago

Summary: 2 simple improvements to permission inheritance.Add class name interpolation in Meta.permissions codenames

comment:17 by Tim Graham, 8 years ago

Patch needs improvement: set

Comments for improvement on the PR.

comment:18 by Sergei Maertens, 8 years ago

Patch needs improvement: unset

Processed the remarks, up for re-review

comment:19 by Tim Graham, 8 years ago

Patch needs improvement: set

Left some more comments.

comment:20 by Sergei Maertens, 8 years ago

Patch needs improvement: unset

Processed comments - 1 comment was replied to on Github & no change made in docs: https://github.com/django/django/pull/6861#discussion-diff-74686863

comment:21 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:22 by Sergei Maertens, 8 years ago

Note to self: the unit test should check that the correct permissions are created in the db, per:

https://github.com/django/django/pull/6861#issuecomment-240427426

Worked on some cosmetic edits: http://dpaste.com/19R4PZE but I also noticed that this doesn't seem to create the correct permission in the database. For the pizza example in the docs I see things like "can_deliver_base" in the database.

comment:23 by Mariusz Felisiak, 3 years ago

Owner: Sergei Maertens removed
Status: assignednew

comment:24 by julian@pinabausch.org, 22 months ago

Description: modified (diff)

Dear community,

just picked this up while figuring out how to build permission names dynamically.

Here’s a new PR: https://github.com/django/django/pull/15936

Open issues:

  • Tim Graham commented that the code may be [generating faulty permissions](https://github.com/django/django/pull/6861#issuecomment-240427426). I checked the behavior in a dummy project, but cannot confirm. I struggled to write a test for this, because it needs real migrations, real data etc. in the database. Please let me know if there’s an example in the existing tests that I can check (the advanced testing docs don’t mention “defining a real model in a testcase” either)
  • Authors and version added are still missing – I’ll add this if it has a chance to get merged.
  • (Changed to class argument to model_name – I believe this is more consistent with content types & auth)
Last edited 22 months ago by julian@pinabausch.org (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top