send_email: Change clear_scheduled_emails to only take one user.

No codepath except tests passes in more than one user_profile -- and
doing so is what makes the deduplication necessary.

Simplify the API by making it only take one user_profile id.
This commit is contained in:
Alex Vandiver
2021-08-14 00:54:46 +00:00
committed by Tim Abbott
parent 6fbe7ad61e
commit ebaafb32f3
4 changed files with 8 additions and 24 deletions

View File

@@ -1214,7 +1214,7 @@ def do_deactivate_user(
change_user_is_active(user_profile, False)
delete_user_sessions(user_profile)
clear_scheduled_emails([user_profile.id])
clear_scheduled_emails(user_profile.id)
event_time = timezone_now()
RealmAuditLog.objects.create(
@@ -5036,7 +5036,7 @@ def do_change_notification_settings(
# Disabling digest emails should clear a user's email queue
if name == "enable_digest_emails" and not value:
clear_scheduled_emails([user_profile.id], ScheduledEmail.DIGEST)
clear_scheduled_emails(user_profile.id, ScheduledEmail.DIGEST)
user_profile.save(update_fields=[name])
event = {

View File

@@ -416,23 +416,20 @@ def clear_scheduled_invitation_emails(email: str) -> None:
@transaction.atomic(savepoint=False)
def clear_scheduled_emails(user_ids: List[int], email_type: Optional[int] = None) -> None:
def clear_scheduled_emails(user_id: int, email_type: Optional[int] = None) -> None:
# We need to obtain a FOR UPDATE lock on the selected rows to keep a concurrent
# execution of this function (or something else) from deleting them before we access
# the .users attribute.
items = ScheduledEmail.objects.filter(users__in=user_ids).select_for_update()
items = ScheduledEmail.objects.filter(users__in=[user_id]).select_for_update()
if email_type is not None:
items = items.filter(type=email_type)
deduplicated_items = {}
for item in items:
deduplicated_items[item.id] = item
for item in deduplicated_items.values():
# Now we want a FOR UPDATE lock on the item.users rows
# to prevent a concurrent transaction from mutating them
# simultaneously.
item.users.all().select_for_update()
item.users.remove(*user_ids)
item.users.remove(user_id)
if item.users.all().count() == 0:
# Due to our transaction holding the row lock we have a guarantee
# that the obtained COUNT is accurate, thus we can reliably use it

View File

@@ -1392,7 +1392,7 @@ class ActivateTest(ZulipTestCase):
assert email is not None and email.users is not None
self.assertEqual(email.users.count(), 2)
def test_clear_scheduled_emails_with_multiple_user_ids(self) -> None:
def test_clear_schedule_emails(self) -> None:
hamlet = self.example_user("hamlet")
iago = self.example_user("iago")
send_future_email(
@@ -1402,20 +1402,7 @@ class ActivateTest(ZulipTestCase):
delay=datetime.timedelta(hours=1),
)
self.assertEqual(ScheduledEmail.objects.count(), 1)
clear_scheduled_emails([hamlet.id, iago.id])
self.assertEqual(ScheduledEmail.objects.count(), 0)
def test_clear_schedule_emails_with_one_user_id(self) -> None:
hamlet = self.example_user("hamlet")
iago = self.example_user("iago")
send_future_email(
"zerver/emails/followup_day1",
iago.realm,
to_user_ids=[hamlet.id, iago.id],
delay=datetime.timedelta(hours=1),
)
self.assertEqual(ScheduledEmail.objects.count(), 1)
clear_scheduled_emails([hamlet.id])
clear_scheduled_emails(hamlet.id)
self.assertEqual(ScheduledEmail.objects.count(), 1)
self.assertEqual(ScheduledEmail.objects.filter(users=hamlet).count(), 0)
self.assertEqual(ScheduledEmail.objects.filter(users=iago).count(), 1)

View File

@@ -39,7 +39,7 @@ def do_missedmessage_unsubscribe(user_profile: UserProfile) -> None:
def do_welcome_unsubscribe(user_profile: UserProfile) -> None:
clear_scheduled_emails([user_profile.id], ScheduledEmail.WELCOME)
clear_scheduled_emails(user_profile.id, ScheduledEmail.WELCOME)
def do_digest_unsubscribe(user_profile: UserProfile) -> None: