feat(android): getNotificationSettings support #237

vincent-paing · 2021-11-18T17:26:33Z

Address #235

on Android, getNotificationSettings will return alert as 0 for permission denied and 1 for permission granted. This allows the library consumer to easily check if app level notification is enabled for Android on demand instead of settings up onBackgroundEvent. To preserve existing functionality, it will still returns 1 for the rest of the properties.

I added a unit test for the getNotifcationSettings for Android. However, I couldn't verify it works because I couldn't run the example app on my local machine with changes I made since it always point to the notifee github page for its dep. I will be more than happy to test it on emulator/simulator if someone could point me in the right direction.

Feel free to suggest fixes to the PR as well.

CLAassistant · 2021-11-18T17:26:37Z

All committers have signed the CLA.

mikehardy

👋 I lived in Chiang Mai for half a year with my family and have really fond memories of Thailand 🇹🇭

Thanks for posting this, it looks really clean - I can't see anything wrong actually

With regards to testing or demonstration I typically override the install in package.json to point to the correct directory, e.g. for the example I would point this to ../../ and yarn install again

notifee/packages/react-native/example/package.json

Line 14 in 4a720dc

"@notifee/react-native": "github:notifee/react-native-notifee",

Similar for the tests_react_native e2e app - follow this to get the core lib right https://github.com/invertase/notifee/blob/main/CONTRIBUTING.md#step-2-install-test-project-dependencies and then in combo with a ../ package.json reference, you should be able to get the code in there to verify it?

The developer experience in the tests / example apps is pretty uncomfortable at the moment, apologies for that

I don't see a reason to block on that but it would be great to see a test in there if you already have one coded up - even if it were failing in CI right now (because the committed package.json files are referencing github#main we could verify it locally and get them in and sort out developer experience after

packages/react-native/src/types/Module.ts

mikehardy · 2021-11-18T18:17:45Z

Oh! One thing worth mentioning: docs - this should be mentioned in the docs I think? Maybe adding a page on the android side similar to the ios one here https://github.com/invertase/notifee/blob/main/docs/react-native/docs/ios/permissions.md

codecov · 2021-11-18T18:19:35Z

Codecov Report

Merging #237 (4e50bf8) into main (80908cb) will increase coverage by 0.06%.
The diff coverage is 69.24%.

@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   78.62%   78.68%   +0.06%     
==========================================
  Files          29       29              
  Lines        1534     1538       +4     
  Branches      504      486      -18     
==========================================
+ Hits         1206     1210       +4     
  Misses        327      327              
  Partials        1        1

vincent-paing · 2021-11-18T18:35:23Z

Thanks you for taking a look. I just recently moved to Thailand so still getting used to it here. I quite like it here as well! 🥰

It's currently 1 AM here, so I will take a look into it tomorrow morning and will follow your direction in setting up the local environment. After I have confirmed the behavior, will update the docs 👨‍💻

packages/react-native/src/NotifeeApiModule.ts

helenaford · 2021-11-19T14:04:03Z

thanks for this addition! just had one suggested change above.

side-note: we also have a ticket to rename IOSNotificationSettings to NotificationSettings but that's best after we merge your p/r as it probably will introduce breaking changes 🙈

Edit: actually I think this will cause a breaking change on Android, as right now people will assume it will always return 1, so maybe it's worth it renaming in this p/r? But appreciate this will be asking more of your time! So happy to do it afterwards. 😎

mikehardy · 2021-11-20T13:45:28Z

@helenaford I think this would be breaking - I just kicked off CI on the current iteration and it's running now.
Thinking larger scale I think we have a collection of "notification related settings":

what are the app-wide settings for notifications for the app, ios and android?
what is power saving enabled (including the special huawei power saving 🙄 ) for the app?
what is the status of the new Android12 set exact timer permission?
for android what is the status of any particular channel?

And all of these things could change dynamically / asynchronously from anything else the app is doing

In my experience on the netinfo repo it was quite successful to have a "fetch state" API and a "listen to state" hook, each of which returned a typed state object. It's been a durable / maintainable interface https://github.com/react-native-netinfo/react-native-netinfo#usage

Over time the result value changes a little bit ("hey, we've got a new Android 12 thing you should know, it's a new property on the object) but people's code is forward-compatible and they don't have to change their API consumption patterns.

The object could look like

fetchNotificationStatus:

{
  'deviceInfo': PowerManagerInfo | undefined,
  'appPermission': {
    'notificationsEnabled': boolean,
    'alarmsEnabled': boolean,
    'isBatteryOptimizationEnabled': boolean,
  },
  'notificationSettings': {
    'general': {
      // ... all of the iOS settings here, badge, alert, etc
    },
    'channelGroups': [
       // one entry per android channel group, with a default group for ungrouped channels, e.g.:
      'defaultGroup': [
        showing channel status, e.g.:
        'excitingPromotions': AndroidChannel
      ],
      // ... rest of AndroidChannelGroups ?
    ]
  }
}

it still probably makes sense to fire events with detail information that represents what actually changed, but having a general shape defined for an "all of the state" call would help users I think so they could treat Notifee as the "store" instead of having to duplicate and manage all the state. Additionally a lot of these changes force-restart the app as a system behavior when permissions change, so the app will reboot and immediately need all the state to behave well.

Thoughts 🤔 ?

vincent-paing · 2021-11-20T17:32:52Z

Edit: actually I think this will cause a breaking change on Android, as right now people will assume it will always return 1, so maybe it's worth it renaming in this p/r? But appreciate this will be asking more of your time! So happy to do it afterwards. 😎

@helenaford I can include that as part of this PR if we're okay with the breaking changes. If I could share my suggestion, I like the way react-native-permissions set up the object structure for checking permission. We can have a similar structure as part of refactoring the iOSNotificationSettings.

type NotificationStatus = "granted" | "blocked" | "provisional"

type NotificationSettings{
  status: NotificationStatus
  iOSSettings: iOSNotificationSettings
}

We can consume this API easily for just checking the permission and don't need to set default values for iOS only settings on Android platform.

mikehardy · 2021-11-20T17:41:51Z

I had forgotten that it's not just a boolean even, it is now an enum for ios notification permission. This stuff is so detail oriented it's crazy. Good catch on that. I also like react-native-permissions as a reference for API design here, it's a pleasant API to interact with

helenaford · 2021-12-21T17:40:37Z

@mikehardy @vincent-paing I like both approaches. For simplicity, to begin with, we could start off with this - modifying our existing endpoint getNotificationSettings:

type NotificationSettings{
  status: AuthorizationStatus (existing type)
  iOSSettings: iOSNotificationSettings
}

And then as phase two, where we implement fetchNotificationStatus.

vincent-paing · 2021-12-25T17:58:09Z

Sure, I will update the commit to this API tomorrow.

type NotificationSettings{
  status: AuthorizationStatus (existing type)
  iOSSettings: iOSNotificationSettings
}

vincent-paing · 2022-01-06T19:06:05Z

@helenaford Sorry it took me a while, was busy with new year trip ✈️.

Anyhow, things to note with the changes are

I couldn't find existing AuthorizationStatus type so I went ahead and use IOSAuthorizationStatus instead.
It cause a duplication of IOSAuthorizationStatus since it's also inside IOSNotificationSettings type. I didn't want to remove it in IOSNotificationSettings because it might make bigger breaking changes. I will wait for your greenlight on refactoring it.
I went ahead and edit the native module of Android to also return authorizationStatus instead of alert to be more consistent.
I still couldn't get E2E test up and running so I haven't added test for that yet 😢

helenaford · 2022-01-13T18:00:33Z

@vincent-paing no problem at all. hope you had a great trip 😄

ah oops, yeah that's the type! I think I wrote AuthorizationStatus instead of IOSAuthorizationStatus because we want to rename it to that.
mmm, interesting re:e2e. are you getting errors when running the e2e commands? Maybe we need to update our README or fix something

vincent-paing · 2022-01-22T21:15:38Z

I am able to run the e2e on my Macbook now, do you think this PR needs an end to end test?

mikehardy · 2022-01-22T21:38:16Z

Well, I always like more testing vs less testing, but I also like more contributions then fewer contributions, the way those preferences interact is that I will certainly cheer for more tests :-), but I won't block on them. I'd merge and move forward if you didn't have time to add a test, personally. So then I think the final say goes to @helenaford. Cheers @vincent-paing

vincent-paing · 2022-01-23T06:04:32Z

Since the e2e test for notification settings requires interacting with System settings (needing to go to App Info to toggle notification on/off), I used UIAutomator to do this. However, I'm not sure how to link that test to cavy. Currently you would need to run ./gradlew androidTest to execute this automation test. Let me know if you have better approaches,

I'm not sure how to do the same for iOS since I'm unfamiliar with iOS APIs.

I've also fixed(?) the app to show the settings button even before the notification id is assigned, so that I can access the button easier from test

vincent-paing · 2022-02-17T08:48:44Z

@helenaford can you take a look at the PR and merge if it looks good to you?

docs/react-native/docs/android/permissions.md

mikehardy

This seems like it will all work - all of my comments were either

minor wording changes for documentation or, more importantly:
a real struggle for consistent, meaningful naming - I think that's actually easy to get right if we agree on a breaking change, it is just a refactor so the names are consistent and platform-neutral, but it requires a real decision to just do it @helenaford ?

docs/react-native/docs/android/introduction.md

docs/react-native/docs/android/permissions.md

mikehardy · 2022-02-17T14:26:02Z

packages/react-native/src/NotifeeApiModule.ts

+        .then(({ authorizationStatus }: Pick<NotificationSettings, 'authorizationStatus'>) => {
+          return {
+            authorizationStatus,
+            iOSSettings: {


It seems strange that we are in an android conditional, and returning an iOSSettings object?

it was to avoid platform dependent code 🤔 But, maybe it's not needed as we have authorizationStatus?

Should it returns undefined for iOSSettings instead of default values?

packages/react-native/src/types/Module.ts

helenaford

@vincent-paing thanks so much for this pr 🙌 love that you added native tests too. Just added some initial comments, I will do another review tomorrow where I'll pull it down locally and run the changes on my devices. This is going to be an awesome addition to notifee

...tive/android/app/src/androidTest/java/com/invertase/testing/GetNotificationSettingsTest.java

docs/react-native/docs/android/introduction.md

docs/react-native/docs/android/permissions.md

helenaford · 2022-02-18T00:01:54Z

packages/react-native/src/NotifeeApiModule.ts

+        .then(({ authorizationStatus }: Pick<NotificationSettings, 'authorizationStatus'>) => {
+          return {
+            authorizationStatus,
+            iOSSettings: {


it was to avoid platform dependent code 🤔 But, maybe it's not needed as we have authorizationStatus?

packages/react-native/src/types/Module.ts

helenaford · 2022-02-18T00:09:01Z

@mikehardy yes we want to rename IOSAuthorizationStatus to AuthorizationStatus as it's impossible to avoid a breaking change, we want to do everything in one go.

I will get back to you about the iOSSettings/IOSNotificationSettings ... need to make sure that it makes sense if we rename it to settings

vincent-paing · 2022-02-20T03:32:50Z

@helenaford Let me know when you have update on the breaking naming changes.

Add alert 0|1 flag for getNotificationSettings in Android This adds a functionality for lib consumer to easily check if permission is given on demand

mikehardy

I'm okay with breaking changes personally I just try to be really good about helping people cope, specifically by putting a ! on the commit line (like: feat(android)!: getNotificationSettings support` and adding a block at the end like:

BREAKING CHANGE: IOSNotificationStatus is renamed to NotificationStatus, and NotificationSettingsIOS is renamed to NotificationSettings now that android also has notification settings support" (please note I typed that out from memory without double-checking, might not be 100 but you get the idea)

When I do that, I do not get any new issues in gihtub or many complaints so I think it works :-)

helenaford · 2022-03-01T14:34:55Z

@vincent-paing @mikehardy yes, breaking changes are unavoidable even without the rename as we now will be returning false or true on Android where previously it would always be true. So yes happy with having all these changes here (sorry if my reply above wasn't clear) 😎 Will do one more review today, but looking good so far. Thanks for all your work on this @vincent-paing 🙌

Co-authored-by: Mike Hardy <[email protected]>

docs/react-native/docs/android/permissions.md

Co-authored-by: Helena Ford <[email protected]>

docs/react-native/docs/android/permissions.md

vincent-paing · 2022-03-01T18:27:20Z

@helenaford not sure why e2e android is failing, we didn't make any changes to cancel API. I run it on my local machine and test passes. Is it a flaky test?

mikehardy · 2022-03-01T18:30:10Z

@vincent-paing yes android e2e is a flaky test in CI at the moment - android e2e tests in general are really tough to stabilize in a CI environment, I'm battling with it across a few repos at the moment

vincent-paing · 2022-03-01T18:37:52Z

@mikehardy Glad to know I'm not alone, Android E2E is a flaky mess in our react native project too! 😢 We're trying to fix it as much as possible but we still see flakiness from time to ime

helenaford · 2022-03-01T19:01:03Z

@vincent-paing what type is requestPermission returning? I think we might have to change it for iOS... is it supposed to return NotificationSettings with a nested iosSettings?

vincent-paing · 2022-03-01T19:03:48Z

@helenaford It's returning with nested iOSSettings, Both getNotificationSetting and requestPermission returns same NotificationSettings type. Should it returns only authorizationStatus for Android, and authorizationStatus +iOSSettings for iOS?

helenaford · 2022-03-01T19:10:08Z

@vincent-paing thanks, makes sense, no need to change Android. But I can see an issue with the iOS. I will push up a fix and it should be ready to merge to master. On iOS the native module isn't returning a nested iOSSettings object.

vincent-paing · 2022-03-01T19:12:42Z

Good catch, I forgot to update the getNotificationSettings iOS native code when I change the type definition. Let me know if you need any help with it.

arcollector · 2022-06-28T21:41:46Z

hello, what version did start using AuthorizationStatus instead of IOSAuthorizationStatus ?

mikehardy · 2022-06-28T22:00:04Z

@arcollector I would guess it was the v5 breaking change(s) ?

https://github.com/invertase/notifee/blob/main/docs-react-native/react-native/docs/release-notes.md#500

mikehardy approved these changes Nov 18, 2021

View reviewed changes

mikehardy reviewed Nov 18, 2021

View reviewed changes

packages/react-native/src/types/Module.ts Outdated Show resolved Hide resolved

helenaford reviewed Nov 19, 2021

View reviewed changes

packages/react-native/src/NotifeeApiModule.ts Outdated Show resolved Hide resolved

helenaford added the do-not-merge label Jan 6, 2022

vincent-paing force-pushed the main branch from cf228b4 to 207e152 Compare February 17, 2022 08:48

mikehardy reviewed Feb 17, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

mikehardy requested changes Feb 17, 2022

View reviewed changes

helenaford reviewed Feb 18, 2022

View reviewed changes

vincent-paing force-pushed the main branch 2 times, most recently from 7a8b966 to c5da74a Compare February 20, 2022 03:45

vincent-paing added 2 commits February 20, 2022 10:47

feat(android): getNotificationSettings support

14dd32a

Add alert 0|1 flag for getNotificationSettings in Android This adds a functionality for lib consumer to easily check if permission is given on demand

Fix eslint prettifer issues

815f9b6

mikehardy approved these changes Feb 20, 2022

View reviewed changes

helenaford and others added 3 commits March 1, 2022 14:41

Update docs/react-native/docs/android/permissions.md

bf603d6

Co-authored-by: Mike Hardy <[email protected]>

Apply suggestions from code review

3e27d42

Co-authored-by: Mike Hardy <[email protected]>

Update docs/react-native/docs/android/introduction.md

ba37795

Co-authored-by: Mike Hardy <[email protected]>

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

Update documentations

e9cbd9f

Co-authored-by: Helena Ford <[email protected]>

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

helenaford reviewed Mar 1, 2022

View reviewed changes

docs/react-native/docs/android/permissions.md Outdated Show resolved Hide resolved

Apply suggestions from code review

d1be369

helenaford added 3 commits March 1, 2022 23:29

fix ios and rename iOSSettings to ios

dd346cd

update binaries

da362c4

fix tests for ios

4e50bf8

helenaford merged commit 9eec25d into invertase:main Mar 2, 2022

mikehardy mentioned this pull request Mar 7, 2022

[TS] authorizationStatus or authorizedStatus #333

Closed

helenaford mentioned this pull request Jun 13, 2022

Naming of IOSNotificationSettings is misleading on Android #168

Closed

feat(android): getNotificationSettings support #237

feat(android): getNotificationSettings support #237

Conversation

vincent-paing commented Nov 18, 2021

CLAassistant commented Nov 18, 2021 • edited Loading

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Nov 18, 2021

codecov bot commented Nov 18, 2021 • edited Loading

Codecov Report

vincent-paing commented Nov 18, 2021

helenaford commented Nov 19, 2021 • edited Loading

mikehardy commented Nov 20, 2021 • edited Loading

vincent-paing commented Nov 20, 2021 • edited Loading

mikehardy commented Nov 20, 2021

helenaford commented Dec 21, 2021 • edited Loading

vincent-paing commented Dec 25, 2021

vincent-paing commented Jan 6, 2022 • edited Loading

helenaford commented Jan 13, 2022

vincent-paing commented Jan 22, 2022

mikehardy commented Jan 22, 2022

vincent-paing commented Jan 23, 2022 • edited Loading

vincent-paing commented Feb 17, 2022

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Feb 17, 2022

Choose a reason for hiding this comment

helenaford Feb 18, 2022

Choose a reason for hiding this comment

vincent-paing Feb 20, 2022 • edited Loading

Choose a reason for hiding this comment

helenaford left a comment

Choose a reason for hiding this comment

helenaford Feb 18, 2022

Choose a reason for hiding this comment

helenaford commented Feb 18, 2022 • edited Loading

vincent-paing commented Feb 20, 2022

mikehardy left a comment

Choose a reason for hiding this comment

helenaford commented Mar 1, 2022 • edited Loading

vincent-paing commented Mar 1, 2022

mikehardy commented Mar 1, 2022

vincent-paing commented Mar 1, 2022

helenaford commented Mar 1, 2022

vincent-paing commented Mar 1, 2022 • edited Loading

helenaford commented Mar 1, 2022

vincent-paing commented Mar 1, 2022 • edited Loading

arcollector commented Jun 28, 2022

mikehardy commented Jun 28, 2022

CLAassistant commented Nov 18, 2021 •

edited

Loading

codecov bot commented Nov 18, 2021 •

edited

Loading

helenaford commented Nov 19, 2021 •

edited

Loading

mikehardy commented Nov 20, 2021 •

edited

Loading

vincent-paing commented Nov 20, 2021 •

edited

Loading

helenaford commented Dec 21, 2021 •

edited

Loading

vincent-paing commented Jan 6, 2022 •

edited

Loading

vincent-paing commented Jan 23, 2022 •

edited

Loading

vincent-paing Feb 20, 2022 •

edited

Loading

helenaford commented Feb 18, 2022 •

edited

Loading

helenaford commented Mar 1, 2022 •

edited

Loading

vincent-paing commented Mar 1, 2022 •

edited

Loading

vincent-paing commented Mar 1, 2022 •

edited

Loading