mirror of https://github.com/zulip/zulip.git
message_edit: Only move muted topic records when moving whole topics.
Our original implementation of moving muted topic records when a topic is moved took a shortcut of treating all change_later usage as something with intent to move the whole topic. This works OK when moving the whole topic via this interface, but not when moving a last off-topic message in the topic. Address this by changing the rule to match the existing moved_all_visible_messages variable.
This commit is contained in:
Tim Abbott
Tim Abbott
parent
219e213c60
commit
f9f111f950
|
|
@ -6,8 +6,8 @@ Organizations can [configure][move-permission-setting] which
|
|||
streams.
|
||||
|
||||
To help others find moved content, you can have Zulip send automated
|
||||
notification messages to the source topic, the destination topic, or both. These
|
||||
notifications include:
|
||||
notification messages to the source topic, the destination topic, or
|
||||
both. These notifications include:
|
||||
|
||||
* A link to the source or destination topic.
|
||||
* How many messages were moved, or whether the whole topic was moved.
|
||||
|
|
@ -82,6 +82,24 @@ will only be accessible to users who both:
|
|||
* Were subscribed to the *original* stream when the content was *sent*.
|
||||
* Are subscribed to the *destination* stream when the content is *moved*.
|
||||
|
||||
## Moving content out of private streams
|
||||
|
||||
In [private streams with protected history](/help/stream-permissions),
|
||||
Zulip determines whether to treat the entire topic as moved using the
|
||||
access permissions of the user requesting the topic move. This means
|
||||
that the automated notifications will report that the entire topic was
|
||||
moved if the requesting user moved every message in the topic that
|
||||
they can access, regardless of whether older messages exist that
|
||||
they cannot access.
|
||||
|
||||
Similarly, [muted topics](/help/mute-a-topic) will be migrated to the
|
||||
new stream and topic if the requesting user moved every message in the
|
||||
topic that they can access.
|
||||
|
||||
This model ensures that the topic editing feature cannot be abused to
|
||||
determine any information about the existence of messages or topics
|
||||
that one does not have permission to access.
|
||||
|
||||
## Related articles
|
||||
|
||||
* [Rename a topic](/help/rename-a-topic)
|
||||
|
|
|
|||
|
|
@ -8,8 +8,8 @@ topic](/help/rename-a-topic).
|
|||
When messages are moved, Zulip's [permanent links to messages in
|
||||
context](/help/link-to-a-message-or-conversation#get-a-link-to-a-specific-message)
|
||||
will automatically redirect to the new location of the message. [Muted
|
||||
topics](/help/mute-a-topic) are automatically migrated when all messages after a
|
||||
certain point are moved, or an entire topic is moved.
|
||||
topics](/help/mute-a-topic) are automatically migrated when an entire
|
||||
topic is moved.
|
||||
|
||||
Organizations can [configure](/help/configure-who-can-edit-topics) which
|
||||
[roles](/help/roles-and-permissions) have permission to modify topics. See the
|
||||
|
|
|
|||
|
|
@ -7166,64 +7166,12 @@ def do_update_message(
|
|||
|
||||
users_to_be_notified += list(map(subscriber_info, sorted(list(subscriber_ids))))
|
||||
|
||||
# Migrate muted topic configuration in the following circumstances:
|
||||
#
|
||||
# * If propagate_mode is change_all, do so unconditionally.
|
||||
#
|
||||
# * If propagate_mode is change_later, it's likely that we want to
|
||||
# move these only when it appears that the intent is to move
|
||||
# most of the topic, not just the last 1-2 messages which may
|
||||
# have been "off topic". At present we do so unconditionally.
|
||||
#
|
||||
# * Never move muted topic configuration with change_one.
|
||||
#
|
||||
# We may want more complex behavior in cases where one appears to
|
||||
# be merging topics (E.g. there are existing messages in the
|
||||
# target topic).
|
||||
#
|
||||
# Moving a topic to another stream is complicated in that we want
|
||||
# to avoid creating a UserTopic row for the user in a stream that
|
||||
# they don't have access to; doing so could leak information about
|
||||
# the existence of a private stream to some users. See the
|
||||
# moved_all_visible_messages below for related details.
|
||||
#
|
||||
# So for now, we require new_stream=None for this feature.
|
||||
if propagate_mode != "change_one" and (topic_name is not None or new_stream is not None):
|
||||
# UserTopic updates and the content of notifications depend on
|
||||
# whether we've moved the entire topic, or just part of it. We
|
||||
# make that determination here.
|
||||
moved_all_visible_messages = False
|
||||
if topic_name is not None or new_stream is not None:
|
||||
assert stream_being_edited is not None
|
||||
for muting_user in get_users_muting_topic(stream_being_edited.id, orig_topic_name):
|
||||
# TODO: Ideally, this would be a bulk update operation,
|
||||
# because we are doing database operations in a loop here.
|
||||
#
|
||||
# This loop is only acceptable in production because it is
|
||||
# rare for more than a few users to have muted an
|
||||
# individual topic that is being moved; as of this
|
||||
# writing, no individual topic in Zulip Cloud had been
|
||||
# muted by more than 100 users.
|
||||
|
||||
if new_stream is not None and muting_user.id in delete_event_notify_user_ids:
|
||||
# If the messages are being moved to a stream the user
|
||||
# cannot access, then we treat this as the
|
||||
# messages/topic being deleted for this user. Unmute
|
||||
# the topic for such users.
|
||||
do_unmute_topic(muting_user, stream_being_edited, orig_topic_name)
|
||||
else:
|
||||
# Otherwise, we move the muted topic record for the user.
|
||||
# We call remove_topic_mute rather than do_unmute_topic to
|
||||
# avoid sending two events with new muted topics in
|
||||
# immediate succession; this is correct only because
|
||||
# muted_topics events always send the full set of topics.
|
||||
remove_topic_mute(muting_user, stream_being_edited.id, orig_topic_name)
|
||||
do_mute_topic(
|
||||
muting_user,
|
||||
new_stream if new_stream is not None else stream_being_edited,
|
||||
topic_name if topic_name is not None else orig_topic_name,
|
||||
)
|
||||
|
||||
send_event(user_profile.realm, event, users_to_be_notified)
|
||||
|
||||
if len(changed_messages) > 0 and new_stream is not None and stream_being_edited is not None:
|
||||
# Notify users that the topic was moved.
|
||||
changed_messages_count = len(changed_messages)
|
||||
|
||||
if propagate_mode == "change_all":
|
||||
moved_all_visible_messages = True
|
||||
|
|
@ -7253,6 +7201,59 @@ def do_update_message(
|
|||
)
|
||||
moved_all_visible_messages = len(visible_unmoved_messages) == 0
|
||||
|
||||
# Migrate muted topic configuration in the following circumstances:
|
||||
#
|
||||
# * If propagate_mode is change_all, do so unconditionally.
|
||||
#
|
||||
# * If propagate_mode is change_later or change_one, do so when
|
||||
# the acting user has moved the entire topic (as visible to them).
|
||||
#
|
||||
# This rule corresponds to checking moved_all_visible_messages.
|
||||
#
|
||||
# We may want more complex behavior in cases where one appears to
|
||||
# be merging topics (E.g. there are existing messages in the
|
||||
# target topic).
|
||||
if moved_all_visible_messages:
|
||||
assert stream_being_edited is not None
|
||||
assert topic_name is not None or new_stream is not None
|
||||
|
||||
for muting_user in get_users_muting_topic(stream_being_edited.id, orig_topic_name):
|
||||
# TODO: Ideally, this would be a bulk update operation,
|
||||
# because we are doing database operations in a loop here.
|
||||
#
|
||||
# This loop is only acceptable in production because it is
|
||||
# rare for more than a few users to have muted an
|
||||
# individual topic that is being moved; as of this
|
||||
# writing, no individual topic in Zulip Cloud had been
|
||||
# muted by more than 100 users.
|
||||
|
||||
if new_stream is not None and muting_user.id in delete_event_notify_user_ids:
|
||||
# If the messages are being moved to a stream the user
|
||||
# cannot access, then we treat this as the
|
||||
# messages/topic being deleted for this user. This is
|
||||
# important for security reasons; we don't want to
|
||||
# give users a UserTopic row in a stream they cannot
|
||||
# access. Unmute the topic for such users.
|
||||
do_unmute_topic(muting_user, stream_being_edited, orig_topic_name)
|
||||
else:
|
||||
# Otherwise, we move the muted topic record for the user.
|
||||
# We call remove_topic_mute rather than do_unmute_topic to
|
||||
# avoid sending two events with new muted topics in
|
||||
# immediate succession; this is correct only because
|
||||
# muted_topics events always send the full set of topics.
|
||||
remove_topic_mute(muting_user, stream_being_edited.id, orig_topic_name)
|
||||
do_mute_topic(
|
||||
muting_user,
|
||||
new_stream if new_stream is not None else stream_being_edited,
|
||||
topic_name if topic_name is not None else orig_topic_name,
|
||||
)
|
||||
|
||||
send_event(user_profile.realm, event, users_to_be_notified)
|
||||
|
||||
if len(changed_messages) > 0 and new_stream is not None and stream_being_edited is not None:
|
||||
# Notify users that the topic was moved.
|
||||
changed_messages_count = len(changed_messages)
|
||||
|
||||
old_thread_notification_string = None
|
||||
if send_notification_to_old_thread:
|
||||
if moved_all_visible_messages:
|
||||
|
|
|
|||
|
|
@ -1342,8 +1342,8 @@ class EditMessageTest(EditMessageTestCase):
|
|||
send_notification_to_new_thread=False,
|
||||
content=None,
|
||||
)
|
||||
self.assertFalse(topic_is_muted(hamlet, stream.id, change_one_topic_name))
|
||||
self.assertTrue(topic_is_muted(hamlet, stream.id, change_later_topic_name))
|
||||
self.assertTrue(topic_is_muted(hamlet, stream.id, change_one_topic_name))
|
||||
self.assertFalse(topic_is_muted(hamlet, stream.id, change_later_topic_name))
|
||||
|
||||
# Move topic between two public streams.
|
||||
desdemona = self.example_user("desdemona")
|
||||
|
|
@ -1445,6 +1445,30 @@ class EditMessageTest(EditMessageTestCase):
|
|||
self.assertTrue(topic_is_muted(cordelia, new_public_stream.id, "changed topic name"))
|
||||
self.assertFalse(topic_is_muted(aaron, new_public_stream.id, "changed topic name"))
|
||||
|
||||
# Moving only half the messages doesn't move MutedTopic records.
|
||||
second_message_id = self.send_stream_message(
|
||||
hamlet, stream_name, topic_name="changed topic name", content="Second message"
|
||||
)
|
||||
with queries_captured() as queries:
|
||||
check_update_message(
|
||||
user_profile=desdemona,
|
||||
message_id=second_message_id,
|
||||
stream_id=new_public_stream.id,
|
||||
topic_name="final topic name",
|
||||
propagate_mode="change_later",
|
||||
send_notification_to_old_thread=False,
|
||||
send_notification_to_new_thread=False,
|
||||
content=None,
|
||||
)
|
||||
self.assert_length(queries, 25)
|
||||
|
||||
self.assertTrue(topic_is_muted(desdemona, new_public_stream.id, "changed topic name"))
|
||||
self.assertTrue(topic_is_muted(cordelia, new_public_stream.id, "changed topic name"))
|
||||
self.assertFalse(topic_is_muted(aaron, new_public_stream.id, "changed topic name"))
|
||||
self.assertFalse(topic_is_muted(desdemona, new_public_stream.id, "final topic name"))
|
||||
self.assertFalse(topic_is_muted(cordelia, new_public_stream.id, "final topic name"))
|
||||
self.assertFalse(topic_is_muted(aaron, new_public_stream.id, "final topic name"))
|
||||
|
||||
@mock.patch("zerver.lib.actions.send_event")
|
||||
def test_wildcard_mention(self, mock_send_event: mock.MagicMock) -> None:
|
||||
stream_name = "Macbeth"
|
||||
|
|
|
|||
Loading…
Reference in New Issue