Delete groups based on owners not creator

When deleting a user account we currently delete any groups that the
user created and that don't contain any annotations by other users.

This made sense when the creator was the only person who could edit the
group (changing its name, description and type) or moderate annotations
in the group: groups with no creator were kind of broken and may as well
be deleted (as long as they didn't contain any annotations).

But we've now added the concept of group membership roles and in
particular a group can have multiple "owners" who all have full
permissions to edit and moderate the group. Even if the group's creator
no longer exists, the group may still have other owners who have full
control of the group.

So instead of deleting groups that the user created, change
`UserDeleteService` to delete groups that the user is the only owner of
(and that don't contain any annotations by other users).

Author: seanh

h/models/group.py

@@ -4,8 +4,8 @@
 
 import slugify
 import sqlalchemy as sa
-from sqlalchemy.orm import relationship
 from sqlalchemy.dialects.postgresql import JSONB
+from sqlalchemy.orm import relationship
 
 from h import pubid
 from h.db import Base, mixins

h/services/user_delete.py

@@ -195,15 +195,15 @@ def delete_annotations(self, user):
 
     def delete_groups(self, user):
         """
-        Delete groups created by `user` that have no annotations.
+        Delete groups owned by `user` that have no annotations.
 
-        Delete groups that were created by `user` and that don't contain any
-        non-deleted annotations.
+        Delete groups that that `user` is the *only* owner of and that don't
+        contain any non-deleted annotations.
 
         If delete_annotations() (above) is called first then all of `user`'s
         own annotations will already have been deleted, so ultimately any
-        groups created by `user` that don't contain any annotations by *other*
-        users will get deleted.
+        groups that `user` is the only owner of and that don't contain any
+        annotations by *other* users will get deleted.
 
         Known issue: if this method does not delete a group because it contains
         annotations by other users, and those annotations other users are later
@@ -215,22 +215,40 @@ def delete_groups(self, user):
         really large number of members this could take too long and cause a
         timeout.
         """
-        # pylint:disable=not-callable,use-implicit-booleaness-not-comparison-to-zero
-        self.worker.delete(
-            Group,
+        # pylint:disable=not-callable
+        # pylint:disable=use-implicit-booleaness-not-comparison-to-zero
+
+        # The IDs of all groups that have only one owner.
+        groups_with_only_one_owner = (
+            select(GroupMembership.group_id)
+            .where(GroupMembership.roles.contains(["owner"]))
+            .group_by(GroupMembership.group_id)
+            .having(func.count(GroupMembership.group_id) == 1)
+        )
+
+        # The IDs of all groups where `user` is the only owner.
+        groups_where_user_is_only_owner = (
+            select(GroupMembership.group_id)
+            .where(GroupMembership.group_id.in_(groups_with_only_one_owner))
+            .where(GroupMembership.roles.contains(["owner"]))
+            .where(GroupMembership.user_id == user.id)
+        )
+
+        # The IDs of all groups where `user` is the only owner *and* the group
+        # doesn't contain any annotations by other users.
+        groups_to_be_deleted = (
             select(Group.id)
-            .where(Group.creator_id == user.id)
+            .where(Group.id.in_(groups_where_user_is_only_owner))
             .outerjoin(
                 Annotation,
-                and_(
-                    Annotation.groupid == Group.pubid,
-                    Annotation.deleted.is_(False),
-                ),
+                and_(Annotation.groupid == Group.pubid, Annotation.deleted.is_(False)),
             )
             .group_by(Group.id)
-            .having(func.count(Annotation.id) == 0),
+            .having(func.count(Annotation.id) == 0)
         )
 
+        self.worker.delete(Group, groups_to_be_deleted)
+
     def delete_group_memberships(self, user):
         """
         Delete `user`'s group memberships.

tests/unit/h/models/document/_uri_test.py

@@ -14,6 +14,7 @@ class TestDocumentURI:
     def test_it_normalizes_the_uri(self):
         document_uri = DocumentURI(uri="http://example.com/")
 
+        # pylint:disable=comparison-with-callable
         assert document_uri.uri_normalized == "httpx://example.com"
 
     def test_type_defaults_to_empty_string(self, db_session, document_uri, factories):

tests/unit/h/services/user_delete_test.py

@@ -4,7 +4,7 @@
 
 import pytest
 from h_matchers import Any
-from sqlalchemy import Select, func, select
+from sqlalchemy import Select, func, inspect, select
 
 from h.models import (
     Annotation,
@@ -13,6 +13,7 @@
     Flag,
     Group,
     GroupMembership,
+    GroupMembershipRoles,
     Job,
     Token,
     User,
@@ -302,51 +303,74 @@ def test_delete_annotations(
             ).only()
         )
 
-    def test_delete_groups(self, user, db_session, worker, factories, purger):
-        factories.Group(creator=user)
-
-        purger.delete_groups(user)
+    def test_delete_groups(self, user, worker, factories, purger):
+        def make_group(name, owner=user):
+            """Create and return a group that `owner` is the only owner of."""
+            return factories.Group(
+                name=name,
+                memberships=[
+                    GroupMembership(user=owner, roles=[GroupMembershipRoles.OWNER])
+                ],
+            )
 
-        worker.delete.assert_called_once_with(Group, Any.instance_of(Select))
-        assert (
-            db_session.scalars(select(Group).where(Group.creator == user)).all() == []
-        )
+        # A list of groups that should be deleted when `user` is deleted.
+        should_be_deleted = []
+
+        # A group that `user` is the only owner of, this group should be
+        # deleted when `user` is deleted.
+        should_be_deleted.append(make_group("only_owner"))
+
+        # A group that `user` is the only owner of but that has other non-owner
+        # members. This group should still be deleted.
+        group = make_group("other_members")
+        for role in GroupMembershipRoles:
+            if role == GroupMembershipRoles.OWNER:
+                continue
+            group.memberships.append(
+                GroupMembership(user=factories.User(), roles=[role])
+            )
+        should_be_deleted.append(group)
 
-    def test_delete_groups_still_deletes_groups_if_they_have_deleted_annotations(
-        self, user, db_session, factories, purger
-    ):
-        group = factories.Group(creator=user)
-        # An annotation in the group, but the annotation has already been
-        # marked as deleted (meaning it'll soon be purged from the DB) so this
-        # shouldn't prevent the group from being deleted.
+        # A group that `user` is the only owner of but that contains a
+        # deleted annotation by another user. This group should still be
+        # deleted.
+        group = make_group("deleted_annotation")
         factories.Annotation(group=group, deleted=True)
-
-        purger.delete_groups(user)
-
-        assert (
-            db_session.scalars(select(Group).where(Group.creator == user)).all() == []
+        should_be_deleted.append(group)
+
+        # A list of groups that should *not* be deleted when `user` is deleted.
+        should_not_be_deleted = []
+
+        # A group that has one owner, but `user` isn't a member of this group.
+        should_not_be_deleted.append(make_group("other_owner", owner=factories.User()))
+
+        # Some groups that have one owner but `user` is a non-owner member.
+        for role in GroupMembershipRoles:
+            if role == GroupMembershipRoles.OWNER:
+                continue
+            group = make_group(role, owner=factories.User())
+            group.memberships.append(GroupMembership(user=user, roles=[role]))
+            should_not_be_deleted.append(group)
+
+        # A group that `user` is an owner of but that also has another owner.
+        group = make_group("another_owner")
+        group.memberships.append(
+            GroupMembership(user=factories.User(), roles=[GroupMembershipRoles.OWNER])
         )
+        should_not_be_deleted.append(group)
 
-    def test_delete_groups_doesnt_delete_groups_created_by_other_users(
-        self, user, db_session, factories, purger
-    ):
-        group = factories.Group()
-
-        purger.delete_groups(user)
-
-        assert group in db_session.scalars(select(Group)).all()
-
-    def test_delete_groups_doesnt_delete_groups_with_annotations(
-        self, user, db_session, factories, purger
-    ):
-        group = factories.Group(creator=user)
-        # The group contains an annotation by another user.
-        # This should prevent the group from being deleted.
-        factories.Annotation(group=group)
+        # A group that `user` is the only owner of but that contains an annotation from another user.
+        group = make_group("annotation")
+        factories.Annotation(group=group, deleted=False)
+        should_not_be_deleted.append(group)
 
         purger.delete_groups(user)
 
-        assert group in db_session.scalars(select(Group)).all()
+        worker.delete.assert_called_once_with(Group, Any.instance_of(Select))
+        for group in should_not_be_deleted:
+            assert not inspect(group).deleted
+        for group in should_be_deleted:
+            assert inspect(group).deleted
 
     def test_delete_group_memberships(
         self, user, factories, purger, worker, db_session