Opening a notification from another account causes crash #4290
Comments
OK, I've completed this audit. There are a few other spots where the behavior may have been affected by the change. I think there aren't any meaningful regressions as a result of those -- they're all in some pretty messy ad-hoc string processing of URLs (which this migration to |
|
A correction! It turns out that if you already had the relevant account active, this bug didn't affect any behavior: we'd log a warning "notification realm_uri not found in accounts", but then we'd go ahead and navigate to the relevant conversation. The main effect of this bug was if you weren't already on the right account. In that case:
So it's not quite as severe as I thought -- which helps explain why we haven't heard reports from other users. In any case the bug is fixed now, since ebbab7e. |
gnprice
changed the title
... Spoke too soon: I hadn't actually tested this since the original report. Now I have. What actually happens is: the app opens, then crashes. With That is, we try to navigate to the right conversation as if it were on the active account... and when there isn't a user by that email, we crash. The crash is a separate issue we should fix. Any time a notification points us at a user or stream that doesn't seem to exist, that probably is a bug in either the app or the server... but because it might be on the server, we should be robust enough not to crash on it. |
gnprice
changed the title
OK, and filed as #4309. |
Thanks as always to our kind volunteer translators. i18n: Sync recently-added message strings across languages. webview build: Spell stdin as `-` for reading rsync filter rules. On Windows (in Git Bash) there's no /dev/stdin; but this works instead, as we learned here: https://chat.zulip.org/#narrow/stream/48-mobile/topic/issue/near/1047294 (Things still aren't working there as a whole, but we seem to get past one error and reach another one.) Conversely, this exact construct `--filter='. -'` appears in an example in the rsync man page, even at the ancient rsync 2.6.9 that Apple provides on macOS. Suggested-by: Anders Kaseorg <[email protected]> README: Migrate Travis badge to travis-ci.com. Signed-off-by: Anders Kaseorg <[email protected]> android notif: Correctly stringify pmUsers to fix navigation to group PMs. Navigation to a group PM on pressing a notification was broken because pmUsers was incorrectly stringified in GroupPm.getPmUsersString. E.g., for a group PM among user IDs 13313, 13434, and 13657, it would stringify to (newline added for readability): "GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657]), GroupPm(pmUsers=[13313, 13434, 13657])" It should instead stringify to "13313, 13434, 13657". (Later in this series of commits, we remove the space.) Fix and add a test. notif tests: Ensure tests pass with representative pm_users values. To be reverted in the next commit. In the previous commit, we changed the return value of GroupPm.getPmUsersString in our Kotlin code from garbage separated by ', ' to numbers separated by ', '. This commit aims to prove that ', '-separated numbers will be handled correctly, at least as far as our tests can tell. But we really want it to be ','-separated (no space), which we do in the next commit. notif: Separate ids in pm_users for group PMs with ',' instead of ', '. ', ' would have been handled correctly, but seemingly by accident; in getNarrowFromNotificationData, pm_users was split on ',' to give ['1', ' 2', ' 3'] (note the spaces), then each element of that array was converted to a number. Also, replace the confusing + syntax, as in +idStrs[i], with parseInt. logging jsdoc: Move "see also" before parameters, to fix parse. 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. notif: Normalize realm_uri by parsing it as a URL. This fixes zulip#4290, a regression in the last release, where trying to open a notification doesn't actually navigate to the conversation. The bug is a bit like a revival of zulip#3567: we get the error "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash `/`, while the `realm_uri` value in the notification doesn't. On further inspection, it looks like this was introduced when we started using a URL object for `realm` values, in 865914f. Previously, since the fix for zulip#3567, we'd stripped trailing slashes from `realm` values, which were URL strings. But: > new URL('https://example').toString() 'https://example/' parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is *add* a trailing slash to a URL like our realm URLs (or in general, fill in the path as `/` if empty.) When a `realm_uri` with no slash is compared to one of those, it never matches. Fix the issue by doubling down on parsing as URL objects. As a side effect, this normalizes case in the URL's host (and scheme). We'd previously discussed doing that, at zulip#3671 and here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/realm.20URL/near/795201 and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriate `URL` implementation handy, but now we do. :-) Fixes: zulip#4290 UserItem [nfc]: Take user as one structured object. This will help us switch from emails to user IDs in downstream bits of code. Also adjust several of these call sites to use user IDs for `key`, rather than emails. UserItem [nfc]: Pass whole user to callback, rather than email. This allows UserItem call sites whose callbacks are ready to work in terms of user IDs to do so without workarounds. At the same time, passing whole user objects rather than *just* IDs allows other call sites to continue to use emails without similar, inverse workarounds. notif: Always sort user IDs in pm_users. We already ensure this in the Android case (in FcmMessage.kt); do so in the iOS case too, and document it in the type. In practice the list should already have always been sorted: the server sends it in that form, and has always done so since the pm_users field was introduced in server commit 1.7.0-2360-g693a9a5e7. (To see this in the history, try the following Git commands: git log -L :get_message_payload:zerver/lib/push_notifications.py git log -L :huddle_users:zerver/lib/message.py .) So the only way this could have gone wrong is if a rogue server changed that behavior for some reason; and the main effect of this commit is really just to document this invariant. narrow [nfc]: Document more details on identifying group PMs. Which turned up a couple of bugs! We'll fix those later in this series. example data: Take sender and recipients as pmMessage arguments. As demonstrated, this allows callers to customize these a lot more cleanly than they can by overriding the actual message properties directly. There are a few call sites we don't update here, in narrowsReducer-test.js; that file hasn't yet been upgraded to be well-typed, and so those call sites don't have real User objects to provide. example data [nfc]: Use cleaner workaround for Flow "unsealed" issue. We discovered this nicer one after having used the other one here. Reminded of the contrast in discussion on other changes in this file: zulip#4294 (comment) types: Make some more indexer-using object types inexact. I just ran into this issue with CaughtUpState when making another change. Apply the workaround there and on the remaining example in this file, and mark all instances with a conditional TODO.
This appears to be a regression in the recent release v27.156 (and the never-released v27.155.)
Steps to reproduce:
Expected behavior: the app opens and navigates to the conversation. (If you're already on the right account; if not, that's Opening a notification of an organisation while switched into another organisation is broken. #2295 (comment).)Actual behavior: the app opens but doesn't navigate.(correction 2020-11-12: the bug is not quite this bad! See #4290 (comment) .)
In a debug console, an error appears: "notification realm_uri not found in accounts", and it turns out that all the accounts have URLs with a trailing slash
/, while therealm_urivalue in the notification doesn't.On further inspection, it looks like this was introduced when we started using a URL object for
realmvalues, in 865914f. Previously, since the fix for #3567, we'd stripped trailing slashes fromrealmvalues, which were URL strings. But:parsing as a URL object and converting that to a string normalizes the URL, and one thing that normalization does is add a trailing slash to a URL like our realm URLs (or in general, fill in the path as
/if empty.) When arealm_uriwith no slash is compared to one of those, it never matches.Seems like the right fix is to double down on parsing as URL objects, and do so with the
realm_uritoo. As a side effect, that'll normalize case in the URL's host (and scheme) -- which we'd previously discussed doing, at #3671 and a chat thread, and concluded that parsing as URL objects would be the cleanest way. We didn't then have an appropriateURLimplementation handy, but now we do. :-)I'm also going to go do a sweep of other places the realm is used where 865914f / #4235 effectively introduced a string ->
URL-> string round trip, and see if there are others that need similar fixing-up. (I know there are some we already fixed soon after that change, in #4265 and #4266.)The text was updated successfully, but these errors were encountered: