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

Projects
None yet
3 participants
@humitos
Member

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):

This comment has been minimized.

@humitos

humitos Mar 13, 2018

Member

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

This comment has been minimized.

Member

humitos commented Mar 13, 2018

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

@ericholscher

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

@ericholscher

This comment has been minimized.

Member

ericholscher commented Mar 14, 2018

@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

This comment has been minimized.

Member

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: rtfd/readthedocs-corporate#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: rtfd/readthedocs-corporate#260)

@humitos

This comment has been minimized.

Member

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agjohnson agjohnson deleted the humitos/wipe/permissions-dotcom branch Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment