unreads: Fix false positives in a debug check; log in another case when helpful #1150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PIG208
reviewed
Jan 3, 2025
test/model/unreads_test.dart
Outdated
| }); | ||
| } | ||
|
|
||
| doTestCommon('unread DM message', unreadDmMessage.id, true); |
There was a problem hiding this comment.
nit: Should we make the third parameter use a keyword? Just by looking at it it is not so obvious what this is supposed to mean.
There was a problem hiding this comment.
Sure; made this a named param.
We don't expect UpdateMessageEvent to signal a change in a message's read state; that's what UpdateMessageFlagsEvent is for. Our unreads model does process `UpdateMessageEvent`s, though, to apply changes in a message's mentioned state. Also, we expect the client to be synced with the server for all messages' read state, assuming !oldUnreadsMissing. Where we process `UpdateMessageEvent`s, we have a `debugLog` line to alert us if one of these expectations isn't being met. It's been giving false positives when we get an UpdateMessageEvent for a bulk message move where some of the moved messages are read and some aren't. Since the server only intends the event's `flags` to apply to the event's `messageId` (one message), we should only be checking that message, not all the messages in the event's `messageIds`. From the API doc on `flags`: https://zulip.com/api/get-events#update_message > The user's personal message flags for the message with ID > message_id following the edit.
And make it public, with dartdoc and tests, while we're at it.
For this debug check to work, it just needs a known read state for one message. When `oldUnreadsMissing` is false (the common case), the model holds one of two answers for a message's read state: known-unread or known-read. When `oldUnreadsMissing` is true, the model holds known-unread or unknown. Before, we were skipping the debug check on seeing that `oldUnreadsMissing` is true. That's earlier than we need to abort, because the model might still hold a known-unread state for the message. Now, we go ahead with the debug check if that's the case, and still skip it otherwise. Done by using our new `isUnread` helper, in this separate commit because it's not NFC in debug mode.
chrisbobbe
force-pushed
the
pr-unreads-debug-log-fix
branch
from
2f7f021 to
7d64e2f
Compare
|
Thanks for the review! Revision pushed. Since you had just one nit, which I've fixed, I'll go ahead and mark for Greg's review. |
chrisbobbe
added
integration review
|
Thanks! Looks good; merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In manual testing for my work on resolve/unresolve #744, I noticed some surprising debug-log lines that turned out to be false positives, so this fixes those.
It also makes the debug check run in a helpful case where it wasn't running before.