[Fix #3182] Better user deletion #3214

safwanrahman · 2017-10-31T19:03:30Z

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?

agjohnson

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.

agjohnson · 2017-10-31T19:31:20Z

readthedocs/core/signals.py


 log = logging.getLogger(__name__)
+User = get_user_model()


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

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

agjohnson · 2017-10-31T19:33:50Z

readthedocs/core/signals.py

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


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

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

agjohnson · 2017-10-31T19:43:29Z

readthedocs/oauth/tasks.py

@@ -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()


This seems like something that should cascade on the database

agjohnson · 2017-10-31T19:44:32Z

readthedocs/templates/profiles/private/delete_account.html

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


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

agjohnson · 2017-10-31T19:44:41Z

readthedocs/templates/profiles/private/delete_account.html

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


Untranslated string here.

agjohnson · 2017-10-31T19:50:23Z

readthedocs/projects/tasks.py

@@ -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()


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.

agjohnson · 2017-10-31T19:50:48Z

readthedocs/projects/tasks.py

+
+@task()
+def bulk_delete_projects(projects_id):
+    Project.objects.filter(id__in=projects_id).delete()


Any reason this needs to be executed via a task?

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

agjohnson · 2017-10-31T19:53:32Z

readthedocs/core/signals.py

+    # 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))


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.

agjohnson · 2017-10-31T19:58:58Z

readthedocs/core/signals.py

+    # 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))


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.

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

agjohnson · 2017-10-31T20:00:56Z

readthedocs/core/signals.py

+                                                          .values_list('id', flat=True))
+
+    bulk_delete_projects.delay(projects_id)
+    bulk_delete_oauth_remote_organizations.delay(oauth_organizations_id)


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

safwanrahman · 2017-10-31T21:02:29Z

@agjohnson Updated. Can have a look?

safwanrahman · 2017-11-09T11:43:16Z

@agjohnson Another phase of review?

ericholscher

This looks good to me.

humitos · 2017-11-29T14:05:09Z

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

safwanrahman · 2017-11-30T19:22:13Z

@humitos Fixed the lint. merge?

agjohnson · 2017-12-06T00:06:20Z

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.

agjohnson · 2017-12-06T00:10:48Z

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

[Fix readthedocs#3182] Better user deletion

c48f611

safwanrahman force-pushed the user_delete branch from 309abd2 to c48f611 Compare October 31, 2017 19:04

safwanrahman requested review from gregmuellegger, ericholscher and agjohnson and removed request for gregmuellegger October 31, 2017 19:05

agjohnson requested changes Oct 31, 2017

View reviewed changes

agjohnson self-assigned this Oct 31, 2017

agjohnson added the PR: work in progress Pull request is not ready for full review label Oct 31, 2017

safwanrahman force-pushed the user_delete branch 2 times, most recently from 75a662c to 4233f06 Compare October 31, 2017 20:52

fixup according to comments

15a36c0

safwanrahman force-pushed the user_delete branch from 4233f06 to 15a36c0 Compare October 31, 2017 20:58

Delete user after user ask to get deleted

71c063e

ericholscher approved these changes Nov 27, 2017

View reviewed changes

ericholscher requested a review from humitos November 27, 2017 19:45

humitos approved these changes Nov 28, 2017

View reviewed changes

fixing lint

547d61c

agjohnson approved these changes Dec 6, 2017

View reviewed changes

agjohnson merged commit 762d342 into readthedocs:master Dec 6, 2017

shacharoo mentioned this pull request Dec 6, 2017

Unable to register after deleting my account #3189

Closed

humitos mentioned this pull request Dec 7, 2017

Github service: cannot unlink after deleting account #3374

Closed

This was referenced Dec 7, 2017

Delete Account #3117

Closed

Delete account #3114

Closed

safwanrahman mentioned this pull request Aug 28, 2018

[Fix #4576] Do not delete projects which have multiple users #4577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #3182] Better user deletion #3214

[Fix #3182] Better user deletion #3214

safwanrahman commented Oct 31, 2017 •

edited

Loading

agjohnson left a comment

agjohnson Oct 31, 2017

safwanrahman Oct 31, 2017

agjohnson Oct 31, 2017

safwanrahman Oct 31, 2017

agjohnson Oct 31, 2017

agjohnson Oct 31, 2017

agjohnson Oct 31, 2017

agjohnson Oct 31, 2017

agjohnson Oct 31, 2017

safwanrahman Oct 31, 2017

agjohnson Oct 31, 2017

agjohnson Oct 31, 2017

safwanrahman Oct 31, 2017 •

edited

Loading

agjohnson Oct 31, 2017

safwanrahman commented Oct 31, 2017

safwanrahman commented Nov 9, 2017

ericholscher left a comment

humitos commented Nov 29, 2017

safwanrahman commented Nov 30, 2017

agjohnson commented Dec 6, 2017

agjohnson commented Dec 6, 2017

[Fix #3182] Better user deletion #3214

[Fix #3182] Better user deletion #3214

Conversation

safwanrahman commented Oct 31, 2017 • edited Loading

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safwanrahman Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safwanrahman commented Oct 31, 2017

safwanrahman commented Nov 9, 2017

ericholscher left a comment

Choose a reason for hiding this comment

humitos commented Nov 29, 2017

safwanrahman commented Nov 30, 2017

agjohnson commented Dec 6, 2017

agjohnson commented Dec 6, 2017

safwanrahman commented Oct 31, 2017 •

edited

Loading

safwanrahman Oct 31, 2017 •

edited

Loading