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

Allow members of "Admin" Team to wipe version envs #3791

Merged
merged 2 commits into from
Mar 15, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 13, 2018

if request.user not in version.project.users.all():
# We need to check by ``for_admin_user`` here to allow members of the
# ``Admin`` team (which doesn't own the project) under the corporate site.
if version.project not in Project.objects.for_admin_user(user=request.user):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm changing the logic a bit here since we were checking for "the user belongs to a group" and now I'm checking if "project is under the project which the has admin permissions". Seems to be correct anyway, but just want to mentioned this change on the logic here.

@humitos
Copy link
Member Author

humitos commented Mar 13, 2018

I did manual QA under .com and .org after applying this patch.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. We should probably check other places that this logic is being used improperly.

@ericholscher
Copy link
Member

@humitos I believe we should be syncing projects.users on the .com with a signal, to properly make this work, so that might also be broken.

@humitos
Copy link
Member Author

humitos commented Mar 14, 2018

This looks good. We should probably check other places that this logic is being used improperly.

I did a grep by project.users and I found many places where this is used, but I think that those places don't affect .com. I created an issue under the corporate site to re-check this again in the near future and do not forget about this: https://github.com/readthedocs/readthedocs-corporate/issues/259

@humitos I believe we should be syncing projects.users on the .com with a signal, to properly make this work, so that might also be broken.

Taking a look at the code of the signal it seems to be right: "all the owners of the organization will be added as owner of the project" which makes sense to me.

We could consider adding "all the users in the Admin team of that organization as owner of the project" also.

(I'm moving this conversation to a new issue under .com repository: https://github.com/readthedocs/readthedocs-corporate/issues/260)

@humitos
Copy link
Member Author

humitos commented Mar 15, 2018

I think we can merge this for now to solve a .com issue and continue discussing the best solution on those issues that I opened.

@agjohnson agjohnson merged commit e73b266 into master Mar 15, 2018
@agjohnson agjohnson deleted the humitos/wipe/permissions-dotcom branch March 15, 2018 17:37
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