From 77c8aea6289419f172eefc65ebee9cb47f15d8b2 Mon Sep 17 00:00:00 2001 From: Joe Kaufeld Date: Sun, 17 Jul 2022 20:41:30 -0400 Subject: [PATCH] finish migration command --- api/migrations/0025_accountmigration.py | 77 ++++ api/models.py | 6 +- api/slack/commands/__init__.py | 2 + api/slack/commands/migrate_user.py | 33 +- .../slack/commands/test_account_migration.py | 340 ++++++++++++++++++ blossom/strings/en_US.toml | 1 + 6 files changed, 439 insertions(+), 20 deletions(-) create mode 100644 api/migrations/0025_accountmigration.py create mode 100644 api/tests/slack/commands/test_account_migration.py diff --git a/api/migrations/0025_accountmigration.py b/api/migrations/0025_accountmigration.py new file mode 100644 index 00000000..bba5662f --- /dev/null +++ b/api/migrations/0025_accountmigration.py @@ -0,0 +1,77 @@ +# Generated by Django 3.2.14 on 2022-07-18 00:07 + +import django.db.models.deletion +import django.utils.timezone +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("api", "0024_alter_transcriptioncheck_status"), + ] + + operations = [ + migrations.CreateModel( + name="AccountMigration", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "create_time", + models.DateTimeField(default=django.utils.timezone.now), + ), + ( + "slack_channel_id", + models.CharField( + blank=True, default=None, max_length=50, null=True + ), + ), + ( + "slack_message_ts", + models.CharField( + blank=True, default=None, max_length=50, null=True + ), + ), + ("affected_submissions", models.ManyToManyField(to="api.Submission")), + ( + "moderator", + models.ForeignKey( + default=None, + null=True, + on_delete=django.db.models.deletion.SET_DEFAULT, + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "new_user", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="new_user", + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "old_user", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="old_user", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + ), + ] diff --git a/api/models.py b/api/models.py index 80f9a505..fd47da53 100644 --- a/api/models.py +++ b/api/models.py @@ -425,11 +425,10 @@ class AccountMigration(models.Model): new_user = models.ForeignKey( "authentication.BlossomUser", on_delete=models.SET_NULL, - related_name="old_user", + related_name="new_user", null=True, blank=True, ) - completed = models.BooleanField(default=False) # keep track of submissions that were modified in case we need to roll back affected_submissions = models.ManyToManyField(Submission) # who has approved this migration? @@ -450,7 +449,7 @@ class AccountMigration(models.Model): def perform_migration(self) -> None: """Move all submissions attributed to one account to another.""" existing_submissions = Submission.objects.filter(completed_by=self.old_user) - self.affected_submissions.add(existing_submissions) + self.affected_submissions.add(*existing_submissions) existing_submissions.update( claimed_by=self.new_user, completed_by=self.new_user @@ -465,4 +464,3 @@ def revert(self) -> None: self.affected_submissions.update( claimed_by=self.old_user, completed_by=self.old_user ) - self.completed = False diff --git a/api/slack/commands/__init__.py b/api/slack/commands/__init__.py index 3661b1df..531f51f7 100644 --- a/api/slack/commands/__init__.py +++ b/api/slack/commands/__init__.py @@ -11,6 +11,7 @@ from api.slack.commands.dadjoke import dadjoke_cmd from api.slack.commands.help import help_cmd from api.slack.commands.info import info_cmd +from api.slack.commands.migrate_user import migrate_user_cmd from api.slack.commands.ping import ping_cmd from api.slack.commands.reset import reset_cmd from api.slack.commands.unwatch import unwatch_cmd @@ -58,6 +59,7 @@ def process_command(data: Dict) -> None: "dadjoke": dadjoke_cmd, "help": help_cmd, "info": info_cmd, + "migrate": migrate_user_cmd, "ping": ping_cmd, "reset": reset_cmd, "unwatch": unwatch_cmd, diff --git a/api/slack/commands/migrate_user.py b/api/slack/commands/migrate_user.py index edeeed77..4b7ceb56 100644 --- a/api/slack/commands/migrate_user.py +++ b/api/slack/commands/migrate_user.py @@ -1,4 +1,4 @@ -from copy import copy +from copy import deepcopy from api.models import AccountMigration from api.slack import client @@ -82,18 +82,18 @@ def _create_blocks( - migration: AccountMigration, approve_cancel: bool = True, revert: bool = False + migration: AccountMigration, approve_cancel: bool = False, revert: bool = False ) -> dict: - blocks = copy(BASE) - header = copy(HEADER_BLOCK) - header["text"]["text"] = header["text"]["text"].format( + blocks = deepcopy(BASE) + header = deepcopy(HEADER_BLOCK) + header["text"]["text"] = HEADER_BLOCK["text"]["text"].format( migration.old_user.username, migration.new_user.username ) blocks["blocks"].append(header) if migration.moderator and revert: # show who approved it while when we show the button to revert it - mod_block = copy(MOD_BLOCK) + mod_block = deepcopy(MOD_BLOCK) mod_block["text"]["text"] = MOD_BLOCK["text"]["text"].format( migration.moderator.username ) @@ -101,18 +101,18 @@ def _create_blocks( blocks["blocks"].append(DIVIDER_BLOCK) - action_block = copy(ACTION_BLOCK) + action_block = deepcopy(ACTION_BLOCK) if approve_cancel: - approve_button = copy(APPROVE_BUTTON) + approve_button = deepcopy(APPROVE_BUTTON) approve_button["value"] = APPROVE_BUTTON["value"].format(migration.id) - cancel_button = copy(CANCEL_BUTTON) + cancel_button = deepcopy(CANCEL_BUTTON) cancel_button["value"] = CANCEL_BUTTON["value"].format(migration.id) action_block["elements"].append(approve_button) action_block["elements"].append(cancel_button) if revert: - revert_button = copy(REVERT_BUTTON) + revert_button = deepcopy(REVERT_BUTTON) revert_button["value"] = revert_button["value"].format(migration.id) action_block["elements"].append(revert_button) @@ -127,9 +127,10 @@ def migrate_user_cmd(channel: str, message: str) -> None: parsed_message = message.split() blocks = None msg = None + migration = None # appease linter if len(parsed_message) < 3: # Needs to have two usernames - msg = i18n["slack"]["errors"]["missing_username"] + msg = i18n["slack"]["errors"]["missing_multiple_usernames"] elif len(parsed_message) == 3: old_user, old_username = parse_user(parsed_message[1]) new_user, new_username = parse_user(parsed_message[2]) @@ -144,9 +145,9 @@ def migrate_user_cmd(channel: str, message: str) -> None: if old_user and new_user: migration = AccountMigration.objects.create( - delay_minutes=3, old_user=old_user, new_user=new_user + old_user=old_user, new_user=new_user ) - blocks = _create_blocks(migration) + blocks = _create_blocks(migration, approve_cancel=True) else: msg = i18n["slack"]["errors"]["too_many_params"] @@ -160,8 +161,8 @@ def migrate_user_cmd(channel: str, message: str) -> None: response = client.chat_postMessage(**args) if blocks: - migration.report_slack_channel_id = response["channel"] - migration.report_slack_message_ts = response["message"]["ts"] + migration.slack_channel_id = response["channel"] + migration.slack_message_ts = response["message"]["ts"] migration.save() @@ -195,7 +196,7 @@ def process_migrate_user(data: dict) -> None: if action == "approve": migration.perform_migration() - blocks = _create_blocks(migration, approve_cancel=False, revert=True) + blocks = _create_blocks(migration, revert=True) elif action == "revert": migration.revert() blocks = _create_blocks(migration) # Show no buttons here. diff --git a/api/tests/slack/commands/test_account_migration.py b/api/tests/slack/commands/test_account_migration.py new file mode 100644 index 00000000..19542d9b --- /dev/null +++ b/api/tests/slack/commands/test_account_migration.py @@ -0,0 +1,340 @@ +from unittest.mock import patch + +from api.models import AccountMigration, Submission +from api.slack.commands.migrate_user import ( + _create_blocks, + migrate_user_cmd, + process_migrate_user, +) +from blossom.strings import translation +from utils.test_helpers import create_submission, create_user + +i18n = translation() + + +def test_perform_migration() -> None: + """Verify that account migration works correctly.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Moddington") + + submission1 = create_submission(claimed_by=user1, completed_by=user1) + create_submission(claimed_by=user2, completed_by=user2) + + assert Submission.objects.filter(completed_by=user1).count() == 1 + assert Submission.objects.filter(completed_by=user2).count() == 1 + + migration = AccountMigration.objects.create(old_user=user1, new_user=user2) + migration.perform_migration() + assert migration.affected_submissions.count() == 1 + assert Submission.objects.filter(completed_by=user1).count() == 0 + assert Submission.objects.filter(completed_by=user2).count() == 2 + + submission1.refresh_from_db() + assert submission1.claimed_by == user2 + assert submission1.completed_by == user2 + + +def test_revert() -> None: + """Verify that reverting an account migration works.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Moddington") + + submission1 = create_submission(claimed_by=user1, completed_by=user1) + create_submission(claimed_by=user2, completed_by=user2) + + assert Submission.objects.filter(completed_by=user1).count() == 1 + assert Submission.objects.filter(completed_by=user2).count() == 1 + + migration = AccountMigration.objects.create(old_user=user1, new_user=user2) + migration.perform_migration() + + assert Submission.objects.filter(completed_by=user1).count() == 0 + assert Submission.objects.filter(completed_by=user2).count() == 2 + + migration.revert() + + assert Submission.objects.filter(completed_by=user1).count() == 1 + assert Submission.objects.filter(completed_by=user2).count() == 1 + + submission1.refresh_from_db() + assert submission1.claimed_by == user1 + assert submission1.completed_by == user1 + + +def test_create_blocks() -> None: + """Verify that blocks are created by default as expected.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Moddington") + migration = AccountMigration.objects.create(old_user=user1, new_user=user2) + + # no buttons requested + blocks = _create_blocks(migration) + # header and divider + assert len(blocks["blocks"]) == 2 + assert "Paddington" in blocks["blocks"][0]["text"]["text"] + assert "Moddington" in blocks["blocks"][0]["text"]["text"] + + +def test_create_blocks_with_revert_button() -> None: + """Verify that blocks are created with the revert button as expected.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Moddington") + migration = AccountMigration.objects.create(old_user=user1, new_user=user2) + + blocks = _create_blocks(migration, revert=True) + assert len(blocks["blocks"]) == 3 + assert len(blocks["blocks"][2]["elements"]) == 1 + assert ( + blocks["blocks"][2]["elements"][0]["value"] + == f"revert_migration_{migration.id}" + ) + + +def test_create_blocks_with_approve_cancel_buttons() -> None: + """Verify that blocks are created with approve and cancel buttons.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Moddington") + migration = AccountMigration.objects.create(old_user=user1, new_user=user2) + + blocks = _create_blocks(migration, approve_cancel=True) + assert len(blocks["blocks"]) == 3 + assert len(blocks["blocks"][2]["elements"]) == 2 + assert ( + blocks["blocks"][2]["elements"][0]["value"] + == f"approve_migration_{migration.id}" + ) + assert ( + blocks["blocks"][2]["elements"][1]["value"] + == f"cancel_migration_{migration.id}" + ) + + +def test_create_blocks_with_mod() -> None: + """Verify that the mod section is created appropriately.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Bear") + user3 = create_user(id=201, username="Mod Moddington") + migration = AccountMigration.objects.create( + old_user=user1, new_user=user2, moderator=user3 + ) + + blocks = _create_blocks(migration, revert=True) + assert len(blocks["blocks"]) == 4 + assert blocks["blocks"][1] == { + "type": "section", + "text": { + "type": "plain_text", + "text": "Approved by *u/Mod Moddington*.", + }, + } + + +def test_migrate_user_cmd() -> None: + """Verify that the slack command for migration works as expected.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Moddington") + + assert AccountMigration.objects.count() == 0 + + with patch("api.slack.commands.migrate_user.client.chat_postMessage") as mock: + mock.return_value = {"channel": "AAA", "message": {"ts": 1234}} + migrate_user_cmd(channel="abc", message="migrate paddington moddington") + + mock.assert_called_once() + assert len(mock.call_args.kwargs["blocks"]["blocks"]) == 3 + assert mock.call_args.kwargs["channel"] == "abc" + + assert AccountMigration.objects.count() == 1 + migration = AccountMigration.objects.first() + assert migration.old_user == user1 + assert migration.new_user == user2 + assert migration.slack_channel_id == "AAA" + assert migration.slack_message_ts == "1234" + + +def test_migrate_user_cmd_missing_users() -> None: + """Verify error for missing users.""" + with patch("api.slack.commands.migrate_user.client.chat_postMessage") as mock: + migrate_user_cmd(channel="abc", message="migrate") + + # no blocks when there's text here + assert mock.call_args.kwargs.get("blocks") is None + assert ( + mock.call_args.kwargs["text"] + == i18n["slack"]["errors"]["missing_multiple_usernames"] + ) + assert mock.call_args.kwargs["channel"] == "abc" + + +def test_migrate_user_cmd_wrong_first_user() -> None: + """Verify error for missing first user.""" + create_user(id=100, username="Paddington") + with patch("api.slack.commands.migrate_user.client.chat_postMessage") as mock: + migrate_user_cmd(channel="abc", message="migrate AAAA Paddington") + + # no blocks when there's text here + assert mock.call_args.kwargs.get("blocks") is None + assert mock.call_args.kwargs["text"] == i18n["slack"]["errors"][ + "unknown_username" + ].format(username="AAAA") + assert mock.call_args.kwargs["channel"] == "abc" + + +def test_migrate_user_cmd_wrong_second_user() -> None: + """Verify error for missing second user.""" + create_user(id=100, username="Paddington") + with patch("api.slack.commands.migrate_user.client.chat_postMessage") as mock: + migrate_user_cmd(channel="abc", message="migrate Paddington BBBB") + + assert mock.call_args.kwargs.get("blocks") is None + assert mock.call_args.kwargs["text"] == i18n["slack"]["errors"][ + "unknown_username" + ].format(username="BBBB") + assert mock.call_args.kwargs["channel"] == "abc" + + +def test_migrate_user_cmd_too_many_users() -> None: + """Verify error for too many users.""" + with patch("api.slack.commands.migrate_user.client.chat_postMessage") as mock: + migrate_user_cmd(channel="abc", message="migrate A B C") + + assert mock.call_args.kwargs.get("blocks") is None + assert mock.call_args.kwargs["text"] == i18n["slack"]["errors"]["too_many_params"] + assert mock.call_args.kwargs["channel"] == "abc" + + +def test_process_migrate_user() -> None: + """Verify migration works when called via buttons.""" + user1 = create_user(id=100, username="Paddington") + user2 = create_user(id=200, username="Moddington") + + create_submission(claimed_by=user1, completed_by=user1) + create_submission(claimed_by=user2, completed_by=user2) + + assert Submission.objects.filter(completed_by=user1).count() == 1 + assert Submission.objects.filter(completed_by=user2).count() == 1 + + migration = AccountMigration.objects.create( + old_user=user1, new_user=user2, slack_message_ts=123, slack_channel_id="AAA" + ) + + with patch( + "api.slack.commands.migrate_user.get_reddit_username", + lambda _, us: us["name"], + ), patch( + "api.slack.commands.migrate_user.client.chat_update" + ) as messageMock, patch( + "api.slack.commands.migrate_user.reply_to_action_with_ping", + return_value={}, + ) as reply_mock: + process_migrate_user( + { + "actions": [{"value": f"approve_migration_{migration.id}"}], + "user": {"name": "Moddington"}, + } + ) + + assert Submission.objects.filter(completed_by=user1).count() == 0 + assert Submission.objects.filter(completed_by=user2).count() == 2 + + messageMock.assert_called_once() + # header, mod approved, divider, revert button + assert len(messageMock.call_args.kwargs["blocks"]["blocks"]) == 4 + + revert_button = messageMock.call_args.kwargs["blocks"]["blocks"][3]["elements"][0] + assert revert_button["value"] == f"revert_migration_{migration.id}" + + with patch( + "api.slack.commands.migrate_user.get_reddit_username", + lambda _, us: us["name"], + ), patch( + "api.slack.commands.migrate_user.client.chat_update" + ) as messageMock, patch( + "api.slack.commands.migrate_user.reply_to_action_with_ping", + return_value={}, + ) as reply_mock: + process_migrate_user( + { + "actions": [{"value": f"revert_migration_{migration.id}"}], + "user": {"name": "Moddington"}, + } + ) + + assert Submission.objects.filter(completed_by=user1).count() == 1 + assert Submission.objects.filter(completed_by=user2).count() == 1 + + # we've reverted -- no more buttons for you + assert len(messageMock.call_args.kwargs["blocks"]["blocks"]) == 2 + + with patch( + "api.slack.commands.migrate_user.get_reddit_username", + lambda _, us: us["name"], + ), patch( + "api.slack.commands.migrate_user.client.chat_update" + ) as messageMock, patch( + "api.slack.commands.migrate_user.reply_to_action_with_ping", + return_value={}, + ) as reply_mock: + process_migrate_user( + { + "actions": [{"value": f"cancel_migration_{migration.id}"}], + "user": {"name": "Moddington"}, + } + ) + + # the cancel button, as of right now, should fall through and nothing + # should happen. + messageMock.assert_not_called() + reply_mock.assert_not_called() + + +def test_migrate_user_no_migration() -> None: + """Verify error for nonexistent migration.""" + create_user(id=200, username="Moddington") + + with patch( + "api.slack.commands.migrate_user.get_reddit_username", + lambda _, us: us["name"], + ), patch( + "api.slack.commands.migrate_user.client.chat_update" + ) as messageMock, patch( + "api.slack.commands.migrate_user.reply_to_action_with_ping", + return_value={}, + ) as reply_mock: + process_migrate_user( + { + "actions": [{"value": "approve_migration_1"}], + "user": {"name": "Moddington"}, + } + ) + + messageMock.assert_not_called() + reply_mock.assert_called_once() + assert reply_mock.call_args.args[-1] == "I couldn't find a check with ID 1!" + + +def test_migrate_user_wrong_username() -> None: + """Verify error for wrong mod username on Slack.""" + user1 = create_user(id=200, username="Moddington") + + migration = AccountMigration.objects.create(old_user=user1, new_user=user1) + + with patch( + "api.slack.commands.migrate_user.get_reddit_username", + lambda _, us: us["name"], + ), patch( + "api.slack.commands.migrate_user.client.chat_update" + ) as messageMock, patch( + "api.slack.commands.migrate_user.reply_to_action_with_ping", + return_value={}, + ) as reply_mock: + process_migrate_user( + { + "actions": [{"value": f"approve_migration_{migration.id}"}], + "user": {"name": "AA"}, + } + ) + + messageMock.assert_not_called() + reply_mock.assert_called_once() + assert "I couldn't find a mod with username u/AA." in reply_mock.call_args.args[-1] diff --git a/blossom/strings/en_US.toml b/blossom/strings/en_US.toml index 5de139a3..9dcea1fc 100644 --- a/blossom/strings/en_US.toml +++ b/blossom/strings/en_US.toml @@ -65,6 +65,7 @@ message_parse_error="Sorry, something went wrong and I couldn't parse your messa empty_message_error="Sorry, I wasn't able to get text out of that. Try again." too_many_params="Too many parameters; please try again." missing_username="I don't see a username in your request -- make sure you're formatting your request as \"@Blossom {action} {argument}\". Example: \"@Blossom dadjoke @itsthejoker\"" +missing_multiple_usernames="This command uses multiple usernames -- make sure you're formatting your command as \"@Blossom {action} {argument} {argument} ...\"." unknown_username="Sorry, I couldn't find a user with the name `u/{username}`. Please check your spelling." unknown_payload="Received unknown payload from Slack with key I don't recognize. Unknown key: {}"