-
Notifications
You must be signed in to change notification settings - Fork 987
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
fix: ask for push notification permissions on toggle in settings if skipped during onboarding #18884
Conversation
Jenkins BuildsClick to see older builds (30)
|
10% of end-end tests have passed
Failed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
|
10% of end-end tests have passed
Failed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (5)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
39564b6
to
08a61df
Compare
96% of end-end tests have passed
Failed tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
|
08a61df
to
45cd254
Compare
c682534
to
f9ca337
Compare
05ac527
to
e325e0d
Compare
Hi @siddarthkay ! Thanks for your PR.
I've tried this and I had Notification toggle OFF in Status app I've googled best practice for Android 13.0+ and found that:
|
related to #18251 The Android app would not ask for permissions to show push notifications if the permissions were skipped during login but later enabled via settings. This commit fixes that. We also take this opportunity to upgrade `react-native-permissions` library to `4.1.1` - Android
e325e0d
to
9494276
Compare
ah @mariia-skrypnyk : I did not think of this user journey, we could add a check on notifications screen when user comes back from background to see if permissions were changed or not outside the app and directly in system settings. |
@siddarthkay I need to recheck the behaviour of PN in the scenario I've described above and let you know! |
@siddarthkay asked Design regarding use cases above here Also, rechecked what happened if user turns ON skipped PN permissions on Onboarding from device settings and we have NO working PN after that (wrong behaviour) |
looks stale |
related to #18251
Summary
The Android app would not ask for permissions to show push notifications if the permissions were skipped during login but later enabled via settings.
This PR fixes that.
We also take this opportunity to upgrade
react-native-permissions
library to4.1.1
Testing notes
Platforms
Screenshot
status: ready