notification: Use user_id to find account if multiple on same realm #5108

chrisbobbe · 2021-11-05T22:31:13Z

Along with making the property required on APNs and FCM payloads.

I noticed the opportunity to do this after reviewing #5100 and #5105.

gnprice

Thanks! It'll be great to fix this bug.

Specific comments below. Then more broadly, we just talked through in the office the question of requiring this 2.1+ feature, and concluded we aren't ready to drop notifications entirely from pre-2.1 servers. But this can be revised to keep it as optional, just pass it through, and make use of it.

gnprice · 2021-11-06T00:02:25Z

android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt

-    fun `optional fields missing cause no error`() {
-        expect.that(parse(Example.pm.minus("user_id")).identity.userId).isNull()


This should turn into a case where we test that required fields are required. See 9620629 or other recent changes in git log --stat -p android/app/src/test/.

gnprice · 2021-11-06T00:05:58Z

src/notification/extract.js

-    TODO(server-1.8): Simplify this comment a lot.
-    TODO(server-1.9): Simplify further.
-    TODO(server-2.1): Simplify further.


In addition to the paragraphs just above, the other simplification I had in mind here is to delete the distinctions between "antiquity" and the earliest supported version on the various properties, and to simplify the paragraphs at the top.

gnprice · 2021-11-06T00:12:00Z

src/notification/extract.js

+  if (user_id === undefined) {
+    throw err('archaic (pre-2.1.x)');


This should get a test case. See the existing mentions of archaic in the tests.

... Which raises the question why this "baseline" case didn't break:

// baseline expect(make({ realm_uri, recipient_type: 'private', sender_email })).toBeTruthy(); // missing recipient_type expect(make({ realm_uri, sender_email })).toThrow(/archaic/);

It looks like the answer is that it's passing because the make return value is just a closure, and so always truthy -- it needs to be called to actually test anything. Oops. So a prep commit should fix that.

gnprice · 2021-11-06T00:13:06Z

src/notification/types.js

+
+type NotificationBase = {|
+  realm_uri: string,
+  user_id: UserId,


notif: Expose payload's realm_uri, present since server 2.1, to inner code

s/realm_uri/user_id/?

gnprice · 2021-11-06T00:19:47Z

src/notification/index.js

 */
 export const getAccountFromNotificationData = (
  data: Notification,
-  identities: $ReadOnlyArray<Identity>,
+  accounts: $ReadOnlyArray<Account>,


The Identity type doesn't have the user ID, and it's not super convenient to add it there. That's because the Identity type is based on the Auth type (and we make Identity objects out of Auth objects at runtime), and the Auth type doesn't have the user ID. It's not super convenient to give it to the Auth type because there are places where we make Auth objects but we don't have a user ID, like `authFromCallbackUrl` in src/start/webAuth.js.

Oof. The Auth does of course have the email. So anywhere we have an Auth and can't readily make an Identity, because we lack the user ID, is an obstacle to converting from emails to IDs, #3764.

In authFromCallbackUrl, we're decoding one of those zulip:// URLs the server used to complete a web login and get us the API key. So we should get the server to start giving us user IDs there, even if we won't be able to count on their presence for a while. That should be a thread in #api design, then.

gnprice · 2021-11-06T00:23:15Z

src/notification/index.js

@@ -79,10 +79,8 @@ export const getAccountFromNotificationData = (
      realm_uri,
      parsed_url: realmUrl,
      match_count: urlMatches.length,
-      unique_identities_count: new Set(urlMatches.map(matchIndex => identities[matchIndex].email))
-        .size,
+      unique_accounts_count: new Set(urlMatches.map(matchIndex => accounts[matchIndex].email)).size,


Best to keep the name for this one commit. 🙂

The commit is marked NFC, which it isn't if a name sent in logging changes.

"Unique identities" is still an accurate description, and clearer what it means, because we're only counting up distinct emails (for this realm), not looking for other properties of an Account.

gnprice · 2021-11-06T00:23:55Z

src/notification/index.js

    });
-    // TODO get user_id into accounts data, and use that


nit: this TODO comment should go away in the later commit where we start using the user_id

gnprice · 2021-11-06T00:25:47Z

src/notification/index.js

  if (urlMatches.length > 1) {
-    // The user has several accounts in the notification's realm.  We should
-    // be able to tell the right one using the notification's `user_id`...
-    // except we don't store user IDs in `accounts`, only emails.  Until we
-    // fix that, just ignore the information.
-    logging.warn('notification realm_uri ambiguous; multiple matches found', {
-      realm_uri,
-      parsed_url: realmUrl,
-      match_count: urlMatches.length,
-      unique_identities_count: new Set(urlMatches.map(matchIndex => identities[matchIndex].email))
-        .size,
-    });
-    // TODO get user_id into accounts data, and use that
-    return null;
+    // The user has several accounts in the notification's realm. Pick one
+    // based on the notification's `user_id`.
+    const userMatch = urlMatches.find(urlMatch => accounts[urlMatch].userId === user_id);
+    if (userMatch == null) {
+      logging.warn('notification account not found with realm_uri and user_id');
+      return null;
+    }
+    return userMatch;
  }

  return urlMatches[0];


Let's do the search for a user_id match regardless of whether there are one or several realm-URL matches. That way we notice if the one account in the realm isn't actually for the right user.

gnprice · 2021-11-06T00:32:59Z

android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt

-        // commit 447a517e6, PR #12172.)
-        // TODO(server-2.1): Require this.
-        userId = data["user_id"]?.parseInt("user_id")
+        userId = data.require("user_id").parseInt("user_id")


Let's leave on this a one-line comment saying it was new in 2.1. That'll be helpful to refer to when seeing Sentry warnings about dropping FCM messages where it's absent.

(I see I didn't do this in 9620629, but I realize now I should have.)

gnprice · 2021-11-06T00:39:29Z

android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt

@@ -112,6 +112,7 @@ data class MessageFcmMessage(
    fun dataForOpen(): Bundle = Bundle().apply {
        // NOTE: Keep the JS-side type definition in sync with this code.
        identity.realmUri.let { putString("realm_uri", it.toString()) }
+        identity.userId.let { putInt("user_id", it) }


The let here is redundant; can simplify it out.

(It's redundant on the previous line too. Looks like it got left behind in 9620629 when I made realmUri non-nullable; before that it was ?.let and was doing something useful.)

chrisbobbe · 2021-11-15T23:58:41Z

Thanks for the review! Revision pushed.

Then more broadly, we just talked through in the office the question of requiring this 2.1+ feature, and concluded we aren't ready to drop notifications entirely from pre-2.1 servers. But this can be revised to keep it as optional, just pass it through, and make use of it.

Indeed. Because of this, there are several of your comments (from before we decided that) that I haven't acted on—please let me know if I've dropped some that I shouldn't have. 🙂

gnprice · 2021-11-16T23:02:20Z

Thanks for the revision! This looks good, modulo a couple of things in the last commit. Comments below, and it looks like #5108 (comment) still applies.

gnprice · 2021-11-16T23:00:29Z

src/notification/index.js

  }

-  return urlMatches[0];
+  // The user has several accounts in the notification's realm. Pick one


This isn't quite true yet -- this code will apply when there's just one element in urlMatches.

gnprice · 2021-11-16T23:01:52Z

src/notification/index.js

+  const userMatch = urlMatches.find(urlMatch => accounts[urlMatch].userId === user_id);
+  if (userMatch == null) {
+    logging.warn('notification account not found with realm_uri and user_id');


Although at this point we have a user_id from the notification, it may be the case that the account it's meant for doesn't have userId non-null in the accounts state. That's because:

/** * The user's numeric ID. * * We learn the user ID each time we complete an initial fetch. * * This is `null` briefly when we've logged in but not yet completed our * first initial fetch on the account. It's also `null` when representing * an account which was last used on a version of the app which didn't * record this information. It's never `null` for an account for which we * have server data. */

Probably if there's just one URL match, and its userId is null, then we should go for it.

Probably if there's just one URL match, and its userId is null, then we should go for it.

My current revision uses a slightly different algorithm. If there are two URL matches, and one of them has a userId that's wrong, but the other one has null, we still want to go for the null one. I think.

chrisbobbe · 2021-11-17T01:33:04Z

Thanks for the review! Revision pushed.

it looks like #5108 (comment) still applies.

Oh, actually that was one that I thought I'd fixed.

gnprice

Thanks! A couple of comments below.

gnprice · 2021-11-23T21:43:52Z

src/notification/index.js

+  // based on the notification's `user_id`.
+  const userMatch = urlMatches.find(urlMatch => {
+    const { userId } = accounts[urlMatch];
+    return userId !== null && userId === user_id;


This userId !== null is always true if userId === user_id is, right? So it can be dropped, and this function simplifies back down to just returning an expression.

gnprice · 2021-11-23T21:47:27Z

src/notification/index.js

+    logging.warn('notification account not found with realm_uri and user_id');
+    return null;


This log message won't always be accurate -- there could have been two accounts that had the right realm_uri and null user_id.

(Well, it could be read as accurate depending on how one interprets "found with realm_uri and user_id". But a reading that makes it accurate would make the same message equally accurate in the case where there's a match we actually use, just above here.)

We've been pretty precise in the different error cases in this function so far, because it's been a fairly common set of errors and it's good to know exactly what conditions are driving that. So let's make the distinction here too between zero matches and multiple matches.

gnprice · 2021-11-23T21:49:48Z

Oh also: let's mark this as fixing #4631! This is an issue I run into regularly when using test accounts -- looking forward to getting it fixed. 🙂

…ion`

…sages Oops -- `make` just returns a function, and functions are truthy. Instead, we want to test that that returned function returns true when called.

As we're already doing on Android; see extractIdentity in FcmMessage.kt. Next, we'll expose this data from iOS and Android notification payloads, when present, to inner app code, by adding `user_id?: UserId` to our Notification type. This brings the implementation in line with the comment at the top of the `extract` module that says that servers have started including a user_id.

Looks like this got left behind in 9620629, as Greg explains at https://github.com/zulip/zulip-mobile/pull/5108/files#r744036460.

…er_id We make a distinction like this in the counterpart Android code, with `data class Identity` in FcmMessage.kt.

By adding it to our `Notification` type.

…ties When we added this function in 056930c, we may have specifically chosen Identity objects over Account objects, to avoid adding a place where we pass around auth credentials. Hmm. Still, we've recently started storing the user ID in Account objects (4fdefb0), and we have a TODO in this function for using it. The Identity type doesn't have the user ID, and it's not currently convenient to add it there. That's because the Identity type is based on the Auth type (and we make Identity objects from Auth objects), and the Auth type doesn't have the user ID. It's not super convenient to add the user ID to the Auth type because there are places where we make Auth objects but we don't have a user ID, like `authFromCallbackUrl` in src/start/webAuth.js. As Greg points out [1], those places are obstacles to zulip#3764, "Convert from emails to user IDs to identify users".

This comment was slightly out of date -- we have been storing user IDs in `state.accounts`, since 4fdefb0. Fixes: zulip#4631

chrisbobbe · 2021-11-23T23:38:57Z

Thanks! Revision pushed.

gnprice · 2021-11-24T00:40:11Z

Thanks! Looks good -- merging.

chrisbobbe added the a-notifications label Nov 5, 2021

chrisbobbe requested a review from gnprice November 5, 2021 22:31

gnprice reviewed Nov 6, 2021

View reviewed changes

chrisbobbe force-pushed the pr-notification-user-id branch from eb706b4 to 68db03f Compare November 15, 2021 23:57

chrisbobbe force-pushed the pr-notification-user-id branch from 68db03f to 145833b Compare November 16, 2021 22:41

gnprice reviewed Nov 16, 2021

View reviewed changes

chrisbobbe force-pushed the pr-notification-user-id branch from 145833b to de294bd Compare November 17, 2021 01:33

gnprice reviewed Nov 23, 2021

View reviewed changes

gnprice added the a-multi-org label Nov 23, 2021

chrisbobbe added 10 commits November 23, 2021 14:23

android notif [nfc]: Comment on when realm_uri was added in Identity

0621ff3

notification types [nfc]: Factor NotificationBase out of `Notificat…

1f0cb71

…ion`

notification tests: Fix a bug in baseline test for very-old-style mes…

ef05b47

…sages Oops -- `make` just returns a function, and functions are truthy. Instead, we want to test that that returned function returns true when called.

android notif [nfc]: Remove a redundant let

aebde42

Looks like this got left behind in 9620629, as Greg explains at https://github.com/zulip/zulip-mobile/pull/5108/files#r744036460.

ios notif [nfc]: Pull out const identity for realm_uri, and soon us…

77ee121

…er_id We make a distinction like this in the counterpart Android code, with `data class Identity` in FcmMessage.kt.

notif: Expose payload's user_id to inner code, when present

f405e0f

By adding it to our `Notification` type.

notification tests [nfc]: Add a TODO for structuring tests more sensibly

15783d6

notification: Use user_id to find account if multiple on same realm

b3e24ed

This comment was slightly out of date -- we have been storing user IDs in `state.accounts`, since 4fdefb0. Fixes: zulip#4631

chrisbobbe force-pushed the pr-notification-user-id branch from de294bd to b3e24ed Compare November 23, 2021 23:38

gnprice merged commit b3e24ed into zulip:main Nov 24, 2021

chrisbobbe deleted the pr-notification-user-id branch November 24, 2021 00:52

		fun `optional fields missing cause no error`() {
		expect.that(parse(Example.pm.minus("user_id")).identity.userId).isNull()

		if (user_id === undefined) {
		throw err('archaic (pre-2.1.x)');

		logging.warn('notification account not found with realm_uri and user_id');
		return null;

Uh oh!

notification: Use user_id to find account if multiple on same realm #5108

notification: Use user_id to find account if multiple on same realm #5108

Uh oh!

Conversation

chrisbobbe commented Nov 5, 2021 • edited by gnprice 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Nov 15, 2021

Uh oh!

gnprice commented Nov 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Nov 17, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Nov 23, 2021

Uh oh!

chrisbobbe commented Nov 23, 2021

Uh oh!

gnprice commented Nov 24, 2021

Uh oh!

Uh oh!

chrisbobbe commented Nov 5, 2021 •

edited by gnprice

Loading

chrisbobbe Nov 17, 2021 •

edited

Loading

chrisbobbe commented Nov 17, 2021 •

edited

Loading