Skip to content

notif: package info indirection #1143

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

notif: package info indirection #1143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/model/binding.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,12 @@ class LinuxDeviceInfo implements BaseDeviceInfo {
class PackageInfo {
final String version;
final String buildNumber;
final String packageName;

const PackageInfo({
required this.version,
required this.buildNumber,
required this.packageName,
});
}

Expand Down Expand Up @@ -411,6 +413,7 @@ class LiveZulipBinding extends ZulipBinding {
_syncPackageInfo = PackageInfo(
version: info.version,
buildNumber: info.buildNumber,
packageName: info.packageName,
);
} catch (e, st) {
assert(debugLog('Failed to prefetch package info: $e\n$st')); // TODO(log)
Expand Down
3 changes: 1 addition & 2 deletions lib/notifications/receive.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,7 @@ 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);
await addApnsToken(connection, token: token, appid: (await ZulipBinding.instance.packageInfo)?.packageName?? 'com.zulip.flutter');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping #1143 (comment)

Let's include a assert(debugLog(…)) here with a TODO(log) comment.

It would be helpful to structure this with an if-statement first.

Suggested change
await addApnsToken(connection, token: token, appid: (await ZulipBinding.instance.packageInfo)?.packageName?? 'com.zulip.flutter');
final packageInfo = await ZulipBinding.instance.packageInfo;
if (packageInfo == null) {
assert(debugLog('missing packageInfo')); // TODO(log)
}
await addApnsToken(connection,
token: token, appid: packageInfo?.packageName ?? 'com.zulip.flutter');

Copy link
Author

@Abhisheksainii Abhisheksainii Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense!
I also thought of adding it at first, but incase packageInfo becomes null, it would stop the execution which is ahead of the assert statement. Wouldn't that interrupt our test?


case TargetPlatform.linux:
case TargetPlatform.macOS:
Expand Down
2 changes: 1 addition & 1 deletion test/api/core_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ void main() {
});
}

const packageInfo = PackageInfo(version: '0.0.1', buildNumber: '1');
const packageInfo = PackageInfo(version: '0.0.1', buildNumber: '1', packageName: 'com.zulip.flutter.test');

const testCases = [
('ZulipFlutter/0.0.1+1 (Android 14)', AndroidDeviceInfo(release: '14', sdkInt: 34), ),
Expand Down
4 changes: 2 additions & 2 deletions test/model/binding.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@ class TestZulipBinding extends ZulipBinding {
BaseDeviceInfo? get syncDeviceInfo => deviceInfoResult;

/// The value that `ZulipBinding.instance.packageInfo` should return.
PackageInfo packageInfoResult = _defaultPackageInfo;
static const _defaultPackageInfo = PackageInfo(version: '0.0.1', buildNumber: '1');
PackageInfo? packageInfoResult = _defaultPackageInfo;
static const _defaultPackageInfo = PackageInfo(version: '0.0.1', buildNumber: '1', packageName: 'com.zulip.flutter.test');

void _resetPackageInfo() {
packageInfoResult = _defaultPackageInfo;
Expand Down
20 changes: 18 additions & 2 deletions test/model/store_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: 'com.zulip.flutter.test');
}

if (defaultTargetPlatform == TargetPlatform.android) {
Expand Down Expand Up @@ -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: 'com.zulip.flutter.test');
}

if (defaultTargetPlatform == TargetPlatform.android) {
Expand All @@ -1082,6 +1082,22 @@ void main() {
checkLastRequestFcm(token: '456def');
}
}));


test('fallback to default appBundleId in case packageInfo is null', ()async{
// This tests the case where packageInfo is null
// and default appBundleId gets passed to the [NotificationService.registerToken] method
Comment on lines +1088 to +1089
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the comment shouldn't be needed here, as the test code itself would be self-explanatory in most cases.

It would be better to have comments on the exact places where something might not be obvious to a reader.


prepareStore();
connection.prepare(json: {});

if(defaultTargetPlatform == TargetPlatform.iOS){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check doesn't seem right. Because the debugDefaultTargetPlatformOverride doesn't get updated, the block inside never gets executed. The other tests use this because they use testAndroidIos instead of test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
if(defaultTargetPlatform == TargetPlatform.iOS){
if (defaultTargetPlatform == TargetPlatform.iOS) {

Please also check for other spots for issues like this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found these instructions in testAndroidIos function :
final origTargetPlatform = debugDefaultTargetPlatformOverride;
addTearDown(() => debugDefaultTargetPlatformOverride = origTargetPlatform);
debugDefaultTargetPlatformOverride = TargetPlatform.iOS;

what do you think of replicating them in the above test. It will look something like this:

prepareStore();
connection.prepare(json: {});

  final origTargetPlatform = debugDefaultTargetPlatformOverride;
  addTearDown(() {
    testBinding.reset();
    debugDefaultTargetPlatformOverride = origTargetPlatform;
  });

  debugDefaultTargetPlatformOverride = TargetPlatform.iOS;

  if (debugDefaultTargetPlatformOverride == TargetPlatform.iOS) {
    testBinding.packageInfoResult = null;
    await NotificationService.registerToken(connection, token: '012abc');
    checkLastRequestApns(token: '012abc', appid: 'com.zulip.flutter');
  }

testBinding.packageInfoResult=null;
await NotificationService.registerToken(connection, token: '012abc');
checkLastRequestApns(token: '012abc', appid: 'com.zulip.flutter');
addTearDown(testBinding.reset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually this is called at the beginning of the test. addTearDown registers a callback function that gets called when the test is finished and "teared down", so there is no need to place it near the end.

}
});
});
}

Expand Down
Loading