typing: Serialize recipients based on server version. #4022

agrawal-d · 2020-04-07T15:58:10Z

Zulip server versions >= 2.0.0-rc1 require user ID arrays in the
'to' argument for the typing endpoint, whereas we currently send
email arrays.

Check the Zulip version and use the appropriate format of the
argument, i.e., for versions >= 2.0.0-rc1 or when the Zulip server
version is undetermined, use user ID arrays, otherwise, use email
arrays.

Fixes #3732.

agrawal-d · 2020-04-07T16:00:34Z

@gnprice @ray-kraesig please review.

rk-for-zulip

I haven't yet tested, but this looks reasonable. Some nits and notes follow.

(I assume you've tested against CZO or something close to it; but have you also tested it against any older Zulip server versions?)

src/users/usersActions.js

agrawal-d · 2020-04-09T14:45:09Z

Thanks for the review, @ray-kraesig, I've pushed some changes.

I tested this in CZO, and it worked -the app sends user ID arrays, and the typing notifications work.
I tried to test this on a local server before commit 2f634f8, but the server wasn't starting up due to some configuration errors, (I have limited internet connectivity right now, and cant destroy and re-provision.) I locally changed the server version to 1.0 in the app, and we do send email arrays then, so the logic is working.

gnprice

Thanks @agrawal-d, and @ray-kraesig for the review!

A few additional comments, mostly small.

gnprice · 2020-04-10T19:47:48Z

src/users/usersActions.js

+  const activeAccount: Account | void = tryGetActiveAccount(state);
+  if (!activeAccount || activeAccount.zulipVersion === undefined) {


No need for the try... form -- if there's no active account then none of this can work anyway 🙂 and also it should be impossible to get to this function.

See the jsdoc:

/** * The account currently foregrounded in the UI, or undefined if none. * * For use in early startup, onboarding, account-switch, or other times * where there may be no active account. * * See `getActiveAccount` for use in the UI for an already-active account * (including the bulk of the app), and code intended for that UI. */ export const tryGetActiveAccount = (state: GlobalState): Account | void => {

That also saves us from having to worry about whether this behavior is the right one in the case of no active account.

gnprice · 2020-04-10T19:51:08Z

src/users/usersActions.js

+  let useEmailArrays: boolean;
+  const activeAccount: Account | void = tryGetActiveAccount(state);
+  if (!activeAccount || activeAccount.zulipVersion === undefined) {
+    useEmailArrays = false;
+  } else {
+    useEmailArrays = !new ZulipVersion(activeAccount.zulipVersion).isAtLeast(


Can simplify some of this by having a const serverVersion = ... above, just getting the ZulipVersion | void that describes the server, before any of the logic specific to this API feature and the version threshold for it.

In particular then that can be factored out into a selector in accountsSelectors.js, like getServerVersion returning ZulipVersion | void.

gnprice · 2020-04-10T20:17:25Z

src/users/usersActions.js

+  // User ID arrays are only supported in server versions >= 2.0.0-rc1, for
+  // versions before this, email arrays are used. If current server version is
+  // undetermined, user ID arrays are optimistically used.
+  // Support for user ID arrays was added in zulip/zulip commit 2f634f8.


nit: use 9 hex digits for a commit ID (git config --global core.abbrev 9 helps make this easy.)

For discussion see https://github.com/zulip/zulip-mobile/blob/master/docs/style.md#mentioning-commits , which I just wrote 😛 to explain why I've done this, and recommended it, for a long time.

gnprice · 2020-04-10T20:18:19Z

src/users/usersActions.js

+  // User ID arrays are only supported in server versions >= 2.0.0-rc1, for
+  // versions before this, email arrays are used. If current server version is
+  // undetermined, user ID arrays are optimistically used.
+  // Support for user ID arrays was added in zulip/zulip commit 2f634f8.


nit: mention the commit ID right next to the version number -- they're two ways of describing the same fact

rk-for-zulip · 2020-04-10T20:57:46Z

The only thing I'll add here is –

... ha! I literally got the notification for this commit as I was typing this sentence. As Greg notes, at least 9 hex digits, both in the commit message and in source comments. (In the commit message, for cross-repository commit references, you can use something along the lines of zulip/zulip@2f634f8c06; GitHub renders this as zulip/zulip@2f634f8c06.)

I don't actually recommend the 1.5.0-547-gd9b10727fa style of reference for most purposes. If you're going to promote that comment to the commit message, I'd suggest something more like ... providing its version as early as zulip/zulip@d9b10727fa (released in 1.6.0, in 2017-06). It's more likely...

Other than that, LGTM.

I tested this in CZO, and it worked -the app sends user ID arrays, and the typing notifications work.
I tried to test this on a local server before commit 2f634f8, but the server wasn't starting up due to some configuration errors, (I have limited internet connectivity right now, and cant destroy and re-provision.) I locally changed the server version to 1.0 in the app, and we do send email arrays then, so the logic is working.

I recently attempted to do the same and failed. I'm not having internet connectivity issues, as far as I can tell; I fear that whatever cloud-side resources 1.9.x depended on may have vanished. I'll bring it up on CZO.

I don't think this needs to block on that, though. Given that sending email arrays successfully reports typing presence in 2.2-dev, I feel safe in assuming that it'll work on previous versions.

agrawal-d · 2020-04-11T09:00:05Z

@gnprice @ray-kraesig thanks for the review. I've pushed some changes. Please take a look.

agrawal-d · 2020-04-11T09:03:39Z

You may notice that the GitHub UI still truncates the commit hash to 7 digits in the commit message, despite me having used the recommended 9 digits.

rk-for-zulip · 2020-04-14T02:31:18Z

You may notice that the GitHub UI still truncates the commit hash to 7 digits in the commit message, despite me having used the recommended 9 digits.

Unfortunately, yes. The underlying commit message still has the correct number of digits in it, though, as can be seen by hovering on the link to your commit above. (I believe GitHub's linkifier also uses all provided digits, even if it doesn't present them.)

My concerns are addressed, and it looks like Greg's have been, too – I'll give him some time to take a second look, but I'm looking forward to having this fix in. Thanks!

'getServerVersion' returns the Zulip server version of the active account in the parsed form as a ZulipVersion instance, or undefined if we do not know the server version.

Zulip server versions >= 2.0.0-rc1 (zulip/zulip@2f634f8c0) require user ID arrays in the 'to' argument for the typing endpoint, whereas we currently send email arrays. Check the Zulip version and use the appropriate format of the argument, i.e., for versions >= 2.0.0-rc1 or when the Zulip server version is undetermined [1], use user ID arrays, otherwise, use email arrays. [1] The server started providing its version as early as zulip/zulip@d9b10727fa (released in 1.6.0, in 2017-06). It's more likely that we failed to parse a new, exotic version-string format than that someone's actually trying to connect to a Zulip server that doesn't provide one. Fixes zulip#3732.

Having just one place a variable gets its value helps simplify reading and thinking about the code.

gnprice · 2020-04-14T20:06:11Z

Merged -- thanks again both!

I added a couple of commits on top: one a code-style fixup, and one in part a jsdoc-style fixup and in part just adding some information to the jsdoc that we hadn't quite written down yet. I also slightly adjusted the commit messages' summary lines.

chrisbobbe · 2020-04-14T20:36:39Z

adding some information to the jsdoc that we hadn't quite written down yet

Thanks, @gnprice!

agrawal-d requested review from gnprice and rk-for-zulip April 7, 2020 15:58

agrawal-d force-pushed the typing-api branch from 7013def to 5c47c37 Compare April 7, 2020 16:02

agrawal-d added P1 high-priority api migrations labels Apr 7, 2020

agrawal-d force-pushed the typing-api branch 2 times, most recently from 2f73ed1 to 8f14e5f Compare April 8, 2020 15:48

rk-for-zulip reviewed Apr 9, 2020

View reviewed changes

src/users/usersActions.js Outdated Show resolved Hide resolved

src/users/usersActions.js Outdated Show resolved Hide resolved

src/users/usersActions.js Outdated Show resolved Hide resolved

src/users/usersActions.js Show resolved Hide resolved

agrawal-d force-pushed the typing-api branch 2 times, most recently from 71ee597 to 2566429 Compare April 9, 2020 14:42

agrawal-d requested a review from rk-for-zulip April 9, 2020 14:45

agrawal-d force-pushed the typing-api branch from 2566429 to 690f7cd Compare April 9, 2020 14:58

gnprice reviewed Apr 10, 2020

View reviewed changes

agrawal-d force-pushed the typing-api branch from 690f7cd to dcfe970 Compare April 11, 2020 08:59

agrawal-d requested a review from gnprice April 11, 2020 09:00

agrawal-d force-pushed the typing-api branch from dcfe970 to 93dc164 Compare April 11, 2020 09:02

agrawal-d and others added 4 commits April 14, 2020 13:02

accounts: Add selector 'getServerVersion'.

11cca9b

'getServerVersion' returns the Zulip server version of the active account in the parsed form as a ZulipVersion instance, or undefined if we do not know the server version.

typing status [nfc]: Use const rather than let .. if.

92e57c1

Having just one place a variable gets its value helps simplify reading and thinking about the code.

accounts [nfc]: Summarize and expand jsdoc on zulipVersion.

2ddb444

gnprice force-pushed the typing-api branch from 93dc164 to 2ddb444 Compare April 14, 2020 20:04

gnprice merged commit 2ddb444 into zulip:master Apr 14, 2020

agrawal-d deleted the typing-api branch April 14, 2020 20:07

rk-for-zulip mentioned this pull request Apr 16, 2020

Store server's version number, include in Sentry events #3745

Closed

		const activeAccount: Account \| void = tryGetActiveAccount(state);
		if (!activeAccount \|\| activeAccount.zulipVersion === undefined) {

Uh oh!

typing: Serialize recipients based on server version. #4022

typing: Serialize recipients based on server version. #4022

Uh oh!

Conversation

agrawal-d commented Apr 7, 2020

Uh oh!

agrawal-d commented Apr 7, 2020

Uh oh!

rk-for-zulip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agrawal-d commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

rk-for-zulip commented Apr 10, 2020

Uh oh!

agrawal-d commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agrawal-d commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rk-for-zulip commented Apr 14, 2020

Uh oh!

gnprice commented Apr 14, 2020

Uh oh!

chrisbobbe commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

agrawal-d commented Apr 9, 2020 •

edited

Loading

agrawal-d commented Apr 11, 2020 •

edited

Loading

agrawal-d commented Apr 11, 2020 •

edited

Loading