From 042bbf29364b544997c79e194bd890d56879651e Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Tue, 14 Mar 2023 19:39:12 +0530 Subject: [PATCH] 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 | 7 ++++ zerver/actions/message_edit.py | 1 - zerver/actions/user_topics.py | 10 ++++-- zerver/lib/user_topics.py | 54 +++++++++++++++---------------- zerver/openapi/zulip.yaml | 24 +++----------- zerver/tests/test_message_edit.py | 35 ++++++++++++-------- zerver/tests/test_user_topics.py | 35 ++++++++++++-------- 7 files changed, 88 insertions(+), 78 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 51caae0b46..bcbc0bceb9 100644 --- a/api_docs/changelog.md +++ b/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), diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index cb97f4af52..6cc1da416f 100644 --- a/zerver/actions/message_edit.py +++ b/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) diff --git a/zerver/actions/user_topics.py b/zerver/actions/user_topics.py index 1e2cf58740..a7aa45712a 100644 --- a/zerver/actions/user_topics.py +++ b/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. diff --git a/zerver/lib/user_topics.py b/zerver/lib/user_topics.py index d873391de7..15ef8a227d 100644 --- a/zerver/lib/user_topics.py +++ b/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, -) -> None: +) -> bool: + # returns True if it modifies the database and False otherwise. if visibility_policy == UserTopic.VisibilityPolicy.INHERIT: - try: - # 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 - except UserTopic.DoesNotExist: - raise JsonableError(_("Nothing to be done")) + (num_deleted, ignored) = UserTopic.objects.filter( + user_profile=user_profile, + stream_id=stream_id, + topic_name__iexact=topic_name, + ).delete() + if num_deleted == 0: + # If the user doesn't already have a visibility_policy for this topic + 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: - return - - if duplicate_request and not ignore_duplicate: - visibility_policy_string: Dict[int, str] = { - 1: "muted", - 2: "unmuted", - 3: "followed", - } - raise JsonableError( - _("Topic already {}").format(visibility_policy_string[visibility_policy]) + if duplicate_request: + logging.info( + "User %s tried to set visibility_policy to its current value of %s", + user_profile.id, + 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: diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 244776d904..81f5fdaf29 100644 --- a/zerver/openapi/zulip.yaml +++ b/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 diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index e3b606fd46..f878c9d6fd 100644 --- a/zerver/tests/test_message_edit.py +++ b/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,15 +1382,23 @@ 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. - check_update_message( - user_profile=hamlet, - message_id=message_id, - stream_id=None, - topic_name=already_muted_topic, - propagate_mode="change_all", - send_notification_to_old_thread=False, - send_notification_to_new_thread=False, - content=None, + with self.assertLogs(level="INFO") as info_logs: + check_update_message( + user_profile=hamlet, + message_id=message_id, + stream_id=None, + topic_name=already_muted_topic, + propagate_mode="change_all", + send_notification_to_old_thread=False, + 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, diff --git a/zerver/tests/test_user_topics.py b/zerver/tests/test_user_topics.py index 8e33d9b139..096de6050f 100644 --- a/zerver/tests/test_user_topics.py +++ b/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 - # muted topic. This should be case-insensitive. + # Now check that no error is raised when attempted to mute + # an already muted topic. This should be case-insensitive. + user_topic_count = UserTopic.objects.count() data["topic"] = "VERONA3" - result = self.api_patch(user, url, data) - self.assert_json_error(result, "Topic already muted") + with self.assertLogs(level="INFO") as info_logs: + result = self.api_patch(user, url, data) + 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(): - # 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"} + # Check that removing mute from a topic for which the user + # doesn't already have a visibility_policy doesn't cause an error. + 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)