Record user deletions in a new DB table #8721

seanh · 2024-05-22T12:18:39Z

Migration: #8722.

The code that actually queries this table is over in #8723.

Testing

Log in to http://localhost:5000/login as devdata_admin, go to http://localhost:5000/admin/users?username=devdata_user&authority=localhost and delete devdata_user. You should see a record like this in the user_deletion table:

id              | 1
userid          | acct:devdata_user@localhost
requested_at    | 2024-05-22 14:32:30.891754
requested_by    | acct:devdata_admin@localhost
tag             | admin.users_delete
created_at      | 2024-05-22 14:31:07.501916
num_annotations | 0

seanh · 2024-05-22T14:34:43Z

h/cli/commands/user.py

-
-
-@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)


Nobody uses this command so I'm just deleting it rather than updating it to write to (or skip) the new user deletions log.

seanh · 2024-05-22T14:36:28Z

h/models/helpers.py

+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())})"


Used this code in a couple of places now so moved it to a helper function.

Longer-term I think we should move all h's models to SQLAlchemy's dataclasses support (which we use in Via) then we won't need this.

seanh · 2024-05-22T14:37:26Z

h/models/user_deletion.py

+    def __repr__(self) -> str:
+        return helpers.repr_(
+            self,
+            [
+                "id",
+                "userid",
+                "requested_at",
+                "requested_by",
+                "tag",
+                "created_at",
+                "num_annotations",
+            ],
+        )


Composition over inheritance 👍

seanh · 2024-05-22T14:40:33Z

h/models/user_deletion.py

+    id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
+    userid: Mapped[str]
+    requested_at: Mapped[datetime] = mapped_column(
+        server_default=func.now(),  # pylint:disable=not-callable
+    )
+    requested_by: Mapped[str]
+    tag: Mapped[str]
+    created_at: Mapped[datetime]
+    num_annotations: Mapped[int]


Might want to add an index to match the query we'll be doing on this table?

Sure, why not, but I don't think table is going to grow that fast, up to you.

I'm gonna leave the index for later

seanh · 2024-05-22T14:41:31Z

h/services/user_delete.py

+                num_annotations=self._db.scalar(
+                    sa.select(
+                        sa.func.count(Annotation.id)  # pylint:disable=not-callable
+                    ).where(Annotation.userid == user.userid)
+                ),


Querying the annotation table here. Any chance we might want to move this to annotation_slim?

Not yet (if you want to get the exact number), just created:

#8727 to finish that.

seanh · 2024-05-22T14:45:59Z

tests/unit/h/models/user_deletion_test.py

+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


We can either have a boilerplatey mock-based test like this, or we could use # pragma: nocover, or we could have a test that does not mock the helper (which sounds nicer but is actually a pain to write because the test would have to pass hard-coded values for the whole long list of model attrs, unless you want the test code to look almost the same as the repr_() helper code). I don't think it makes much difference 🤷‍♂️

Yeah, # pragma: nocover could have work here. This works as well of course.

marcospri · 2024-05-23T09:11:47Z

h/models/user_deletion.py

+    )
+    requested_by: Mapped[str]
+    tag: Mapped[str]
+    created_at: Mapped[datetime]


created_at is "user.registered_at" 👍

Yeah. Actually maybe I should rename this to registered_at to match, and also add docsttings to these attrs...

marcospri · 2024-05-23T09:22:37Z

h/services/user_delete.py

+                num_annotations=self._db.scalar(
+                    sa.select(
+                        sa.func.count(Annotation.id)  # pylint:disable=not-callable
+                    ).where(Annotation.userid == user.userid)
+                ),


Not yet (if you want to get the exact number), just created:

#8727 to finish that.

marcospri · 2024-05-23T09:25:11Z

tests/unit/h/models/user_deletion_test.py

+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


Yeah, # pragma: nocover could have work here. This works as well of course.

marcospri · 2024-05-23T09:26:55Z

h/models/user_deletion.py

+    id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
+    userid: Mapped[str]
+    requested_at: Mapped[datetime] = mapped_column(
+        server_default=func.now(),  # pylint:disable=not-callable
+    )
+    requested_by: Mapped[str]
+    tag: Mapped[str]
+    created_at: Mapped[datetime]
+    num_annotations: Mapped[int]


Might want to add an index to match the query we'll be doing on this table?

Sure, why not, but I don't think table is going to grow that fast, up to you.

marcospri

This PR target is no the migration branch, but we have to deploy the migration first.

Looks good 👍

Remove the CLI command for deleting a user: this interacts awkwardly with recording deletions in a table because there's no authenticated user to record as the requester of the deletion. There could be various solutions to this but no one uses this CLI command anyway so let's just delete it.

seanh requested a review from marcospri May 22, 2024 12:18

seanh force-pushed the record-user-deletions branch 3 times, most recently from 41b5c22 to 524bdcf Compare May 22, 2024 14:29

seanh commented May 22, 2024

View reviewed changes

seanh force-pushed the record-user-deletions branch 2 times, most recently from cead5cf to d8ef46e Compare May 22, 2024 14:39

seanh commented May 22, 2024

View reviewed changes

seanh force-pushed the record-user-deletions branch from d8ef46e to 7ab0ba2 Compare May 22, 2024 15:07

seanh marked this pull request as ready for review May 22, 2024 15:11

seanh mentioned this pull request May 22, 2024

Add migration to add user_deletion table #8722

Merged

marcospri reviewed May 23, 2024

View reviewed changes

marcospri approved these changes May 23, 2024

View reviewed changes

seanh force-pushed the record-user-deletions branch from 7ab0ba2 to b875bfd Compare May 23, 2024 10:38

seanh merged commit 3ddb0f7 into main May 23, 2024
9 checks passed

seanh deleted the record-user-deletions branch May 23, 2024 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record user deletions in a new DB table #8721

Record user deletions in a new DB table #8721

seanh commented May 22, 2024 •

edited

Loading

seanh May 22, 2024

seanh May 22, 2024

seanh May 22, 2024

seanh May 22, 2024

marcospri May 23, 2024

seanh May 23, 2024

seanh May 22, 2024

marcospri May 23, 2024

seanh May 22, 2024 •

edited

Loading

marcospri May 23, 2024

marcospri May 23, 2024

seanh May 23, 2024

seanh May 23, 2024

marcospri May 23, 2024

marcospri May 23, 2024

marcospri May 23, 2024

marcospri left a comment

Record user deletions in a new DB table #8721

Record user deletions in a new DB table #8721

Conversation

seanh commented May 22, 2024 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri left a comment

Choose a reason for hiding this comment

seanh commented May 22, 2024 •

edited

Loading

seanh May 22, 2024 •

edited

Loading