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.
This commit is contained in:
Prakhar Pratyush 2024-10-23 19:22:35 +05:30 committed by Tim Abbott
parent 555ac613ac
commit fdf90f7ad1
2 changed files with 36 additions and 2 deletions

View File

@ -1,10 +1,11 @@
from collections import defaultdict from collections import defaultdict
from collections.abc import Iterable, Sequence from collections.abc import Iterable, Sequence
from contextlib import suppress
from datetime import timedelta from datetime import timedelta
from typing import Any from typing import Any
from django.conf import settings from django.conf import settings
from django.db import transaction from django.db import IntegrityError, transaction
from django.db.models import F from django.db.models import F
from django.utils.timezone import now as timezone_now from django.utils.timezone import now as timezone_now
from django.utils.translation import gettext as _ 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. # The 'visibility_policy_banner' is only displayed to existing users.
# Mark it as read for a new user. # 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: def notify_created_user(user_profile: UserProfile, notify_user_ids: list[int]) -> None:

View File

@ -67,6 +67,7 @@ from zerver.models import (
CustomProfileFieldValue, CustomProfileFieldValue,
DefaultStream, DefaultStream,
Message, Message,
OnboardingStep,
OnboardingUserMessage, OnboardingUserMessage,
PreregistrationRealm, PreregistrationRealm,
PreregistrationUser, PreregistrationUser,
@ -2826,6 +2827,9 @@ class UserSignUpTest(ZulipTestCase):
hamlet_in_zulip.email_address_visibility = UserProfile.EMAIL_ADDRESS_VISIBILITY_EVERYONE hamlet_in_zulip.email_address_visibility = UserProfile.EMAIL_ADDRESS_VISIBILITY_EVERYONE
hamlet_in_zulip.save() 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) result = self.client_post("/accounts/home/", {"email": email}, subdomain=subdomain)
self.assertEqual(result.status_code, 302) self.assertEqual(result.status_code, 302)
result = self.client_get(result["Location"], subdomain=subdomain) 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.high_contrast_mode, False)
self.assertEqual(hamlet.enable_stream_audible_notifications, False) self.assertEqual(hamlet.enable_stream_audible_notifications, False)
self.assertEqual(hamlet.enter_sends, 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: def test_signup_with_user_settings_from_another_realm(self) -> None:
hamlet_in_zulip = self.example_user("hamlet") 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.email_address_visibility = UserProfile.EMAIL_ADDRESS_VISIBILITY_EVERYONE
hamlet_in_zulip.save() 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 # 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 # to avoid polluting the session in the test environment by still being
# logged in. # logged in.
@ -2909,6 +2925,13 @@ class UserSignUpTest(ZulipTestCase):
self.assertEqual( self.assertEqual(
hamlet_in_lear.email_address_visibility, UserProfile.EMAIL_ADDRESS_VISIBILITY_NOBODY 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) zulip_path_id = avatar_disk_path(hamlet_in_zulip)
lear_path_id = avatar_disk_path(hamlet_in_lear) lear_path_id = avatar_disk_path(hamlet_in_lear)