Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Ability to assign user to role with model & id + cache improvements #186

Open
wants to merge 1 commit into
base: 1.0
Choose a base branch
from

Conversation

konovalov-nk
Copy link

@konovalov-nk konovalov-nk commented Dec 14, 2016

  • Added ability to assign user to role with model & id
  • Added cache reset after assignRole/revokeRole
  • getRoles and getPermissions behavior changed to see actual role and permission mappings
  • Integration Tests to cover new features

I needed a simple feature that allows assigning user roles to models and model ids:

$user->assignRole('manager', 'organization', 123);
$user->can('manage.news.organization', 'organization', 123);

See this issue.

You can see how it basically works by reading through this integration test.

- Added cache reset after assignRole/revokeRole
- getRoles and getPermissions behavior changed to see actual role and permission mappings
- Integration Tests to cover new features
@mtveerman
Copy link

This is awesome and the reasons why I'm looking into this package anyway.

Do you think it would be an idea/possibility to make the user_role pivot table such that we can use Many To Many Polymorphic Relations? I think the current setup is almost there already, except for the db-table column naming.

This would allow people to easily retrieve all the models on which a user has a certain role.

@konovalov-nk
Copy link
Author

@mtveerman I think that is actually possible but I'm not sure how Many to Many Polymorphic Relations actually work. An example how it would work with this package would be really helpful 👍
At the moment I'm just using define() for model names and permissions but your solution seems to be more appropriate. Also if you would like to try out this pull request you can just use it like this:

    "require": {
        "konovalov-nk/laravel-acl": "~1.0@dev",
    },

    "repositories": [
        {
            "url": "https://github.com/konovalov-nk/laravel-acl.git",
            "type": "git"
        }
    ],

@kodeine
Copy link
Owner

kodeine commented Feb 28, 2017

@konovalov-nk is it backwards compatible because i see u merged this into 1.0, based on #181 i think we had to merge this into master and then release version 2 right?

@konovalov-nk
Copy link
Author

@kodeine It breaks compatibility with 1.0 so it's more like version 2.0; yes, that is intended for version 2.0 release.

@kodeine
Copy link
Owner

kodeine commented Jun 12, 2017

@konovalov-nk can you PR this for master branch so we can merge it with master for v2?

@konovalov-nk
Copy link
Author

@kodeine sure, I'll change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants