messages: Add missing conditions for shouldBeMuted.

[ishammahajan]

Based on the WebApp there are three conditions to be added -
(based in ../zulip/static/js/notifications.js under
exports.message_is_notifiable)

• If the message is sent by the current user.
• If the message @mentions the current user.
• If the notification has already been sent out (to avoid spam).

This commit handles the first two.
The third can be handled at a later point of time, since (IIUC) we don't already handle that type of logic. At this point, this PR is made to fix 3472 (and conveniently another issue).

Fixes #3472.

[gnprice]

Thanks @ishammahajan ! The comparison to that webapp function is quite helpful.

• On this bit:

+  if (message.isOutbox) {
+    return true;
+  }

I think this bit is meant to correspond to "sent by the current user" -- is that right?

That's not what Outbox or isOutbox turn out to mean -- see e.g. the Outbox jsdoc:

/**
 * A message we're in the process of sending.
 *
 * See also `Message`.
 */
export type Outbox = {|

• Apart from that, I think this points to an important difference between what this shouldBeMuted function means, and what the message_is_notifiable function in the webapp means.

  This unfortunately isn't a super conceptually-clear part of the code: there isn't jsdoc for this function to say what it's supposed to mean, and I think the meaning is a little complicated and also doesn't really quite agree with the name. So to understand it you'll have to look at the code, and in particular at the places this function is used.

  So, what does this function mean? In particular, concretely: if we added a condition to return true for messages this user sent, what would be the effect of that?

[gnprice]

• Then on the other bit:

+  if (message.flags && message.flags.includes('mentioned')) {
+    return false; // never hide a message which mentions current user
+  }

I believe this will never trigger. Take a look at the comment on flags in the Message type.

Did you try reproducing #3472, and did this change actually fix it? If it did, there's something puzzling going on which I'll want to understand better.

[ishammahajan]

• This unfortunately isn't a super conceptually-clear part of the code: there isn't jsdoc for this function to say what it's supposed to mean, and I think the meaning is a little complicated and also doesn't really quite agree with the name. So to understand it you'll have to look at the code, and in particular at the places this function is used.

I went through it and as I understand it now, the usages of the function just filters the messages belonging to muted topics and narrows out of the final array of messages to display in the message list. I tried to look around to find how we send notifications and how we filter those notifications, but I couldn't locate the exact file. I tried: ctrl + p (to bring up the file searcher pop-up on vscode), then typing in notification into it to find files with the names notification / or something related. Would you please direct me to the correct file?

[ishammahajan]

Looking through this again, I think I misunderstood what the quoted issue was really all about.

I thought it was about the user not getting notifications about some messages which they should get. But now, going through the code which processes FCM messages, in FcmMessage.kt

sealed class FcmMessage {
    companion object {
        fun fromFcmData(data: Map<String, String>): FcmMessage =
            when (val eventType = data["event"]) {
                "message" -> MessageFcmMessage.fromFcmData(data)
                "remove" -> RemoveFcmMessage.fromFcmData(data)
                null -> throw FcmMessageParseException("missing event type")
                else -> throw FcmMessageParseException("unknown event type: $eventType")
            }
    }
}

It doesn't really process anything, just filters out the MessageFcmMessages which have remove as their event key (if I understand correctly), which comes from the server itself -- which makes sense to me.

What was actually talking about is what editing shouldBeMuted fixes, which is showing what messages make it to the stream narrows, etc.

• In particular, concretely: if we added a condition to return true for messages this user sent, what would be the effect of that?

It will stop showing the messages the current user sent in all narrows. Disastrous!

[ishammahajan]

So the actual wanted code was in src/js/message_list_data.js, under

	unmuted_messages: function (messages) {
        return _.reject(messages, function (message) {
            return muting.is_topic_muted(message.stream_id, util.get_message_topic(message)) &&
                   !message.mentioned;
        });
    },

Which takes care of only one extra case, which is when the user is mentioned. I'll update the commit message and the post saying this.

[ishammahajan]

It turns out that we don't properly set the flag state for mentions either. On https://zulipchat.com/api/update-message-flags

We store mentioned as mentions in our FlagsState. Fixing that as well. This is specially weird because where we do use FlagsState, we do use mentioned and not mentions, and the flaw isn't detected by flow because all usages are in the form of a string and not object dot notation.

[ishammahajan]

@gnprice this should be ready for another round of review!

[gnprice]

Thanks @ishammahajan for these updates!

going through the code which processes FCM messages, in FcmMessage.kt ...
It doesn't really process anything, just filters out the MessageFcmMessages which have remove as their event key (if I understand correctly), which comes from the server itself -- which makes sense to me.

Yep, for notifications we rely on the server to identify which messages the user wants to get notified about.

• In particular, concretely: if we added a condition to return true for messages this user sent, what would be the effect of that?

It will stop showing the messages the current user sent in all narrows. Disastrous!

Yep, I believe that's right 🙂

We store mentioned as mentions in our FlagsState. Fixing that as well.

Good catch, thanks!

This is specially weird because where we do use FlagsState, we do use mentioned and not mentions, and the flaw isn't detected by flow because all usages are in the form of a string and not object dot notation.

Hmm, do we?

The syntax choice between flags.mentioned and flags['mentioned'] shouldn't matter to Flow... and at a quick test, it looks like it doesn't. If I write flags['mentioneddd'], misspelling the name, Flow promptly shows me an error.

I think in fact the answer to this puzzle is that we have not been looking at this flag in FlagsState at all. 🤔 Which raises the question of how the "mentioned" feature has been managing to work even a little bit...

OK, I think part of the answer is that it does not work for messages that newly come in! The narrowsReducer relies on isMessageInNarrow, which has this logic:

  return caseNarrow(narrow, {
    home: () => true,
    stream: name => name === message.display_recipient,
    // ...
    starred: () => message.type === 'starred',
    mentioned: () => message.type === 'mentioned',
    allPrivate: () => message.type === 'private',

Those starred and mentioned conditions can't ever be true.

I guess I'll file a separate issue shortly for that.

Right, so: thanks for spotting that mismatch. 😄

[gnprice]

In the code:

4a55e3a flags state: Fix type to match api.

• Looks good! Will merge shortly.

51aec0ad4 messages: Add missing condition to shouldBeMuted.

• Please add tests. 🙂

• Small correction in commit message: not src/js/message_list_data.js but static/js/message_list_data.js .

• Also, I believe this will not handle wildcard mentions (like @mobile on czo), and we should.

The webapp code does say just !message.mentioned... but wildcard mentions ought to be handled the same, right? Indeed, rg wildcard static/js finds:

static/js/message_store.js
75:    message.mentioned = convert_flag('mentioned') || convert_flag('wildcard_mentioned');
109:    message.mentioned = convert_flag('mentioned') || convert_flag('wildcard_mentioned');

That seems like a nice solution for not having an endless stream of bugs where some code adds a condition for mentions and neither the author nor reviewer put special thought into wildcard-mentions.

The very next line after each of those looks like this:

76:    message.mentioned_me_directly =  convert_flag('mentioned');
110:    message.mentioned_me_directly =  convert_flag('mentioned');

before we go and discard the raw flags list. And then mentioned_me_directly is consulted in just one place, which is the message_is_notifiable function we were looking at earlier. So in our case we may not need the specific mentioned flag anywhere, only the combined condition "mentioned or wildcard_mentioned".

[gnprice]

4a55e3a flags state: Fix type to match api.

Merged as 4a55e3a.

[ishammahajan]

Which raises the question of how the "mentioned" feature has been managing to work even a little bit...

So if I understand correctly from the rest of the review and the new issue, this is happening: The new messages coming in are being correctly saved in the state (under the appropriate redux subtree -- the same as that being sent by the server), but they aren't correctly being read by the specific narrow (eg. mentioned). The reason why the narrows are working correctly is solely due to the fact that we fetch messages around an anchor from the server every time we open it. As a result of this, if the above is correct, there's a higher consumption of data from the client side because we essentially aren't being able to cache the messages -- i.e. they're always fetched from the server fresh, ready to be served.

Is that correct?

[ishammahajan]

• Please add tests. 🙂

Ah, about to do that. Thanks! (also made the tests flow strict)

• Small correction in commit message: not src/js/message_list_data.js but static/js/message_list_data.js .

Oh, sorry. Fixed! 🙂

• Also, I believe this will not handle wildcard mentions (like @mobile on czo), and we should.

The webapp code does say just !message.mentioned... but wildcard mentions ought to be handled the same, right? Indeed, rg wildcard static/js finds:

Ah, I missed that. Fixing! :)

[ishammahajan]

@gnprice this should be ready for another round now! :)

[gnprice]

Thanks @ishammahajan !

7fa8ef7 flags reducer: Combine mentioned and wildcard_mentioned flags.

Hmm -- this combineMentionedFlags logic seems a bit confusing.

Will this work correctly if we have the following sequence?

• Message comes in, and has wildcard_mentioned set.
• (We go and copy that over to mentioned.)
• Message is edited -- event comes in to remove wildcard_mentioned flag.
• (We duly try to remove the message from wildcard_mentioned -- but we don't look in mentioned.)
• ... Then I think the mentioned flag stays set, even though the message no longer contains any kind of mention.

In any event, please write a test for that case -- then we'll be confident whether the code has that problem 🙂

Because of the remove case, we probably do need to keep around the original flag, like what the web frontend calls mentioned_me_directly.

Let's also avoid using deeperMerge here. I think it does more than you want -- note this line:

           : { ...obj1[key], ...obj2[key] }],

If both flags are true, that will produce {}, not true.

[gnprice]

This PR has gotten stale, and I think when we fix the things it was fixing we'll do so in new PRs.

Part of it is subsumed by #4807, just merged, and #4910, which I just sent. I've just filed #4911 for what the first commit here was aimed at doing, fixing up the wildcard-mentions situation. With that I think everything is covered either by merged changes or open issues, so closing this.