From a096f34cab75166685c428dfd3c0eae8c4b86db3 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Wed, 3 Jun 2020 20:14:57 +0530 Subject: [PATCH] move_topic_to_stream: Add option to disable breadcrumb messages. Option to disable breadcrumb messages were given in both message edit form and topic edit stream popover. User now has the option to select which stream to send the notification of stream edit of a topic via checkboxes in the UI. --- static/js/message_edit.js | 24 ++++++++- static/js/stream_popover.js | 15 ++++-- static/styles/zulip.scss | 25 +++++++++ static/templates/message_edit_form.hbs | 12 +++++ static/templates/move_topic_to_stream.hbs | 12 +++++ templates/zerver/api/changelog.md | 5 ++ version.py | 2 +- zerver/lib/actions.py | 31 ++++++----- zerver/openapi/zulip.yaml | 26 ++++++++- zerver/tests/test_events.py | 2 +- zerver/tests/test_messages.py | 66 ++++++++++++++++++++++- zerver/views/messages.py | 4 ++ 12 files changed, 202 insertions(+), 22 deletions(-) diff --git a/static/js/message_edit.js b/static/js/message_edit.js index 5df0ad5f75..150503433e 100644 --- a/static/js/message_edit.js +++ b/static/js/message_edit.js @@ -6,6 +6,12 @@ const currently_editing_messages = new Map(); let currently_deleting_messages = []; const currently_echoing_messages = new Map(); +// These variables are designed to preserve the user's most recent +// choices when editing a group of messages, to make it convenient to +// move several topics in a row with the same settings. +exports.notify_old_thread_default = true; +exports.notify_new_thread_default = true; + const editability_types = { NO: 1, NO_LONGER: 2, @@ -283,6 +289,8 @@ function edit_message(row, raw_content) { available_streams: available_streams, stream_id: message.stream_id, stream_name: message.stream, + notify_new_thread: exports.notify_new_thread_default, + notify_old_thread: exports.notify_old_thread_default, })); const edit_obj = {form: form, raw_content: raw_content}; @@ -298,6 +306,7 @@ function edit_message(row, raw_content) { const message_edit_content = row.find('textarea.message_edit_content'); const message_edit_topic = row.find('input.message_edit_topic'); const message_edit_topic_propagate = row.find('select.message_edit_topic_propagate'); + const message_edit_breadcrumb_messages = row.find('div.message_edit_breadcrumb_messages'); const message_edit_countdown_timer = row.find('.message_edit_countdown_timer'); const copy_message = row.find('.copy_message'); @@ -380,6 +389,7 @@ function edit_message(row, raw_content) { if (message.type === 'stream') { message_edit_topic.prop("readonly", "readonly"); message_edit_topic_propagate.hide(); + message_edit_breadcrumb_messages.hide(); } // We don't go directly to a "TOPIC_ONLY" type state (with an active Save button), // since it isn't clear what to do with the half-finished edit. It's nice to keep @@ -424,6 +434,7 @@ function edit_message(row, raw_content) { const is_topic_edited = new_topic !== original_topic && new_topic !== ""; const is_stream_edited = new_stream_id !== original_stream_id; message_edit_topic_propagate.toggle(is_topic_edited || is_stream_edited); + message_edit_breadcrumb_messages.toggle(is_stream_edited); } if (!message.locally_echoed) { @@ -628,7 +639,13 @@ exports.save_message_row_edit = function (row) { if (topic_changed || stream_changed) { const selected_topic_propagation = row.find("select.message_edit_topic_propagate").val() || "change_later"; + const send_notification_to_old_thread = row.find('.send_notification_to_old_thread').is(':checked'); + const send_notification_to_new_thread = row.find('.send_notification_to_new_thread').is(':checked'); request.propagate_mode = selected_topic_propagation; + request.send_notification_to_old_thread = send_notification_to_old_thread; + request.send_notification_to_new_thread = send_notification_to_new_thread; + exports.notify_old_thread_default = send_notification_to_old_thread; + exports.notify_new_thread_default = send_notification_to_new_thread; changed = true; } @@ -904,12 +921,17 @@ exports.handle_narrow_deactivated = function () { }; exports.move_topic_containing_message_to_stream = - function (message_id, new_stream_id, new_topic_name) { + function (message_id, new_stream_id, new_topic_name, send_notification_to_new_thread, + send_notification_to_old_thread) { const request = { stream_id: new_stream_id, propagate_mode: 'change_all', topic: new_topic_name, + send_notification_to_old_thread: send_notification_to_old_thread, + send_notification_to_new_thread: send_notification_to_new_thread, }; + exports.notify_old_thread_default = send_notification_to_old_thread; + exports.notify_new_thread_default = send_notification_to_new_thread; channel.patch({ url: '/json/messages/' + message_id, data: request, diff --git a/static/js/stream_popover.js b/static/js/stream_popover.js index 1454092496..d95a4562bc 100644 --- a/static/js/stream_popover.js +++ b/static/js/stream_popover.js @@ -273,7 +273,11 @@ function build_move_topic_to_stream_popover(e, current_stream_id, topic_name) { // streams in the future. const available_streams = stream_data.subscribed_subs() .filter(s => s.stream_id !== current_stream_id); - const args = { available_streams, topic_name, current_stream_id }; + const args = { + available_streams, topic_name, current_stream_id, + notify_new_thread: message_edit.notify_new_thread_default, + notify_old_thread: message_edit.notify_old_thread_default, + }; exports.hide_topic_popover(); @@ -551,8 +555,11 @@ exports.register_topic_handlers = function () { }, {}); const {old_topic_name, select_stream_id} = params; - let {current_stream_id, new_topic_name} = params; + let {current_stream_id, new_topic_name, + send_notification_to_new_thread, send_notification_to_old_thread} = params; new_topic_name = new_topic_name.trim(); + send_notification_to_new_thread = send_notification_to_new_thread === 'on'; + send_notification_to_old_thread = send_notification_to_old_thread === 'on'; current_stream_id = parseInt(current_stream_id, 10); // The API endpoint for editing messages to change their @@ -594,7 +601,9 @@ exports.register_topic_handlers = function () { if (old_topic_name && select_stream_id) { message_edit.move_topic_containing_message_to_stream( - message_id, select_stream_id, new_topic_name); + message_id, select_stream_id, new_topic_name, + send_notification_to_new_thread, + send_notification_to_old_thread); $('#move_topic_modal').modal('hide'); } }, diff --git a/static/styles/zulip.scss b/static/styles/zulip.scss index 7cbef11bd5..b22e0c224b 100644 --- a/static/styles/zulip.scss +++ b/static/styles/zulip.scss @@ -1358,6 +1358,31 @@ div.focused_table { max-width: 100%; } +.topic_move_breadcrumb_messages, +.message_edit_breadcrumb_messages { + display: inline-block; + margin: 0 5px 5px 0 !important; + align-self: center; + + label.checkbox { + margin-right: 0 !important; + + input { + margin: 0; + vertical-align: baseline; + } + } + + label { + display: inline-block; + margin-right: 10px; + } +} + +.topic_move_breadcrumb_messages { + margin-top: 10px !important; +} + .message_length_controller { display: none; text-align: center; diff --git a/static/templates/message_edit_form.hbs b/static/templates/message_edit_form.hbs index ef3bf33fd6..d8e689e6af 100644 --- a/static/templates/message_edit_form.hbs +++ b/static/templates/message_edit_form.hbs @@ -18,6 +18,18 @@ + +
+ + + + +
diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 80677e36d4..da3850956f 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,8 @@ below features are supported. ## Changes in Zulip 2.2 +**Feature level 10** + **Feature level 9** * [`POST users/me/subscriptions`](/api/add-subscriptions), [`DELETE @@ -17,6 +19,9 @@ below features are supported. subscribe/unsubscribe, declared in the `principals` parameter, can now be referenced by user_id, rather than Zulip display email address. +* [PATCH /messages/{message_id}](/api/update-message): Added + `send_notification_to_old_thread` and + `send_notification_to_new_thread` optional parameters. **Feature level 8** diff --git a/version.py b/version.py index 9296cc36c1..fe2531737d 100644 --- a/version.py +++ b/version.py @@ -29,7 +29,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 8 +API_FEATURE_LEVEL = 9 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index e75369a697..86f350e1ac 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -4188,7 +4188,9 @@ class MessageUpdateUserInfoResult(TypedDict): def notify_topic_moved_streams(user_profile: UserProfile, old_stream: Stream, old_topic: str, - new_stream: Stream, new_topic: Optional[str]) -> None: + new_stream: Stream, new_topic: Optional[str], + send_notification_to_old_thread: bool, + send_notification_to_new_thread: bool) -> None: # Since moving content between streams is highly disruptive, # it's worth adding a couple tombstone messages showing what # happened. @@ -4200,17 +4202,18 @@ def notify_topic_moved_streams(user_profile: UserProfile, user_mention = "@_**%s|%s**" % (user_profile.full_name, user_profile.id) old_topic_link = "#**%s>%s**" % (old_stream.name, old_topic) new_topic_link = "#**%s>%s**" % (new_stream.name, new_topic) + if send_notification_to_new_thread: + internal_send_stream_message( + new_stream.realm, sender, new_stream, new_topic, + _("This topic was moved here from %(old_location)s by %(user)s") + % dict(old_location=old_topic_link, user=user_mention)) - internal_send_stream_message( - new_stream.realm, sender, new_stream, new_topic, - _("This topic was moved here from %(old_location)s by %(user)s") - % dict(old_location=old_topic_link, user=user_mention)) - - # Send a notification to the old stream that the topic was moved. - internal_send_stream_message( - old_stream.realm, sender, old_stream, old_topic, - _("This topic was moved by %(user)s to %(new_location)s") - % dict(user=user_mention, new_location=new_topic_link)) + if send_notification_to_old_thread: + # Send a notification to the old stream that the topic was moved. + internal_send_stream_message( + old_stream.realm, sender, old_stream, old_topic, + _("This topic was moved by %(user)s to %(new_location)s") + % dict(user=user_mention, new_location=new_topic_link)) def get_user_info_for_message_updates(message_id: int) -> MessageUpdateUserInfoResult: @@ -4322,7 +4325,8 @@ def do_update_embedded_data(user_profile: UserProfile, @transaction.atomic def do_update_message(user_profile: UserProfile, message: Message, new_stream: Optional[Stream], topic_name: Optional[str], - propagate_mode: str, content: Optional[str], + propagate_mode: str, send_notification_to_old_thread: bool, + send_notification_to_new_thread: bool, content: Optional[str], rendered_content: Optional[str], prior_mention_user_ids: Set[int], mention_user_ids: Set[int], mention_data: Optional[bugdown.MentionData]=None) -> int: """ @@ -4527,7 +4531,8 @@ def do_update_message(user_profile: UserProfile, message: Message, stream_being_edited is not None): # Notify users that the topic was moved. notify_topic_moved_streams(user_profile, stream_being_edited, orig_topic_name, - new_stream, topic_name) + new_stream, topic_name, send_notification_to_old_thread, + send_notification_to_new_thread) return len(changed_messages) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 22d080cd51..2587f3f943 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -898,9 +898,9 @@ paths: - name: propagate_mode in: query description: | - "Which message(s) should be edited: just the one indicated in + Which message(s) should be edited: just the one indicated in `message_id`, messages in the same topic that had been sent after this - one, or all of them." + one, or all of them. schema: type: string enum: @@ -909,6 +909,28 @@ paths: - change_all default: change_one example: change_all + - name: send_notification_to_old_thread + in: query + description: | + Whether to send breadcrumb message to the old thread to + notify users where the messages were moved to. + + **Changes**: New in Zulip 2.2 (feature level 9). + schema: + type: boolean + default: true + example: true + - name: send_notification_to_new_thread + in: query + description: | + Whether to send a notification message to the new thread to + notify users where the messages came from. + + **Changes**: New in Zulip 2.2 (feature level 9). + schema: + type: boolean + default: true + example: true - $ref: '#/components/parameters/Content' - $ref: '#/components/parameters/StreamIdInQuery' responses: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 003f5c3968..757b51cce0 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -745,7 +745,7 @@ class EventsRegisterTest(ZulipTestCase): events = self.do_test( lambda: do_update_message(self.user_profile, message, None, topic, - propagate_mode, content, rendered_content, + propagate_mode, False, False, content, rendered_content, prior_mention_user_ids, mentioned_user_ids, mention_data), state_change_expected=True, diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index 5ae5760a21..0a480a7d8d 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -3264,6 +3264,8 @@ class EditMessageTest(ZulipTestCase): new_stream=None, topic_name=topic_name, propagate_mode="change_later", + send_notification_to_old_thread=False, + send_notification_to_new_thread=False, content=None, rendered_content=None, prior_mention_user_ids=set(), @@ -3540,6 +3542,68 @@ class EditMessageTest(ZulipTestCase): self.assertEqual(messages[3].content, "This topic was moved here from #**test move stream>test** by @_**Iago|%s**" % (user_profile.id,)) self.assert_json_success(result) + def test_no_notify_move_message_to_stream(self) -> None: + (user_profile, old_stream, new_stream, msg_id, msg_id_lt) = self.prepare_move_topics( + "iago", "test move stream", "new stream", "test") + + result = self.client_patch("/json/messages/" + str(msg_id), { + 'message_id': msg_id, + 'stream_id': new_stream.id, + 'propagate_mode': 'change_all', + 'send_notification_to_old_thread': 'false', + 'send_notification_to_new_thread': 'false', + }) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, old_stream, "test") + self.assertEqual(len(messages), 0) + + messages = get_topic_messages(user_profile, new_stream, "test") + self.assertEqual(len(messages), 3) + + def test_notify_new_thread_move_message_to_stream(self) -> None: + (user_profile, old_stream, new_stream, msg_id, msg_id_lt) = self.prepare_move_topics( + "iago", "test move stream", "new stream", "test") + + result = self.client_patch("/json/messages/" + str(msg_id), { + 'message_id': msg_id, + 'stream_id': new_stream.id, + 'propagate_mode': 'change_all', + 'send_notification_to_old_thread': 'false', + 'send_notification_to_new_thread': 'true', + }) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, old_stream, "test") + self.assertEqual(len(messages), 0) + + messages = get_topic_messages(user_profile, new_stream, "test") + self.assertEqual(len(messages), 4) + self.assertEqual(messages[3].content, "This topic was moved here from #**test move stream>test** by @_**Iago|%d**" % (user_profile.id,)) + + def test_notify_old_thread_move_message_to_stream(self) -> None: + (user_profile, old_stream, new_stream, msg_id, msg_id_lt) = self.prepare_move_topics( + "iago", "test move stream", "new stream", "test") + + result = self.client_patch("/json/messages/" + str(msg_id), { + 'message_id': msg_id, + 'stream_id': new_stream.id, + 'propagate_mode': 'change_all', + 'send_notification_to_old_thread': 'true', + 'send_notification_to_new_thread': 'false', + }) + + self.assert_json_success(result) + + messages = get_topic_messages(user_profile, old_stream, "test") + self.assertEqual(len(messages), 1) + self.assertEqual(messages[0].content, "This topic was moved by @_**Iago|%s** to #**new stream>test**" % (user_profile.id,)) + + messages = get_topic_messages(user_profile, new_stream, "test") + self.assertEqual(len(messages), 3) + def test_move_message_to_stream_to_private_stream(self) -> None: user_profile = self.example_user("iago") self.login("iago") @@ -4152,7 +4216,7 @@ class MessageHasKeywordsTest(ZulipTestCase): realm_id = hamlet.realm.id rendered_content = render_markdown(msg, content) mention_data = bugdown.MentionData(realm_id, content) - do_update_message(hamlet, msg, None, None, "change_one", content, + do_update_message(hamlet, msg, None, None, "change_one", False, False, content, rendered_content, set(), set(), mention_data=mention_data) def test_finds_link_after_edit(self) -> None: diff --git a/zerver/views/messages.py b/zerver/views/messages.py index 14979772c1..58620413b9 100644 --- a/zerver/views/messages.py +++ b/zerver/views/messages.py @@ -1503,6 +1503,8 @@ def update_message_backend(request: HttpRequest, user_profile: UserMessage, propagate_mode: Optional[str]=REQ( default="change_one", str_validator=check_string_in(PROPAGATE_MODE_VALUES)), + send_notification_to_old_thread: bool=REQ(default=True, validator=check_bool), + send_notification_to_new_thread: bool=REQ(default=True, validator=check_bool), content: Optional[str]=REQ(default=None)) -> HttpResponse: if not user_profile.realm.allow_message_editing: return json_error(_("Your organization has turned off message editing")) @@ -1607,6 +1609,8 @@ def update_message_backend(request: HttpRequest, user_profile: UserMessage, number_changed = do_update_message(user_profile, message, new_stream, topic_name, propagate_mode, + send_notification_to_old_thread, + send_notification_to_new_thread, content, rendered_content, prior_mention_user_ids, mention_user_ids, mention_data)