From e9cf2659cf34ff08f182f85e37b0d6b832af08f3 Mon Sep 17 00:00:00 2001 From: Kartik Srivastava Date: Wed, 21 Sep 2022 19:21:48 +0530 Subject: [PATCH] user_topics: Refactor remove_topic_mute. This commit replaces 'remove_topic_mute' with 'set_user_topic_visibility_policy_in_database' and updates it to delete UserTopic row with any configured visibility_policy and not just muting. --- zerver/actions/user_topics.py | 7 ++++--- zerver/lib/user_topics.py | 22 +++++++++++----------- zerver/tests/test_user_topics.py | 13 ++++++++++--- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/zerver/actions/user_topics.py b/zerver/actions/user_topics.py index a8cadbea03..bb60d1e22e 100644 --- a/zerver/actions/user_topics.py +++ b/zerver/actions/user_topics.py @@ -8,7 +8,6 @@ from zerver.lib.exceptions import JsonableError from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.user_topics import ( get_topic_mutes, - remove_topic_mute, set_user_topic_visibility_policy_in_database, ) from zerver.models import Stream, UserProfile, UserTopic @@ -30,9 +29,11 @@ def do_set_user_topic_visibility_policy( if visibility_policy == UserTopic.VISIBILITY_POLICY_INHERIT: try: - remove_topic_mute(user_profile, stream.id, topic) + set_user_topic_visibility_policy_in_database( + user_profile, stream.id, topic, visibility_policy=visibility_policy + ) except UserTopic.DoesNotExist: - raise JsonableError(_("Topic is not muted")) + raise JsonableError(_("Nothing to be done")) else: assert stream.recipient_id is not None set_user_topic_visibility_policy_in_database( diff --git a/zerver/lib/user_topics.py b/zerver/lib/user_topics.py index b683f6492e..44c77b96cf 100644 --- a/zerver/lib/user_topics.py +++ b/zerver/lib/user_topics.py @@ -115,10 +115,20 @@ def set_user_topic_visibility_policy_in_database( topic_name: str, *, visibility_policy: int, - recipient_id: int, + recipient_id: Optional[int] = None, last_updated: Optional[datetime.datetime] = None, ignore_duplicate: bool = False, ) -> None: + if visibility_policy == UserTopic.VISIBILITY_POLICY_INHERIT: + # Will throw UserTopic.DoesNotExist if the user doesn't + # already have a visibility policy for this topic. + UserTopic.objects.get( + user_profile=user_profile, + stream_id=stream_id, + topic_name__iexact=topic_name, + ).delete() + return + assert last_updated is not None (row, created) = UserTopic.objects.get_or_create( user_profile=user_profile, @@ -155,16 +165,6 @@ def set_user_topic_visibility_policy_in_database( row.save(update_fields=["visibility_policy", "last_updated"]) -def remove_topic_mute(user_profile: UserProfile, stream_id: int, topic_name: str) -> None: - row = UserTopic.objects.get( - user_profile=user_profile, - stream_id=stream_id, - topic_name__iexact=topic_name, - visibility_policy=UserTopic.MUTED, - ) - row.delete() - - def topic_is_muted(user_profile: UserProfile, stream_id: int, topic_name: str) -> bool: is_muted = UserTopic.objects.filter( user_profile=user_profile, diff --git a/zerver/tests/test_user_topics.py b/zerver/tests/test_user_topics.py index 38e05e13a1..2377ad2099 100644 --- a/zerver/tests/test_user_topics.py +++ b/zerver/tests/test_user_topics.py @@ -2,6 +2,7 @@ from datetime import datetime, timezone from typing import Any, Dict, List from unittest import mock +from django.db import transaction from django.utils.timezone import now as timezone_now from zerver.actions.user_topics import do_set_user_topic_visibility_policy @@ -184,9 +185,15 @@ class MutedTopicsTests(ZulipTestCase): result = self.api_patch(user, url, data) self.assert_json_error(result, "Topic is not muted") - data = {"stream": stream.name, "topic": "BOGUS", "op": "remove"} - result = self.api_patch(user, url, data) - self.assert_json_error(result, "Topic is not muted") + with transaction.atomic(): + # This API call needs a new nested transaction with 'savepoint=True', + # because it calls 'set_user_topic_visibility_policy_in_database', + # which on failure rollbacks the test-transaction. + # If it is not used, the test-transaction will be rolled back during this API call, + # and the next API call will result in a "TransactionManagementError." + data = {"stream": stream.name, "topic": "BOGUS", "op": "remove"} + result = self.api_patch(user, url, data) + self.assert_json_error(result, "Nothing to be done") data = {"stream_id": 999999999, "topic": "BOGUS", "op": "remove"} result = self.api_patch(user, url, data)