narrow: Switch to a reasonable representation, and store user IDs#4346
narrow: Switch to a reasonable representation, and store user IDs#4346gnprice merged 8 commits intozulip:masterfrom
Conversation
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, @gnprice! This LGTM pending a few small comments, below.
| ]; | ||
| export const specialNarrow = (operand: string): Narrow => { | ||
| if (operand === 'starred') { | ||
| return { type: 'starred' }; |
There was a problem hiding this comment.
eb965a7 (narrow [nfc]: Switch the Narrow type to a reasonable structure!)
Should we Object.freeze the returned object, like we do for other Narrows in this commit?
| if (operand === 'private') { | ||
| return { type: 'all-pm' }; | ||
| } | ||
| throw new Error(`specialNarrow: got invalid operand: ${operand}`); |
There was a problem hiding this comment.
+ throw new Error(`specialNarrow: got invalid operand: ${operand}`);Would "unsupported" be slightly better than "invalid"? I'm thinking of valid but unsupported narrows like is:unread, which could reasonably be encoded in links that specialNarrow makes narrows from.
|
|
||
| case 'pm:': { | ||
| if (!rest.startsWith('s:')) { | ||
| const match = /^d:(.*?):(.*)/.exec(rest); |
There was a problem hiding this comment.
Ah OK, I think I've worked out what we're doing here:
The ? in the first capture group, (.*?), makes the * quantifier lazy instead of greedy, which it would be by default. This is fine because the IDs are numeric and obviously don't contain a :. It's also necessary, because email addresses might contain colons (er, I think? Wikipedia gives the example "jsmith@[IPv6:2001:db8::1]", which I think may be permitted by IETF RFC 2821 section 4.1.3).
It'll be simpler to think about once we've taken emails out of the picture. If that weren't planned, I'd say we might want to have an explanatory code comment here.
This reminds me of what we're doing for topic narrows, starting in af1713a (though it's seeming like that'll need to be reworked because it caused a regression; the \x00 character we use there (which has a very similar role to the : here) doesn't round-trip into being stored as the value of the data-narrow attribute in our message-list HTML; see discussion).
There was a problem hiding this comment.
The
?in the first capture group,(.*?), makes the*quantifier lazy instead of greedy, which it would be by default.
Yep!
It's also necessary, because email addresses might contain colons (er, I think? Wikipedia gives the example "jsmith@[IPv6:2001:db8::1]", which I think may be permitted by IETF RFC 2821 section 4.1.3).
Yeah, good find with that example. I didn't even think about whether an email address could contain a colon. But the great thing about using .*? here is that we don't have to, because it will match up to that first colon (which has to be the one we put there as a delimiter), and then everything (*) after that is the email addresses.
In general for capturing contents up to a delimiter, .*? is what you want rather than .*, for just this reason. It similarly appears in the other regexps in this parseNarrow function.
I did deliberately put the IDs first and the emails at the end, so that the emails would be the part in that position where we don't have to think about what's there.
(*) Though looking again at the topic: case reminds me that there is an asterisk on that, because without the /s modifier there are a few characters the . pattern rejects. I'm not sure if those characters are possible in email addresses -- I'm sure ASCII newline isn't, but there are a couple of other "newline" characters that wrinkle applies to and I'm not certain if they can appear. I'll add /s there too so we don't have to think about it.
(though it's seeming like that'll need to be reworked because it caused a regression; the
\x00character we use there (which has a very similar role to the:here) doesn't round-trip into being stored as the value of thedata-narrowattribute in our message-list HTML; see discussion).
(We ended up fixing that issue at a different layer, in #4352.)
This has no callers at all except its own tests. The only one it's ever had was a trivial one inside the `narrow` module -- and that appeared nearly a year after the function itself was introduced.
Because of all the work we've done over the last few series to encapsulate the details of the Narrow type within this module, not much has to change! Just the constructors; the pattern-match engine caseNarrow; and apiNarrowFromNarrow, where we'd been taking a shortcut using the fact that Narrow secretly was ApiNarrow. Well, those and one call site to `specialNarrow`, where we newly enforce an invariant we'd long assumed about what kind of `is:foo` narrows we support.
This isn't as good as not even *using* emails, in favor of user IDs... but it's a step on the way there, as well as a small nice simplification in itself.
This sets up a mass conversion we'll do in the next commit.
Done with the following commands:
$ perl -i -0pe '
s#pmNarrowFromEmail\((.*?)\.email\)#pm1to1NarrowFromUser($1)#g
&& s/pmNarrowFromEmail\b/pm1to1NarrowFromUser/g
' src/**/__tests__/*.js
$ tools/fmt
then fixing one duplicate import, in narrow-test.js.
We already do this for opening a notification for a group PM; now do the same for 1:1 PMs too. If we manage to have a notification where we don't know of a sender with that email -- which can happen if e.g. the sender has changed it since the notification was originally sent -- then this will cause us not to try to navigate to the conversation at all, which is an improvement over anything the previous behavior might have been: navigating to a conversation where we show something broken-looking, or crash. The motivation for this is so that we pass the Narrow constructor a user ID as well as an email, so that we can start storing user IDs in Narrow objects and start using them in all the code that consumes them. This is the last spot in our codebase where we're not doing so. Really we should use the sender ID from the server. That started in Zulip Server 1.8.0, according to the handy comments in FcmMessage.kt. We leave that be for now, though -- this email use is easy to find with a grep for `email`.
After all that prep work... this just works. Apart from a storage migration, only this module has to change, only in a few places -- and Flow pointed them all out.
gnprice
force-pushed
the
pr-narrow-store-userids
branch
from
aa6fd7e to
df64214
Compare
|
Thanks for the review! Merged after making the suggested changes. |
As the comment in migration 19 said, we were doing something a bit risky there in order to avoid duplicating the then-current version of `keyFromNarrow`. We simply invoked `keyFromNarrow` directly -- which called for being careful about migrations for any future change we made to `keyFromNarrow`. Then a bit later we changed `keyFromNarrow` quite dramatically, and didn't pay attention to migrations at all. Oops. Specifically, in cf4207f we changed the `Narrow` type to have quite a different structure. That's the type that `keyFromNarrow` accepts -- so the result is that `keyFromNarrow` stopped working on the old values. Which are exactly what `JSON.parse` is going to supply us when this migration is run. The result is probably that if the user has any drafts when they take an upgrade that spans migration 19 and commit cf4207f, the app will crash on startup. We'd probably have found this bug at the alpha stage when next making a release, so it wouldn't have made it to users other than ourselves. Better to fix ASAP, though. The plan for migration 19, as described here: zulip#4339 (comment) knowingly involved breaking the basic rule of migrations: don't invoke any of the app's model code, because it will change. I figured it was OK because there were changes coming up soon which I'd planned out how I'd add migrations for. Then later the same week I sent zulip#4346, which followed that plan... and I hadn't thought through this particular interaction, and it introduced this bug. So this is a fresh lesson in the basic rule of migrations: don't invoke any of the app's model code, because it will change. See discussion where we found this bug: zulip#4382 (comment)
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 follows on #4342 and its predecessors, and accomplishes part of #4333: at the end of this PR, we now have user IDs in our internal representation of PM narrows.
After this, in the next PR or two we'll go on to switch all the various places we consume narrows to use the IDs instead of the emails -- which as mentioned at #4035 (comment) will include completing #4035 -- and stop storing emails in our narrows at all.
Thanks to all the prep work in the preceding several PRs, not much actually has to change in this one -- we've already quite thoroughly encapsulated the details of the Narrow type, and also eliminated nearly all the places where we were constructing PM narrows without passing a user ID to the constructor along with an email. Most of what's in this PR is just taking care of the last few of those.