search

irceline / aq-mobile-be

App for consulting air quality data in Belgium using the ionic framework for Android and iOS
Apache License 2.0
7 stars 8 forks source link

fcm interaction not working on iOS #272

Closed opeeters closed 3 years ago

opeeters commented 3 years ago

Something is going wrong with iOS fcm registration and retrieval of the subscription_key and feedback_key from FB.

Steps to reproduce:

Results:

Alternatively, try sending feedback via the feedback tab. Result: impossible to submit - when clicking on the send nothing happens, the marker does not appear on the feedback map

opeeters commented 3 years ago

After several tests, the problem was reproduced on the following devices:

It's important to relaunch the app. After relaunch, Firebase registration seems to have disappeared. Hence there is no access to subscription_key and feedback_key and all set notifications (official and automated) are disabled.

opeeters commented 3 years ago

@SpeckiJ Apparently cordova-plugin-firebase is abandoned. There is fork cordova-plugin-firebasex which is still maintained. We should best switch now still, no?

SpeckiJ commented 3 years ago

There has been very recent work on our integration of the firebase plugin - we switched to a custom branch here: https://github.com/irceline/aq-mobile-be/commit/dabfd99a31a05b26813c7ceaf9d10113e2dcb98d so i guess there have been other issues with the old plugin?

The dpa99c firebasex-version has breaking changes, but there is a migration guide available for upgrading to the firebasex version here: https://github.com/dpa99c/cordova-plugin-firebasex#migrating-from-cordova-plugin-firebase.

SpeckiJ commented 3 years ago

Note: Unfortunately we do not meet the minimum supported versions for using firebase-x out of the box:

opeeters commented 3 years ago

@bieblebrox the commit referred to above (dabfd99) comes from your team. Can you confirm that this was only to get things working in the build-pipeline? I would think that v2.0.6 is just pulled in differently, but the code is the same, no?

bieblebrox commented 3 years ago

Yes the build was failing on the old package, which was: https://github.com/irceline/aq-mobile-be/blob/45ec7058c922b63424d69f2cdf3c4e9b21d752fc/package.json 

    "cordova-plugin-firebase": "^2.0.5",

So we added it on the dev dependencies to get the builds working.

I believe that swapping for the maintained package will be unavoidable

opeeters commented 3 years ago

And to meet the minimum supported versions of cordova-plugin-firebasex is not a problem I would think. For iOS it's fine. For Android it would still be good to keep 8.1.. But we need notifications and the keys.

SpeckiJ commented 3 years ago

I apparently looked at the wrong branch - for ios we are upgraded here: https://github.com/irceline/aq-mobile-be/commit/aaa5e7fd8754f921d9975d457200d8be5a14c457

So it's only the cordova-android upgrade which is pending. I will look into it now.

opeeters commented 3 years ago

Just noticed that this issue partially also effects Android. Only the registration for notifications part. So open app, register, close app (not only background but dead as a doornail), open app again.. notifications are deactivated.. Feedback works however.

SpeckiJ commented 3 years ago

Progress can be tracked in https://github.com/irceline/aq-mobile-be/tree/refactor/upgrade_to_firebasex

I have so far:

SpeckiJ commented 3 years ago

I have fixed an issues with persistence not being triggered at all in https://github.com/irceline/aq-mobile-be/commit/ae3e61e596e9eb04c42b0bbe314e357042d41ba1 which basically reverts https://github.com/irceline/aq-mobile-be/commit/6c7bdb8b022d6c6781a944e90216a45b0486b9dd

@opeeters for me (Android 8.0) I did not even need to close the App in order to delete the Notification settings - just close and reopen the menu. Can you confirm that this is the case for you too?

opeeters commented 3 years ago

@SpeckiJ Indeed, same on Android 11. Open settings, activate, close settings, open settings, activation is gone. But feedback works on Android..

bieblebrox commented 3 years ago

Apologies for this. The developer was focused on UI and removed the subscriptions calls because in some cases it interfered with app responsiveness. (eg on subscribe and unexpected server response, would sometimes return the toggle to previous state)

Either way there needs to be a handler in case the subscription fails.

SpeckiJ commented 3 years ago

I was also not aware that removing an unused subscribe would invalidate all of the pipeline that comes before the actual handler. That is not very intuitive imo

opeeters commented 3 years ago

But so we could make a build with the reverted commit that at least Android is OK by tomorrow?

Anyhow, @SpeckiJ please let me know when it's worth it to try and make a build off refactor/upgrade_to_firebasex

SpeckiJ commented 3 years ago

@opeeters https://github.com/irceline/aq-mobile-be/commit/ae3e61e596e9eb04c42b0bbe314e357042d41ba1 is safe to be cherry-picked into the next release for sure.

As refactor/upgrade_to_firebasex also includes the major refactor of firebase to firebase-x I would hold that back until it is properly tested. For Android it worked for me (using my own demo firebase account), on iOS it is still untested as i did not manage to get my setup running yet.

I am still unsure whether the bug I found/fixed is actually the bug you reported, or it is an unrelated bug.

opeeters commented 3 years ago

OK @SpeckiJ , I'll building with the cherry-picked commit for Android.

I was able to build refactor/upgrade_to_firebasex for iOS without any issues (after doing brew install cocoapods). In the simulators feedback submission is fine and the registration for official notifications seems to persist after relaunch. However, the registration for automated notifications (which uses the subscription_key) does not persist. We should test whether this is the same on device..

SpeckiJ commented 3 years ago

Unfortunately i cannot really replicate the behaviour you describe in the simulator. Even with Device restarts it keeps the notifications toggled on/off correctly.

(based on latest refactor/upgrade_to_firebasex branch)

opeeters commented 3 years ago

Well, that is good news maybe. Maybe all is well.. I will test on device later.

opeeters commented 3 years ago

tested on device.. all is well also on iOS