Skip to content
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

feat(messaging, ios): add provideAppNotificationSettings iOS permission / handler #5972

Conversation

baylesa-dev
Copy link
Contributor

Description

providesAppNotificationSettings is useful to display a button/link in iOS settings to redirect user to in-app notification settings. However the option is not available in @react-native-firebase/messaging

Apple developer documentation | UNNotificationSettings | providesAppNotificationSettings

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

🔥

@CLAassistant
Copy link

CLAassistant commented Dec 29, 2021

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Dec 29, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/3rMJRS7R2RT9dyH89BfYJYGki3pz
✅ Preview: https://react-native-firebase-git-fork-baylesa-dev-fea-988d8a-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/6yCLAXQXeNkZjGt3UXjCudux6cg6
✅ Preview: Canceled

[Deployment for 236e29e canceled]

@mikehardy
Copy link
Collaborator

Hey @baylesa-dev - this looks good as a start at letting the system know that you want to provide the feature, but I think there is a callback the system will invoke if the user selects it? So there needs to be an implementation of the callback that is ... swizzled in via our AppDelegate hooks I think? ... and the ability to specify a javascript method as a handler for when that method is called?

@codecov
Copy link

codecov bot commented Dec 29, 2021

Codecov Report

Merging #5972 (65a882c) into main (8efda50) will decrease coverage by 0.06%.
The diff coverage is 8.34%.

❗ Current head 65a882c differs from pull request most recent head 236e29e. Consider uploading reports for the commit 236e29e to get more accurate results

@@             Coverage Diff              @@
##               main    #5972      +/-   ##
============================================
- Coverage     53.04%   52.99%   -0.05%     
  Complexity      622      622              
============================================
  Files           208      208              
  Lines         10171    10183      +12     
  Branches       1618     1618              
============================================
+ Hits           5394     5395       +1     
- Misses         4523     4534      +11     
  Partials        254      254              

@baylesa-dev
Copy link
Contributor Author

baylesa-dev commented Dec 29, 2021

Hey @mikehardy ! Yes you're right, this is why the PR is still draft. The method on UNUserNotificationCenter is openSettingsForNotification but I'm a bit lost for now, I'm not an expert with Objective C :/

@mikehardy
Copy link
Collaborator

I'm pretty awful at Objective-C as well, however, it looks like this is mostly done!

We are already checking + delegating to the original implementation if it responds to the selector:

- (void)userNotificationCenter:(UNUserNotificationCenter *)center
openSettingsForNotification:(nullable UNNotification *)notification {
if (_originalDelegate != nil && originalDelegateRespondsTo.openSettingsForNotification) {
[_originalDelegate userNotificationCenter:center openSettingsForNotification:notification];
}
}

I think the thing to do is after that implementation as an "else" (so it is a non-breaking change), you can try to call a javascript-level handler if one is set.

Seems like the other items in the same area are emitting an event to the JS-level, then at the JS level if a handler is set, the event listener can call the handler?

@vercel vercel bot temporarily deployed to Preview – react-native-firebase-next December 29, 2021 22:00 Inactive
@baylesa-dev baylesa-dev marked this pull request as ready for review December 29, 2021 22:01
@baylesa-dev baylesa-dev marked this pull request as draft December 29, 2021 22:52
@baylesa-dev
Copy link
Contributor Author

baylesa-dev commented Dec 29, 2021

Not yet working when app is killed -
Edit: The event is emitted correctly on objective-c side (userNotificationCenter:openSettingsForNotification) - It seems it's not catch on JS side (handler is not set?)

@mikehardy

@baylesa-dev
Copy link
Contributor Author

should be fixed -> need documentation now

@mikehardy
Copy link
Collaborator

I'm travelling today but will do my best to review quickly. In the meantime if you check our GitHub actions tab and find the patch package action runs for your branch they can be used for easy test integrations

mikehardy
mikehardy previously approved these changes Dec 30, 2021
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM - cool feature, this is a really nice addition, thank you!

@mikehardy mikehardy changed the title feat(messaging,ios): add provideAppNotificationSettings iOS permission feat(messaging, ios): add provideAppNotificationSettings iOS permission / handler Dec 30, 2021
@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Dec 30, 2021
@vercel vercel bot temporarily deployed to Preview – react-native-firebase-next December 30, 2021 14:43 Inactive
@baylesa-dev
Copy link
Contributor Author

Missed a safe-guard for android: 7006d30

mikehardy
mikehardy previously approved these changes Dec 30, 2021
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I just fiddled with the documentation a tiny tiny bit since it was still being rejected as I see you working through lint and spelling and all the other little thing. Hopefully this one goes through if not I can push little changes to get it done since it has to wait on me to run the CI jobs anyway ever since github stopped running them auto for first PRs to avoid bitcoin miners.

@mikehardy
Copy link
Collaborator

The numbering on the documentation is irritatingly + persistently incorrect. No idea why, it's specifically 1/2/3 now but on Vercel it's 1/1/1 --> https://react-native-firebase-git-fork-baylesa-dev-fea-988d8a-invertase.vercel.app/messaging/ios-permissions#handle-button-for-in-app-notifications-settings

Looks fine on the markdown preview via github https://github.com/invertase/react-native-firebase/blob/236e29e14f02e8a0dc483c4e4c10cf071f9b051a/docs/messaging/ios-permissions.md#handle-button-for-in-app-notifications-settings - which isn't worth as much, but I'm inclined to merge it and see what happens after the native e2e runs are done

That's the only thing I'm fiddling with at the moment

@mikehardy
Copy link
Collaborator

with apologies we seem to be having some sort of CI runner shortage issue at the moment. This is good to go, from my perspective but just to make sure I want to see a clean e2e run. I'll do a local run of the PR through android/ios e2e if it doesn't resolve shortly, then we can merge and release this thing. Thanks again, this is a really high quality PR with docs and everything, that's sincerely appreciated

@mikehardy
Copy link
Collaborator

Looks like it finally ran through okay on android. I had a bit of time earlier to run it through locally myself, was fine on android and ios. Still not sure what's going on with the numbering in the docs but going to merge it to see if it was some sort of cached version somehow since I can't understand how it would possibly result in 1/1/1 when it's clearly 1/2/3 now

@mikehardy mikehardy merged commit 59cbe9f into invertase:main Dec 31, 2021
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Dec 31, 2021
@baylesa-dev
Copy link
Contributor Author

Thanks a lot @mikehardy 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants