Merge branch 'pr/4577'

Author: agjohnson

readthedocs/core/signals.py

@@ -1,3 +1,5 @@
+# -*- coding: utf-8 -*-
+
 """Signal handling for core app."""
 
 from __future__ import absolute_import
@@ -12,6 +14,7 @@
 from django.dispatch import receiver
 from future.backports.urllib.parse import urlparse
 
+from readthedocs.oauth.models import RemoteOrganization
 from readthedocs.projects.models import Project, Domain
 
 log = logging.getLogger(__name__)
@@ -23,7 +26,6 @@
     '/api/v2/sustainability',
 ]
 
-
 webhook_github = Signal(providing_args=['project', 'data', 'event'])
 webhook_gitlab = Signal(providing_args=['project', 'data', 'event'])
 webhook_bitbucket = Signal(providing_args=['project', 'data', 'event'])
@@ -79,12 +81,20 @@ def decide_if_cors(sender, request, **kwargs):  # pylint: disable=unused-argumen
 def delete_projects_and_organizations(sender, instance, *args, **kwargs):
     # Here we count the owner list from the projects that the user own
     # Then exclude the projects where there are more than one owner
-    projects = instance.projects.all().annotate(num_users=Count('users')).exclude(num_users__gt=1)
+    # Add annotate before filter
+    # https://github.com/rtfd/readthedocs.org/pull/4577
+    # https://docs.djangoproject.com/en/2.1/topics/db/aggregation/#order-of-annotate-and-filter-clauses # noqa
+    projects = (
+        Project.objects.annotate(num_users=Count('users'))
+        .filter(users=instance.id).exclude(num_users__gt=1)
+    )
 
     # Here we count the users list from the organization that the user belong
     # Then exclude the organizations where there are more than one user
-    oauth_organizations = (instance.oauth_organizations.annotate(num_users=Count('users'))
-                                                       .exclude(num_users__gt=1))
+    oauth_organizations = (
+        RemoteOrganization.objects.annotate(num_users=Count('users'))
+        .filter(users=instance.id).exclude(num_users__gt=1)
+    )
 
     projects.delete()
     oauth_organizations.delete()

readthedocs/core/tests/__init__.py

@@ -0,0 +1,56 @@
+import pytest
+import django_dynamic_fixture
+
+from django.contrib.auth.models import User
+
+from readthedocs.oauth.models import RemoteOrganization
+from readthedocs.projects.models import Project
+
+
+@pytest.mark.django_db
+class TestProjectOrganizationSignal(object):
+
+    @pytest.mark.parametrize('model_class', [Project, RemoteOrganization])
+    def test_project_organization_get_deleted_upon_user_delete(self, model_class):
+        """
+        If the user has Project or RemoteOrganization where he is the only user,
+        upon deleting his account, the Project or RemoteOrganization should also get
+        deleted.
+        """
+
+        obj = django_dynamic_fixture.get(model_class)
+        user1 = django_dynamic_fixture.get(User)
+        obj.users.add(user1)
+
+        obj.refresh_from_db()
+        assert obj.users.all().count() == 1
+
+        # Delete the user
+        user1.delete()
+        # The object should not exist
+        obj = model_class.objects.all().filter(id=obj.id)
+        assert not obj.exists()
+
+    @pytest.mark.parametrize('model_class', [Project, RemoteOrganization])
+    def test_multiple_users_project_organization_not_delete(self, model_class):
+        """
+        Check Project or RemoteOrganization which have multiple users do not get deleted
+        when any of the user delete his account.
+        """
+
+        obj = django_dynamic_fixture.get(model_class)
+        user1 = django_dynamic_fixture.get(User)
+        user2 = django_dynamic_fixture.get(User)
+        obj.users.add(user1, user2)
+
+        obj.refresh_from_db()
+        assert obj.users.all().count() > 1
+        # Delete 1 user of the project
+        user1.delete()
+
+        # The project should still exist and it should have 1 user
+        obj.refresh_from_db()
+        assert obj.id
+        obj_users = obj.users.all()
+        assert len(obj_users) == 1
+        assert user2 in obj_users