Show muted messages in mentioned narrow #4807

WesleyAC · 2021-06-16T21:14:59Z

Related: #3472

WesleyAC · 2021-06-16T21:19:15Z

This isn't exactly the right approach in the long term, I think (#3542 has the start of a better solution), but I think that this is a useful tactical fix anyways.

chrisbobbe

Thanks, @WesleyAC! See a few small comments below.

chrisbobbe · 2021-06-22T19:37:12Z

src/utils/__tests__/message-test.js

  });
+
+  test('mention narrow messages are never muted', () => {
+    const message = {


Hmm, these new objects aren't well-typed. Want to get the file type-checked and start using exampleData (e.g., eg.streamMessage)?

I don't really want to, but I've gone ahead and done it. It is nice to have this typed :)

I can relate! 😅 Thanks for doing this. 🙂

chrisbobbe · 2021-06-22T19:41:20Z

src/utils/message.js


+  if (isMentionedNarrow(narrow)) {
+    return false;
+  }


I see the other early returns each have a comment explaining the reasoning. Could we add one that makes this one clearer (in particular, that this is just a tactical fix for now)?

Ah, yeah. I don't really like the other comments (they're explaining what the code's doing, not why), but this is a case where explaining why would be useful. I'll add that.

Yeah, exactly.

The new comment is good!

I agree about the existing comments. The one on "when narrowed to topic" is especially unhelpful. I think a proper fix for that comment (and that conditional certainly does need some kind of comment) would involve writing some jsdoc to articulate what this function's semantics is supposed to be in the first place.

… And then where that leads, I think, is that this is really a helper function specialized for its one caller, getShownMessagesForNarrow, and should appear next to that function. (That is: it's hard to coherently write down what its meaning is supposed to be without effectively saying "this says whether getShownMessagesForNarrow should filter this message out".) And in turn looking at that call site… it should really get inlined there, because the top level of its control flow is mostly about switching on the type of narrow, and in many cases we can skip the filter entirely.

Anyway, none of that needs to happen in this PR. Perhaps after this is merged, I'll go and try cleaning that up and writing associated jsdoc.

WesleyAC · 2021-06-22T21:19:09Z

Updated :)

chrisbobbe

Thanks! See one nit below.

Also I wonder if @gnprice wants to check that this is a good tactical fix to make? I think it is, but I might be missing something.

chrisbobbe · 2021-06-22T21:28:27Z

src/utils/__tests__/message-test.js

-      subject: 'some topic',
-    };
-    const narrow = topicNarrow('some topic');
+    const narrow = topicNarrow('stream', 'some topic');


Ooh, nice to get this topicNarrow call fixed!

chrisbobbe · 2021-06-22T21:35:22Z

src/__tests__/lib/exampleData.js

    ...streamInner,
    color: '#123456',
-    in_home_view: true,
+    in_home_view: in_home_view === undefined ? true : in_home_view,


nit: Could shorten to in_home_view ?? true.

This expression has the one difference that it also gives true when in_home_view is null. But in_home_view won't be null because of its type annotation as an optional boolean property of the args object.

Done! Thanks :)

gnprice

Thanks @WesleyAC and @chrisbobbe ! One comment below, which I'll fix and then merge.

gnprice · 2021-07-19T22:27:45Z

src/utils/message.js

+  // problem we have with muted mentioned messages, which is that they show up
+  // in the count for the "Mentions" tab, but not in the actual narrow.


One confusing thing in this comment: it describes the state before this bit of code was added, but at the time the comment is added, it's no longer accurate. Muted mentioned messages do now show up in the actual narrow under the "Mentions" tab.

I didn't notice that when first reading this comment -- it's subtle when reading now, when we're thinking about this change itself 😉 and so the previous state is salient in our minds. But in a future when someone (us included) is just trying to understand how the code is and doesn't remember this bugfix, it'll be confusing because the comment is saying one thing and then the code is ensuring the opposite.

About to merge, so I'll just fix this.

Currently, we don't show muted messages in the mentioned narrow. This is confusing and inconsistent with the webapp. This commit fixes that. We should do a more thorough audit of this code, and compare it to the webapp version. I haven't attempted to do that right now, since this small tactical fix seems important and high-priority. Related: zulip#3472

This follows up on adding types and using exampleData, to further make it easier to scan through these tests and see what scenarios they cover and don't cover.

The test says the message is muted if the stream is *not* in the "mute list"? Is that backward? … Ah, no, the point is that this covers when the stream isn't in the *subscriptions* list -- i.e., you're not subscribed. Clarify that in the code, with a comment on why this case isn't just an error case that shouldn't happen, and why it gets this behavior; and fix the test description too. Also add a test for the basic happy case, where you *are* subscribed and it isn't muted. That's what the next few cases are implicitly comparing against -- it wouldn't be very interesting that they returned false if this one didn't return true.

gnprice · 2021-07-19T22:49:21Z

And merged! I also added a couple of followup commits -- the nice new well-typed, shorter version of the tests made it easier to see what was going on there, and surfaced some things to fix. I have some more I'll send as a new PR, too.

This comment had been really just reciting *what* this bit of code does, not explaining why, as Wesley pointed out: zulip#4807 (comment)

gnprice · 2021-07-20T04:52:06Z

(I also just added a mention of #3472 in the description, so that it's possible to navigate to this PR from that issue.)

This comment had been really just reciting *what* this bit of code does, not explaining why, as Wesley pointed out: zulip#4807 (comment)

WesleyAC requested review from chrisbobbe and gnprice June 16, 2021 21:14

chrisbobbe reviewed Jun 22, 2021

View reviewed changes

WesleyAC force-pushed the show-muted-messages-in-mentioned-narrow branch from f34173b to b93bfb4 Compare June 22, 2021 21:18

chrisbobbe reviewed Jun 22, 2021

View reviewed changes

WesleyAC force-pushed the show-muted-messages-in-mentioned-narrow branch from b93bfb4 to 035a11d Compare June 22, 2021 21:50

narrow [nfc]: Add isMentionedNarrow function.

9d2dcce

gnprice reviewed Jul 19, 2021

View reviewed changes

WesleyAC and others added 5 commits July 19, 2021 15:30

exampleData [nfc]: Allow constructing muted subscriptions.

0ba425c

mute tests: Add types.

6ac73d5

mute tests [nfc]: Compact further by inlining actuals.

091eb7c

This follows up on adding types and using exampleData, to further make it easier to scan through these tests and see what scenarios they cover and don't cover.

gnprice force-pushed the show-muted-messages-in-mentioned-narrow branch from 035a11d to 734c2ef Compare July 19, 2021 22:47

gnprice merged commit 734c2ef into zulip:master Jul 19, 2021

gnprice mentioned this pull request Jul 20, 2021

Starred-messages view should show even topic- or stream-muted messages #4909

Closed

gnprice mentioned this pull request Jul 20, 2021

Fix muted @-mentions and starred messages; clean up muting logic #4910

Merged

gnprice mentioned this pull request Jul 20, 2021

messages: Add missing conditions for shouldBeMuted. #3542

Closed

WesleyAC mentioned this pull request Sep 2, 2021

@-mentions in muted topics not visible elsewhere #3472

Closed

		// problem we have with muted mentioned messages, which is that they show up
		// in the count for the "Mentions" tab, but not in the actual narrow.

Uh oh!

Show muted messages in mentioned narrow #4807

Show muted messages in mentioned narrow #4807

Uh oh!

Conversation

WesleyAC commented Jun 16, 2021 • edited by gnprice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WesleyAC commented Jun 16, 2021

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WesleyAC commented Jun 22, 2021

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jul 19, 2021

Uh oh!

gnprice commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WesleyAC commented Jun 16, 2021 •

edited by gnprice

Loading