From fdf90f7ad15b289494fdbef76f5df05390096eec Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Wed, 23 Oct 2024 19:22:35 +0530 Subject: [PATCH] create_user: Handle integrity error when importing settings. During account creation when a user opted to import settings from an existing account, the "Mark visibility_policy_banner as read" step was raising integrity error. It is because 'copy_onboarding_steps' is already executed earlier in the 'do_create_user' codeflow. If the source profile had already marked 'visibility_policy_banner' as read, we were facing integrity error. This commit fixes the bug. --- zerver/actions/create_user.py | 15 +++++++++++++-- zerver/tests/test_signup.py | 23 +++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/zerver/actions/create_user.py b/zerver/actions/create_user.py index 6502767657..b0b6cef415 100644 --- a/zerver/actions/create_user.py +++ b/zerver/actions/create_user.py @@ -1,10 +1,11 @@ from collections import defaultdict from collections.abc import Iterable, Sequence +from contextlib import suppress from datetime import timedelta from typing import Any from django.conf import settings -from django.db import transaction +from django.db import IntegrityError, transaction from django.db.models import F from django.utils.timezone import now as timezone_now from django.utils.translation import gettext as _ @@ -351,7 +352,17 @@ def process_new_human_user( # The 'visibility_policy_banner' is only displayed to existing users. # Mark it as read for a new user. - OnboardingStep.objects.create(user=user_profile, onboarding_step="visibility_policy_banner") + # + # If the new user opted to import settings from an existing account, and + # 'visibility_policy_banner' is already marked as read for the existing account, + # 'copy_onboarding_steps' function already did the needed copying. + # Simply ignore the IntegrityError in that case. + # + # The extremely brief nature of this subtransaction makes a savepoint safe. + # See https://postgres.ai/blog/20210831-postgresql-subtransactions-considered-harmful + # for context on risks around savepoints. + with suppress(IntegrityError), transaction.atomic(savepoint=True): + OnboardingStep.objects.create(user=user_profile, onboarding_step="visibility_policy_banner") def notify_created_user(user_profile: UserProfile, notify_user_ids: list[int]) -> None: diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 6d479fb65e..8ab0bfd07b 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -67,6 +67,7 @@ from zerver.models import ( CustomProfileFieldValue, DefaultStream, Message, + OnboardingStep, OnboardingUserMessage, PreregistrationRealm, PreregistrationUser, @@ -2826,6 +2827,9 @@ class UserSignUpTest(ZulipTestCase): hamlet_in_zulip.email_address_visibility = UserProfile.EMAIL_ADDRESS_VISIBILITY_EVERYONE hamlet_in_zulip.save() + OnboardingStep.objects.filter(user=hamlet_in_zulip).delete() + OnboardingStep.objects.create(user=hamlet_in_zulip, onboarding_step="intro_resolve_topic") + result = self.client_post("/accounts/home/", {"email": email}, subdomain=subdomain) self.assertEqual(result.status_code, 302) result = self.client_get(result["Location"], subdomain=subdomain) @@ -2844,6 +2848,12 @@ class UserSignUpTest(ZulipTestCase): self.assertEqual(hamlet.high_contrast_mode, False) self.assertEqual(hamlet.enable_stream_audible_notifications, False) self.assertEqual(hamlet.enter_sends, False) + # 'visibility_policy_banner' is marked as read in 'process_new_human_user'. + # 'copy_default_settings' is not executed as the user decided to NOT import + # settings from another realm, hence 'intro_resolve_topic' not marked as seen. + onboarding_steps = OnboardingStep.objects.filter(user=hamlet) + self.assertEqual(onboarding_steps.count(), 1) + self.assertEqual(onboarding_steps[0].onboarding_step, "visibility_policy_banner") def test_signup_with_user_settings_from_another_realm(self) -> None: hamlet_in_zulip = self.example_user("hamlet") @@ -2864,6 +2874,12 @@ class UserSignUpTest(ZulipTestCase): hamlet_in_zulip.email_address_visibility = UserProfile.EMAIL_ADDRESS_VISIBILITY_EVERYONE hamlet_in_zulip.save() + OnboardingStep.objects.filter(user=hamlet_in_zulip).delete() + OnboardingStep.objects.create(user=hamlet_in_zulip, onboarding_step="intro_resolve_topic") + OnboardingStep.objects.create( + user=hamlet_in_zulip, onboarding_step="visibility_policy_banner" + ) + # Now we'll be making requests to another subdomain, so we need to logout # to avoid polluting the session in the test environment by still being # logged in. @@ -2909,6 +2925,13 @@ class UserSignUpTest(ZulipTestCase): self.assertEqual( hamlet_in_lear.email_address_visibility, UserProfile.EMAIL_ADDRESS_VISIBILITY_NOBODY ) + # Verify that 'copy_default_settings' copies the onboarding steps. + onboarding_steps = OnboardingStep.objects.filter(user=hamlet_in_lear) + self.assertEqual(onboarding_steps.count(), 2) + self.assertEqual( + set(onboarding_steps.values_list("onboarding_step", flat=True)), + {"intro_resolve_topic", "visibility_policy_banner"}, + ) zulip_path_id = avatar_disk_path(hamlet_in_zulip) lear_path_id = avatar_disk_path(hamlet_in_lear)