From 0f35dc3aa7f360c65e38ca0828fa378e3e83e8d2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 3 May 2022 18:57:59 -0500 Subject: [PATCH 1/4] User: delete organizations where the user is the last owner This also shows a warning with a list of projects/organizations to be deleted. --- readthedocs/core/signals.py | 31 +++-- readthedocs/core/tests/test_signals.py | 114 +++++++++++------- readthedocs/organizations/querysets.py | 9 +- readthedocs/organizations/signals.py | 41 ++----- .../organizations/tests/test_querysets.py | 22 ++++ readthedocs/profiles/views.py | 24 ++-- readthedocs/projects/querysets.py | 15 ++- .../rtd_tests/tests/test_project_querysets.py | 18 +++ .../profiles/private/delete_account.html | 38 ++++++ 9 files changed, 209 insertions(+), 103 deletions(-) create mode 100644 readthedocs/organizations/tests/test_querysets.py diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index 872353091f0..1882324d278 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -1,13 +1,10 @@ """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 @@ -15,6 +12,7 @@ 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.only_owner(user): + project.delete() + + for organization in Organization.objects.only_owner(user): + organization.delete() @receiver(pre_create_historical_record) diff --git a/readthedocs/core/tests/test_signals.py b/readthedocs/core/tests/test_signals.py index 64d98ae353f..e79086344d3 100644 --- a/readthedocs/core/tests/test_signals.py +++ b/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 diff --git a/readthedocs/organizations/querysets.py b/readthedocs/organizations/querysets.py index 4e0e8a1c8b3..d508146f40d 100644 --- a/readthedocs/organizations/querysets.py +++ b/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 only_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): diff --git a/readthedocs/organizations/signals.py b/readthedocs/organizations/signals.py index 89677d948eb..bc67fdb0a9c 100644 --- a/readthedocs/organizations/signals.py +++ b/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) diff --git a/readthedocs/organizations/tests/test_querysets.py b/readthedocs/organizations/tests/test_querysets.py new file mode 100644 index 00000000000..63d713d78d0 --- /dev/null +++ b/readthedocs/organizations/tests/test_querysets.py @@ -0,0 +1,22 @@ +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.only_owner(user))) + self.assertEqual( + {org_three}, set(Organization.objects.only_owner(another_user)) + ) diff --git a/readthedocs/profiles/views.py b/readthedocs/profiles/views.py index 2c33eb45065..1c61f9d01ae 100644 --- a/readthedocs/profiles/views.py +++ b/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.only_owner(user) + context["organizations_to_be_deleted"] = Organization.objects.only_owner(user) + return context + def get_success_url(self): return reverse('homepage') diff --git a/readthedocs/projects/querysets.py b/readthedocs/projects/querysets.py index 986326eafa8..f33cf1cea3f 100644 --- a/readthedocs/projects/querysets.py +++ b/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 only_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 diff --git a/readthedocs/rtd_tests/tests/test_project_querysets.py b/readthedocs/rtd_tests/tests/test_project_querysets.py index 6a1f2896c87..fe14538fbb7 100644 --- a/readthedocs/rtd_tests/tests/test_project_querysets.py +++ b/readthedocs/rtd_tests/tests/test_project_querysets.py @@ -198,6 +198,24 @@ def test_for_user_and_viewer_same_user(self): self.assertEqual(query.count(), len(projects)) self.assertEqual(set(query), projects) + def test_only_owner(self): + 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]) + get(Project, slug="four", users=[user, another_user]) + get(Project, slug="five", users=[]) + + project_with_organization = get(Project, slug="six", users=[user]) + get(Organization, owners=[user], projects=[project_with_organization]) + + self.assertEqual( + {project_one, project_two}, set(Project.objects.only_owner(user)) + ) + self.assertEqual({project_three}, set(Project.objects.only_owner(another_user))) + class FeatureQuerySetTests(TestCase): diff --git a/readthedocs/templates/profiles/private/delete_account.html b/readthedocs/templates/profiles/private/delete_account.html index 567eff17cfb..c8c64030243 100644 --- a/readthedocs/templates/profiles/private/delete_account.html +++ b/readthedocs/templates/profiles/private/delete_account.html @@ -9,6 +9,44 @@ {% block edit_content_header %} {% trans "Delete Account" %} {% endblock %} {% block edit_content %} + {% if projects_to_be_deleted %} +
+

+ {% blocktrans trimmed %} + All projects where you are the only owner will be deleted. + If you want to keep a project, add another owner or transfer ownership. + The following projects and all their documentation will be deleted. + {% endblocktrans %} +

+ +
+ {% endif %} + + {% if organizations_to_be_deleted %} +
+

+ {% blocktrans trimmed %} + All organizations where you are the only owner will be deleted. + If you want to keep an organization, add another owner or transfer ownership. + The following organizations and all their projects will be deleted. + {% endblocktrans %} +

+ +
+ {% endif %} +
{% csrf_token %} {{ form }} From d382b22d1c90c8c26f74af0c9c724e9c0e44b24d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 3 May 2022 19:32:26 -0500 Subject: [PATCH 2/4] Fix test --- readthedocs/organizations/tests/test_orgs.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/readthedocs/organizations/tests/test_orgs.py b/readthedocs/organizations/tests/test_orgs.py index cc038904e6e..819fe72e2b9 100644 --- a/readthedocs/organizations/tests/test_orgs.py +++ b/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()) From be01621d3f809965ce99e50802b19fc6d600acd6 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 11 May 2022 10:12:52 -0500 Subject: [PATCH 3/4] List projects from organizations --- .../templates/profiles/private/delete_account.html | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/readthedocs/templates/profiles/private/delete_account.html b/readthedocs/templates/profiles/private/delete_account.html index c8c64030243..c453aed4c3a 100644 --- a/readthedocs/templates/profiles/private/delete_account.html +++ b/readthedocs/templates/profiles/private/delete_account.html @@ -32,7 +32,7 @@

{% blocktrans trimmed %} - All organizations where you are the only owner will be deleted. + All organizations and its projects where you are the only owner will be deleted. If you want to keep an organization, add another owner or transfer ownership. The following organizations and all their projects will be deleted. {% endblocktrans %} @@ -41,6 +41,13 @@ {% for organization in organizations_to_be_deleted %}

  • {{ organization.slug }} +
      + {% for project in organization.projects.all %} +
    • + {{ project.slug }} +
      • + {% endfor %} +
    • {% endfor %} From b3b7161418e277ca8c38f50264333b6862d57a03 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 11 May 2022 18:47:33 -0500 Subject: [PATCH 4/4] Updates from review --- readthedocs/core/signals.py | 4 ++-- readthedocs/organizations/querysets.py | 2 +- .../organizations/tests/test_querysets.py | 6 ++++-- readthedocs/profiles/views.py | 4 ++-- readthedocs/projects/querysets.py | 2 +- .../rtd_tests/tests/test_project_querysets.py | 6 ++++-- .../profiles/private/delete_account.html | 21 ++++++++++++------- 7 files changed, 27 insertions(+), 18 deletions(-) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index 1882324d278..caa3b24b1dc 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -121,10 +121,10 @@ def delete_projects_and_organizations(sender, instance, *args, **kwargs): """ user = instance - for project in Project.objects.only_owner(user): + for project in Project.objects.single_owner(user): project.delete() - for organization in Organization.objects.only_owner(user): + for organization in Organization.objects.single_owner(user): organization.delete() diff --git a/readthedocs/organizations/querysets.py b/readthedocs/organizations/querysets.py index d508146f40d..c8a61bfeb28 100644 --- a/readthedocs/organizations/querysets.py +++ b/readthedocs/organizations/querysets.py @@ -214,7 +214,7 @@ def clean_artifacts(self): ) return orgs.distinct() - def only_owner(self, user): + def single_owner(self, user): """Returns organizations where `user` is the only owner.""" return self.annotate(count_owners=Count("owners")).filter( owners=user, diff --git a/readthedocs/organizations/tests/test_querysets.py b/readthedocs/organizations/tests/test_querysets.py index 63d713d78d0..cfeb52c4e80 100644 --- a/readthedocs/organizations/tests/test_querysets.py +++ b/readthedocs/organizations/tests/test_querysets.py @@ -16,7 +16,9 @@ def test_only_owner(self): get(Organization, slug="four", owners=[user, another_user]) get(Organization, slug="five", owners=[]) - self.assertEqual({org_one, org_two}, set(Organization.objects.only_owner(user))) self.assertEqual( - {org_three}, set(Organization.objects.only_owner(another_user)) + {org_one, org_two}, set(Organization.objects.single_owner(user)) + ) + self.assertEqual( + {org_three}, set(Organization.objects.single_owner(another_user)) ) diff --git a/readthedocs/profiles/views.py b/readthedocs/profiles/views.py index 1c61f9d01ae..838dd3bb127 100644 --- a/readthedocs/profiles/views.py +++ b/readthedocs/profiles/views.py @@ -89,8 +89,8 @@ def get_form(self, data=None, files=None, **kwargs): def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) user = self.request.user - context["projects_to_be_deleted"] = Project.objects.only_owner(user) - context["organizations_to_be_deleted"] = Organization.objects.only_owner(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): diff --git a/readthedocs/projects/querysets.py b/readthedocs/projects/querysets.py index f33cf1cea3f..c7674b0ba61 100644 --- a/readthedocs/projects/querysets.py +++ b/readthedocs/projects/querysets.py @@ -147,7 +147,7 @@ def dashboard(self, user): def api(self, user=None): return self.public(user) - def only_owner(self, user): + def single_owner(self, user): """ Returns projects where `user` is the only owner. diff --git a/readthedocs/rtd_tests/tests/test_project_querysets.py b/readthedocs/rtd_tests/tests/test_project_querysets.py index fe14538fbb7..0a9424b22ca 100644 --- a/readthedocs/rtd_tests/tests/test_project_querysets.py +++ b/readthedocs/rtd_tests/tests/test_project_querysets.py @@ -212,9 +212,11 @@ def test_only_owner(self): get(Organization, owners=[user], projects=[project_with_organization]) self.assertEqual( - {project_one, project_two}, set(Project.objects.only_owner(user)) + {project_one, project_two}, set(Project.objects.single_owner(user)) + ) + self.assertEqual( + {project_three}, set(Project.objects.single_owner(another_user)) ) - self.assertEqual({project_three}, set(Project.objects.only_owner(another_user))) class FeatureQuerySetTests(TestCase): diff --git a/readthedocs/templates/profiles/private/delete_account.html b/readthedocs/templates/profiles/private/delete_account.html index c453aed4c3a..ce2170e533a 100644 --- a/readthedocs/templates/profiles/private/delete_account.html +++ b/readthedocs/templates/profiles/private/delete_account.html @@ -40,14 +40,19 @@