unreads: Don't add messages with "read" flag to unreads. #4710

WesleyAC · 2021-05-01T09:06:11Z

Previously, the main (only?) case where we wouldn't add a message to
unreads was if we were the one who sent it. This check was not correct -
instead, we want to check to see if the message has the "read" flag,
which is set by the server.

In the past, these checks were very close to the same (with the
exception of "Notification Bot" announcing a stream that you created,
which would be "read" for you), but with the addition of muted users,
the check is now incorrect.

This commit removes the logic to not mark a message as read when we are
the one who sent it, and instead just looks at the server flag, since
that should be the canonical place for that information to be.

gnprice · 2021-05-03T19:14:18Z

Previously, the main (only?) case where we wouldn't add a message to
unreads was if we were the one who sent it.

Hmm, there is a good question here! Were there any other cases where we wouldn't add the message to unreads?

The answer I wanted to give was: no, there weren't -- the other conditions that exist in the three sub-reducers for "streams", "pms" meaning 1:1 PMs, and "huddles" meaning group PMs, are a partition of the space of possibilities.

But looking closer, that is not quite so! After the message.type split between 'stream' and 'private', here's the conditions for "huddles" and "pms" respectively:

   if (recipientsOfPrivateMessage(action.message).length < 3) {
     return state;
   }

   if (recipientsOfPrivateMessage(action.message).length !== 2) {
     return state;
   }

What those still leave out is the case where the number of recipients is 1 -- i.e., the self-1:1 thread.

I think it's pretty unlikely that will ever matter -- i.e. that we'll ever have a case where you can receive a self-1:1 that isn't already read. Still, as part of completely fixing this bug, it'd be good to excise that flawed logic too.

Would you add a test that says if we get a self-1:1 and it isn't read, it does show up in the unreads state? And then fix that condition so the test passes. (This probably goes as a separate commit from the existing one.)

WesleyAC · 2021-05-04T09:45:45Z

Added a follow-up commit to fix that, and removed the (only?) in the first commit.

Should be ready for another review :)

chrisbobbe

Thanks, @WesleyAC!

Want to try getting unreadMentionsReducer-test.js type-checked? 🙂 I tried adding /* @flow strict-local */ to the top. The Flow output looked super long and complicated compared with what I expect the fixes will actually involve; I expect it'll actually be quite straightforward. Mostly things like:

seeing an error with an action that's defined like

      const action = deepFreeze({
        type: ACCOUNT_SWITCH,
      });

noticing that AccountSwitchAction describes that action
noticing that the problem is that the value is missing an index property, and
adding that

Greg's comments in 0a66a1a might be helpful.

chrisbobbe · 2021-05-04T21:17:17Z

src/unread/unreadHuddlesReducer.js

@@ -24,7 +24,7 @@ const eventNewMessage = (state, action) => {
    return state;
  }

-  if (action.ownUserId === action.message.sender_id) {
+  if (action.message.flags?.includes('read')) {


Looking at the comment on .flags in Message (which it gets from BaseMessage now), I see that we can expect flags to always be present here:

/** * The `flags` story is a bit complicated: * * Absent in `event.message` for a `message` event... but instead * (confusingly) there is an `event.flags`. * * Present under `EVENT_NEW_MESSAGE`. * * Present under `MESSAGE_FETCH_COMPLETE`, and in the server `/message` * response that action is based on. * * Absent in the Redux `state.messages`; we move the information to a * separate subtree `state.flags`. */

Probably a check like the one in unreadMentionsReducer would be cleaner (fewer cases to reason about) than accepting a missing flags as valid input:

if (!flags) { throw new Error('action.message.flags should be defined.'); }

though I would do it more concisely with invariant, which I think we started using regularly after that bit of code was written.

I took a look at how to do this, but I'm actually even more confused now: I don't understand how flags ends up being a property of the message, rather than the event. From this, it sounds like the server will send down a EVENT_NEW_MESSAGE with flags set on the event, but the code that I wrote looks for flags under the message in the event, and I don't understand why that works.

Specifically, the blocker to making this change is that when I try to make it, a bunch of tests fail, since the tests don't set the flags on the message. Do you know what's going on with that?

The thing that fills in action.message.flags in live, non-test code is the commented-on line in this bit of src/events/eventToAction.js:

case 'message': return { type: EVENT_NEW_MESSAGE, id: event.id, message: { ...event.message, // Move `flags` key from `event` to `event.message` for // consistency; default to empty if `event.flags` is not set. flags: event.message.flags ?? event.flags ?? [], avatar_url: AvatarURL.fromUserOrBotData({ rawAvatarUrl: event.message.avatar_url, email: event.message.sender_email, userId: event.message.sender_id, realm: getCurrentRealm(state), }), }, local_message_id: event.local_message_id, caughtUp: state.caughtUp, ownUserId: getOwnUserId(state), };

In test code, it looks like eg.streamMessage and eg.pmMessage don't put .flags in the Message objects they return.

:(

There's this line in messagePropertiesBase in exampleData.js (that's a thing those functions use to fill in boring properties common to all example Message objects):

// flags omitted

And eg.streamMessage and eg.pmMessage don't bother to fill it in either.

We could have eg.pmMessage and eg.streamMessage take a flags param; that seems useful here and elsewhere. I'm not sure if the right fallback for when flags isn't passed is to omit flags or use an empty array, though. I'm not sure if there are current tests that depend on flags being omitted. If there are, they probably deal with this case:

* * Absent in the Redux `state.messages`; we move the information to a * separate subtree `state.flags`.

(That's from the comment on Message.flags in modelTypes.js.)

Hmm, and after having eg.pmMessage and eg.streamMessage return an (empty-array) .flags, I see some unhelpful-looking Jest messages. I expect those'll be made actionable when we start using Jest 26, in #4700, and we get jestjs/jest#10414. (I recall that we specifically wanted that in 64eaab4, when we "took" Jest 26, but then we silently slipped back to 25 a few weeks later, in c4fca9d, when we added jest-expo back; jest-expo's effect there is described in #4700; 836a8d7 in the current revision.)

Unless you'd like to push further, I'd say it's fine to leave a quick comment in each place, saying that we should (and are prepared to) error on a missing action.message.flags in EVENT_NEW_MESSAGE, except that our tests will have to catch up before we can do that.

Yeah, the situation where there are subtly-different types of message objects in different contexts (with or without flags) is kind of confusing.

I think omitting flags is the right thing for eg.pmMessage and eg.streamMessage, because that's what's expected in places like state.messages. But we should certainly make it easier for tests to get data that supplies it when that's what's needed.

I've just sent #4760 which generalizes mkMessageAction -- a test helper whose basically one job is to supply a default flags of [] -- into a new eg.mkActionEventNewMessage. Then after converting to that everywhere, we get to turn these into assertions.

Hmm, and after having eg.pmMessage and eg.streamMessage return an (empty-array) .flags, I see some unhelpful-looking Jest messages. I expect those'll be made actionable when we start using Jest 26, in #4700, and we get facebook/jest#10414.

FTR I just tried this, now that #4700 is in. The Jest error messages are clear now, and all center on diffs like this:

- "flags": Array [],

They're all in messagesReducer-test, and have the form "this message gotten from the resulting messages state didn't have a flags, and one was expected". Having no flags is the correct behavior there, so this illustrates that we need to continue to have an easy way for test code to make a Message object with no flags.

chrisbobbe · 2021-05-04T21:18:35Z

src/unread/unreadModel.js

@@ -146,7 +145,7 @@ function streamsReducer(
        return state;
      }

-      if (message.sender_id === getOwnUserId(globalState)) {
+      if (message.flags?.includes('read')) {


(also here, the thing about invariant)

chrisbobbe · 2021-05-04T21:18:44Z

src/unread/unreadPmsReducer.js

@@ -24,7 +24,7 @@ const eventNewMessage = (state, action) => {
    return state;
  }

-  if (action.ownUserId === action.message.sender_id) {
+  if (action.message.flags?.includes('read')) {


WesleyAC · 2021-05-06T07:51:21Z

Thanks for the review @chrisbobbe! Added typechecking. I'm confused about the invariant thing, left a reply about that.

chrisbobbe

Glad to have that test file type-checked! It looks good to me. I'll reply about invariant inline, above.

chrisbobbe · 2021-05-07T00:02:34Z

src/unread/__tests__/unreadMentionsReducer-test.js

-          flags: [],
-        },
-      });
+      const action = mkMessageAction(eg.streamMessage({ flags: [] }));


Nice! Glad to get more use out of Greg's mkMessageAction. Nice that it reduces the number of needed lines.

Previously, the main case where we wouldn't add a message to unreads was if we were the one who sent it. This check was not correct - instead, we want to check to see if the message has the "read" flag, which is set by the server. In the past, these checks were very close to the same (with the exception of "Notification Bot" announcing a stream that you created, which would be "read" for you), but with the addition of muted users, the check is now incorrect. This commit removes the logic to not mark a message as read when we are the one who sent it, and instead just looks at the server flag, since that should be the canonical place for that information to be.

If the server sends us a message that isn't marked as read, we should always add it to unreads. However, we previously didn't do this in the case of a self-PM. This commit fixes this, even though we don't expect this to ever happen.

WesleyAC · 2021-05-07T05:22:44Z

Added the comment you requested @chrisbobbe, would appreciate you looking it over :)

WesleyAC · 2021-05-21T13:17:37Z

Going to go ahead and merge this, since there's no response and IIRC I only added a comment since the last review.

chrisbobbe · 2021-05-21T15:53:18Z

Going to go ahead and merge this, since there's no response and IIRC I only added a comment since the last review.

Makes sense, thanks!

gnprice · 2021-05-24T04:01:59Z

Thanks @WesleyAC and @chrisbobbe! I added a reply on the thread above at #4710 (comment) .

WesleyAC requested review from gnprice and chrisbobbe May 1, 2021 09:06

WesleyAC mentioned this pull request May 1, 2021

Muted users: part 2 #4699

Merged

WesleyAC force-pushed the fix-unreads-read-flag branch from 5076aae to 56b2394 Compare May 4, 2021 09:45

chrisbobbe reviewed May 4, 2021

View reviewed changes

WesleyAC force-pushed the fix-unreads-read-flag branch from 56b2394 to 606161c Compare May 6, 2021 07:31

chrisbobbe reviewed May 7, 2021

View reviewed changes

WesleyAC added 3 commits May 7, 2021 13:21

unreads: Fix latent bug with unread self-PMs.

e36d08a

If the server sends us a message that isn't marked as read, we should always add it to unreads. However, we previously didn't do this in the case of a self-PM. This commit fixes this, even though we don't expect this to ever happen.

unreads tests [nfc]: Enable flow.

8cac82c

WesleyAC force-pushed the fix-unreads-read-flag branch from 606161c to 8cac82c Compare May 7, 2021 05:21

chrisbobbe mentioned this pull request May 10, 2021

Have Jest cover Android-only codepaths. #4700

Merged

WesleyAC merged commit fd93443 into zulip:master May 21, 2021

gnprice mentioned this pull request May 24, 2021

example data: Better support EVENT_NEW_MESSAGE. #4760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unreads: Don't add messages with "read" flag to unreads. #4710

unreads: Don't add messages with "read" flag to unreads. #4710

WesleyAC commented May 1, 2021

gnprice commented May 3, 2021

WesleyAC commented May 4, 2021

chrisbobbe left a comment •

edited

chrisbobbe May 4, 2021

WesleyAC May 6, 2021

chrisbobbe May 7, 2021 •

edited

chrisbobbe May 7, 2021 •

edited

gnprice May 24, 2021

gnprice May 24, 2021

chrisbobbe May 4, 2021

chrisbobbe May 4, 2021

WesleyAC commented May 6, 2021

chrisbobbe left a comment

chrisbobbe May 7, 2021

WesleyAC commented May 7, 2021

WesleyAC commented May 21, 2021

chrisbobbe commented May 21, 2021

gnprice commented May 24, 2021

unreads: Don't add messages with "read" flag to unreads. #4710

unreads: Don't add messages with "read" flag to unreads. #4710

Conversation

WesleyAC commented May 1, 2021

gnprice commented May 3, 2021

WesleyAC commented May 4, 2021

chrisbobbe left a comment • edited

Choose a reason for hiding this comment

chrisbobbe May 4, 2021

Choose a reason for hiding this comment

WesleyAC May 6, 2021

Choose a reason for hiding this comment

chrisbobbe May 7, 2021 • edited

Choose a reason for hiding this comment

chrisbobbe May 7, 2021 • edited

Choose a reason for hiding this comment

gnprice May 24, 2021

Choose a reason for hiding this comment

gnprice May 24, 2021

Choose a reason for hiding this comment

chrisbobbe May 4, 2021

Choose a reason for hiding this comment

chrisbobbe May 4, 2021

Choose a reason for hiding this comment

WesleyAC commented May 6, 2021

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe May 7, 2021

Choose a reason for hiding this comment

WesleyAC commented May 7, 2021

WesleyAC commented May 21, 2021

chrisbobbe commented May 21, 2021

gnprice commented May 24, 2021

chrisbobbe left a comment •

edited

chrisbobbe May 7, 2021 •

edited

chrisbobbe May 7, 2021 •

edited