User: delete organizations where the user is the last owner (#9164)

This also shows a warning with a list of projects/organizations to be
deleted.

Author: stsewd

readthedocs/core/signals.py

@@ -1,20 +1,18 @@
 """Signal handling for core app."""
 
 import structlog
-
 from corsheaders import signals
 from django.conf import settings
-from django.db.models import Count
 from django.db.models.signals import pre_delete
 from django.dispatch import Signal, receiver
-
 from rest_framework.permissions import SAFE_METHODS
 from simple_history.models import HistoricalRecords
 from simple_history.signals import pre_create_historical_record
 
 from readthedocs.analytics.utils import get_client_ip
 from readthedocs.builds.models import Version
 from readthedocs.core.unresolver import unresolve
+from readthedocs.organizations.models import Organization
 from readthedocs.projects.models import Project
 
 log = structlog.get_logger(__name__)
@@ -114,19 +112,20 @@ def decide_if_cors(sender, request, **kwargs):  # pylint: disable=unused-argumen
 
 
 @receiver(pre_delete, sender=settings.AUTH_USER_MODEL)
-def delete_projects(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
-    # 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)
-    )
-
-    projects.delete()
+def delete_projects_and_organizations(sender, instance, *args, **kwargs):
+    """
+    Delete projects and organizations where the user is the only owner.
+
+    We delete projects that don't belong to an organization first,
+    then full organizations.
+    """
+    user = instance
+
+    for project in Project.objects.single_owner(user):
+        project.delete()
+
+    for organization in Organization.objects.single_owner(user):
+        organization.delete()
 
 
 @receiver(pre_create_historical_record)

readthedocs/core/tests/test_signals.py

@@ -1,50 +1,82 @@
-# -*- coding: utf-8 -*-
-import django_dynamic_fixture
 import pytest
-
 from django.contrib.auth.models import User
+from django_dynamic_fixture import get
 
+from readthedocs.builds.models import Version
+from readthedocs.organizations.models import Organization, Team, TeamInvite, TeamMember
 from readthedocs.projects.models import Project
 
 
 @pytest.mark.django_db
 class TestProjectOrganizationSignal:
 
-    def test_project_get_deleted_upon_user_delete(self):
-        """If the user has Project where he is the only
-        user, upon deleting his account, the Project
-        should also get deleted."""
-
-        project = django_dynamic_fixture.get(Project)
-        user1 = django_dynamic_fixture.get(User)
-        project.users.add(user1)
-
-        project.refresh_from_db()
-        assert project.users.all().count() == 1
-
-        # Delete the user
-        user1.delete()
-        # The object should not exist
-        project = Project.objects.all().filter(id=project.id)
-        assert not project.exists()
-
-    def test_multiple_users_project_not_delete(self):
-        """Check Project which have multiple users do not
-        get deleted when any of the user delete his account."""
-
-        project = django_dynamic_fixture.get(Project)
-        user1 = django_dynamic_fixture.get(User)
-        user2 = django_dynamic_fixture.get(User)
-        project.users.add(user1, user2)
-
-        project.refresh_from_db()
-        assert project.users.all().count() > 1
-        # Delete 1 user of the project
-        user1.delete()
-
-        # The project should still exist and it should have 1 user
-        project.refresh_from_db()
-        assert project.id
-        obj_users = project.users.all()
-        assert len(obj_users) == 1
-        assert user2 in obj_users
+    def test_delete_user_deletes_projects(self):
+        """When deleting a user, delete projects where it's the only owner."""
+        user = get(User)
+        another_user = get(User)
+        project_one = get(Project, slug="one", users=[user])
+        project_two = get(Project, slug="two", users=[user])
+        project_three = get(Project, slug="three", users=[another_user])
+        project_four = get(Project, slug="four", users=[user, another_user])
+
+        project_five = get(
+            Project,
+            slug="five",
+            users=[],
+        )
+        assert Project.objects.all().count() == 5
+        assert Version.objects.all().count() == 5
+
+        user.delete()
+        assert {project_three, project_four, project_five} == set(Project.objects.all())
+        assert Version.objects.all().count() == 3
+
+    def test_delete_user_deletes_organizations(self):
+        """When deleting a user, delete organizations where it's the only owner."""
+        user = get(User)
+        member = get(User)
+        another_user = get(User)
+        project_one = get(Project, slug="one")
+        project_two = get(Project, slug="two")
+        project_three = get(Project, slug="three")
+        org_one = get(Organization, slug="one", owners=[user], projects=[project_one])
+        org_two = get(
+            Organization,
+            slug="two",
+            owners=[user, another_user],
+            projects=[project_two],
+        )
+        org_three = get(
+            Organization, slug="three", owners=[another_user], projects=[project_three]
+        )
+
+        team_one = get(
+            Team, organization=org_one, members=[member, user], projects=[project_one]
+        )
+        team_two = get(
+            Team,
+            organization=org_three,
+            members=[another_user],
+            projects=[project_three],
+        )
+
+        get(TeamInvite, team=team_one, organization=org_one)
+
+
+        assert Organization.objects.all().count() == 3
+        assert Project.objects.all().count() == 3
+        assert Version.objects.all().count() == 3
+        assert Team.objects.all().count() == 2
+        assert TeamInvite.objects.all().count() == 1
+        assert TeamMember.objects.all().count() == 3
+        assert User.objects.all().count() == 3
+
+        user.delete()
+
+        assert {org_two, org_three} == set(Organization.objects.all())
+        assert {project_two, project_three} == set(Project.objects.all())
+        assert Version.objects.all().count() == 2
+        assert {team_two} == set(Team.objects.all())
+        assert TeamInvite.objects.all().count() == 0
+        assert TeamMember.objects.all().count() == 1
+        assert User.objects.all().count() == 2

readthedocs/organizations/querysets.py

@@ -4,7 +4,7 @@
 
 from django.conf import settings
 from django.db import models
-from django.db.models import Q
+from django.db.models import Count, Q
 from django.utils import timezone
 
 from readthedocs.core.utils.extend import SettingsOverrideObject
@@ -214,6 +214,13 @@ def clean_artifacts(self):
         )
         return orgs.distinct()
 
+    def single_owner(self, user):
+        """Returns organizations where `user` is the only owner."""
+        return self.annotate(count_owners=Count("owners")).filter(
+            owners=user,
+            count_owners=1,
+        )
+
 
 class OrganizationQuerySet(SettingsOverrideObject):
 

readthedocs/organizations/signals.py

@@ -1,24 +1,20 @@
 """Organization signals."""
 
 import structlog
-
 from allauth.account.signals import user_signed_up
 from django.db.models import Count
 from django.db.models.signals import pre_delete
 from django.dispatch import receiver
 
 from readthedocs.builds.constants import BUILD_STATE_FINISHED
-from readthedocs.builds.models import Build, Version
+from readthedocs.builds.models import Build
 from readthedocs.builds.signals import build_complete
-from readthedocs.organizations.models import (
-    Organization,
-    Team,
-    TeamInvite,
-    TeamMember,
-)
+from readthedocs.organizations.models import Organization, Team, TeamMember
 from readthedocs.projects.models import Project
 
-from .tasks import mark_organization_assets_not_cleaned as mark_organization_assets_not_cleaned_task
+from .tasks import (
+    mark_organization_assets_not_cleaned as mark_organization_assets_not_cleaned_task,
+)
 
 log = structlog.get_logger(__name__)
 
@@ -56,28 +52,13 @@ def remove_organization_completely(sender, instance, using, **kwargs):
     # to be sure that the projects we are deleting here belongs only to the
     # organization deleted
     projects = Project.objects.annotate(
-        Count('organizations'),
-    ).filter(
-        organizations__in=[organization],
-        organizations__count=1,
-    )
-
-    versions = Version.objects.filter(project__in=projects)
-    teams = Team.objects.filter(organization=organization)
-    team_invites = TeamInvite.objects.filter(organization=organization)
-    team_memberships = TeamMember.objects.filter(team__in=teams)
-
-    # Bulk delete
-    team_memberships.delete()
-    team_invites.delete()
-    teams.delete()
-
-    # Granular delete that trigger other complex tasks
-    for version in versions:
-        # Triggers a task to remove artifacts from storage
-        version.delete()
+        count_organizations=Count("organizations")
+    ).filter(organizations__in=[organization], count_organizations=1)
 
-    projects.delete()
+    # Granular delete that trigger other complex tasks.
+    for project in projects:
+        # Triggers a task to remove artifacts from storage.
+        project.delete()
 
 
 @receiver(build_complete, sender=Build)

readthedocs/organizations/tests/test_orgs.py

@@ -195,10 +195,7 @@ def test_organization_delete(self):
         with mock.patch('readthedocs.projects.tasks.utils.clean_project_resources') as clean_project_resources:
             # Triggers a pre_delete signal that removes all leaf overs
             self.organization.delete()
-            clean_project_resources.assert_has_calls([
-                mock.call(self.project, mock.ANY),  # latest
-                mock.call(self.project, mock.ANY),  # version
-            ])
+            clean_project_resources.assert_called_once()
 
         self.assertNotIn(self.organization, Organization.objects.all())
         self.assertNotIn(team, Team.objects.all())

readthedocs/organizations/tests/test_querysets.py

@@ -0,0 +1,24 @@
+from django.contrib.auth.models import User
+from django.test import TestCase
+from django_dynamic_fixture import get
+
+from readthedocs.organizations.models import Organization
+
+
+class TestOrganizationQuerysets(TestCase):
+    def test_only_owner(self):
+        user = get(User)
+        another_user = get(User)
+
+        org_one = get(Organization, slug="one", owners=[user])
+        org_two = get(Organization, slug="two", owners=[user])
+        org_three = get(Organization, slug="three", owners=[another_user])
+        get(Organization, slug="four", owners=[user, another_user])
+        get(Organization, slug="five", owners=[])
+
+        self.assertEqual(
+            {org_one, org_two}, set(Organization.objects.single_owner(user))
+        )
+        self.assertEqual(
+            {org_three}, set(Organization.objects.single_owner(another_user))
+        )

readthedocs/profiles/views.py

@@ -12,26 +12,17 @@
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
 from rest_framework.authtoken.models import Token
-from vanilla import (
-    CreateView,
-    DeleteView,
-    DetailView,
-    FormView,
-    ListView,
-    UpdateView,
-)
+from vanilla import CreateView, DeleteView, DetailView, FormView, ListView, UpdateView
 
 from readthedocs.audit.filters import UserSecurityLogFilter
 from readthedocs.audit.models import AuditLog
-from readthedocs.core.forms import (
-    UserAdvertisingForm,
-    UserDeleteForm,
-    UserProfileForm,
-)
+from readthedocs.core.forms import UserAdvertisingForm, UserDeleteForm, UserProfileForm
 from readthedocs.core.history import set_change_reason
 from readthedocs.core.mixins import PrivateViewMixin
 from readthedocs.core.models import UserProfile
 from readthedocs.core.utils.extend import SettingsOverrideObject
+from readthedocs.organizations.models import Organization
+from readthedocs.projects.models import Project
 from readthedocs.projects.utils import get_csv_file
 
 
@@ -95,6 +86,13 @@ def get_form(self, data=None, files=None, **kwargs):
         kwargs['initial'] = {'username': ''}
         return super().get_form(data, files, **kwargs)
 
+    def get_context_data(self, **kwargs):
+        context = super().get_context_data(**kwargs)
+        user = self.request.user
+        context["projects_to_be_deleted"] = Project.objects.single_owner(user)
+        context["organizations_to_be_deleted"] = Organization.objects.single_owner(user)
+        return context
+
     def get_success_url(self):
         return reverse('homepage')
 

readthedocs/projects/querysets.py

@@ -1,8 +1,7 @@
 """Project model QuerySet classes."""
-
 from django.conf import settings
 from django.db import models
-from django.db.models import OuterRef, Prefetch, Q, Subquery
+from django.db.models import Count, OuterRef, Prefetch, Q, Subquery
 
 from readthedocs.core.permissions import AdminPermission
 from readthedocs.core.utils.extend import SettingsOverrideObject
@@ -148,6 +147,18 @@ def dashboard(self, user):
     def api(self, user=None):
         return self.public(user)
 
+    def single_owner(self, user):
+        """
+        Returns projects where `user` is the only owner.
+
+        Projects that belong to organizations aren't included.
+        """
+        return self.annotate(count_users=Count("users")).filter(
+            users=user,
+            count_users=1,
+            organizations__isnull=True,
+        )
+
 
 class ProjectQuerySet(SettingsOverrideObject):
     _default_class = ProjectQuerySetBase