search

amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

Utilize Role isAdmin() functionality #225

Closed damiankloip closed 8 years ago

damiankloip commented 8 years ago

Currently we extend Role with OgRole but remove the is_admin flag usage, but all the methods are available. We are also using all the Role methods to check a role has a permission etc.. isAdmin() is built into all that. As if a role have that, it will always return true. So this is a simpler way of implementing admin permission checking, and being more in line with core.

damiankloip commented 8 years ago

I think we need to keep OgAccess::ADMINISTER_GROUP_PERMISSION as that is a global thing.

amitaibu commented 8 years ago

Wow, it's @damiankloip! :)

damiankloip commented 8 years ago

@amitaibu Surprise!

pfrenssen commented 8 years ago

This looks good to me!

pfrenssen commented 8 years ago

Note that getMembership() is not tested here, but this has been cherry-picked from #217. A test for this method will be added in that PR.

pfrenssen commented 8 years ago

Small remark: I think it's a good idea to rename Og::getUserMembership() to Og::getMembership(). For consistency, can you also rename Og:getUserMemberships() to Og::getMemberships()?

damiankloip commented 8 years ago

@pfrenssen yes, we already discussed that on here but it's on one of the 'out of date diff' comments, see https://github.com/amitaibu/og/pull/225#discussion_r66575563 . Basically renaming that will extend the scope of this further than we need so I think that can be a simple follow up. That keeps things easier to review.

pfrenssen commented 8 years ago

:+1:

damiankloip commented 8 years ago

I think no conflicts with thw og access service PR. wow! :)

amitaibu commented 8 years ago

needs a re-roll

damiankloip commented 8 years ago

Yeah, this is a nasty re-roll. I can't even be bothered to do this now... It will have to wait.

amitaibu commented 8 years ago

I can give it a try. I can take the shortcut and do a git merge 8.x-1.x. I hate rebasing..

amitaibu commented 8 years ago

@damiankloip since it's on your own repo, I'll create a new PR for this. Is that ok with you?

damiankloip commented 8 years ago

Yes sure. Just base it from this branch and it should be all good?

amitaibu commented 8 years ago

git is great! Closed in favor of #231

damiankloip commented 8 years ago

git is great. Thanks @amitaibu

pfrenssen commented 8 years ago

Created followup novice issue to rename Og::getUserMemberships() to Og::getMemberships().