-
Notifications
You must be signed in to change notification settings - Fork 55
Dev next #347
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
Dev next #347
Conversation
…onRule, hasPermission and hasPermissionForProject
…l.hasPermission method to check permission instead of reimplementing logic
fixed topcoder-platform #234 : added checks for user already member of the team
Thanks @maxceem. For the permission logic, do you think the util you have written now, is potentially candidate to be moved to the core library itself so that others can take benefit of the same logic and we have streamlined way to define permissions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2 of 3 methods don't depend on the DB, so they for sure can be moved to the core library (potentially). Also, I wrote them by keeping in mind that we are trying to implement simmilar methods in Connect App appirio-tech/connect-app#2841. Anyway, the most important thing is not methods itself, but the format to define permissions. It would be great if we could use the same permission format across all the repos. So I guess the plan here could be like:
|
Sounds like a plan. Go ahead. |
@vikasrohit this PR is for your information and confirmation for new changes in the DEV branch: