Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notif: Use live value for app ID on registering APNs token #407

Open
gnprice opened this issue Nov 21, 2023 · 2 comments · May be fixed by #1143
Open

notif: Use live value for app ID on registering APNs token #407

gnprice opened this issue Nov 21, 2023 · 2 comments · May be fixed by #1143

Comments

@gnprice
Copy link
Member

gnprice commented Nov 21, 2023

In my draft for #321 I have a TODO like this:

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

This will be just fine as long as the app ID is indeed com.zulip.flutter. That means it will basically be fine right through the beta period, until we start rolling this app out as the new version of org.zulip.Zulip (the app ID currently occupied by the zulip-mobile RN app).

The fix is easy, locally:

        final appBundleId = (await PackageInfo.fromPlatform()).packageName;

The one wrinkle is that that version breaks tests, because it's directly calling a plugin. To fix it, we'll want to indirect PackageInfo.fromPlatform through a method on ZulipBinding, like we've done for other plugins. This still isn't hard, but given the impending beta, I'm deferring it for now.

@Abhisheksainii
Copy link

My approach to solve the issue:

  1. Update 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.

  2. Override the method in TestZulipBinding: In the TestZulipBinding, override getAppBundleId to return a mock or predefined value for testing.

  3. Refactor the code to use getAppBundleId: Replace direct calls to PackageInfo.fromPlatform() with the new getAppBundleId method from ZulipBinding. Like this (final appBundleId = await ZulipBinding.instance.getAppBundleId();).

  4. Write a test for the above logic.

@Abhisheksainii
Copy link

I have started working on the issue and would like to claim it.
I will be making changes to the Zulip binding class and its corresponding test class. Adding packageInfo wrapper for bundleID.
Plus will be adding test for the same.

@Abhisheksainii Abhisheksainii linked a pull request Dec 12, 2024 that will close this issue
Abhisheksainii added a commit to Abhisheksainii/zulip-flutter that referenced this issue Dec 21, 2024
made changes to access packageName from the PackageInfo class.
remove getAppBundleId() from Zulip Binding class
Add packageName variable to PackageInfo class
Fixes zulip#407
Abhisheksainii added a commit to Abhisheksainii/zulip-flutter that referenced this issue Jan 2, 2025
made changes to access packageName from the PackageInfo class.
remove getAppBundleId() from Zulip Binding class
Add packageName variable to PackageInfo class
Fixes zulip#407
Abhisheksainii added a commit to Abhisheksainii/zulip-flutter that referenced this issue Jan 30, 2025
… registration

To fetch live value of appBundleId through Zulip Binding class, we made use of already establised PackageInfo class by introducing new parameter because using this version directly(final appBundleId = (await PackageInfo.fromPlatform()).packageName;) would break tests as it's directly calling a plugin. Fixes zulip#407

The reason for making packageInfoResult nullable in Zulip Binding class was to test the case where appId fallbacks to default appBundleId (com.zulip.flutter)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants