notif: Use live value for app ID on registering APNs token #1451
Conversation
|
Thanks for the contribution! Before we can review this in detail, it will need tests. See the guidance in our README file about writing tests. |
|
Tests are added. |
There was a problem hiding this comment.
Thanks for working on this @AhmedTareek! Some comments below.
There was a problem hiding this comment.
Thanks for the revision @AhmedTareek! Some nits otherwise looks good, moving to integration review from Greg.
rajveermalviya
added
integration review
There was a problem hiding this comment.
Thanks @AhmedTareek for picking this up, and @rajveermalviya for the previous reviews! Comments below.
| @@ -149,9 +149,10 @@ class NotificationService { | |||
| await addFcmToken(connection, token: token); | |||
|
|
|||
| case TargetPlatform.iOS: | |||
| const appBundleId = 'com.zulip.flutter'; // TODO(#407) find actual value live | |||
There was a problem hiding this comment.
nit in commit message:
Fixes zulip#407
follow existing patterns for how to format this — use git log (better, git log --grep Fixes) to see examples
There was a problem hiding this comment.
notif: Use live value for app ID on registering APNs token
Fixes #407
Closer, but make it a complete sentence:
notif: Use live value for app ID on registering APNs token
Fixes #407.
AhmedTareek
force-pushed
the
#407
branch
2 times, most recently
from
f1ee1ef to
09a1c01
Compare
| @@ -149,9 +149,10 @@ class NotificationService { | |||
| await addFcmToken(connection, token: token); | |||
|
|
|||
| case TargetPlatform.iOS: | |||
| const appBundleId = 'com.zulip.flutter'; // TODO(#407) find actual value live | |||
There was a problem hiding this comment.
notif: Use live value for app ID on registering APNs token
Fixes #407
Closer, but make it a complete sentence:
notif: Use live value for app ID on registering APNs token
Fixes #407.
Fixes zulip#407. `await ZulipBinding.instance.packageInfo` was added to get the packageName, making `registerToken` take longer to complete. Previously, in "token initially unknown" test in the UpdateMachine.registerNotificationToken `start()` and `_registerNotificationToken()` finished in sync with the test’s expectations. Now, `flushMicrotasks` is added to ensure that the listener callback `_registerNotificationToken()` finishes its async work before continuing the test. Added packageInfo function in `example_data.dart` and updated tests that implicitly relied on packageInfo data to explicitly set it.
|
Thanks @AhmedTareek for the revision! These changes all look good. Before merging, I'd like to manually test this end to end, to check that notifications still work as expected. I believe they should — but the changes this PR is making are the kind where an oversight could easily cause notifications to totally break, and without manual testing we likely wouldn't learn that until it went out in the next release. I tried doing that manual testing just now, but it seems like the instructions we've previously had for manually testing notification changes aren't currently working. So some debugging will be required to work out how to update them. (We'll need that sort of testing to be working again for other changes too, not only this PR.) I started a chat thread for that here: Once that's sorted out, I can do a round of manual testing on this PR; and provided that comes out clean as expected, we'll then be able to merge it. |
Fixes #407
Changes made:
packageInfo.packageName.The first change caused the "token initially unknown" test in
store_test.dartto fail due to the added asynchronous call toZulipBinding.instance.packageInfo, which delayed the completion ofregisterToken.To resolve this, a
flushMicrotaskswas added afterawait startFuturein the test to ensure_registerNotificationTokencompletes before the test checks for the request.