recipient: Finish reconciling the ways we identify a PM conversation#4356
recipient: Finish reconciling the ways we identify a PM conversation#4356gnprice merged 6 commits intozulip:masterfrom
Conversation
| if (userIds.length === 1) { | ||
| // A 1:1 PM. Both forms include just one user: the other user if any, | ||
| // and self for a self-1:1. | ||
| return userIds[0].toString(); |
There was a problem hiding this comment.
Are you thinking we'll eventually combine state.unread.pms and state.unread.huddles into one bucket with a shared key space, as part of dissolving unhelpful distinctions between group and 1:1 PMs?
There was a problem hiding this comment.
Yeah, I think that'd make sense. There's this comment already in pmConversationsSelectors.js:
unread:
// This business of looking in one place and then the other is kind
// of messy. Fortunately it always works, because the key spaces
// are disjoint: all `unreadHuddles` keys contain a comma, and all
// `unreadPms` keys don't.
/* $FlowFixMe: The keys of unreadPms are logically numbers, but because it's an object they
end up converted to strings, so this access with string keys works. We should probably use
a Map for this and similar maps. */
unreadPms[recipient.unreadsKey] || unreadHuddles[recipient.unreadsKey],That one is looking in a couple of cached-selector results, not state.unread.pms and state.unread.huddles themselves, but the selectors correspond directly to those.
| const unread = unreadHuddles.find(x => x.user_ids_string === unreadsKey); | ||
| return unread ? unread.unread_message_ids.length : 0; | ||
| } else { | ||
| const senderId = ids[0]; |
There was a problem hiding this comment.
Could call pmUnreadsKeyFromPmKeyIds to get senderId here, I suppose—as long as senderId and unreadPms[].sender_id are made to agree in being a number or a string (for the .find that they're used in here).
There was a problem hiding this comment.
Yeah. I think that will most naturally come after converting these to our own Immutable.Map or similar data structure, and perhaps after converting the both of these (state.unread.pms and state.unread.huddles) into a single such data structure.
With the data structures as they are, with .sender_id a number, if we ran ids through pmUnreadsKeyFromPmKeyIds we'd have to convert it back from string to number in order to do the search (or else convert every sender_id into a string to do the comparison, which would be inefficient.) That conversion would make sense only because we know the key isn't just any string but has a particular structure... but the same facts about its structure mean we can just use the number directly and skip converting to a string and back.
There was a problem hiding this comment.
Yeah. I think that will most naturally come after converting these to our own
Immutable.Mapor similar data structure, and perhaps after converting the both of these (state.unread.pmsandstate.unread.huddles) into a single such data structure.
Makes sense—I figured that was in the plan. 🙂
This sets us up to stop using emails here entirely in favor of user IDs; we'll do that in the next commit.
The way this change might affect behavior is if we receive a message where the email for some user doesn't agree (e.g. because the user changed their email) with one we've seen before, in particular one we've previously used to identify that narrow in `state.narrows`. The old code would fail to make the connection, and so wouldn't put the new message in the existing narrow, which could make it fail to show up in the message list for that conversation. After this change, we'll correctly place the new message in the narrow.
We deleted the last use of this function in the preceding commit. This in turn was one of two remaining callers of the quite hairy normalizeRecipients -- so deleting it gets us close to being able to delete that function too.
The hairy logic in normalizeRecipients was one of the last few cases of the confusion around PM recipients we're seeking to remove for zulip#4035. It also represents one of the many places we're in the process of converting from using emails to using numeric user IDs. The case where this could potentially change behavior is where we have several messages that are in the same PM conversation, but the `display_recipient` property on different messages disagrees about the email of some participant (e.g. because that user changed their email between when we learned about one message and when we learned about the other.) The old code would fail to notice the messages were in the same conversation, and so when showing them consecutively in an interleaved narrow (like all-messages or a search result), we'd show a new recipient bar between them. With this fix, we correctly identify the messages as belonging to the same conversation.
As a bonus this eliminates one of the places using NULL_USER. The case where that could have affected behavior was... if looking at a PM conversation where one of the emails didn't correspond to any user we knew, perhaps because the user had changed their email. With this fix, we'll correctly show an UnreadNotice "N unread messages" banner if applicable. With the old code, this selector would return 0 and so we wouldn't show the banner.
This lets us take out one more small bit of ad-hoc implementation of this sort of logic. In fact, after all the previous work we've done in this direction, this one appears to have been the last remaining such ad-hoc implementation! So this completes zulip#4035. Fixes: zulip#4035
gnprice
force-pushed
the
pr-recipient-cleanup
branch
from
909b69f to
e7315cd
Compare
|
Thanks for the review! Merged. |
This completes a major objective of the long string of refactoring that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342, then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368. After this change, the portion of zulip#4333 that's about PMs, emails, and user IDs -- aka the portion of zulip#3764 that's about narrows -- is complete. Still open from zulip#4333 is to convert stream and topic narrows from stream names to stream IDs. I'm hopeful that that will be simpler: (a) unlike the situation for PMs, there's just one stream mentioned at a time, so there's no question of sorting, and (b) there isn't the "include self or not" complication that's bedeviled much of our code related to PMs. In other words, stream and topic narrows don't suffer from zulip#4035.
2 participants
This is the next PR, following #4346, in our series straightening out how we identify narrows and especially PM narrows. It takes some further steps toward #4333 and #3764, converting some narrow-consumers to use user IDs rather than emails; and it completes the last remaining pieces of #4035.
Along the way, we fix some bugs that potentially hit if a user changes their email address, so that we potentially have data reflecting both their old and new email addresses. This will probably be a recurring feature of many of the individual conversions to using user IDs that make up #3764.
Fixes: #4035