Page MenuHomePhabricator

Android bugfix: call failedToRegisterPushPermission of PushHandler if notifications are disabled
ClosedPublic

Authored by marcin on Apr 4 2022, 7:21 AM.
Tags
None
Referenced Files
F3356616: D3608.id12201.diff
Sat, Nov 23, 7:39 PM
F3356609: D3608.id12343.diff
Sat, Nov 23, 7:37 PM
Unknown Object (File)
Thu, Nov 21, 3:52 AM
Unknown Object (File)
Wed, Nov 13, 12:46 PM
Unknown Object (File)
Wed, Nov 13, 12:46 PM
Unknown Object (File)
Wed, Nov 13, 12:46 PM
Unknown Object (File)
Wed, Nov 13, 12:46 PM
Unknown Object (File)
Wed, Nov 13, 12:46 PM

Details

Summary

It appears that firebase.messaging().requestPermissions() does not throw error when notification permission are not granted on Android. Using firebase.messaging().hasPermissions() is enough to detect if user did not grant permissions to the app is enough.

Test Plan

Introducing this diff at the bottom on D3565 stack allows to test D3565 features on Android. This is the test plan

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Awesome, great catch and thanks for fixing it. One note on how we can improve the process.

From ENG-944:

I haven't seen the diff yet, but would be good to include the above information in the "Test Plan" section so it's clear to your reviewers.

Ideally this discussion doesn't need to happen on the task, or on the diff – you would just submit the diff and include all of the relevant info (including the link to the relevant Android code, docs, etc. – all of it) on the diff. Sometimes discussion will be necessary beforehand, but even in those scenarios all of the relevant info should always be included in the diff.

This revision is now accepted and ready to land.Apr 5 2022, 7:30 PM