PR#1177 Merged Access levels in projects: ticket, commit and admin

Proposed a year ago by vivekanand1101
Modified a year ago
From forks/vivekanand1101/pagure access_levels  into pagure project_acls

This doesn't have tests yet. I will add them once the approach is approved.

No commits found

farhaan commented on line 8 of pagure/lib/__init__.py.
a year ago
(Hide)
The style for docstring is not following the convention.
farhaan commented on line 23 of pagure/lib/__init__.py.
a year ago
(Hide)
comment convention not followed here
farhaan commented on line 73 of pagure/lib/__init__.py.
a year ago
(Hide)
here
farhaan commented on line 241 of pagure/lib/__init__.py.
a year ago
(Hide)
Just a suggestion , I feel you should elaborate on `obj` param in docstring .
a year ago

1 new commit added

a year ago

33 new commits added

  • Fix code style as suggested by @farhaan
  • Add missing import in ui/fork.py
  • Pass repo_committer to repo_info.html
  • Repo info outside the settings should be available to committers
  • 'Request pull' option should be available to committers
  • Adjust variable name in _formhelper.html since it needs committer rights now
  • Add missing import in ui/repo
  • Pass repo_committer and repo_admin values to all the templates inheriting repo_master.html
  • Show all open tickets to repo_committer, including the private ones
  • It's users/committers/admins and not user/committer/admin while checking for list of people with access in project
  • API: committers have access to private tickets
  • API: PR merging and closing rights should be with the committers
  • Internal endpoint: committers of the project should be able to view the private tickets
  • Add missing comma
  • To-do list of user should only show the projects in which the user is a committer
  • Adjust comments in search_projects since it returns the project the user has any right on
  • Set efault value for access level for user/group be 'admin'
  • Adjust comment since we are including all the users with any access in the repo to get mailed
  • Give gitolite W+ rights to committers and committer_groups
  • Anything related to pull request should be handled by repo_committer
  • Add is_repo_user and is_repo_committer methods
  • Enable repo user to edit metadata of issues and committer to edit the issue
  • Committer should be able to delete a branch, edit file online
  • Adjust is_repo_admin since we have different access levels now
  • Adjust error msg for it's removing user with any access level now
  • Let's have only one func for getting access in cases of users and groups
  • Don't allow multiple accesses even at DB level
  • Add user and group with different access level
  • Adjust forms after different access levels
  • Add front end for allowing different access levels
  • Add a lib func to get all the access levels
  • Add table access_levels, foreign key in user_projects/projects_groups and adjust the model accordingly
  • Allow alembic to get naming convention from sqlalchemy
a year ago

33 new commits added

  • Fix code styles as suggested by @farhaan
  • Add missing import in ui/fork.py
  • Pass repo_committer to repo_info.html
  • Repo info outside the settings should be available to committers
  • 'Request pull' option should be available to committers
  • Adjust variable name in _formhelper.html since it needs committer rights now
  • Add missing import in ui/repo
  • Pass repo_committer and repo_admin values to all the templates inheriting repo_master.html
  • Show all open tickets to repo_committer, including the private ones
  • It's users/committers/admins and not user/committer/admin while checking for list of people with access in project
  • API: committers have access to private tickets
  • API: PR merging and closing rights should be with the committers
  • Internal endpoint: committers of the project should be able to view the private tickets
  • Add missing comma
  • To-do list of user should only show the projects in which the user is a committer
  • Adjust comments in search_projects since it returns the project the user has any right on
  • Set efault value for access level for user/group be 'admin'
  • Adjust comment since we are including all the users with any access in the repo to get mailed
  • Give gitolite W+ rights to committers and committer_groups
  • Anything related to pull request should be handled by repo_committer
  • Add is_repo_user and is_repo_committer methods
  • Enable repo user to edit metadata of issues and committer to edit the issue
  • Committer should be able to delete a branch, edit file online
  • Adjust is_repo_admin since we have different access levels now
  • Adjust error msg for it's removing user with any access level now
  • Let's have only one func for getting access in cases of users and groups
  • Don't allow multiple accesses even at DB level
  • Add user and group with different access level
  • Adjust forms after different access levels
  • Add front end for allowing different access levels
  • Add a lib func to get all the access levels
  • Add table access_levels, foreign key in user_projects/projects_groups and adjust the model accordingly
  • Allow alembic to get naming convention from sqlalchemy
pingou commented on line 8 of alembic/env.py.
a year ago
(Hide)
Hm, why is this needed?
pingou commented on line 20 of alembic/versions/5290c20d290d_new_access_levels.py.
a year ago
(Hide)
I usually do not use alembic to create tables, only to modify existing ones. For new tables I just use the existing createdb script
(Hide)
80 chars?
pingou commented on line 32 of pagure/lib/__init__.py.
a year ago
(Hide)
This will require a change to fedmsg_meta_fedora_infrastructure
pingou commented on line 82 of pagure/lib/__init__.py.
a year ago
(Hide)
This as well :)
pingou commented on line 193 of pagure/lib/__init__.py.
a year ago
(Hide)
I see what you mean by reading the code but not by reading the docstring. Could you expand on it?
pingou commented on line 236 of pagure/lib/__init__.py.
a year ago
(Hide)
So if there is a 4th access_level entered in the DB, the lines above will allow it but the code below basically won't do anything. Since the code below must be adjusted for new access levels, it raises two questions: * Does it make sense to allow entering new access levels in the DB? * Does it make sense to query the DB for these here instead of, say, using a hard-coded list?
pingou commented on line 236 of pagure/lib/__init__.py.
a year ago
(Hide)
I feel like the answer to the first question is likely `yes` since we use that in other places
pingou commented on line 42 of pagure/lib/model.py.
a year ago
(Hide)
Maybe split the `or_` over two lines?
pingou commented on line 41 of pagure/lib/model.py.
a year ago
(Hide)
Since you are within brackets, you likely do not need the `\`

Pretty impressive change set, nice job!

I have a few questions but nothing major standing out.

We shouldn't forget unit-tests and docs before we merge this into master but these should be different PRs

vivekanand1101 commented on line 236 of pagure/lib/__init__.py.
a year ago
(Hide)
the list should be better, will avoid a query.
vivekanand1101 commented on line 8 of alembic/env.py.
a year ago
(Hide)
I talked about this in irc. This is to get the naming convention from the model for alembic
vivekanand1101 commented on line 20 of alembic/versions/5290c20d290d_new_access_levels.py.
a year ago
(Hide)
ok, will remove this.
a year ago

36 new commits added

  • use list of access levels instead of a db query for diff access levels
  • explain combine in get_project_groups as well
  • Make the table access_levels using createdb.py not alembic
  • Fix code style as suggested by @pypingou and @farhaan
  • Add missing import in ui/fork.py
  • Pass repo_committer to repo_info.html
  • Repo info outside the settings should be available to committers
  • 'Request pull' option should be available to committers
  • Adjust variable name in _formhelper.html since it needs committer rights now
  • Add missing import in ui/repo
  • Pass repo_committer and repo_admin values to all the templates inheriting repo_master.html
  • Show all open tickets to repo_committer, including the private ones
  • It's users/committers/admins and not user/committer/admin while checking for list of people with access in project
  • API: committers have access to private tickets
  • API: PR merging and closing rights should be with the committers
  • Internal endpoint: committers of the project should be able to view the private tickets
  • Add missing comma
  • To-do list of user should only show the projects in which the user is a committer
  • Adjust comments in search_projects since it returns the project the user has any right on
  • Set efault value for access level for user/group be 'admin'
  • Adjust comment since we are including all the users with any access in the repo to get mailed
  • Give gitolite W+ rights to committers and committer_groups
  • Anything related to pull request should be handled by repo_committer
  • Add is_repo_user and is_repo_committer methods
  • Enable repo user to edit metadata of issues and committer to edit the issue
  • Committer should be able to delete a branch, edit file online
  • Adjust is_repo_admin since we have different access levels now
  • Adjust error msg for it's removing user with any access level now
  • Let's have only one func for getting access in cases of users and groups
  • Don't allow multiple accesses even at DB level
  • Add user and group with different access level
  • Adjust forms after different access levels
  • Add front end for allowing different access levels
  • Add a lib func to get all the access levels
  • Add table access_levels, foreign key in user_projects/projects_groups and adjust the model accordingly
  • Allow alembic to get naming convention from sqlalchemy
vivekanand1101 commented on line 5 of alembic/versions/987edda096f5_access_id_in_user_projects.py.
a year ago
(Hide)
I will update this
a year ago

2 new commits added

  • Send access level in fedmsg while adding or updating user/group access
  • update revision number in comments after removal of alembic file
pingou commented on line 64 of pagure/lib/__init__.py.
a year ago
(Hide)
80 chars?

Ok, so there was a simple comment about style, since this will not be the last PR for this feature, I'll merge this one and we can iterate based on this.

Thanks for picking this up, it's not a small one and it will be good to have it :)

a year ago

Pull-Request has been merged by pingou