slash commands: Create /mute_topic #9754

rheaparekh · 2018-06-14T18:17:30Z

Two slash commands:

/mute_topic: For this command, the usage is /mute_topic <topic> #<stream_name> and /mute_topic #<stream_name> <topic>. Both would work. The muting occurs using muting_ui in the frontend, where as the server side code checks if the stream and topic to be muted exists.
Added tests for the server side code as well.
/settings

rheaparekh · 2018-06-14T18:34:48Z

static/js/zcommand.js

@@ -60,6 +60,7 @@ function update_setting(command) {
 exports.process = function (message_content) {

    var content = message_content.trim();
+    var tokens = content.split(' ');


A better name for tokens can be suggested.

tokens seems fine here.

rheaparekh · 2018-06-14T18:36:02Z

zerver/views/messages.py

+                msg = ("A valid topic name is required.")
+            else:
+                if topic_is_muted(user_profile, stream.id, topic):
+                    msg = ("The topic '%s' is already muted." % (topic))


If the user tries to mute a topic which has already been muted, should we unmute it? Slack does this.

I think this should be the case since hotkey M does that.

lonerz

model: Add helper function to check if topic exists. commit looks good. Other commits have minor comments.

lonerz · 2018-06-15T04:11:07Z

static/js/zcommand.js

@@ -60,6 +60,7 @@ function update_setting(command) {
 exports.process = function (message_content) {

    var content = message_content.trim();
+    var tokens = content.split(' ');


tokens seems fine here.

lonerz · 2018-06-15T04:18:07Z

zerver/views/messages.py

+                msg = ("A valid topic name is required.")
+            else:
+                if topic_is_muted(user_profile, stream.id, topic):
+                    msg = ("The topic '%s' is already muted." % (topic))


I think this should be the case since hotkey M does that.

lonerz · 2018-06-15T04:19:35Z

static/js/zcommand.js

@@ -84,6 +84,10 @@ exports.process = function (message_content) {
        return true;
    }

+    if (content === '/settings') {
+        window.location.hash = 'settings/your-account';


Hm, not sure if this is the correct thing here. One part of me says this works (though seems hacky). Is there a way to open the modal with a function call?

Also, remember to return true;.

Yeah, at first, this seems hacky to me too(I guessed overlays.open_settings does that but it only open overlays), but now I think this should work properly because we have our own event handler for hashchange which handles this properly and I also think this is the only way to make this work.
@showell can clear this!

I agree with @shubhamdhama. I tried doing this with a function call, but It was working. (Mainly by calling settings.setup_page()). Even in settings menu this is being done.

neiljp

The basics are here, though I think there are some possible issues for the edge cases for stream and topic names. Other aspects such as what counts as a 'match' (case-sensitivity, sub-strings), providing help, and muting based on 'position', we can discuss on czo first, perhaps for v2?

In addition to what you have here, if a slash command doesn't get recognized, we don't currently show anything, it seems. You could add that in another commit here, or a separate PR?

neiljp · 2018-06-16T00:17:24Z

zerver/tests/test_messages.py

@@ -1042,6 +1042,36 @@ def test_zcommand(self) -> None:
        self.assert_json_success(result)
        self.assertIn('still in day mode', result.json()['msg'])

+        payload = dict(command="mute_topic help")


I appreciate the idea of including help, but I'm not sure whether this works for slash commands - unless each does not normally take one parameter, or that parameter cannot be 'help'. Otherwise help for each command would be found inconsistently.

A separate /help or /helpslash command would be my personal preference, but that's to discuss on czo.

I have removed the help parameter for now.

neiljp · 2018-06-16T00:22:43Z

zerver/tests/test_messages.py

+        result = self.client_post("/json/zcommand", payload)
+        self.assert_json_success(result)
+        self.assertIn("The topic 'Verona1' is already muted.", result.json()['msg'])
+


From the feedback, I assume you'll update this to have it toggle the mute, too?

Additionally, while I appreciate the flexibility of being able to mute an arbitrary topic, and this looks good as a v1, I wonder whether the following might be useful to include from a user's POV, as long as they can understand (and we can define) 'position':

/mute_topic -> stream&topic implied from current position

/mute_topic <topic> -> stream implied from current position, topic from <topic>

/mute topic <stream> <topic> -> as above :)

neiljp · 2018-06-16T00:28:10Z

zerver/views/messages.py

+            return json_success(ret)
+
+        topic = ' '.join(tokens[2:])
+        stream_name = tokens[1]


These two lines concern me slightly, though that might be from lack of knowledge of the server representation of streams and topics, and what is allowed in each: (see my suggestion re tests above too)

can streams not contain spaces too?

could we use # to identify if a stream/topic identifier from the command is intended to be a stream or not?

could we use another character/separate to identify a topic?

If we can make this distinction, then this might allow for a case in addition to those I listed above, along the lines of:

/mute_topic #<stream> -> mute #<stream>

In that case, we can shorten the command to just /mute, or maybe /mute should be reserved for streams.

neiljp · 2018-06-16T00:30:46Z

zerver/views/messages.py

@@ -706,6 +709,30 @@ def zcommand_backend(request: HttpRequest, user_profile: UserProfile,
        ret = dict(msg=msg)
        return json_success(ret)

+    if tokens[0] == 'mute_topic':
+        if len(tokens) < 3 or tokens[1] == 'help':


If we retain 'help', should this be tokens[1:]?

neiljp · 2018-06-16T00:33:10Z

zerver/views/messages.py

@@ -684,6 +684,9 @@ def find_first_unread_anchor(sa_conn: Any,
 @has_request_variables
 def zcommand_backend(request: HttpRequest, user_profile: UserProfile,
                     command: str=REQ('command')) -> HttpResponse:
+
+    tokens = command.split(' ')


This is only used in your new commands, so this should likely move to line ~711?

neiljp · 2018-06-16T00:47:34Z

zerver/tests/test_messages.py

+        result = self.client_post("/json/zcommand", payload)
+        self.assert_json_success(result)
+        self.assertIn('A valid stream name is required.', result.json()['msg'])
+


I suggest you add some tests for topics, and maybe streams, which have spaces in them.

Updated the commit and added these tests.

neiljp · 2018-06-16T00:55:13Z

zerver/views/messages.py

+                    do_mute_topic(user_profile, stream, recipient, topic)
+                    msg = ("The topic '%s' has been muted." % (topic))
+        except ObjectDoesNotExist:
+            msg = ('A valid stream name is required.')


All of this uses exact matching (case-sensitive, word for word); this seems challenging for longer stream and/or topic names.
(particularly since I don't think we have a shortcut key for previous message, like 'up arrow' in some chat clients, to grab the previous text to edit a typo)

showell · 2018-06-17T14:35:09Z

I didn't read this super closely, but it seems like we are introducing a lot of server code for muting, when we already have decent support for muting in the client already with the hotkey and menu options. It would be nice to have a super simple V1 to play with, as some of Neil's feedback is pointing to.

showell · 2018-06-17T14:50:07Z

@timabbott I'm not sure what the best way to open the settings menu is, but it seems like setting window.location.hash mimics what happens when you go through the gear menu, and that's how @rheaparekh coded it. It appears that the info overlays work differently, for some reason. Do you know all the history behind that? It seems like it would be nice to have a less hacky way to open settings than setting the href (even if V1 was just a function in gear_menu.js called open_settings() or something).

neiljp

In addition to my few comments (made recently, but the review didn't go through), there's also potential to use the approaches in the other zcommand PR?

neiljp · 2018-06-22T17:04:18Z

zerver/lib/zcommand.py


 def process_zcommands(content: str, user_profile: UserProfile) -> Dict[str, Any]:
    if not content.startswith('/'):
-        raise JsonableError(_('zcommand does not have leading slash.'))
-    command = content[1:]
+        raise JsonableError(_('There should be a leading slash in the zcommand.'))


Strictly, this change in text should be in the previous commit?

neiljp · 2018-06-22T17:08:12Z

static/js/zcommand.js

+            command: content,
+            on_success: function (data) {
+                if (data.topic && data.stream) {
+                    if (muting.is_topic_muted(data.stream, data.topic)) {


Can you use muting_ui here? That has a built-in toggle_mute. I'm not familiar with the front-end, though.

neiljp · 2018-06-22T17:22:01Z

zerver/lib/zcommand.py

+                ret = dict(topic=topic, stream=stream_name)
+            else:
+                ret = dict(msg="A valid topic name is required.")
+        except ObjectDoesNotExist:


Does this only get raised from get_realm_stream? If so, this might be clearer with the try/except wrapping only that, and maybe an extra else clause.

rheaparekh · 2018-06-27T14:28:36Z

I have updated this PR.

showell · 2018-06-27T15:13:45Z

zerver/lib/zcommand.py

+    This functions helps to extract the topic name from the mute_topic command
+    """
+    stream_link = ("#**%s**" % (stream_name))
+    topic_name = content.replace('mute_topic', '').replace(stream_link, '')


This feels like a slightly brittle way to get the topic. I feel we want a single function that uses a regex to get both values, where the regex looks something like '#\*\*(.*)\*\* (.*).

I am already using possible_linked_stream_names from bugdown to extract the stream name. What I am doing here is removing both, the stream mention (which is in the format #**stream_name**)and mute_topic from the command(which is in the format /mute_topic #**stream_name** topic), hence we would have only topic left in the command.

showell · 2018-07-02T00:33:16Z

I merged two of these commands, so you'll need to rebase.

rheaparekh · 2018-07-02T11:30:36Z

I have rebased this!

showell · 2018-07-02T12:28:40Z

Thanks for rebasing. I edited the PR title to reflect that it's just /mute_topic now.

neiljp

LGTM excepting the points I noted; I'll leave it to @showell to comment further re any other updates and re your response to the earlier regex discussion.

neiljp · 2018-07-08T07:14:36Z

zerver/lib/zcommand.py

+            return dict(msg="Usage: /mute_topic #<stream_name> <topic_name>")
+
+        [stream_name] = linked_stream
+        topic = get_topic_from_mute_topic_content(content, stream_name)


Minor: Reiterating what I said previously or before, linked_stream[0] might be simpler?

@neiljp linked_stream is a set containing only one element. So I can user list(linked_stream)[0], but I thought this looks much more cleaner.

neiljp · 2018-07-08T07:16:50Z

zerver/lib/zcommand.py

+            return dict(msg='A valid stream name is required.')
+
+        if topic_exists(topic, get_stream_recipient(stream.id)):
+            ret = dict(subject=topic, stream=stream_name, type='stream')


Is there a reason you are passing type? This seems unnecessary.

The data passed in toggle_mute requires a type='stream'.

timabbott · 2018-07-10T09:03:13Z

zerver/models.py

@@ -1309,6 +1309,12 @@ def get_context_for_message(message: Message) -> Sequence[Message]:
        pub_date__gt=message.pub_date - timedelta(minutes=15),
    ).order_by('-id')[:10]

+def topic_exists(topic_name: str, recipient: Recipient) -> bool:
+    topic_exists = Message.objects.filter(
+        subject=topic_name,


This logic is wrong; topics are always matched in a case-insensitive fashion.

So we should do subject__iexact= here.

(Edit; fixed a typo that said icontains -- that's for case-insensitive substrings)

timabbott · 2018-07-10T09:04:12Z

zerver/tests/test_messages.py

+                                 content="whatever", topic_name="my topic")
+        stream = get_realm_stream("Denmark", user_profile.realm.id)
+        recipient = get_stream_recipient(stream.id)
+        self.assertTrue(topic_exists('my topic', recipient))


Let's add a test using differnet case (e.g. "My Topic"), which should pass.

timabbott · 2018-07-10T09:09:14Z

I posted one comment, but there's a deeper bug, which is that space isn't a good separator for stream/topic names, since spaces are valid characters in those. I think we might need to do something more like Zulip's *-mention syntax for a stream to encode the stream name (might be able to reuse the typeahead as well).

The muting occurs using muting_ui in the frontend, where as the server side code checks if the stream and topic to be muted exists. Added tests for the server side code as well.

rheaparekh · 2018-07-18T15:05:50Z

I have updated this PR with the mentioned changes!

neiljp · 2018-07-18T16:48:29Z

I thought we'd discussed spaces as an issue previously, but this looks to fix it fine now.

As a followup PR, I'd be inclined to suggest simplifying the tests, which are fairly concise up to night/day, but these now involve quite a lot of repetition.

zulipbot · 2018-07-31T18:15:55Z

Heads up @rheaparekh, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

alya · 2023-06-09T17:44:48Z

Thanks for the work here @rheaparekh and reviewers! Closing as we have no current product plans to add this feature. Keyboard can mute a topic via the Shift+M shortcut.

zulipbot added the size: L label Jun 14, 2018

rheaparekh force-pushed the slash_command branch from 164c2fb to ee8f751 Compare June 14, 2018 18:33

rheaparekh commented Jun 14, 2018

View reviewed changes

lonerz reviewed Jun 15, 2018

View reviewed changes

rheaparekh force-pushed the slash_command branch 3 times, most recently from 8afecf4 to 76cb159 Compare June 15, 2018 12:46

zulipbot added size: XL and removed size: L labels Jun 15, 2018

rheaparekh force-pushed the slash_command branch from ad7e3a2 to 7f5774d Compare June 15, 2018 17:10

rheaparekh changed the title ~~[WIP] slash commands: Create basic slash commands using zcommands.~~ slash commands: Create basic slash commands using zcommands. Jun 15, 2018

rheaparekh force-pushed the slash_command branch from 7f5774d to d797bda Compare June 15, 2018 18:54

zulipbot added size: L and removed size: XL labels Jun 15, 2018

rheaparekh mentioned this pull request Jun 15, 2018

zcommands: Add zcommand module and separate test module. #9758

Merged

neiljp reviewed Jun 16, 2018

View reviewed changes

rheaparekh force-pushed the slash_command branch from d797bda to afb25f9 Compare June 17, 2018 21:04

zulipbot added size: XL and removed size: L labels Jun 17, 2018

rheaparekh force-pushed the slash_command branch 8 times, most recently from 043979e to dce412a Compare June 19, 2018 19:25

neiljp reviewed Jun 26, 2018

View reviewed changes

rheaparekh force-pushed the slash_command branch 4 times, most recently from 01460d3 to 01ec7fd Compare June 27, 2018 13:51

showell reviewed Jun 27, 2018

View reviewed changes

showell mentioned this pull request Jul 2, 2018

Add /settings command (split out from #9754) #9861

Merged

rheaparekh force-pushed the slash_command branch from 01ec7fd to 7cc6019 Compare July 2, 2018 11:29

showell changed the title ~~slash commands: Create basic slash commands using zcommands.~~ slash commands: Create /mute_topic Jul 2, 2018

rheaparekh force-pushed the slash_command branch from 7cc6019 to 774d302 Compare July 4, 2018 10:33

neiljp reviewed Jul 8, 2018

View reviewed changes

timabbott reviewed Jul 10, 2018

View reviewed changes

rheaparekh force-pushed the slash_command branch from 774d302 to c008ed0 Compare July 13, 2018 19:49

model: Add helper function to check if topic exists.

37d97a0

rheaparekh force-pushed the slash_command branch 3 times, most recently from 9968e9d to 7377a1d Compare July 13, 2018 20:40

slash commands: Add /mute_topic command (via zcommand).

7377a1d

The muting occurs using muting_ui in the frontend, where as the server side code checks if the stream and topic to be muted exists. Added tests for the server side code as well.

zulipbot added the has conflicts label Aug 7, 2018

alya closed this Jun 9, 2023

slash commands: Create /mute_topic #9754

slash commands: Create /mute_topic #9754

Conversation

rheaparekh commented Jun 14, 2018 • 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

lonerz left a 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

shubhamdhama Jun 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a 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

showell commented Jun 17, 2018

showell commented Jun 17, 2018

neiljp left a 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

rheaparekh commented Jun 27, 2018

Choose a reason for hiding this comment

rheaparekh Jun 28, 2018 • edited

Choose a reason for hiding this comment

showell commented Jul 2, 2018

rheaparekh commented Jul 2, 2018

showell commented Jul 2, 2018

neiljp left a 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 Jul 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Jul 10, 2018

rheaparekh commented Jul 18, 2018

neiljp commented Jul 18, 2018

zulipbot commented Jul 31, 2018

alya commented Jun 9, 2023

rheaparekh commented Jun 14, 2018 •

edited

shubhamdhama Jun 15, 2018 •

edited

rheaparekh Jun 28, 2018 •

edited

timabbott Jul 10, 2018 •

edited