UserTopic: Set visibility_policy or delete UserTopic row without error.

This commit updates 'set_user_topic_visibility_policy_in_database'
to not raise an error when deleting a UserTopic row and the user
doesn't have a visibility_policy for the topic yet, or when setting
the visibility_policy to its current value.

Also, it includes the changes to not send unnecessary events
in such cases.

api_docs/changelog.md

@@ -20,6 +20,13 @@ format used by the Zulip server that they are interacting with.
 
 ## Changes in Zulip 7.0
 
+**Feature level 169**
+
+* [`PATCH /users/me/subscriptions/muted_topics`](/api/mute-topic):
+  Trying to mute a topic that is already muted or unmute a topic
+  that was not previously muted now results in a success response
+  rather than an error.
+
 **Feature level 168**
 
 * [`PATCH /realm/user_settings_defaults`](/api/update-realm-user-settings-defaults),

zerver/actions/message_edit.py

@@ -809,7 +809,6 @@ def do_update_message(
                     new_stream if new_stream is not None else stream_being_edited,
                     topic_name if topic_name is not None else orig_topic_name,
                     visibility_policy=UserTopic.VisibilityPolicy.MUTED,
-                    ignore_duplicate=True,
                 )
 
     send_event(user_profile.realm, event, users_to_be_notified)

zerver/actions/user_topics.py

@@ -19,22 +19,26 @@ def do_set_user_topic_visibility_policy(
     *,
     visibility_policy: int,
     last_updated: Optional[datetime.datetime] = None,
-    ignore_duplicate: bool = False,
     skip_muted_topics_event: bool = False,
 ) -> None:
     if last_updated is None:
         last_updated = timezone_now()
 
-    set_user_topic_visibility_policy_in_database(
+    database_changed = set_user_topic_visibility_policy_in_database(
         user_profile,
         stream.id,
         topic,
         visibility_policy=visibility_policy,
         recipient_id=stream.recipient_id,
         last_updated=last_updated,
-        ignore_duplicate=ignore_duplicate,
     )
 
+    # Requests to set the visibility_policy to its current value
+    # or to delete a UserTopic row that doesn't exist shouldn't
+    # send an unnecessary event.
+    if not database_changed:
+        return
+
     # This first muted_topics event is deprecated and will be removed
     # once clients are migrated to handle the user_topic event type
     # instead.

zerver/lib/user_topics.py

@@ -1,14 +1,13 @@
 import datetime
-from typing import Callable, Dict, List, Optional, Tuple, TypedDict
+import logging
+from typing import Callable, List, Optional, Tuple, TypedDict
 
 from django.db import transaction
 from django.db.models import QuerySet
 from django.utils.timezone import now as timezone_now
-from django.utils.translation import gettext as _
 from sqlalchemy.sql import ClauseElement, and_, column, not_, or_
 from sqlalchemy.types import Integer
 
-from zerver.lib.exceptions import JsonableError
 from zerver.lib.timestamp import datetime_to_timestamp
 from zerver.lib.topic import topic_match_sa
 from zerver.lib.types import UserTopicDict
@@ -117,20 +116,22 @@ def set_user_topic_visibility_policy_in_database(
     visibility_policy: int,
     recipient_id: Optional[int] = None,
     last_updated: Optional[datetime.datetime] = None,
-    ignore_duplicate: bool = False,
+) -> bool:
-) -> None:
+    # returns True if it modifies the database and False otherwise.
     if visibility_policy == UserTopic.VisibilityPolicy.INHERIT:
-        try:
+        (num_deleted, ignored) = UserTopic.objects.filter(
-            # 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
+        if num_deleted == 0:
-        except UserTopic.DoesNotExist:
+            # If the user doesn't already have a visibility_policy for this topic
-            raise JsonableError(_("Nothing to be done"))
+            logging.info(
+                "User %s tried to remove visibility_policy, which actually doesn't exist",
+                user_profile.id,
+            )
+            return False
+        return True
 
     assert last_updated is not None
     assert recipient_id is not None
@@ -147,26 +148,23 @@ def set_user_topic_visibility_policy_in_database(
     )
 
     if created:
-        return
+        return True
 
     duplicate_request: bool = row.visibility_policy == visibility_policy
 
-    if duplicate_request and ignore_duplicate:
+    if duplicate_request:
-        return
+        logging.info(
-
+            "User %s tried to set visibility_policy to its current value of %s",
-    if duplicate_request and not ignore_duplicate:
+            user_profile.id,
-        visibility_policy_string: Dict[int, str] = {
+            visibility_policy,
-            1: "muted",
-            2: "unmuted",
-            3: "followed",
-        }
-        raise JsonableError(
-            _("Topic already {}").format(visibility_policy_string[visibility_policy])
         )
+        return False
+
     # The request is to just 'update' the visibility policy of a topic
     row.visibility_policy = visibility_policy
     row.last_updated = last_updated
     row.save(update_fields=["visibility_policy", "last_updated"])
+    return True
 
 
 def topic_is_muted(user_profile: UserProfile, stream_id: int, topic_name: str) -> bool:

zerver/openapi/zulip.yaml

@@ -8202,6 +8202,10 @@ paths:
         This endpoint mutes/unmutes a topic within a stream that the current
         user is subscribed to. Muted topics are displayed faded in the Zulip
         UI, and are not included in the user's unread count totals.
+
+        **Changes**: Before Zulip 7.0 (feature level 169), this endpoint
+        returned an error if asked to mute a topic that was already muted
+        or asked to unmute a topic that had not previously been muted.
       x-curl-examples-parameters:
         oneOf:
           - type: exclude
@@ -8257,26 +8261,6 @@ paths:
       responses:
         "200":
           $ref: "#/components/responses/SimpleSuccess"
-        "400":
-          description: Bad request.
-          content:
-            application/json:
-              schema:
-                oneOf:
-                  - allOf:
-                      - $ref: "#/components/schemas/JsonError"
-                      - example:
-                          {"msg": "Topic already muted", "result": "error"}
-                        description: |
-                          An example JSON response for when an `add` operation is requested for a topic
-                          that has already been muted:
-                  - allOf:
-                      - $ref: "#/components/schemas/JsonError"
-                      - example:
-                          {"msg": "Topic is not muted", "result": "error"}
-                        description: |
-                          An example JSON response for when a `remove` operation is requested for a
-                          topic that had not been previously muted:
   /users/me/muted_users/{muted_user_id}:
     post:
       operationId: mute-user

zerver/tests/test_message_edit.py

@@ -35,6 +35,7 @@ from zerver.models import (
     Stream,
     UserMessage,
     UserProfile,
+    UserTopic,
     get_realm,
     get_stream,
 )
@@ -1338,7 +1339,7 @@ class EditMessageTest(EditMessageTestCase):
 
         # This code path adds 9 (1 + 4/user with muted topics) + 1 to
         # the number of database queries for moving a topic.
-        with self.assert_database_query_count(21):
+        with self.assert_database_query_count(19):
             check_update_message(
                 user_profile=hamlet,
                 message_id=message_id,
@@ -1381,6 +1382,7 @@ class EditMessageTest(EditMessageTestCase):
         self.assertTrue(topic_is_muted(hamlet, stream.id, change_later_topic_name))
 
         # Make sure we safely handle the case of the new topic being already muted.
+        with self.assertLogs(level="INFO") as info_logs:
             check_update_message(
                 user_profile=hamlet,
                 message_id=message_id,
@@ -1391,6 +1393,13 @@ class EditMessageTest(EditMessageTestCase):
                 send_notification_to_new_thread=False,
                 content=None,
             )
+        self.assertEqual(
+            set(info_logs.output),
+            {
+                f"INFO:root:User {hamlet.id} tried to set visibility_policy to its current value of {UserTopic.VisibilityPolicy.MUTED}",
+                f"INFO:root:User {cordelia.id} tried to set visibility_policy to its current value of {UserTopic.VisibilityPolicy.MUTED}",
+            },
+        )
         self.assertFalse(topic_is_muted(hamlet, stream.id, change_later_topic_name))
         self.assertTrue(topic_is_muted(hamlet, stream.id, already_muted_topic))
 
@@ -1422,7 +1431,7 @@ class EditMessageTest(EditMessageTestCase):
         set_topic_mutes(desdemona, muted_topics)
         set_topic_mutes(cordelia, muted_topics)
 
-        with self.assert_database_query_count(32):
+        with self.assert_database_query_count(30):
             check_update_message(
                 user_profile=desdemona,
                 message_id=message_id,
@@ -1453,7 +1462,7 @@ class EditMessageTest(EditMessageTestCase):
         set_topic_mutes(desdemona, muted_topics)
         set_topic_mutes(cordelia, muted_topics)
 
-        with self.assert_database_query_count(33):
+        with self.assert_database_query_count(31):
             check_update_message(
                 user_profile=desdemona,
                 message_id=message_id,
@@ -1486,7 +1495,7 @@ class EditMessageTest(EditMessageTestCase):
         set_topic_mutes(desdemona, muted_topics)
         set_topic_mutes(cordelia, muted_topics)
 
-        with self.assert_database_query_count(32):
+        with self.assert_database_query_count(30):
             check_update_message(
                 user_profile=desdemona,
                 message_id=message_id,

zerver/tests/test_user_topics.py

@@ -2,7 +2,6 @@ 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
@@ -120,11 +119,20 @@ class MutedTopicsTests(ZulipTestCase):
         assert stream.recipient is not None
         result = self.api_patch(user, url, data)
 
-        # Now check that error is raised when attempted to mute an already
+        # Now check that no error is raised when attempted to mute
-        # muted topic. This should be case-insensitive.
+        # an already muted topic. This should be case-insensitive.
+        user_topic_count = UserTopic.objects.count()
         data["topic"] = "VERONA3"
+        with self.assertLogs(level="INFO") as info_logs:
             result = self.api_patch(user, url, data)
-        self.assert_json_error(result, "Topic already muted")
+            self.assert_json_success(result)
+        self.assertEqual(
+            info_logs.output[0],
+            f"INFO:root:User {user.id} tried to set visibility_policy to its current value of {UserTopic.VisibilityPolicy.MUTED}",
+        )
+        # Verify that we didn't end up with duplicate UserTopic rows
+        # with the two different cases after the previous API call.
+        self.assertEqual(UserTopic.objects.count() - user_topic_count, 0)
 
     def test_remove_muted_topic(self) -> None:
         user = self.example_user("hamlet")
@@ -195,15 +203,16 @@ class MutedTopicsTests(ZulipTestCase):
         result = self.api_patch(user, url, data)
         self.assert_json_error(result, "Topic is not muted")
 
-        with transaction.atomic():
+        # Check that removing mute from a topic for which the user
-            # This API call needs a new nested transaction with 'savepoint=True',
+        # doesn't already have a visibility_policy doesn't cause an error.
-            # 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"}
+        with self.assertLogs(level="INFO") as info_logs:
             result = self.api_patch(user, url, data)
-            self.assert_json_error(result, "Nothing to be done")
+            self.assert_json_success(result)
+        self.assertEqual(
+            info_logs.output[0],
+            f"INFO:root:User {user.id} tried to remove visibility_policy, which actually doesn't exist",
+        )
 
         data = {"stream_id": 999999999, "topic": "BOGUS", "op": "remove"}
         result = self.api_patch(user, url, data)