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

[Fix #3182] Better user deletion #3214

Merged
merged 4 commits into from Dec 6, 2017
Merged

Conversation

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Oct 31, 2017

Some tests are needed for this PR. Moreover, I think having a migration to delete all the is_active=False user is necessary.
Moreover, removing the project files is necessary.
@agjohnson @ericholscher r?

Copy link
Contributor

@agjohnson agjohnson left a comment

Noted a few areas where we can reduce logic significantly. Part of the issue is that it's not clear how tasks are executed from our build queue, but I think we can avoid tasks for the most part in this PR.

There is a conflict here with what we're doing on our commercial hosting, so I'll need to come back to this and provide some guidance.

Loading


log = logging.getLogger(__name__)
User = get_user_model()
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

What's the reason for using get_user_model() here? The pattern we've used throughout the codebase is to import from django.contrib.auth.models import User

Loading

Copy link
Member Author

@safwanrahman safwanrahman Oct 31, 2017

Choose a reason for hiding this comment

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

I have gone through the documentation and it seems that using settings.AUTH_USER_MODEL is conventional way

Loading

@@ -62,4 +67,23 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen

return False


@receiver(pre_delete, sender=User)
def delete_projects_and_organizations(instance, *args, **kwargs):
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

The function arguments are wrong here. The first argument is sender, or the sending class (User in this case). I believe kwargs will have instance -- that is:

@receiver(pre_delete, sender=User)
def delete_projects_and_organizations(sender, *args, **kwargs):
    instance = kwargs.pop('instance')

https://docs.djangoproject.com/en/1.9/ref/signals/#pre-delete

Loading

Copy link
Member Author

@safwanrahman safwanrahman Oct 31, 2017

Choose a reason for hiding this comment

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

Oh! Sorry, I missed the sender part. I will add it in next commit

Loading

@@ -21,5 +23,9 @@ def run_public(self, user_id):
for service in service_cls.for_user(user):
service.sync()

@task()
def bulk_delete_oauth_remote_organizations(organizations_id):
RemoteOrganization.objects.filter(id__in=organizations_id).delete()
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

This seems like something that should cascade on the database

Loading

@@ -12,6 +12,8 @@
<form method="POST" action=".">
{% csrf_token %}
{{ form }}
<br>
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

Styling should be handled with css, avoid <br> tags entirely.

Loading

@@ -12,6 +12,8 @@
<form method="POST" action=".">
{% csrf_token %}
{{ form }}
<br>
<strong>Be careful! This can not be undone!</strong>
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

Untranslated string here.

Loading

@@ -984,3 +984,8 @@ def clear_html_artifacts(version):
if isinstance(version, int):
version = Version.objects.get(pk=version)
remove_dir(version.project.rtd_build_path(version=version.slug))


@task()
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

Without specifying the queue on this task, it will execute on the build instances, which have no access to the database. Specify @task(queue='web') to ensure tasks are executed on the right instances.

Loading


@task()
def bulk_delete_projects(projects_id):
Project.objects.filter(id__in=projects_id).delete()
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

Any reason this needs to be executed via a task?

Loading

Copy link
Member Author

@safwanrahman safwanrahman Oct 31, 2017

Choose a reason for hiding this comment

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

I was hoping to delete the project directory with the project removal. So was thinking to do it with task. I think we can exclude it from the task

Loading

# Then exclude the projects where there are more than one owner
projects_id = (instance.projects.all().annotate(num_users=Count('users'))
.exclude(num_users__gt=1)
.values_list('id', flat=True))
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

I'll have to think more on this logic, but at first glance this will cause problems with our hosting at readthedocs.com, as users don't own projects, a more top-level object, Organizations do. Projects shouldn't be automatically removed like this in that case.

Loading

# Then exclude the organizations where there are more than one user
oauth_organizations_id = (instance.oauth_organizations.annotate(num_users=Count('users'))
.exclude(num_users__gt=1)
.values_list('id', flat=True))
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

OAuth organizations aren't currently shared, so User.oauth_organizations.delete() should be fine. We'll need tests here to ensure this is the case.

Loading

Copy link
Member Author

@safwanrahman safwanrahman Oct 31, 2017

Choose a reason for hiding this comment

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

I dont know whats the case actually. maybe it was m2m to give a organization user same permission? to docs

Loading

.values_list('id', flat=True))

bulk_delete_projects.delay(projects_id)
bulk_delete_oauth_remote_organizations.delay(oauth_organizations_id)
Copy link
Contributor

@agjohnson agjohnson Oct 31, 2017

Choose a reason for hiding this comment

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

If there is no strong reason that either of these need to be executed via a task, we can just remove them directly here.

Loading

@agjohnson agjohnson self-assigned this Oct 31, 2017
@safwanrahman safwanrahman force-pushed the user_delete branch 2 times, most recently from 75a662c to 4233f06 Oct 31, 2017
@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Oct 31, 2017

@agjohnson Updated. Can have a look?

Loading

@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Nov 9, 2017

@agjohnson Another phase of review?

Loading

Copy link
Member

@ericholscher ericholscher left a comment

This looks good to me.

Loading

@ericholscher ericholscher requested a review from humitos Nov 27, 2017
@humitos
Copy link
Member

@humitos humitos commented Nov 29, 2017

Travis is not passing because of a lint error. Can you check that?

Loading

@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Nov 30, 2017

@humitos Fixed the lint. merge?

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 6, 2017

Thanks @safwanrahman!

I've just realized we have a bug we need to handle before we can merge this. The bug is unrelated to your work though, so it shouldn't be a problem to merge after.

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 6, 2017

Actually, thinking about this more, I think there is enough protection in this PR to mitigate the problem. I'll merge this now. 🎉

Loading

@agjohnson agjohnson merged commit 762d342 into readthedocs:master Dec 6, 2017
1 check passed
Loading
@safwanrahman safwanrahman mentioned this pull request Dec 7, 2017
@safwanrahman safwanrahman mentioned this pull request Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants