outbox: Fix some bugs from Narrow confusion; remove Narrow #4299
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
gnprice
added
the
a-compose/send
chrisbobbe
reviewed
Nov 4, 2020
| @@ -34,6 +34,7 @@ describe('getMessagesForNarrow', () => { | |||
| }, | |||
| messages, | |||
| outbox: [], | |||
| realm: eg.realmState({ email: eg.selfUser.email }), | |||
There was a problem hiding this comment.
It looks like this is setting realm.email in the state because that value gets passed directly as ownEmail to isMessageInNarrow, having been selected by getOwnEmail (that getOwnEmail use having been added in this same commit).
I do notice that getOwnEmail has this in its JSDoc:
* Prefer using `getOwnUserId` or `getOwnUser`; see #3764.
(#3764)
It looks like getOwnUser itself uses getOwnEmail, so it's not obviously better, I guess.
But I could imagine that being changed someday; I could see getOwnUser instead using, e.g., getOwnUserId (there's even a comment on getOwnUserId suggesting to do so) and getUserForId.
If such a change were made, it would no longer be useful to just set state.realm.email; we'd want to set state.realm.user_id. Possibly that's totally fine, and a bridge we'll happily cross when we get to it. But I wondered if we might like to consider providing a simple way to grab state.realm from the example data such that it contains all the stuff that's specific to the self-user: there's email, user_id, probably more.
There was a problem hiding this comment.
But I wondered if we might like to consider providing a simple way to grab state.realm from the example data such that it contains all the stuff that's specific to the self-user: there's email, user_id, probably more.
Yeah, I agree -- that would be useful, and we'll want to add something like that. A more "batteries included" example Redux state, one might call it.
The reason I haven't yet gone and done that is that there are some API choices to be made about it:
- Probably it includes the self-user's email and related properties, as given by
eg.selfUser, instate.realm. - Probably it also includes
eg.selfUserinstate.users, and a corresponding account (witheg.realm) instate.accounts. - Does it include
eg.otherUserinstate.users? Alsoeg.thirdUser?eg.crossRealmBot? - There's
eg.streamandeg.subscription. Does it include those instate.streamsandstate.subscriptions? - Maybe there's even several different levels of "batteries included" that we want? In addition to probably still the initial state, the thing that's now
eg.reduxState. - What about when we add new example values along those lines, like the recent
thirdUser?- If they're included, that potentially changes the behavior of existing tests. If it makes them fail, that's one thing; more insidiously, it could potentially make them pass trivially, even if the code they're meant to test stops behaving the way they're meant to check that it does.
- If they're left out, then the "batteries included" state comes to include only some batteries, and potentially a less and less predictable subset of them.
- Probably the right answer is to include new example values, and make a practice of systematically writing tests so that they don't depend on any assumptions that the state doesn't have miscellaneous other data they weren't expecting. That is, if a test makes its own
eg.makeStream()call, it can assume that stream isn't in the example state; but it can't make an assumption like "there's only one stream in the state".- This requires a bit of work finding a clear pattern to prescribe for how to systematically do that.
This diff (as you saw, there are a good handful of lines like this one) definitely made me think about adding this. 🙂 Not enough that I felt I wanted to do it before shipping this branch, but it moved that feeling closer.
There was a problem hiding this comment.
Not enough that I felt I wanted to do it before shipping this branch, but it moved that feeling closer.
Makes sense, that's fine! So I'd say you can merge this branch at will.
This way the assumption about what `narrow[0].operand` is supposed to look like is right next to the conditional that ensures it does look like that.
This eliminates one more place we're mucking around with things like `narrow[0].operand`, outside of the `narrow` module itself.
Also to deactivated users, if that's a possible thing to do (I'm
not sure whether it is.)
This appears to be only a latent bug, with no user-visible
symptoms. But: if you send a PM with a cross-realm bot as
recipient, and use the debug console to look at the outbox message
in the state, you find that the display_recipient has an entry like
`{email: "[email protected]", id: -1, full_name: ""}`.
We're apparently relying on the email in the most relevant places
to interpret that value, avoiding visible symptoms... but this would
become a live bug when we transition any of that code to use numeric
IDs, as we've generally been doing.
That bogus data in the ID and name were taken from NULL_USER, in
that fallback over in mapEmailsToUsers. And that fallback was hit
because we're using the limited `getUsersByEmail` rather than the
comprehensive `getAllUsersByEmail`. Switch to the latter to fix it.
Now that we're working from the full dataset of all users -- including deactivated users and cross-realm bots -- nothing good can come in any case from trying to send a PM to a user who doesn't exist.
There's no way we should be able to get this far if we don't even have user data about self. Also make the types more normal.
This fixes a bug affecting sending self-PMs. The bug's current symptom is that when you hit send, the outbox message shows in the message list... with its own sender header with name and avatar, even when it's right after another message (which necessarily has the same sender, namely yourself.) If the send happens quickly, this shows up mostly as a flicker glitch, where the other messages move briefly up and then back down, because the message is taller with the sender. The root issue is that when constructing the display_recipient of an outbox PM, we were indiscriminately appending the self user after taking the list of users given in the Narrow value. Presumably this was motivated by the fact that display_recipient values for PMs list all participants including self, while for most PM narrows we leave out the self user... but for self-PMs, we don't. The result is that for self-PMs we were producing a malformed display_recipient, consisting of the self listed twice. The observed symptom then comes because in `renderMessages`, we see the outbox message has a different recipient from the previous one, and treat it as if we expected to show a recipient bar for it. (We don't actually show a recipient bar, but that causes us to show the sender header.) Fix the issue by only adding the self user if it's not already in the list. While we're here, also sort the recipients in a canonical way, by ID. Fixes: zulip#4301
The actual narrow passed to `addToOutbox` comes from `ComposeBox#getDestinationNarrow`, and is always something reasonable to send to -- in particular never a whole-stream narrow.
There's no such thing as sending "to the home narrow"; the `narrow` on an Outbox value is always a specific conversation.
This makes the mess the caller's responsibility -- which will make it much more straightforward for us to let other callers pass Outbox values, as we'd like to start doing.
For conversation narrows -- individual topics, and PM conversations -- this relied completely on our various representations of the narrow completely agreeing on the order of operators and operands. For PM conversations it also relied on the set of users identified (including or excluding self) and the order they're listed in. This causes several bugs, and exacerbates possible others: * We have a number of different ways those users can be ordered in a group PM conversation, and each of those means a bug here. One of those is zulip#4298. * Until quite recently, we also had longstanding bugs where the set of users would vary, and we may well still have more. This meant an additional symptom for each of those bugs. * Because we rely on comparing two `JSON.stringify` results for equality, we're implicitly assuming that objects we want to consider equal always have their properties enumerate in the same order. In particular, we regularly say `operator` first and then `operand`; if there's some way we can also end up with `operand` first and then `operator`, then this code wouldn't see the resulting narrow as equal to narrows it should be equal to. That makes this at least a latent bug, as there's nothing else in the code expressing this assumption (even in comments) and it would have been easy to break. This code also had a dumb bug where if the haystack was a whole-stream narrow, we'd blindly assume the needle was a stream or topic narrow. If you had an outbox message addressed to a PM conversation, and then managed to visit a stream whose name looked like your correspondent's email address (or their addresses, comma-separated), then you'd see the outbox PM in that stream narrow. This commit fixes all those issues by replacing the use of this function with `isMessageInNarrow`, which avoids all the above assumptions. More broadly: the question we're actually asking at this function's one caller is whether a specific *outbox message* belongs in a given narrow. It's sort of an accident that the inclusion relation on *narrows* even mostly corresponds to that question. Better to use a function that's explicitly about our actual question. Fixes: zulip#4298
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: * The old code would leave out the self user, except on self-PMs. * This new code will always include the self user. Both conventions are found in different areas of the server and webapp. The new one here matches the display_recipient the server sends on PMs, and is simpler. In any event the server doesn't care, and that's a deliberate API choice: we want to be sure to make easy things easy when writing bots, and allowing a range of natural ways to say who to send a message to is a key part of that. Concretely in the server implementation, since earliest times it drops dupes and does its own normalization whether to include the sender, so it's essentially always lived up to that plan. The relevant logic lives today (as of zulip/zulip@620e9cbf7) in the function get_recipient_from_user_profiles; and though it's moved around and been refactored, its logic is essentially unchanged since zulip/zulip@f851d9437, in 2012 a few months after Zulip began.
Made possible by the preceding series of changes.
These are only supported beginning with v2.0 of the server, so we can only do this either conditioned on the server version, or after older versions are no longer supported. This isn't something where the old way is deprecated and we're eager to remove it from a future server version -- to the contrary, we intend to support stream names and user emails here forever, as part of keeping easy things easy when writing bots. So there isn't a lot to be gained by eagerly switching; the main thing is it'd mitigate a race where a user we're sending to happens to have changed their email address right as we're trying to send.
gnprice
force-pushed
the
pr-outbox-no-narrow-take2
branch
from
e1df665 to
e47aa00
Compare
|
Thanks @chrisbobbe for the review! Merged. |
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.
This grew out of #3889. It follows #4294 by returning to the outbox code, and includes versions of all the changes made in #3889.
Along the way it fixes additional bugs, and the "Replace narrowContains" commit turns out to fix some bugs of its own. (That is, the old implementation turns out to be outright buggy. Meanwhile the new implementation here is from #4294 and well tested, and avoids the bugs in #3889's new implementation.)
Fixes: #4298
Fixes: #4301