notif: package info indirection

[Abhisheksainii]

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

Steps I followed :

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

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 the direct calls with the indirection from Zulip Binding class.

4. 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]

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

[Abhisheksainii]

@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.

[Abhisheksainii]

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

[PIG208]

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]

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]

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

[Abhisheksainii]

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!

[Abhisheksainii]

@PIG208 kindly review the changes.

[PIG208]

Thanks for the update @Abhisheksainii! Left some comments.

[Abhisheksainii]

@PIG208 made changes as requested. Kindly review.

[PIG208]

Thanks for the update! Left some comments. In this PR's case, I think we should squash all commits into one, so that the implementation and the tests packed together for coherency.

[Abhisheksainii]

@PIG208 made changes as requested. Kindly review.

[PIG208]

Thanks for the update!

As I mentioned earlier, let's squash all commits into one.

I see that there are some comments marked as resolved that didn't actually get addressed in this update. Please be sure to double-check your changes before pushing.

[Abhisheksainii]

@PIG208 changes made as requested. Kindly review

[Abhisheksainii]

@PIG208 changes done as requested. Please review them

[gnprice]

The commit in this PR is still quite messy and hard to read, even after four rounds of review (plus a fifth posted just now above). @Abhisheksainii in order for us to continue spending time reviewing your work, you need to start doing a better job of self-reviewing it first. See our guidelines:
https://zulip.readthedocs.io/en/latest/contributing/code-reviewing.html#reviewing-your-own-code
https://zulip.readthedocs.io/en/latest/contributing/reviewable-prs.html

For your next revision, I recommend you rewrite the change from scratch. (There's not very much code here anyway, so that isn't much work.) You can look back at the current revision for reference (and you should definitely pay attention to @PIG208's latest round of comments), but I recommend not copy-pasting anything and instead typing it out. The important thing is to think through each change you're making based on what it's accomplishing and what's needed.

Then when you're writing text like a test description or a commit message, make sure it accurately describes what the code is actually doing. In this revision they don't.

When you've finished writing a new version, re-read the whole thing carefully and edit as needed to make everything as clear and accurate as you can, and to make sure you've addressed all review comments. For detailed guidance, see our docs that I've linked above.

[Abhisheksainii]

@PIG208 made changes from scratch. Kindly review

@PIG208 do I need to make some changes?

[PIG208]

Thanks for the revision! Left a review. Since it doesn't seem ready yet, we won't prioritize this PR for some time until we move on to the next milestone.

[PIG208]

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.

[PIG208]

nit:

Suggested change

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

Please also check for other spots for issues like this.

[Abhisheksainii]

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');
  }

[PIG208]

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.

[PIG208]

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.

[PIG208]

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');

[Abhisheksainii]

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?

[gnprice]

Thanks @Abhisheksainii for working on this. Because it looks like work here isn't continuing, I'm closing this PR to open up the issue for someone else to give it a try.