diff --git a/h/cli/commands/user.py b/h/cli/commands/user.py index cecbaad67de..f787f104736 100644 --- a/h/cli/commands/user.py +++ b/h/cli/commands/user.py @@ -101,30 +101,3 @@ def password(ctx, username, authority, password): request.tm.commit() click.echo(f"Password changed for {username}", err=True) - - -@user.command() -@click.argument("username") -@click.option("--authority") -@click.pass_context -def delete(ctx, username, authority): - """ - Delete a user with all their group memberships and annotations. - - You must specify the username of a user to delete. - """ - request = ctx.obj["bootstrap"]() - - if not authority: - authority = request.default_authority - - user = models.User.get_by_username(request.db, username, authority) - if user is None: - raise click.ClickException( - f'no user with username "{username}" and authority "{authority}"' - ) - - request.find_service(name="user_delete").delete_user(user) - request.tm.commit() - - click.echo(f"User {username} deleted.", err=True) diff --git a/h/models/__init__.py b/h/models/__init__.py index c8d1b7c50e3..fb748cbd782 100644 --- a/h/models/__init__.py +++ b/h/models/__init__.py @@ -38,6 +38,7 @@ from h.models.subscriptions import Subscriptions from h.models.token import Token from h.models.user import User +from h.models.user_deletion import UserDeletion from h.models.user_identity import UserIdentity __all__ = ( @@ -64,5 +65,6 @@ "Subscriptions", "Token", "User", + "UserDeletion", "UserIdentity", ) diff --git a/h/models/helpers.py b/h/models/helpers.py new file mode 100644 index 00000000000..aac1227c210 --- /dev/null +++ b/h/models/helpers.py @@ -0,0 +1,4 @@ +def repr_(obj, attrs): + class_name = type(obj).__name__ + attrs = {attrname: getattr(obj, attrname) for attrname in attrs} + return f"{class_name}({', '.join(f'{name}={value!r}' for name, value in attrs.items())})" diff --git a/h/models/job.py b/h/models/job.py index f87b371f3a1..48d6baed041 100644 --- a/h/models/job.py +++ b/h/models/job.py @@ -41,6 +41,7 @@ from sqlalchemy.dialects.postgresql import JSONB from h.db import Base +from h.models import helpers class Job(Base): @@ -67,10 +68,9 @@ class Job(Base): ) def __repr__(self): - class_name = type(self).__name__ - attrs = { - attrname: repr(getattr(self, attrname)) - for attrname in [ + return helpers.repr_( + self, + [ "id", "name", "enqueued_at", @@ -79,6 +79,5 @@ def __repr__(self): "priority", "tag", "kwargs", - ] - } - return f"{class_name}({', '.join(f'{name}={value}' for name, value in attrs.items())})" + ], + ) diff --git a/h/models/user_deletion.py b/h/models/user_deletion.py new file mode 100644 index 00000000000..47a1bf7f193 --- /dev/null +++ b/h/models/user_deletion.py @@ -0,0 +1,38 @@ +from datetime import datetime + +from sqlalchemy import func +from sqlalchemy.orm import Mapped, mapped_column + +from h.db import Base +from h.models import helpers + + +class UserDeletion(Base): + """A record of a user account that was deleted.""" + + __tablename__ = "user_deletion" + + id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) + userid: Mapped[str] = mapped_column(nullable=False) + requested_at: Mapped[datetime] = mapped_column( + default=func.now(), # pylint:disable=not-callable + nullable=False, + ) + requested_by: Mapped[str] = mapped_column(nullable=False) + tag: Mapped[str] = mapped_column(nullable=False) + created_at: Mapped[datetime] = mapped_column(nullable=False) + num_annotations: Mapped[int] = mapped_column(nullable=False) + + def __repr__(self) -> str: + return helpers.repr_( + self, + [ + "id", + "userid", + "requested_at", + "requested_by", + "tag", + "created_at", + "num_annotations", + ], + ) diff --git a/h/services/user_delete.py b/h/services/user_delete.py index 3ffde572fe5..83f77a1071b 100644 --- a/h/services/user_delete.py +++ b/h/services/user_delete.py @@ -1,6 +1,6 @@ import sqlalchemy as sa -from h.models import Annotation, Group, Token, User +from h.models import Annotation, Group, Token, User, UserDeletion from h.services.annotation_delete import AnnotationDeleteService @@ -13,7 +13,7 @@ def __init__( self._db = db_session self._annotation_delete_service = annotation_delete_service - def delete_user(self, user: User): + def delete_user(self, user: User, requested_by: User, tag: str): """ Delete a user with all their group memberships and annotations. @@ -46,6 +46,20 @@ def delete_user(self, user: User): else: self._db.delete(group) + self._db.add( + UserDeletion( + userid=user.userid, + requested_by=requested_by.userid, + tag=tag, + created_at=user.registered_date, + num_annotations=self._db.scalar( + sa.select( + sa.func.count(Annotation.id) # pylint:disable=not-callable + ).where(Annotation.userid == user.userid) + ), + ) + ) + self._db.delete(user) diff --git a/h/views/admin/users.py b/h/views/admin/users.py index 6c98388a4ce..db57c740c08 100644 --- a/h/views/admin/users.py +++ b/h/views/admin/users.py @@ -134,7 +134,7 @@ def users_delete(request): user = _form_request_user(request) svc = request.find_service(name="user_delete") - svc.delete_user(user) + svc.delete_user(user, requested_by=request.user, tag=request.matched_route.name) request.session.flash( f"Successfully deleted user {user.username} with authority {user.authority}" "success", diff --git a/tests/common/factories/__init__.py b/tests/common/factories/__init__.py index e57baa23ff1..ef5e0f3710f 100644 --- a/tests/common/factories/__init__.py +++ b/tests/common/factories/__init__.py @@ -20,4 +20,5 @@ from tests.common.factories.subscriptions import Subscriptions from tests.common.factories.token import DeveloperToken, OAuth2Token from tests.common.factories.user import User +from tests.common.factories.user_deletion import UserDeletion from tests.common.factories.user_identity import UserIdentity diff --git a/tests/common/factories/user_deletion.py b/tests/common/factories/user_deletion.py new file mode 100644 index 00000000000..81faeb25687 --- /dev/null +++ b/tests/common/factories/user_deletion.py @@ -0,0 +1,20 @@ +from factory import Faker, LazyAttribute, SubFactory + +from h import models + +from .base import ModelFactory +from .user import User + + +class UserDeletion(ModelFactory): + class Meta: + model = models.UserDeletion + exclude = ("user", "requesting_user") + + user = SubFactory(User) + userid = LazyAttribute(lambda o: o.user.userid) + requesting_user = SubFactory(User) + requested_by = LazyAttribute(lambda o: o.requesting_user.userid) + tag = "factory" + created_at = LazyAttribute(lambda o: o.user.registered_date) + num_annotations = Faker("random_int") diff --git a/tests/unit/h/cli/commands/user_test.py b/tests/unit/h/cli/commands/user_test.py index d72e4d66ffe..62edc0af5ef 100644 --- a/tests/unit/h/cli/commands/user_test.py +++ b/tests/unit/h/cli/commands/user_test.py @@ -195,46 +195,6 @@ def user(self, factories): return factories.User() -class TestDeleteUserCommand: - def test_it_deletes_user(self, invoke_cli, user, user_delete_service): - result = invoke_cli(user_cli.delete, [user.username]) - - assert not result.exit_code - user_delete_service.delete_user.assert_called_once_with(user) - - def test_it_deletes_user_with_specific_authority( - self, invoke_cli, user, user_delete_service - ): - user.authority = "partner.org" - - result = invoke_cli( - user_cli.delete, ["--authority", "partner.org", user.username] - ) - - assert not result.exit_code - user_delete_service.delete_user.assert_called_once_with(user) - - def test_it_errors_when_user_could_not_be_found( - self, invoke_cli, user_delete_service - ): - result = invoke_cli(user_cli.delete, ["bogus_username"]) - - assert result.exit_code == 1 - user_delete_service.delete_user.assert_not_called() - - def test_it_errors_when_user_with_specific_authority_could_not_be_found( - self, invoke_cli, user, user_delete_service - ): - result = invoke_cli(user_cli.delete, ["--authority", "foo.com", user.username]) - - assert result.exit_code == 1 - user_delete_service.delete_user.assert_not_called() - - @pytest.fixture - def user(self, factories): - return factories.User() - - @pytest.fixture def invoke_cli(cli, pyramid_request): pyramid_request.tm = mock.Mock() diff --git a/tests/unit/h/models/helpers_test.py b/tests/unit/h/models/helpers_test.py new file mode 100644 index 00000000000..2a42fc473ae --- /dev/null +++ b/tests/unit/h/models/helpers_test.py @@ -0,0 +1,9 @@ +from unittest.mock import Mock + +from h.models import helpers + + +def test_repr_(): + obj = Mock(foo="FOO", bar="BAR") + + assert helpers.repr_(obj, ["foo", "bar"]) == "Mock(foo='FOO', bar='BAR')" diff --git a/tests/unit/h/models/job_test.py b/tests/unit/h/models/job_test.py index 2b9b49439d7..a4948dfd2e1 100644 --- a/tests/unit/h/models/job_test.py +++ b/tests/unit/h/models/job_test.py @@ -1,43 +1,29 @@ -from datetime import datetime +import pytest -from h.models.job import Job +def test___repr__(factories, helpers): + job = factories.Job() + repr_ = repr(job) -class TestJob: - def test___repr__(self): - job = Job( - id=42, - name="job_name", - enqueued_at=datetime( - year=2024, - month=5, - day=8, - hour=11, - minute=51, - second=23, - ), - scheduled_at=datetime( - year=2024, - month=6, - day=1, - hour=0, - minute=0, - second=0, - ), - expires_at=datetime( - year=2025, - month=1, - day=1, - hour=0, - minute=0, - second=0, - ), - priority=3, - tag="job_tag", - kwargs={"foo": "FOO", "bar": "BAR"}, - ) + helpers.repr_.assert_called_once_with( + job, + [ + "id", + "name", + "enqueued_at", + "scheduled_at", + "expires_at", + "priority", + "tag", + "kwargs", + ], + ) + assert repr_ == helpers.repr_.return_value - assert ( - repr(job) - == "Job(id=42, name='job_name', enqueued_at=datetime.datetime(2024, 5, 8, 11, 51, 23), scheduled_at=datetime.datetime(2024, 6, 1, 0, 0), expires_at=datetime.datetime(2025, 1, 1, 0, 0), priority=3, tag='job_tag', kwargs={'foo': 'FOO', 'bar': 'BAR'})" - ) + +@pytest.fixture(autouse=True) +def helpers(mocker): + helpers = mocker.patch("h.models.job.helpers") + # __repr__() needs to return a string or repr() raises. + helpers.repr_.return_value = "test_string_representation" + return helpers diff --git a/tests/unit/h/models/user_deletion_test.py b/tests/unit/h/models/user_deletion_test.py new file mode 100644 index 00000000000..4b013daaca0 --- /dev/null +++ b/tests/unit/h/models/user_deletion_test.py @@ -0,0 +1,28 @@ +import pytest + + +def test___repr__(factories, helpers): + user_deletion = factories.UserDeletion() + repr_ = repr(user_deletion) + + helpers.repr_.assert_called_once_with( + user_deletion, + [ + "id", + "userid", + "requested_at", + "requested_by", + "tag", + "created_at", + "num_annotations", + ], + ) + assert repr_ == helpers.repr_.return_value + + +@pytest.fixture(autouse=True) +def helpers(mocker): + helpers = mocker.patch("h.models.user_deletion.helpers") + # __repr__() needs to return a string or repr() raises. + helpers.repr_.return_value = "test_string_representation" + return helpers diff --git a/tests/unit/h/services/user_delete_test.py b/tests/unit/h/services/user_delete_test.py index 09f5fce3cb0..c167592ccc9 100644 --- a/tests/unit/h/services/user_delete_test.py +++ b/tests/unit/h/services/user_delete_test.py @@ -4,7 +4,7 @@ import sqlalchemy from h_matchers import Any -from h.models import GroupMembership, Token +from h.models import GroupMembership, Token, UserDeletion from h.services.user_delete import UserDeleteService, service_factory @@ -16,13 +16,14 @@ def test_it( db_session, annotation_delete_service, user, + requested_by, created_group, joined_group, user_annotations, other_developer_token, other_oauth2_token, ): - svc.delete_user(user) + svc.delete_user(user, requested_by, "test_tag") # Check the user was deleted assert user in db_session.deleted @@ -48,13 +49,29 @@ def test_it( db_session.scalars(sqlalchemy.select(Token)).all() == Any.list.containing([other_developer_token, other_oauth2_token]).only() ) + assert ( + db_session.scalars(sqlalchemy.select(UserDeletion)).all() + == Any.list.containing( + [ + Any.instance_of(UserDeletion).with_attrs( + { + "userid": user.userid, + "requested_by": requested_by.userid, + "tag": "test_tag", + "created_at": user.registered_date, + "num_annotations": len(user_annotations), + } + ) + ] + ).only() + ) def test_it_doesnt_delete_groups_others_have_annotated_in( - self, svc, factories, user, member, created_group + self, svc, factories, user, requested_by, member, created_group ): factories.Annotation(userid=member.userid, groupid=created_group.pubid) - svc.delete_user(user) + svc.delete_user(user, requested_by, "test_tag") # Check we don't delete groups which other people have annotated in assert not sqlalchemy.inspect(created_group).was_deleted @@ -63,6 +80,12 @@ def test_it_doesnt_delete_groups_others_have_annotated_in( @pytest.fixture def user(self, factories): + """Return the user who will be deleted.""" + return factories.User() + + @pytest.fixture + def requested_by(self, factories): + """Return the user who will be requesting the user deletion.""" return factories.User() @pytest.fixture diff --git a/tests/unit/h/views/admin/users_test.py b/tests/unit/h/views/admin/users_test.py index 3b6848ccdcf..d1547e09dbb 100644 --- a/tests/unit/h/views/admin/users_test.py +++ b/tests/unit/h/views/admin/users_test.py @@ -168,7 +168,11 @@ def test_users_delete_deletes_user(user_service, user_delete_service, pyramid_re users_delete(pyramid_request) - user_delete_service.delete_user.assert_called_once_with(user) + user_delete_service.delete_user.assert_called_once_with( + user, + requested_by=pyramid_request.user, + tag=pyramid_request.matched_route.name, + ) @pytest.fixture(autouse=True)