recipient [nfc]: Clean up and document how we compute unread_msgs keys.#4040
recipient [nfc]: Clean up and document how we compute unread_msgs keys.#4040rk-for-zulip merged 6 commits intozulip:masterfrom
unread_msgs keys.#4040Conversation
gnprice
force-pushed
the
pm-recipients-unread
branch
from
3c7bed6 to
4df4644
Compare
|
Tests are failing. (You may just need to rebase.) |
Though the name of this function doesn't yet say so, it's really all about navigating the `unread` state subtree we maintain from the server's `unread_msgs` data structure. Perhaps that's why it hadn't taken account of the possibility of a self-1:1 appearing -- such a message has to be *sent* by oneself, so it's a bit of a surprising edge case that it can be unread. But it can, if e.g. you send yourself a PM through the API. Or perhaps the reason is that the code already worked in that case! This change just makes that more explicit, rather than seeming like an accident.
This will let us switch to passing one of these helpers a user ID in the next commit. Also fill in some missing pieces of data in these ill-typed tests, without which they start failing.
gnprice
force-pushed
the
pm-recipients-unread
branch
from
4df4644 to
8998aec
Compare
|
Thanks, fixed. The issue was some ill-typed tests that didn't have the needed bit of the Redux state tree. (And the closing and reopening of the PR just now were because I went to click on the add-comment text box to focus it for typing... and then it jumped around as the "checks failed" box changed because I'd just pushed the branch, and the "Close pull request" button popped under my cursor instead.) |
Not that we would _intentionally_ sort this lexicographically -- but it's an easy trap to fall into with `Array.prototype.sort`, and it's probably worth noting that we didn't.
rk-for-zulip
left a comment
rk-for-zulip
left a comment
There was a problem hiding this comment.
Adverb added (in a trivial follow-on commit). Merging!
| * @param ownEmail - Required if the message could be a 1:1 PM; optional if | ||
| * it is definitely a group PM. |
There was a problem hiding this comment.
Yuck. I think this should really be always-required, just for sanity's sake. (Which would have been trivially possible before this PR, but would be more involved afterward.)
Not that I'm saying this PR should be delayed for that – just noting it as follow-up work under #3764 (converting EventNewMessageAction's ownEmail: to ownUserId: or ownUser:).
| */ | ||
| // Specifically, this includes all user IDs for group PMs and self-PMs, | ||
| // and just the other user ID for non-self 1:1s; and in each case the list | ||
| // is sorted and encoded in ASCII-decimal, comma-separated. |
There was a problem hiding this comment.
"Sorted" should probably specify "numerically" (as opposed to "lexicographically").
I hate that I can say that with a straight face, but it is what it is. 😞
|
Thanks for the review! |
This reverts the following commits (which constitute PR #4040): * a111cfc * d8f4512 * d464e33 * 79c8671 * 423cd19 * 2a2a2e1 Commit 79c8671 introduced a crash bug. `getOwnUser` relies on the presence of the user database which is provided by REALM_INIT, but it's being called as part of the instantiation of `UnreadCards` before REALM_INIT occurs.
|
... and, of course, now I see that there was a "Revert" button right there. (I swear I looked for it.) At any rate, as noted in the commit message, this caused a crash:
It can be reproduced as follows:
(I don't appear to have the option to reopen this PR, alas. Do you?) |
Nope -- I tried putting the branch back (there's a button for that, even: Not sure I can even disagree with that product choice, as it's a fact that the PR was merged, even if there's a revert commit after it. I'll make a new PR for a revised version. |
The commit 084d726 "pms / recipient: Revert PR #4040." reverted a series of 6 earlier commits: a111cfc recipient [nfc]: Add minor documentation clarification. d8f4512 recipient [nfc]: Simplify pmUnreadsKeyFromMessage a bit. d464e33 recipient [nfc]: Stop using email in pmUnreadsKeyFromMessage. 79c8671 pm-conversations [nfc]: Get ownUser rather than just ownEmail. 423cd19 unread [nfc]: Give getRecipientsIds a more informative name, and some jsdoc. 2a2a2e1 unread [nfc]: Handle self-1:1 thread explicitly in getRecipientsIds. But the bug being addressed was in specifically commit 79c8671, so there's no need to revert the two commits that preceded it. There's also no need to revert a111cfc, a documentation fix that doesn't interact with the code added in 79c8671. So, restore those changes.
I think when I originally wrote this change, I hadn't yet done most of the investigations that led to zulip#4040 and wasn't as confident of how to handle the many ways we represent PM conversations. But now I have.
2 participants
This is part of #4035, specifically the main part of it that's blocking #3535.
We have a function
getRecipientsIdswhose real meaning is that it computes the key to use to look up a PM conversation in ourunreadstate-subtree, derived from theunread_msgsdata structure from the server. This series gives it a more meaningful name,pmUnreadsKeyFromMessage; adds jsdoc and comments; and cleans up the interface and implementation a bit, including by switching from emails to user IDs (a case of #3764).