Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from May 21, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a10708
unreads: Don't add messages with "read" flag to unreads.
WesleyAC May 1, 2021
e36d08a
unreads: Fix latent bug with unread self-PMs.
WesleyAC May 4, 2021
8cac82c
unreads tests [nfc]: Enable flow.
WesleyAC May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/unread/__tests__/unreadHuddlesReducer-test.js
View file
Open in desktop
Expand Up @@ -118,16 +118,17 @@ describe('unreadHuddlesReducer', () => {
expect(actualState).toBe(initialState);
});

test('if message is sent by self, do not mutate state', () => {
test('if message has "read" flag, do not mutate state', () => {
const selfUser = { ...eg.selfUser, user_id: makeUserId(1) };
const user2 = { ...eg.otherUser, user_id: makeUserId(2) };
const user3 = { ...eg.thirdUser, user_id: makeUserId(3) };

const initialState = deepFreeze([]);

const message2 = eg.pmMessage({
sender: selfUser,
sender: user2,
recipients: [selfUser, user2, user3],
flags: ['read'],
});

const action = deepFreeze({
Expand Down
74 changes: 41 additions & 33 deletions src/unread/__tests__/unreadMentionsReducer-test.js
View file
Open in desktop
@@ -1,13 +1,12 @@
/* @flow strict-local */
import deepFreeze from 'deep-freeze';
import Immutable from 'immutable';

import unreadMentionsReducer from '../unreadMentionsReducer';
import {
REALM_INIT,
ACCOUNT_SWITCH,
EVENT_NEW_MESSAGE,
EVENT_UPDATE_MESSAGE_FLAGS,
} from '../../actionConstants';
import { ACCOUNT_SWITCH, EVENT_UPDATE_MESSAGE_FLAGS } from '../../actionConstants';
import { NULL_ARRAY } from '../../nullObjects';
import * as eg from '../../__tests__/lib/exampleData';
import { mkMessageAction } from './unread-testlib';

describe('unreadMentionsReducer', () => {
describe('ACCOUNT_SWITCH', () => {
Expand All @@ -16,6 +15,7 @@ describe('unreadMentionsReducer', () => {

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

const expectedState = [];
Expand All @@ -30,17 +30,19 @@ describe('unreadMentionsReducer', () => {
test('received data from "unread_msgs.mentioned" key replaces the current state ', () => {
const initialState = deepFreeze([]);

const action = deepFreeze({
type: REALM_INIT,
const action = {
...eg.action.realm_init,
data: {
...eg.action.realm_init.data,
unread_msgs: {
streams: [{}, {}],
huddles: [{}, {}, {}],
pms: [{}, {}, {}],
...eg.action.realm_init.data.unread_msgs,
streams: [],
huddles: [],
pms: [],
mentions: [1, 2, 3],
},
},
});
};

const expectedState = [1, 2, 3];

Expand All @@ -54,13 +56,17 @@ describe('unreadMentionsReducer', () => {
test('if message does not contain "mentioned" flag, do not mutate state', () => {
const initialState = deepFreeze([]);

const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message: {
id: 2,
flags: [],
},
});
const action = mkMessageAction(eg.streamMessage({ flags: [] }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const actualState = unreadMentionsReducer(initialState, action);

expect(actualState).toBe(initialState);
});

test('if message has "read" flag, do not mutate state', () => {
const initialState = deepFreeze([]);

const action = mkMessageAction(eg.streamMessage({ flags: ['mentioned', 'read'] }));

const actualState = unreadMentionsReducer(initialState, action);

Expand All @@ -70,13 +76,7 @@ describe('unreadMentionsReducer', () => {
test('if message id already exists, do not mutate state', () => {
const initialState = deepFreeze([1, 2]);

const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message: {
id: 2,
flags: ['mentioned'],
},
});
const action = mkMessageAction(eg.streamMessage({ id: 2, flags: ['mentioned', 'read'] }));

const actualState = unreadMentionsReducer(initialState, action);

Expand All @@ -86,13 +86,7 @@ describe('unreadMentionsReducer', () => {
test('if "mentioned" flag is set and message id does not exist, append to state', () => {
const initialState = deepFreeze([1, 2]);

const action = deepFreeze({
type: EVENT_NEW_MESSAGE,
message: {
id: 3,
flags: ['mentioned'],
},
});
const action = mkMessageAction(eg.streamMessage({ id: 3, flags: ['mentioned'] }));

const expectedState = [1, 2, 3];

Expand All @@ -108,6 +102,9 @@ describe('unreadMentionsReducer', () => {

const action = {
type: EVENT_UPDATE_MESSAGE_FLAGS,
id: 0,
all: false,
allMessages: Immutable.Map(),
messages: [1, 2, 3],
flag: 'star',
op: 'add',
Expand All @@ -123,6 +120,9 @@ describe('unreadMentionsReducer', () => {

const action = deepFreeze({
type: EVENT_UPDATE_MESSAGE_FLAGS,
id: 0,
all: false,
allMessages: Immutable.Map(),
messages: [2],
flag: 'read',
op: 'add',
Expand All @@ -138,6 +138,9 @@ describe('unreadMentionsReducer', () => {

const action = deepFreeze({
type: EVENT_UPDATE_MESSAGE_FLAGS,
id: 0,
all: false,
allMessages: Immutable.Map(),
messages: [2, 3],
flag: 'read',
op: 'add',
Expand All @@ -155,6 +158,9 @@ describe('unreadMentionsReducer', () => {

const action = deepFreeze({
type: EVENT_UPDATE_MESSAGE_FLAGS,
id: 0,
all: false,
allMessages: Immutable.Map(),
messages: [1, 2],
flag: 'read',
op: 'remove',
Expand All @@ -170,6 +176,8 @@ describe('unreadMentionsReducer', () => {

const action = deepFreeze({
type: EVENT_UPDATE_MESSAGE_FLAGS,
id: 0,
allMessages: Immutable.Map(),
messages: [],
flag: 'read',
op: 'add',
Expand Down
4 changes: 2 additions & 2 deletions src/unread/__tests__/unreadModel-test.js
View file
Open in desktop
Expand Up @@ -84,10 +84,10 @@ describe('stream substate', () => {
expect(state.streams).toBe(baseState.streams);
});

test('if message is sent by self, do not mutate state', () => {
test('if message has "read" flag, do not mutate state', () => {
const state = reducer(
baseState,
action(eg.streamMessage({ sender: eg.selfUser })),
action(eg.streamMessage({ sender: eg.selfUser, flags: ['read'] })),
eg.plusReduxState,
);
expect(state).toBe(baseState);
Expand Down
30 changes: 28 additions & 2 deletions src/unread/__tests__/unreadPmsReducer-test.js
View file
Open in desktop
Expand Up @@ -111,11 +111,12 @@ describe('unreadPmsReducer', () => {
expect(actualState).toBe(initialState);
});

test('if message is sent by self, do not mutate state', () => {
test('if message is marked read, do not mutate state', () => {
const initialState = deepFreeze([]);
const message1 = eg.pmMessage({
sender: eg.selfUser,
sender: eg.otherUser,
recipients: [eg.otherUser, eg.selfUser],
flags: ['read'],
});

const action = deepFreeze({
Expand All @@ -129,6 +130,31 @@ describe('unreadPmsReducer', () => {
expect(actualState).toBe(initialState);
});

test('if message is marked unread in self-PM, append to state', () => {
const initialState = deepFreeze([]);
const message1 = eg.pmMessage({
sender: eg.selfUser,
recipients: [eg.selfUser],
});

const action = deepFreeze({
...eg.eventNewMessageActionBase,
message: message1,
ownUserId: eg.selfUser.user_id,
});

const expectedState = [
{
sender_id: eg.selfUser.user_id,
unread_message_ids: [message1.id],
},
];

const actualState = unreadPmsReducer(initialState, action);

expect(actualState).toEqual(expectedState);
});

test('if message id does not exist, append to state', () => {
const message1 = eg.pmMessage({ id: 1, sender_id: 1 });
const message2 = eg.pmMessage({ id: 2, sender_id: 1 });
Expand Down
9 changes: 8 additions & 1 deletion src/unread/unreadHuddlesReducer.js
View file
Open in desktop
Expand Up @@ -24,7 +24,14 @@ const eventNewMessage = (state, action) => {
return state;
}

if (action.ownUserId === action.message.sender_id) {
// TODO: In reality, we should error if `flags` is undefined, since it's
// always supposed to be set. However, our tests currently don't pass flags
// into these events, making it annoying to fix this. We should fix the
// tests, then change this to error if `flags` is undefined. See [1] for
// details.
//
// [1]: https://github.com/zulip/zulip-mobile/pull/4710/files#r627850775
if (action.message.flags?.includes('read')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@chrisbobbe chrisbobbe May 7, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

@chrisbobbe chrisbobbe May 7, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return state;
}

Expand Down
3 changes: 3 additions & 0 deletions src/unread/unreadMentionsReducer.js
View file
Open in desktop
Expand Up @@ -46,6 +46,9 @@ export default (state: UnreadMentionsState = initialState, action: Action): Unre
if (!flags) {
throw new Error('action.message.flags should be defined.');
}
if (flags.includes('read')) {
return state;
}
return (flags.includes('mentioned') || flags.includes('wildcard_mentioned'))
&& !state.includes(action.message.id)
? addItemsToArray(state, [action.message.id])
Expand Down
10 changes: 8 additions & 2 deletions src/unread/unreadModel.js
View file
Open in desktop
Expand Up @@ -22,7 +22,6 @@ import {
MESSAGE_FETCH_COMPLETE,
REALM_INIT,
} from '../actionConstants';
import { getOwnUserId } from '../users/userSelectors';

//
//
Expand Down Expand Up @@ -146,7 +145,14 @@ function streamsReducer(
return state;
}

if (message.sender_id === getOwnUserId(globalState)) {
// TODO: In reality, we should error if `flags` is undefined, since it's
// always supposed to be set. However, our tests currently don't pass flags
// into these events, making it annoying to fix this. We should fix the
// tests, then change this to error if `flags` is undefined. See [1] for
// details.
//
// [1]: https://github.com/zulip/zulip-mobile/pull/4710/files#r627850775
if (message.flags?.includes('read')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also here, the thing about invariant)

return state;
}

Expand Down
11 changes: 9 additions & 2 deletions src/unread/unreadPmsReducer.js
View file
Open in desktop
Expand Up @@ -20,11 +20,18 @@ const eventNewMessage = (state, action) => {
return state;
}

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

if (action.ownUserId === action.message.sender_id) {
// TODO: In reality, we should error if `flags` is undefined, since it's
// always supposed to be set. However, our tests currently don't pass flags
// into these events, making it annoying to fix this. We should fix the
// tests, then change this to error if `flags` is undefined. See [1] for
// details.
//
// [1]: https://github.com/zulip/zulip-mobile/pull/4710/files#r627850775
if (action.message.flags?.includes('read')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and here)

return state;
}

Expand Down