From c804ab27d570d007fe33a2efebe8a8451a5b0d67 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Thu, 8 Jul 2021 17:27:06 -0700 Subject: [PATCH] actions: Rename do_activate_user to do_activate_mirror_dummy_user. This function had a confusing name, which could result in someone using it unintentionally when they meant do_reactivate_user. We also add docstrings for both functions. --- analytics/tests/test_counts.py | 6 +++--- corporate/tests/test_stripe.py | 6 +++--- zerver/lib/actions.py | 20 +++++++++++++++++--- zerver/tests/test_audit_log.py | 4 ++-- zerver/views/registration.py | 6 +++--- 5 files changed, 28 insertions(+), 14 deletions(-) diff --git a/analytics/tests/test_counts.py b/analytics/tests/test_counts.py index c265c38fb9..1ae56006ed 100644 --- a/analytics/tests/test_counts.py +++ b/analytics/tests/test_counts.py @@ -33,7 +33,7 @@ from analytics.models import ( installation_epoch, ) from zerver.lib.actions import ( - do_activate_user, + do_activate_mirror_dummy_user, do_create_realm, do_create_user, do_deactivate_user, @@ -1329,7 +1329,7 @@ class TestLoggingCountStats(AnalyticsTestCase): "value__sum" ], ) - do_activate_user(user, acting_user=None) + do_activate_mirror_dummy_user(user, acting_user=None) self.assertEqual( 1, RealmCount.objects.filter(property=property, subgroup=False).aggregate(Sum("value"))[ @@ -1663,7 +1663,7 @@ class TestActiveUsersAudit(AnalyticsTestCase): "email4", "password", self.default_realm, "full_name", acting_user=None ) do_deactivate_user(user2, acting_user=None) - do_activate_user(user3, acting_user=None) + do_activate_mirror_dummy_user(user3, acting_user=None) do_reactivate_user(user4, acting_user=None) end_time = floor_to_day(timezone_now()) + self.DAY do_fill_count_stat_at_hour(self.stat, end_time) diff --git a/corporate/tests/test_stripe.py b/corporate/tests/test_stripe.py index 0f608f89d5..60228c6536 100644 --- a/corporate/tests/test_stripe.py +++ b/corporate/tests/test_stripe.py @@ -78,7 +78,7 @@ from corporate.models import ( get_customer_by_realm, ) from zerver.lib.actions import ( - do_activate_user, + do_activate_mirror_dummy_user, do_create_realm, do_create_user, do_deactivate_realm, @@ -3366,8 +3366,8 @@ class LicenseLedgerTest(StripeTestCase): user = do_create_user("email", "password", get_realm("zulip"), "name", acting_user=None) do_deactivate_user(user, acting_user=None) do_reactivate_user(user, acting_user=None) - # Not a proper use of do_activate_user, but fine for this test - do_activate_user(user, acting_user=None) + # Not a proper use of do_activate_mirror_dummy_user, but fine for this test + do_activate_mirror_dummy_user(user, acting_user=None) ledger_entries = list( LicenseLedger.objects.values_list( "is_renewal", "licenses", "licenses_at_next_renewal" diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 6362a038f1..8a91924e18 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -707,7 +707,22 @@ def do_create_user( return user_profile -def do_activate_user(user_profile: UserProfile, *, acting_user: Optional[UserProfile]) -> None: +def do_activate_mirror_dummy_user( + user_profile: UserProfile, *, acting_user: Optional[UserProfile] +) -> None: + """Called to have a user "take over" a "mirror dummy" user + (i.e. is_mirror_dummy=True) account when they sign up with the + same email address. + + Essentially, the result should be as though we had created the + UserProfile just now with do_create_user, except that the mirror + dummy user may appear as the recipient or sender of messages from + before their account was fully created. + + TODO: This function likely has bugs resulting from this being a + parallel code path to do_create_user; e.g. it likely does not + handle preferences or default streams properly. + """ with transaction.atomic(): change_user_is_active(user_profile, True) user_profile.is_mirror_dummy = False @@ -744,8 +759,7 @@ def do_activate_user(user_profile: UserProfile, *, acting_user: Optional[UserPro def do_reactivate_user(user_profile: UserProfile, *, acting_user: Optional[UserProfile]) -> None: - # Unlike do_activate_user, this is meant for re-activating existing users, - # so it doesn't reset their password, etc. + """Reactivate a user that had previously been deactivated""" with transaction.atomic(): change_user_is_active(user_profile, True) diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 73c604cab7..d87696843e 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -9,7 +9,7 @@ from analytics.models import StreamCount from zerver.lib.actions import ( bulk_add_subscriptions, bulk_remove_subscriptions, - do_activate_user, + do_activate_mirror_dummy_user, do_change_avatar_fields, do_change_bot_owner, do_change_default_all_public_streams, @@ -70,7 +70,7 @@ class TestRealmAuditLog(ZulipTestCase): now = timezone_now() user = do_create_user("email", "password", realm, "full_name", acting_user=None) do_deactivate_user(user, acting_user=user) - do_activate_user(user, acting_user=user) + do_activate_mirror_dummy_user(user, acting_user=user) do_deactivate_user(user, acting_user=user) do_reactivate_user(user, acting_user=user) self.assertEqual(RealmAuditLog.objects.filter(event_time__gte=now).count(), 6) diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 55efbb60b4..d12590f92c 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -36,7 +36,7 @@ from zerver.forms import ( ) from zerver.lib.actions import ( bulk_add_subscriptions, - do_activate_user, + do_activate_mirror_dummy_user, do_change_full_name, do_change_password, do_create_realm, @@ -406,11 +406,11 @@ def accounts_register(request: HttpRequest) -> HttpResponse: if existing_user_profile is not None and existing_user_profile.is_mirror_dummy: user_profile = existing_user_profile - do_activate_user(user_profile, acting_user=user_profile) + do_activate_mirror_dummy_user(user_profile, acting_user=user_profile) do_change_password(user_profile, password) do_change_full_name(user_profile, full_name, user_profile) do_set_user_display_setting(user_profile, "timezone", timezone) - # TODO: When we clean up the `do_activate_user` code path, + # TODO: When we clean up the `do_activate_mirror_dummy_user` code path, # make it respect invited_as_admin / is_realm_admin. if user_profile is None: