-
-
Notifications
You must be signed in to change notification settings - Fork 643
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.
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
unreads: Don't add messages with "read" flag to unreads. #4710
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the comment on /**
* 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 if (!flags) {
throw new Error('action.message.flags should be defined.');
} though I would do it more concisely with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing that fills in 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 :( There's this line in // flags omitted And We could have
(That's from the comment on Hmm, and after having There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I think omitting I've just sent #4760 which generalizes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
return state; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,6 @@ import { | |
MESSAGE_FETCH_COMPLETE, | ||
REALM_INIT, | ||
} from '../actionConstants'; | ||
import { getOwnUserId } from '../users/userSelectors'; | ||
|
||
// | ||
// | ||
|
@@ -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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (also here, the thing about |
||
return state; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (and here) |
||
return state; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.