Moving topics between streams #13912

dobleuber · 2020-02-14T17:44:04Z

Feature Behaviour

timabbott · 2020-02-16T07:27:33Z

@dobleuber can you clean up the commit history? Check out the Zulip commit guidelines for more details: https://zulip.readthedocs.io/en/latest/contributing/version-control.html

timabbott · 2020-02-16T07:28:45Z

zerver/lib/actions.py

+    user_profile: UserProfile,
+    stream: Stream,
+    topic_name: str
+) -> List[Message]:


This line-wrapping doesn't match zulip's style. Please look at nearby code and follow our conventions.

timabbott · 2020-02-16T07:28:59Z

zerver/lib/actions.py

+        user_profile=user_profile
+    )
+
+    recipient = stream.recipient


This variable is unnecessary.

timabbott · 2020-02-16T07:29:46Z

zerver/lib/actions.py

+        topic_name=topic_name,
+    )
+
+    return messages


See the above, but also I think this whole function could be like 2 lines of reasonable code; you possibly should just have the caller call filter_by_topic_name_via_message.

I was getting this lint error:

avoid subject as a var at zerver/lib/actions.py line 5910: message__subject__iexact=topic_name

So I called the method filter_by_topic_name_via_message

Yeah I just mean it could be:

query = UserMessage.objects.filter(user_profile=user_profile, message__recipient=stream.recipient) return filter_by_topic_name_via_message(query=query, topic_name=topic_name)

Which might be better inlined unless we need it for tests.

timabbott · 2020-02-16T07:30:02Z

zerver/lib/actions.py

+def do_move_topic_messages(
+    user_profile: UserProfile,
+    stream_origin: Stream,
+    stream_destiny: Stream,


Call these old_stream and new_stream.

timabbott · 2020-02-16T07:31:08Z

zerver/lib/actions.py

+    user_messages = get_topic_messages(user_profile, stream_origin, topic_name)
+    recipient_destiny = get_stream_recipient(stream_destiny.id)
+
+    # We could use a bot for the breadcrump message too


Yeah, let's change this to a message sent by Notification Bot.

@mateuszmandera I just noticed we have these get_stream_recipient and get_personal_recipient helper functions in a few places that this code is copying; we should just remove those and use stream.recipient etc., right? (And delete get_recipient and its cache as well, I think???)

In any case, for this PR, @dobleuber you can just use new_stream.recipient.

@timabbott Yeah, we still have a bunch of places where we use those functions that haven't been changed. It should be pretty quick to do though and then we can remove the functions completely. I'll add it to my list.

timabbott · 2020-02-16T07:34:14Z

zerver/lib/actions.py

+
+    for user_message in user_messages:
+        user_message.message.recipient = recipient_destiny
+        user_message.message.save(update_fields=['recipient'])


This isn't going to work for caching and real-time sync reasons.

See https://zulip.readthedocs.io/en/latest/subsystems/caching.html and https://zulip.readthedocs.io/en/latest/subsystems/events-system.html for details.

After some investigation, the caching situation might actually be fine, because flush_message ignores update_fields currently and just deletes any modified messages from the cache.

Real-time sync is going to be much more problematic; we'll need to extend the interface for "edit message" events to allow the stream to change. It shouldn't be too bad because the problem is very similar to that of topic editing which we already do that correctly for. I think ultimately what we'll want to do have this go through the edit message code path.

Ok, I'll continue reviewing do_update_message. It does a lot of things, I'm not sure if I can reuse that method (adding a stream parameter), because we could have performance issues. I'm going to continue reviewing the code, if I have any questions I'll let you know, thanks

timabbott · 2020-02-19T01:37:07Z

Thanks for working on this @dobleuber! I pushed back to this PR a set of commits that rewrite this to use the do_update_messages infrastructure. Some notes:

The tests need some cleanup; we never hardcode IDs like 10 in them, instead using user.id substitutions.
The use of do_update_messages partially fixes the message edit history for this feature. We'll also need to add display code for it, which we can leave for follow-up work as long as the data is there.
It also sets us up with just a bit of plumbing work in its views.py caller to provide this feature via the API rather than a management command, which both makes UI possible and doesn't require server shell access to use. I think probably we should plan to delete the management command work and instead focus on making the .
The send_event code for real-time sync needs work, both in this backend code (to decide what users to notify correctly) and on the frontend (to update the UI in exports.update_messages within message_events.js). I'm OK with a potentially somewhat weird intermediate state, since one can reload browser windows, but we should at least make sure we've thought through how this might work.

The events thing may be hard for you to fix efficiently, but @dobleuber in your next pass, can you work on cleaning up the tests, changing the guard messages to use narrow_url to provide nice links? Feel free to squash my new commit into yours; I think it's better squashed, but wanted to leave it separate just to make it easy for you to see what I did. Also you could work on making update_message_backend support this code path as an realm admins-only feature.

dobleuber · 2020-02-19T20:17:10Z

@timabbott I worked in your last comments.
Please let me know if you have any other comment, thanks for your help :-)

timabbott · 2020-02-19T22:50:00Z

@dobleuber can you comment on which of the TODOs noted above you resolved, and how?

dobleuber · 2020-02-20T00:15:27Z

cleanup the tests: I refactor a little the tests to avoid using hardcoded ids and to do the tests independent of the environment.
changing the guard messages to use narrow_url to provide nice links: I called the method topic_narrow_url to get the new topic url and added it to the message.
I added a condition to allow this method be run just for realm admin users

Oh! I miss the method update_message_backend. Working on it. thanks

dobleuber · 2020-02-21T00:28:25Z

Updated the method update_message_backend to allow changing messages stream.
Updated tests for api call to update_message_backend

dobleuber · 2020-02-21T19:00:57Z

@timabbott Any feedback for this? meanwhile I am working in UI part:

pirate · 2020-02-22T00:06:09Z

I love that UI, nice work so far @dobleuber.

timabbott · 2020-02-26T23:48:03Z

For UI, I think we should start with modifying the existing message editing UI where one can change topics to also have a "Stream" field; I think that will be a lot simpler as a way to start.

timabbott · 2020-02-27T00:08:51Z

@dobleuber I just pushed an extra commit to this PR that does an incomplete version of a refactor that I think we should do before merging this; I didn't have time to clean up the tests (etc.), but basically, my hope is to eliminate both the management command and the extra function, and just have a single code path calling do_update_message with various arguments.

I also changed the code in a way that should fix some obviously broken behavior with the breadcrumb messages (various pieces were swapped).

You'll need to:

Fix the linter/syntax/etc. errors; this is totally untested
Rewrite the backend tests to use the API rather than calling do_* functions. (Or maybe just delete the do_move_topic tests, they may be basically duplicates of the API tests)
Do some manual testing and confirm that the links we generate are actually correct.

dobleuber · 2020-04-07T17:07:02Z

@timabbott I just created this issue related with your comment: #14492

timabbott · 2020-04-07T21:26:36Z

@dobleuber OK I made the following changes:

Changed the parameter for message editing to stream_id, rather than new_stream_id, for consistency with the existing topic_name parameter.
Changed the backend tests to be cleaner and better match our style (including, critically, not assuming Iago's user ID will always be 10).
Switched the mentions in the automatic notifications to be silent mentions.
Rewrote much of the message_events.js code to
Added a Co-Authored-By for you since we seem to have squashed away your Git author info at some point along the way :).

The result is 843345d and 7990676.

The remaining commit is just the UI, which should be the simplest part. I'm going to open a few follow-up issues before I get to looking at that, however.

timabbott · 2020-04-07T21:37:11Z

I opened #14498 for the main known bug in this feature.

timabbott · 2020-04-07T21:51:47Z

And I opened #14499 for tracking the future work we'll want to do once #14498 is done and we've gotten some experience playing with the feature and we have more user feedback on how they'd want to configure the permissions for this in their organization.

Previously, we had a restriction that we could only edit and move the topics of 7 days old messages. This buggy behaviour is now removed as in this commit. Fixes zulip#14492. Part of zulip#13912.

timabbott · 2020-04-23T23:55:02Z

static/js/stream_popover.js

+        const stream_id = topic_row.attr('data-stream-id');
+        const topic_name = topic_row.attr('data-topic-name');
+        const message = home_msg_list.all_messages()
+            .find(m => m.topic === topic_name && m.stream_id === +stream_id);


This check doesn't seem reliable; we may not have any messages from the topic cached in the frontend. I think the right fix is to fetch the latest message ID from stream_topic_history.js; that is guaranteed to have data given that this widget is visible at all.

@showell is there an existing function we can call, or does one need to be added to expose "any message ID from a given stream/topic appearing in the left sidebar"?

I don't think sending a message id is the right thing here. Shouldn't it be handled better on the server side using old and new streams and topics?

Well, for this UI, yes; but we'll also add this to "edit a single message" UI, and for that, the message ID is an important aspect of the meaning. (Basically whenever propagate_mode != all)

Oh! That's good then.

timabbott · 2020-04-23T23:55:18Z

static/js/stream_popover.js

+        if (old_topic_name && select_stream_id) {
+            message_edit.move_message_to_stream(message_id, select_stream_id, topic_name);
+            $('#move_topic_modal').modal('hide');
+        }


It looks like we're missing stopPropagation/preventDefault here.

Fixed this and pushed back here.

This initial implementation of moving a topic to another stream works as follows: * Added a method to get all topic's messages in a stream: get_topic_messages * Refactored update_message_backend method to allow changing the recipient of a message and the message topic * Created method notify_topic_moved_streams to create notifications messages whenever a messages is moved * Added a new option in topic's options to change stream * Added logic to update ui after a topic is moved * Added tests to support the code * Added documentation page for the feature

timabbott · 2020-04-23T23:57:11Z

static/js/message_edit.js

@@ -854,4 +854,23 @@ exports.handle_narrow_deactivated = function () {
    }
 };

+exports.move_message_to_stream = function (message_id, stream_id, topic_name) {


Let's rename this to exports.move_topic_containing_message_to_stream, for clarity about what it does.

And maybe add a comment right here making clear the stream_id and topic_name are the NEW values, and the old ones are wherever the message is located, since that may not be intuitive.

I appended new_ before the variable. That is clear enough.

timabbott · 2020-04-23T23:57:24Z

static/js/message_edit.js

+        },
+        error: function (xhr) {
+            ui_report.error(i18n.t("Error moving the topic"), xhr,
+                            $("#home-error"));


Can you post a screenshot of this appearing?

The cross icon in missing from the error popup.

I think it's probably good enough to just mark it to disappear after a few seconds.

timabbott · 2020-04-23T23:58:18Z

static/js/stream_popover.js

@@ -262,6 +263,17 @@ function build_starred_messages_popover(e) {

 }

+function build_move_topic_to_stream_popover(e, message_id, stream_id, topic_name) {
+    const streams = stream_data.subscribed_subs().filter(s => s.stream_id !== stream_id);


Let's call this variable available_streams.

I'll note that we currently include private streams in this list.

Done. Added a TODO for keyboard navigation of the dropdown.

Do we want to exclude private streams?

Well, I think there's a product question about whether it's correct to limit it to just public streams. We should make a decision on that and adjust accordingly. Initially we did it just to manage complexity.

timabbott · 2020-04-23T23:59:31Z

static/js/stream_popover.js

+
+        if (old_topic_name.trim() === topic_name.trim()) {
+            topic_name = undefined;
+        }


Maybe add a comment here noting that this is intended to request no change in the topic if it didn't change?

timabbott · 2020-04-24T00:00:22Z

templates/zerver/help/rename-a-topic.md

+
+1. Select **Move <topic name\> to another stream**.
+
+1. Select the destination stream (and if desired, enter a new topic name).


Let's just call it "a new topic" here.

timabbott · 2020-04-24T00:01:18Z

OK, I posted a batch of mostly small comments; the main thing we need to fix here is the computation of a message ID, which hopefully @showell can help with.

timabbott · 2020-05-02T17:51:12Z

static/js/stream_popover.js

+
+    $("#move-a-topic-modal-holder").html(render_move_topic_to_stream(args));
+    $('#move_topic_modal').modal('show');
+    e.stopPropagation();


We can delete this stopPropagation; it's in the caller (and should be there)

timabbott · 2020-05-02T18:23:41Z

Closing in favor of #14844; @amanagr you still still read the last few comments here. Thanks @dobleuber!

timabbott · 2020-05-04T17:06:28Z

The UI for this was merged today. Thanks again for all the work on this @dobleuber!

Previously, we had a restriction that we could only edit and move the topics of 7 days old messages. This buggy behaviour is now removed as in this commit. Fixes #14492. Part of #13912.

Previously, we had a restriction that we could only edit and move the topics of 7 days old messages. This buggy behaviour is now removed as in this commit. Fixes zulip#14492. Part of zulip#13912.

zulipbot added the size: XL label Feb 14, 2020

timabbott reviewed Feb 16, 2020

View reviewed changes

zerver/lib/actions.py Outdated

user_profile=user_profile

)

recipient = stream.recipient

Copy link

Sponsor Member

timabbott Feb 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is unnecessary.

timabbott reviewed Feb 16, 2020

View reviewed changes

dobleuber force-pushed the moving-topics-between-streams branch 2 times, most recently from 3cb4381 to db42c12 Compare February 17, 2020 23:23

timabbott force-pushed the moving-topics-between-streams branch 2 times, most recently from b4805f8 to a3b2708 Compare February 19, 2020 01:32

dobleuber force-pushed the moving-topics-between-streams branch 4 times, most recently from 2ea3206 to bfafaf1 Compare February 19, 2020 20:13

dobleuber force-pushed the moving-topics-between-streams branch 2 times, most recently from 9561282 to e60953c Compare February 20, 2020 23:58

timabbott force-pushed the moving-topics-between-streams branch 2 times, most recently from 49510b7 to 353c78d Compare February 27, 2020 00:06

timabbott force-pushed the moving-topics-between-streams branch from 353c78d to fea7009 Compare February 27, 2020 00:19

dobleuber force-pushed the moving-topics-between-streams branch 2 times, most recently from 55b3c66 to fedb464 Compare April 7, 2020 20:44

timabbott force-pushed the moving-topics-between-streams branch from fedb464 to 7be3c14 Compare April 7, 2020 21:23

timabbott mentioned this pull request Apr 7, 2020

Fix live update for moving messages to a stream the user isn't able to see #14498

Closed

timabbott mentioned this pull request Apr 7, 2020

Relax restrictions on moving messages to other streams #14499

Closed

3 tasks

timabbott mentioned this pull request Apr 8, 2020

Remove the TODO "7 days" restriction for edit and move topics #14492

Closed

hashirsarwar mentioned this pull request Apr 15, 2020

topic: Remove 7 days restriction for editing topics. #14591

Merged

timabbott reviewed Apr 23, 2020

View reviewed changes

timabbott force-pushed the moving-topics-between-streams branch from 7be3c14 to 95d10a4 Compare April 23, 2020 23:55

timabbott reviewed Apr 23, 2020

View reviewed changes

timabbott reviewed Apr 24, 2020

View reviewed changes

amanagr mentioned this pull request May 2, 2020

messages: Allow moving a topic to another stream. #14844

Closed

timabbott reviewed May 2, 2020

View reviewed changes

timabbott closed this May 2, 2020


		1. Select Move <topic name\> to another stream.

		1. Select the destination stream (and if desired, enter a new topic name).

Moving topics between streams #13912

Moving topics between streams #13912

Conversation

dobleuber commented Feb 14, 2020 • edited

timabbott commented Feb 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Feb 19, 2020

dobleuber commented Feb 19, 2020

timabbott commented Feb 19, 2020

dobleuber commented Feb 20, 2020

dobleuber commented Feb 21, 2020

dobleuber commented Feb 21, 2020

pirate commented Feb 22, 2020

timabbott commented Feb 26, 2020

timabbott commented Feb 27, 2020

dobleuber commented Apr 7, 2020

timabbott commented Apr 7, 2020

timabbott commented Apr 7, 2020

timabbott commented Apr 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanagr May 2, 2020 • edited

Choose a reason for hiding this comment

amanagr May 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Apr 24, 2020

Choose a reason for hiding this comment

timabbott commented May 2, 2020

timabbott commented May 4, 2020

dobleuber commented Feb 14, 2020 •

edited

amanagr May 2, 2020 •

edited

amanagr May 2, 2020 •

edited