narrow [nfc]: Cut Narrow from the Outbox type.
#3889
Conversation
| // These two are uncommon cases it'd take some work to get right; just | ||
| // leave the outbox messages out. |
There was a problem hiding this comment.
It's almost certainly not worth the trouble to fix at the moment, but I'd suggest explicitly marking this as a // BUG:, and possibly filing an issue.
| })(); | ||
| // prettier-ignore | ||
| const to = | ||
| item.type === 'private' |
| // This will include the self user, possibly twice. That's | ||
| // fine; on send, the server (since at least 2013) drops dupes | ||
| // and normalizes whether to include the sender. | ||
| ? item.display_recipient.map(r => r.email).join(',') |
There was a problem hiding this comment.
I wouldn't want to just .slice off an element – but it does feel like the self-user should be .filtered out here, rather than relying on the permissiveness of the server implementation.
Also, if these are emails rather than IDs, we should probably use JSON rather than CSV. (If I'd realized that's what they were, I would have done the additional testing on #3734. 😞) I'm not about to ask you to do that for this PR, but tagging it with an inline TODO: would be appreciated.
There was a problem hiding this comment.
In #4299 I ended up taking care of that duplicate self-user by fixing how we construct display_recipient in the first place, elsewhere in this file. Having it wrong there turns out to cause its own buggy behavior, before any request even reaches the server.
| needle.type === 'stream' | ||
| && needle.display_recipient === streamName | ||
| && needle.subject === topic, | ||
| pm: emails => emails === needle.display_recipient.map(r => r.email).join(','), |
There was a problem hiding this comment.
This line (pm: emails => ...) doesn't look like it will work, for multiple reasons:
- It'll fail if
narrow[0].operandanddisplay_recipientare not in the same order. (In practice, we seem to frequently rely on serializations being consistent. 😞) - If one of your comments in a later commit is accurate – and, more questionably, if I'm understanding the code correctly – the left-hand side will lack the self-user, while the right-hand side will have it.
- Both of the above bug(?)s are concealed by the fact that there's no
groupPmcase in thiscaseNarrowPartial. (I'd suggest usingcaseNarrowhere instead.) - It's a
TypeErrorwhenneedle.type === 'stream', as"stream name".map is not a function.
There was a problem hiding this comment.
Eek, good catch re: missing the groupPm case!
I suspect what happened there is that this change (and the others in this branch) was originally taken from a longer Narrow-refactoring series where it came after a refactor to unify the pm and groupPm cases. I have a couple of longer Narrow-refactoring series as drafts, and I'm pretty sure I made that unification somewhere in there. Definitely caseNarrow, the total version, is the right one to use here for just that sort of reason; I have no recollection of what I might have been thinking when I wrote caseNarrowPartial instead.
- It'll fail if
narrow[0].operandanddisplay_recipientare not in the same order. (In practice, we seem to frequently rely on serializations being consistent. disappointed)
Yeah, and I believe that's already the case in the existing version of this code, as a result of just string-comparing the results of JSON.stringify. Given that (which I'll re-check empirically, because although I think I did so originally, it's been some months since then and I'm not certain I did), I'm OK with preserving that behavior in the course of this basically orthogonal refactor. Probably deserves a comment about it, though.
- If … the left-hand side will lack the self-user, while the right-hand side will have it.
- It's a
TypeErrorwhenneedle.type === 'stream', as"stream name".map is not a function.
Hmm. Will check on these. And probably write some unit tests for this function, as I seem to have demonstrated the need for them >_>
There was a problem hiding this comment.
JSON.stringify… which I'll re-check empirically
Ah, no, here it is more directly:
- In the old code, we're comparing the
JSON.stringifyof each narrow, which means the sequences of operators and operands must be equal. In particular,narrow[0].operandmust be equal. - The
display_recipienton an outbox message is constructed from the sameNarrowvalue (passed toaddToOutbox) that in the old code was included as a property. A quick look at the private helpers ofaddToOutboxshows that it's computed like so:
narrow[0].operand
.split(',')
.map(item => {
const user = usersByEmail.get(item) || NULL_USER;
return { email: item, id: user.user_id, full_name: user.full_name };
})
.concat({ email: selfDetail.email, id: selfDetail.user_id, full_name: selfDetail.full_name });So yep, we've been counting on the order of these.
I think that same look at the code also confirms that the display_recipient will have the self user added at the end, like you said, while the narrow's list of emails won't.
There was a problem hiding this comment.
In #4299 I ended up dropping this implementation entirely... because there's already an isMessageInNarrow! Which I noticed partly as a consequence of #3889 (comment) above. That function needed some significant work to become well tested, and then that testing turned up some bugs which needed fixing; those changes were #4294, and #4299 returns to the outbox code to switch to using that.
When writing a call to a function that has jsdoc, VS Code shows a handy popup with the documentation. It shows first the text for the parameter you're currently typing, then the text for the function as a whole. That popup was showing the "See also" as part of the last parameter's documentation, rather than that for the function as a whole. In particular this means it was only visible when typing the last parameter. Fix the jsdoc parse, by moving everything that isn't part of a parameter's documentation to before the first @param marker.
This was never quite an accurate fit for what we need here: it gives the wrong answers for the @-mentions narrow. More broadly, it's sort of an accident that the inclusion relation on *narrows* even mostly corresponds to the question we're actually asking here. We're really interested in a narrow on one side, and an individual *message* (specifically an outbox message) on the other.
As the TODO implicitly suggests, the condition that really says what we mean here is `item.type === private`. And indeed that is exactly equivalent to the existing code. This code gets the Outbox objects to act on from the Redux store at `state.outbox`. That's populated entirely by MESSAGE_SEND_START actions, which are dispatched only from `addToOutbox`. And those get their `type` from `extractTypeToAndSubjectFromNarrow`, and store as `narrow` the same `Narrow` value they passed there. That will produce `type: 'private'` just if `isPrivateOrGroupNarrow(narrow)`, so the latter condition is equivalent to the former.
This was the last place we were using the `narrow` property on outbox messages, so taking it out will mean we can remove that property in the next commit. Which is good because (a) the `Narrow` type is a great big mess, and the fewer different parts of the code are using it, the fewer need to be migrated to a cleaned-up version of it; and (b) it was never an appropriate type to use to describe a single message's destination. I don't love the "display recipient" values either, but their structure is at least less wild than Narrow, and their semantic range is an appropriate match for "where this individual message is sent". And whenever we decide it's that type's turn to get straightened out, this usage should fit in nicely to be swept up with all the others, because it comes right after a check of the display_recipient's friend the `type: 'stream' | 'private'` field. This commit makes a small change in the data we send to the server for sending a PM: compared to the old code's behavior, this code will always differ by including one more email at the end of the list, namely the self-user's email. (To see that that's exactly what changes, look at `mapEmailsToUsers`, which produces the `display_recipient` value.) The comment in the code explains why that causes no functional change. That comment, in turn, is based on reading both the code as of zulip/zulip@97d7d31b6 (in 2013-03), the same commit referred to in our e4a83be, and also as of current master (zulip/zulip@fa1059aa2). The relevant logic lives today in get_recipient_from_user_profiles, and is essentially unchanged from the 2013 version. (We could have arranged to send the server exactly the same data as the old code, by slicing off the last element of `display_recipient`. But that feels like it'd be more brittle, relying on what's really an implementation detail of `mapEmailsToUsers`.)
Made possible by the preceding series of changes.
gnprice
force-pushed
the
pr-outbox-no-narrow
branch
from
b327f5d
to
f09a0f2
Compare
|
@gnprice, would you like me to look closely at this one? |
|
Not yet, thanks -- still needs some further revisions. I'll be sure to post a comment on this thread when it's ready for another look. |
|
I've pushed the first commit of this branch, which is a pure jsdoc change, as 31ecb8d. (Still working on the more substantial changes.) |
Our
Narrowtype is kind of a mess: it's defined as just an array of operator/operand string pairs, but in practice most of our code assumes it has a lot more structure than that. There's also a lot of scattered places where we parse that structure, and so express a variety of those assumptions. Separately, it's a major source of #3764 in that it identifies users by email, not ID, for PM threads (as well as streams by name, not ID); and the many scattered places we parse its implicit structure are each obstacles to fixing that.I spent some time last week sketching out what it would look like to migrate
Narrowto something that would use IDs rather than emails or stream names, and also be more explicitly structured so that e.g. after conditioning on being a PM conversation we'd have something likeuserIds: number[]rather than having to parse a string. I found it actually seems surprisingly doable, and I have a rough draft in a branch.One piece is that there are places where we just shouldn't be using a "narrow" value at all, because we're not actually talking about a feed of messages or a possible message list -- we're talking about one message and who it goes to. The cleanest way to migrate those is to just get them off
Narrowentirely. One of those places is a property onOutbox; this branch fixes that.