Start implementing muted users by WesleyAC · Pull Request #4678 · zulip/zulip-mobile

WesleyAC · 2021-04-19T09:19:15Z

Related: #4655

Tested by opening the app, and running the following commands in my terminal:

curl -sSX POST https://chat.zulip.org/api/v1/users/me/muted_users/19335 -u wesleyac@zulip.com:MY_API_KEY
curl -sSX DELETE https://chat.zulip.org/api/v1/users/me/muted_users/19335 -u wesleyac@zulip.com:MY_API_KEY

And using the debug console to verify that the correct data got added to the redux state.

gnprice

Thanks @WesleyAC ! Will be great to have this feature in.

Comments below. I'm also going to go expand our testing style guide with some of this information, and some other parts will feed into the "how to add new data from the server" doc that #4655 (comment) was a draft of.

gnprice · 2021-04-19T19:04:55Z

+  describe('EVENT_MUTED_USERS', () => {
+    test('update `muted_users` when event comes in', () => {
+      const initialState = Immutable.Map([[makeUserId(42), 1618822632]]);
+
+      const action = deepFreeze({
+        type: EVENT_MUTED_USERS,
+        muted_users: [
+          {
+            id: 42,
+            timestamp: 1618822632,
+          },
+          {
+            id: 1234,
+            timestamp: 1618822635,
+          },
+        ],
+      });
+
+      const expectedState = Immutable.Map([
+        [makeUserId(42), 1618822632],
+        [makeUserId(1234), 1618822635],
+      ]);
+
+      const newState = mutedUsersReducer(initialState, action);
+
+      expect(newState).toEqual(expectedState);
+    });
+  });


So this is a style that you'll see in a lot of our legacy tests, but I think the verbosity of it isn't helpful. It's possible to write all the same content with a lot less boilerplate; when we do, I think that (a) makes it easier to see what the test is really saying, and (b) makes it easier to scan through all the tests in the file and see what conditions are covered and think about what might not be.

For good examples to follow, you can look at tests written relatively recently by me or Chris. Scanning through git log --stat -p --author 'greg\|cbobbe' -- src/**/*-test.js and looking for newly-written tests (i.e. scanning past where we're just touching an existing test), there's 0a218e0 as a nice though short example. Another good example is src/session/__tests__/sessionReducer-test.js. And a longer example, covering more complexity than we need in this file, is src/unread/__tests__/unreadModel-test.js.

WesleyAC · 2021-04-19T20:35:37Z

Ok, fixed all of those comments, and also went ahead and pushed the initial version of hiding messages from muted users in the MessageList. Should be ready for another look :)

WesleyAC · 2021-04-19T20:37:25Z

Ah, a few of my fixes here are in the wrong commit. I'll fix that in a moment.

WesleyAC · 2021-04-19T20:50:48Z

Ok, fixed up the commit history.

Also, here are screenshots of what the UI looks like.

chrisbobbe · 2021-04-19T23:31:27Z

 export type MuteState = MuteTuple[];

+/** A map from user IDs to the Unix timestamp in seconds when they were muted. */
+export type MutedUsersState = Immutable.Map<UserId, number>;


I'm glad to have another case of Immutable.Maps with numeric keys! 🎉 b54d68d should ensure that these get "revived" from storage with their keys as numbers, instead of strings. The data is persisted as JSON so we've had to be careful about that.

gnprice · 2021-04-20T05:33:25Z

  messages: number[],
 |};
+
+export type MutedUsersEvent = {|


nit: we've had the event types in alphabetical order in this file (same as in the EventTypes enum/class), so let's maintain that

This file isn't quite in alphabetical order (since Submessage wants to be next to Message, it seems?), but I made it a bit closer :)

Hmm true! And UserStatusEvent in what seems like a completely arbitrary place. Well, thanks for making it closer 🙂

gnprice

Thanks for the revisions! Comments below, mainly on the later commits.

gnprice · 2021-04-20T05:52:41Z

+    test('update `muted_users` when event comes in', () => {
+      const initialState = Immutable.Map([[makeUserId(42), 1618822632]]);
+
+      const action = deepFreeze({
+        ...eg.eventMutedUsersActionBase,
+        muted_users: [
+          {
+            id: makeUserId(42),
+            timestamp: 1618822632,
+          },
+          {
+            id: makeUserId(1234),
+            timestamp: 1618822635,
+          },
+        ],
+      });
+
+      const expectedState = Immutable.Map([
+        [makeUserId(42), 1618822632],
+        [makeUserId(1234), 1618822635],
+      ]);
+
+      const newState = mutedUsersReducer(initialState, action);
+
+      expect(newState).toEqual(expectedState);
+    });


This is still in a pretty verbose style which is typical of some of our tests, but which I think we can do a lot better than. See the examples I linked here:
#4678 (comment)

I changed it to look more like src/session/__tests__/sessionReducer-test.js, but that didn't make it much less verbose — this doesn't benefit too much from baseReduxData, since the state isn't a struct.

Let me know if there's a specific way that I could make this less verbose, though.

gnprice · 2021-04-20T06:02:33Z

+.message-muted {
+  /* If a message is both brief and muted, we don't want to save the extra
+   * space for the avatar, since it's not shown anyways. */
+  padding: 16px;


I don't think I follow what this comment is saying about "both brief and muted". Perhaps that'll end up being moot in a final version of this branch? I see this goes away in a later commit in this revision of the branch.

Yeah, this goes away later.

A "brief" message is one that doesn't show the avatar or username (not a great name, but the one we use). This is so that we don't have the message about the fact that it's muted have a big margin on the left side, the way there normally is when there's no user avatar.

This goes away because I coalesce messages later, so brief messages never have any content in them.

gnprice · 2021-04-20T06:12:54Z

  }
 });

+const revealMutedMessages = (target: Element) => {


This should get a bit of jsdoc to describe what its behavior is supposed to be. It looks like it's fairly subtle.

Done. Let me know if this needs any more elaboration.

gnprice · 2021-04-20T06:17:59Z

+  let messageNode = getMessageNode(target);
+  let first_run = true;
+  while (
+    messageNode
+    && messageNode instanceof Element
+    && (messageNode.classList.contains('message-brief') || first_run)
+  ) {
+    messageNode.setAttribute('data-mute-state', 'shown');
+    messageNode = nextMessage(messageNode);
+    first_run = false;
+  }


The logic controlling this loop is somewhat messy. For example nextMessage should always be returning an Element which is a message, or else null -- so the instanceof Element check and the classList check shouldn't be needed. (Unless there's an intended contrast between message-brief and message-full? If so, that calls for an explanatory comment.)

Rewrote this as do { } while (), which is much nicer.

The reason for the instanceof Element was because of getMessageNode, which returns Node rather than Element, which I've now fixed.

Not sure what you mean about message-brief vs message-full, the classList check is definitely needed. Hopefully the jsdoc makes it clear what's going on?

Definitely helpful, thanks!

I'll pick up this thread with a comment on the new version.

gnprice · 2021-04-20T06:20:53Z

 });

+const revealMutedMessages = (target: Element) => {
+  let messageNode = getMessageNode(target);


As the jsdoc says, this function is deprecated 🙂 It looks like target.closest('.message-brief') will give you what you're looking for.

Fixed, and added a note to use that instead in the jsdoc deprecation comment :)

Good idea, thanks :)

gnprice · 2021-04-20T06:22:32Z

  padding: 0 16px 16px 80px;
 }

+.message-muted {


I wonder if message-muted-user would be a clearer name for this class. I keep thinking message-muted sounds like it could apply somehow to a topic mute, which is a pretty different-feeling use case that gets treated differently in the UI.

This goes away in a later commit, so I'm not too inclined to change it.

WesleyAC · 2021-04-20T08:57:58Z

@gnprice should be ready for another review :)

gnprice

Thanks @WesleyAC for the revision! Comments below.

Because this PR thread is getting unwieldy, I haven't looked at the last few new commits:
f78745b PmConversationList [nfc]: Convert to functional component.
e9298d3 PmConversationList: Hide PMs from muted users.
0267617 contentHtmlFromPieceDescriptors: Switch to object for args.
13e530e messageAsHtml: Translate muted-user text.

I'm also going to go ahead and try merging some of these commits that are ready, in order to simplify things for the rest.

gnprice · 2021-04-27T01:21:21Z

+/** If the given message is muted, show it and all following brief messages. */
+const revealMutedMessages = (target: Element) => {
+  let messageNode = target.closest('.message');
+  if (!messageNode) {
+    throw new Error('messageNode is not defined');
+  }
+  do {
+    messageNode.setAttribute('data-mute-state', 'shown');
+    messageNode = nextMessage(messageNode);
+  } while (messageNode && messageNode.classList.contains('message-brief'));
+};


(continuing from #4678 (comment) )

This revision is definitely nicer, thanks!

There's still a similar messageNode instanceof Element at the call site. Can the call site switch to doing the .closest call? I think that would simplify the call site, and also let the if (!messageNode) here go away.

I think I see now what the intention is with the message-brief check. The point is that you want to reveal this message and all the ones after it that are part of the same block -- all the messages up to the next one that, well, that either isn't muted in the first place or has its own bit of UI to reveal it.

Ideally the jsdoc would say something in that direction. As is, the "brief messages" thing covers the "what" of the code's logic, but it leaves the "why" somewhat of a puzzle for the reader to piece together.

(Also "all following brief messages" reads to me very much like it means all the brief messages that come after this point, in the whole rest of the message list.)

gnprice · 2021-04-27T02:53:57Z

    >`;
  const messageTime = shortTime(new Date(timestamp * 1000), backgroundData.twentyFourHourTime);

+  // TODO: i18n


Currently this is the same as the status quo as for widgets, which maybe isn't a good excuse, but does make more more comfortable merging it before this is done.

That is certainly fair reasoning! 🙂

I guess in that case, in general, the thing that'd be most helpful up front is to explain your intention when sending the PR, if it isn't made clear by a comment or the commit message. If I notice something missing like this I'll be curious about it; and if there's a TODO comment that's helpful, but I'll still be curious why not do it the correct way from the beginning.

Making these shorter this way makes it easier to scan through a test case and see what it's saying, and to scan through the whole test file to see what's being covered and think about if there's anything else to cover.

This further compacts the tests to make it easy to scan through them all. It also cuts out a lot of tokens like `newState` that didn't convey much meaning.

This further shortens the test cases -- the simplest ones become a single line. It also helps make it clear that there isn't meant to be anything significant in the details of this piece of test data.

This helps make it explicit that there's nothing special about these particular user IDs, other than the fact that one appears in certain places in these tests and the other appears in certain other places. It also means that if in the future we were to have any code that expects these user IDs to be the IDs of users that we actually know about -- which is an invariant that it'd be quite reasonable for code to rely on -- these tests will be readily compatible with that because there are already full User objects for these.

Also remove one property that was providing empty data, where any test case is going to want to explicitly specify some data in particular.

Also inline the EVENT_MUTED_USERS example to the one place it's used. Having the data right there explicitly is nice for seeing exactly what's happening, which makes it helpful to do that when it doesn't get repetitious.

We'll use this soon, for implementing the "muted users" feature in what we show in the message list.

We'll use this same styling soon for muted-user messages.

…ptors.

gnprice · 2021-04-27T04:30:46Z

OK, so of these commits:

2d77cfe initialDataTypes: Add MutedUsers.
01b7638 Redux: Add muted_users reducer.
2298150 directSelectors: add getMutedUsers.
82d92ab backgroundData: Add muted_users.
ff17736 css: Rename widget class to special-message.
d445419 webview: Add initial support for hiding muted messages.
5cc0ee0 webview: Allow revealing messages from muted users.
dff0385 caughtUpReducer: Fix bug that overwrote CaughtUp state.
aad5b96 webview: Coalesce consecutive muted messages.
2c2dfbc webview: Add note about alternative to getMessageNode.
f78745b PmConversationList [nfc]: Convert to functional component.
e9298d3 PmConversationList: Hide PMs from muted users.
0267617 contentHtmlFromPieceDescriptors: Switch to object for args.
13e530e messageAsHtml: Translate muted-user text.

I'm merging versions of these:

2d77cfe initialDataTypes: Add MutedUsers.
01b7638 Redux: Add muted_users reducer.
2298150 directSelectors: add getMutedUsers.
82d92ab backgroundData: Add muted_users.
ff17736 css: Rename widget class to special-message.
2c2dfbc webview: Add note about alternative to getMessageNode.
0267617 contentHtmlFromPieceDescriptors: Switch to object for args.

The merged commits are:

10a07b9 api: Add types for muted users; add muted_users to requested data.
14ee13e muted users: Add data to Redux state.
c438b54 muted users tests [nfc]: Compact object literals to 1 line, vs. 4.
efb301b muted users tests [nfc]: Inline small actual/expected expressions.
b67e440 muted users tests [nfc]: Factor out boring arbitrary base state.
c8ed0b0 muted users tests: Use eg.otherUser and eg.thirdUser.
c7c58c8 example data [nfc]: Describe the "action fragments" idea more clearly.
0a66a1a example data [nfc]: Describe when to define example actions here.
f0c01d0 msglist: Add muted_users to backgroundData.
aeb56ec msglist css [nfc]: Rename widget class to special-message.
954a67f webview js [nfc]: Add note about alternative to getMessageNode.
713f1c6 msglist html [nfc]: Use an args object for contentHtmlFromPieceDescriptors.

Do take a look. The main changes are:

I squashed the directSelectors commit into the Redux commit, and moved a few of the changes from the Redux commit ahead into the API commit -- specifically the parts that are basically more of the API binding, so that all the API-binding changes are in one commit.
- That means the API commit now includes the changes to src/api/eventTypes.js, and also the corresponding bits to turn that event into a Redux action, because those bits are (thankfully) just boilerplate.
I added [nfc] to summary lines where appropriate.
- Hmm, I guess that isn't in our style guide! It is documented in our glossary. I should go add it to the style guide (which I think we hadn't started yet when I wrote that entry.)
I revised some summary lines mostly to make the prefixes broader, like changing "contentHtmlFromPieceDescriptors" to "msglist html", and putting the specific function name in the later part of the summary line.
- This looks like it may not really be explained anywhere in any detail; it probably should be! But as the Zulip-wide Git style guide says briefly, that prefix is generally "the name of the subsystem". So "the message list" or "the HTML generation for the message list" is a good match for the "subsystem" level of generality we're going for there. Sometimes I'll do a quick git log src/foo/ to see how we've referred to a given subsystem in past commits.
I wrote some additional commits to tighten up the new test file along the lines I had in mind up at Start implementing muted users #4678 (comment) . These are split out into commits more finely than I probably would otherwise do, to help make visible the different things I'm doing there. In net the file gets 22 lines shorter, and the two very simple tests become one line each.
- I also added there a couple of changes to exampleData.js that hopefully make clear some things that really weren't written down there before -- so thank you for running into those 🙂

gnprice · 2021-04-27T04:44:09Z

And merged those commits -- thanks again @WesleyAC!

Because this thread has gotten unwieldy for GitHub (with parts of it hidden to keep their page-load numbers good, etc.), let's pick up the rest on a new PR. I've pushed a rebased version to my repo as a branch dev-muted-users -- the commits are:
9cb90e8 caughtUp: Fix bug that overwrote CaughtUp state.
84aa24f webview: Add initial support for hiding muted messages.
c83092f webview: Allow revealing messages from muted users.
b738ace webview: Coalesce consecutive muted messages.
1391b41 PmConversationList [nfc]: Convert to functional component.
16fbb47 PmConversationList: Hide PMs from muted users.
89b8f95 messageAsHtml: Translate muted-user text.

One thing I noticed on the caughtUp commit, just before I was going to merge with it included:

-        caughtUp = { older: action.foundOldest, newer: action.foundNewest };
+        caughtUp = {
+          // $FlowIssue[unnecessary-optional-chain]
+          older: state[key]?.older || action.foundOldest,
+          // $FlowIssue[unnecessary-optional-chain]
+          newer: state[key]?.newer || action.foundNewest,
+        };

What happens if state[key] is indeed absent?

I believe we'll end up with the right result -- false or true for each value, depending on the data in action -- but it's a bit subtle. In particular if we had the two sides of the || in the other order, which looks pretty innocuous, I believe it'd go wrong and stick an undefined in there when foundFooest was false.

So it'd be good to make the fallback logic here more explicit. I think the logic at the end of legacyInferCaughtUp above is pretty effective for this:

  const { older: prevOlder, newer: prevNewer } = prevCaughtUp || DEFAULT_CAUGHTUP;

  return {
    older: prevOlder || caughtUpOlder,
    newer: prevNewer || caughtUpNewer,
  };

Where the jsdoc on DEFAULT_CAUGHTUP is:

/** The value implicitly represented by a missing entry in CaughtUpState. */

This event grew some documentation in zulip/zulip@65c400e06, and in that commit it also started sending the server version and feature level. Set up the boilerplate on the pattern Greg started in 979d283 and promoted just a week or two ago, in zulip#4678 (comment).

WesleyAC requested review from chrisbobbe and gnprice April 19, 2021 09:19

WesleyAC force-pushed the implement-muted-users branch from afd652f to de9515f Compare April 19, 2021 10:07

gnprice reviewed Apr 19, 2021

View reviewed changes

WesleyAC force-pushed the implement-muted-users branch from de9515f to e36f43b Compare April 19, 2021 20:30

WesleyAC changed the title ~~Add muted_users to redux state~~ Start implementing muted users Apr 19, 2021

WesleyAC force-pushed the implement-muted-users branch from e36f43b to 90aa095 Compare April 19, 2021 20:43

WesleyAC force-pushed the implement-muted-users branch from 90aa095 to a021f3c Compare April 19, 2021 20:53

chrisbobbe reviewed Apr 19, 2021

View reviewed changes

gnprice reviewed Apr 20, 2021

View reviewed changes

WesleyAC force-pushed the implement-muted-users branch from b61dd20 to 86440e8 Compare April 20, 2021 08:47

WesleyAC force-pushed the implement-muted-users branch 3 times, most recently from 2e6c799 to e9298d3 Compare April 23, 2021 08:57

gnprice reviewed Apr 27, 2021

View reviewed changes

WesleyAC and others added 10 commits April 26, 2021 21:09

api: Add types for muted users; add muted_users to requested data.

10a07b9

muted users: Add data to Redux state.

14ee13e

muted users tests [nfc]: Compact object literals to 1 line, vs. 4.

c438b54

Making these shorter this way makes it easier to scan through a test case and see what it's saying, and to scan through the whole test file to see what's being covered and think about if there's anything else to cover.

muted users tests [nfc]: Inline small actual/expected expressions.

efb301b

This further compacts the tests to make it easy to scan through them all. It also cuts out a lot of tokens like `newState` that didn't convey much meaning.

muted users tests [nfc]: Factor out boring arbitrary base state.

b67e440

This further shortens the test cases -- the simplest ones become a single line. It also helps make it clear that there isn't meant to be anything significant in the details of this piece of test data.

example data [nfc]: Describe the "action fragments" idea more clearly.

c7c58c8

Also remove one property that was providing empty data, where any test case is going to want to explicitly specify some data in particular.

example data [nfc]: Describe when to define example actions here.

0a66a1a

Also inline the EVENT_MUTED_USERS example to the one place it's used. Having the data right there explicitly is nice for seeing exactly what's happening, which makes it helpful to do that when it doesn't get repetitious.

msglist: Add muted_users to backgroundData.

f0c01d0

We'll use this soon, for implementing the "muted users" feature in what we show in the message list.

msglist css [nfc]: Rename widget class to special-message.

aeb56ec

We'll use this same styling soon for muted-user messages.

WesleyAC added 2 commits April 26, 2021 21:09

webview js [nfc]: Add note about alternative to getMessageNode.

954a67f

msglist html [nfc]: Use an args object for contentHtmlFromPieceDescri…

713f1c6

…ptors.

gnprice force-pushed the implement-muted-users branch from 13e530e to 89b8f95 Compare April 27, 2021 04:30

gnprice force-pushed the implement-muted-users branch from 89b8f95 to 713f1c6 Compare April 27, 2021 04:31

gnprice merged commit 713f1c6 into zulip:master Apr 27, 2021

WesleyAC mentioned this pull request Apr 27, 2021

Muted users: part 2 #4699

Merged

gnprice mentioned this pull request Apr 27, 2021

caughtUp: Fix bug that overwrote CaughtUp state. #4698

Merged

Subhasish-Behera mentioned this pull request Jul 14, 2023

Add support for muted users zulip/zulip-terminal#1001

Open

Uh oh!

Conversation

WesleyAC commented Apr 19, 2021

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WesleyAC commented Apr 19, 2021

Uh oh!

WesleyAC commented Apr 19, 2021

Uh oh!

WesleyAC commented Apr 19, 2021

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!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

WesleyAC commented Apr 20, 2021

Uh oh!

gnprice 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!

Uh oh!