notif: package info indirection #1143

Abhisheksainii · 2024-12-12T12:32:58Z

In this PR, I made the changes to indirect PackageInfo.fromPlatform through a method on ZulipBinding.

Steps I followed :

Update the ZulipBinding to provide package info: Add a method in ZulipBinding to return the package name. This method will wrap the call to PackageInfo.fromPlatform() and make it test-friendly.
Override the method in TestZulipBinding: In the TestZulipBinding, override getAppBundleId to return a mock or predefined value for testing.
Refactor the code to use getAppBundleId: Replace the direct calls with the indirection from Zulip Binding class.
Write a test for the above logic under a group called 'tokens'. The allow the grouping of all tests related to tokens, I created a separate group : tokens.

Fixes #407

chrisbobbe · 2024-12-12T22:20:52Z

Hi @Abhisheksainii, thanks for the PR. It will need to follow our Git style before it gets reviewed by a core team member.

Abhisheksainii · 2024-12-13T06:00:18Z

@chrisbobbe changes made according to the git style.
Please check.

PIG208

Thanks for the PR @Abhisheksainii! Looks like there is a simplification to be made, because we already access and store packageInfo. It will probably require you to modify our copy of PackageInfo class by adding packageName to it.

lib/model/binding.dart

lib/notifications/receive.dart

Abhisheksainii · 2024-12-19T05:54:34Z

@PIG208 changes done as requested, kindly take a look at them.

PIG208 · 2024-12-19T17:16:09Z

Thanks for the update. Please review the "commit style guide" referenced in the README and update your commits: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

~~Also, it looks like the CI is failing. Could you fix that?~~
Looks like the CI failure is not relevant to this PR.

Abhisheksainii · 2024-12-20T17:34:07Z

Thanks for the update. Please review the "commit style guide" referenced in the README and update your commits: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

~~Also, it looks like the CI is failing. Could you fix that?~~ Looks like the CI failure is not relevant to this PR.

@PIG208 I checked the commits and they pretty much explain what they do. Am I supposed to add some more description?

gnprice · 2024-12-20T17:55:27Z

Please read the Zulip commit style guide, which is linked from the README where Zixuan pointed to.

Abhisheksainii · 2024-12-20T19:16:31Z

Thanks for the update. Please review the "commit style guide" referenced in the README and update your commits: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

~~Also, it looks like the CI is failing. Could you fix that?~~ Looks like the CI failure is not relevant to this PR.

@PIG208 commits updated according to the style guide. Kindly review them
Thanks!

add test for the appBundleId wrapper made a group called tokens to check if appBundleId equals 'com.zulip.flutter.test'

made changes to access packageName from the PackageInfo class. remove getAppBundleId() from Zulip Binding class Add packageName variable to PackageInfo class Fixes zulip#407

made packageName accessible to receive.dart file and test files via (await ZulipBinding.instance.packageInfo)?.packageName ?? "com.zulip.flutter"

Abhisheksainii · 2024-12-23T05:08:01Z

@PIG208 kindly review the changes.

PIG208 · 2024-12-23T22:01:51Z

test/model/binding.dart

@@ -380,6 +380,7 @@ class TestZulipBinding extends ZulipBinding {
  Future<void> toggleWakelock({required bool enable}) async {
    _wakelockEnabled = enable;
  }
+


Looks like this is unintentional.

PIG208 · 2024-12-23T22:07:29Z

lib/model/binding.dart

@@ -303,10 +303,12 @@ class LinuxDeviceInfo implements BaseDeviceInfo {
 class PackageInfo {


binding: add packageName to the packageInfo class in the binding class made changes to access packageName from the PackageInfo class. remove getAppBundleId() from Zulip Binding class Add packageName variable to PackageInfo class Fixes #407

The commit claims that getAppBundleId() got removed, but it is not present in the commit itself. For changes that address reviews on a previous revision of the commit, the PR thread is more appropriate.

The commit summary and the "Fixes" line also need to be updated. This commit doesn't fix #407. We should move this line to the next commit where we start using packageName.

For the commit message, changes that one can tell from reading the diffs generally don't need to be reiterated: "made changes to access packageName from the PackageInfo class"
"Add packageName variable to PackageInfo class".

Please review Zulip's commit discipline linked in the README https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

PIG208 · 2024-12-23T22:09:46Z

test/notifications/receive_test.dart

+      check(await testBinding.getAppBundleId())
+        .equals('com.zulip.flutter.test');
+    }, );
+  });


Hmm, this commit should be dropped as getAppBundleId doesn't exist anymore.

This is one reason that we should include the tests in the same commit that implements the feature tested.

PIG208 · 2024-12-23T22:10:15Z

lib/notifications/receive.dart

@@ -149,8 +149,8 @@ class NotificationService {
        await addFcmToken(connection, token: token);

      case TargetPlatform.iOS:
-        const appBundleId = 'com.zulip.flutter'; // TODO(#407) find actual value live
-        await addApnsToken(connection, token: token, appid: appBundleId);
+         // TODO(#407) find actual value live


Since this resolves the TODO, we can remove this line.

PIG208 · 2024-12-23T22:12:48Z

test/model/store_test.dart

@@ -1032,7 +1032,7 @@ void main() {
      if (defaultTargetPlatform == TargetPlatform.android) {
        checkLastRequestFcm(token: '012abc');
      } else {
-        checkLastRequestApns(token: '012abc', appid: 'com.zulip.flutter');
+        checkLastRequestApns(token: '012abc', appid:(await testBinding.packageInfo)?.packageName?? 'com.zulip.flutter.test');


The test should encode the clear expectation of what the expected appid is. Let's avoid looking up the value and use a literal string instead.

PIG208 · 2024-12-23T22:12:54Z

test/model/store_test.dart

@@ -1071,7 +1071,7 @@ void main() {
      if (defaultTargetPlatform == TargetPlatform.android) {
        checkLastRequestFcm(token: '012abc');
      } else {
-        checkLastRequestApns(token: '012abc', appid: 'com.zulip.flutter');
+        checkLastRequestApns(token: '012abc', appid:(await testBinding.packageInfo)?.packageName?? 'com.zulip.flutter.test');


Same as the other case.

PIG208 · 2024-12-23T22:19:12Z

lib/notifications/receive.dart

-        const appBundleId = 'com.zulip.flutter'; // TODO(#407) find actual value live
-        await addApnsToken(connection, token: token, appid: appBundleId);
+         // TODO(#407) find actual value live
+        await addApnsToken(connection, token: token, appid: (await ZulipBinding.instance.packageInfo)?.packageName ?? "com.zulip.flutter");


I think there is an underlying issue if packageInfo is missing, so let's include a // TODO(log) comment here.

This case is interesting so it might worth a test for it. Let's find a way to test when we fallback to this default value, if packageInfo is somehow missing.

PIG208 · 2024-12-23T22:21:33Z

test/notifications/receive_test.dart

@@ -34,7 +34,7 @@ void main() {
  group('tokens', () {
    test('APNs token registration using correct app bundle ID', () async {
      await init();
-      check(await testBinding.getAppBundleId())
+      check((await testBinding.packageInfo)?.packageName?? 'com.zulip.flutter.test')


Falling back to com.zulip.flutter.test defeats the purpose of this test. I think we don't expect packageInfo to be missing in this case, right?

PIG208 · 2024-12-23T22:23:53Z

Thanks for the update @Abhisheksainii! Left some comments.

Abhisheksainii changed the title ~~Fix/package info indirection~~ notif: package info indirection Dec 12, 2024

Abhisheksainii force-pushed the fix/package-info-indirection branch from 75331d7 to 5970957 Compare December 13, 2024 16:13

PIG208 reviewed Dec 17, 2024

View reviewed changes

lib/model/binding.dart Outdated Show resolved Hide resolved

lib/model/binding.dart Outdated Show resolved Hide resolved

lib/notifications/receive.dart Outdated Show resolved Hide resolved

PIG208 requested review from PIG208 and removed request for PIG208 December 17, 2024 23:32

PIG208 self-assigned this Dec 17, 2024

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Dec 17, 2024

Abhisheksainii requested a review from PIG208 December 19, 2024 06:34

Abhisheksainii force-pushed the fix/package-info-indirection branch from c119c94 to 2cc9de6 Compare December 20, 2024 19:15

Abhisheksainii force-pushed the fix/package-info-indirection branch from 2cc9de6 to 3a87065 Compare December 20, 2024 19:43

Abhisheksainii added 3 commits December 21, 2024 11:45

test: add test for the appBundleId wrapper

64dd2fa

add test for the appBundleId wrapper made a group called tokens to check if appBundleId equals 'com.zulip.flutter.test'

binding: add packageName to the packageInfo class in the binding class

198e540

made changes to access packageName from the PackageInfo class. remove getAppBundleId() from Zulip Binding class Add packageName variable to PackageInfo class Fixes zulip#407

indirection: made packageName accessible using the binding class

ff11675

made packageName accessible to receive.dart file and test files via (await ZulipBinding.instance.packageInfo)?.packageName ?? "com.zulip.flutter"

Abhisheksainii force-pushed the fix/package-info-indirection branch from 3a87065 to ff11675 Compare December 21, 2024 06:19

PIG208 reviewed Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notif: package info indirection #1143

notif: package info indirection #1143

Abhisheksainii commented Dec 12, 2024

chrisbobbe commented Dec 12, 2024

Abhisheksainii commented Dec 13, 2024 •

edited

Loading

PIG208 left a comment

Abhisheksainii commented Dec 19, 2024

PIG208 commented Dec 19, 2024 •

edited

Loading

Abhisheksainii commented Dec 20, 2024

gnprice commented Dec 20, 2024

Abhisheksainii commented Dec 20, 2024 •

edited

Loading

Abhisheksainii commented Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 Dec 23, 2024

PIG208 commented Dec 23, 2024

		@@ -303,10 +303,12 @@ class LinuxDeviceInfo implements BaseDeviceInfo {
		class PackageInfo {

notif: package info indirection #1143

Are you sure you want to change the base?

notif: package info indirection #1143

Conversation

Abhisheksainii commented Dec 12, 2024

chrisbobbe commented Dec 12, 2024

Abhisheksainii commented Dec 13, 2024 • edited Loading

PIG208 left a comment

Choose a reason for hiding this comment

Abhisheksainii commented Dec 19, 2024

PIG208 commented Dec 19, 2024 • edited Loading

Abhisheksainii commented Dec 20, 2024

gnprice commented Dec 20, 2024

Abhisheksainii commented Dec 20, 2024 • edited Loading

Abhisheksainii commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Dec 23, 2024

Abhisheksainii commented Dec 13, 2024 •

edited

Loading

PIG208 commented Dec 19, 2024 •

edited

Loading

Abhisheksainii commented Dec 20, 2024 •

edited

Loading