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

[Feedback]: the SDK should expose a refreshPermission function #1854

Closed
1 task done
danielesegato opened this issue Sep 26, 2023 · 16 comments
Closed
1 task done

[Feedback]: the SDK should expose a refreshPermission function #1854

danielesegato opened this issue Sep 26, 2023 · 16 comments

Comments

@danielesegato
Copy link

danielesegato commented Sep 26, 2023

What's on your mind?

The documentation instruct to request the notification permission through OneSignal:

OneSignal.Notifications.requestPermission(true)

this isn't great for us.
I'd rather use the standard permission request with the android SDK and than notify to OneSignal that it needs to refresh the permission state.

fun onNotificationPermissionChanged() {
  OneSignal.Notifications.refreshPermission()
}

You already have a method for this in the SDK you just need to make it publicly accessible.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@jennantilla
Copy link
Contributor

@danielesegato thank you for the feedback! The team will discuss.

@emawby
Copy link
Contributor

emawby commented Oct 26, 2023

It is unfortunate that Android does not provide some form of listener for OneSignal to hook into. One workaround rather than introducing a new method is for the optIn function to trigger a refresh of the state

@danielesegato
Copy link
Author

I'd argue the refresh of the state should be the main API. Forcing the developer to use the one signal API to request the notification permission is really bad.

@michael-winkler
Copy link

Any response here from a member of OneSignal?

@souravpalitrana
Copy link

This is so weird. Even I tried to give the responsibility of permission handling to onesignal SDK. Still when I see the the subscription status in the dashboard it is showing permission not granted. After relanuching the app it is showing subscribed. I want to handle notificaiton myself but you guys forcing the user to use your sdk option. Again your sdk not detecting the permission changed immediately. This is serious issue. If user do not open the app so he could not receive the notification.

@jkasten2
Copy link
Member

jkasten2 commented Apr 12, 2024

There are a few solutions / next steps that come to mind for how the OneSignal SDK can support Notification Permission Prompting outside of OneSignal code:

  1. (Automatic) Improve the Notification Permission change detection. Options :
    a. Android API to listen for change of the permission directly. (Google doesn't seem to offer this API)
    b. Investigate if there are any side-channels events, UI focus events or others that could be used to know when to grab the permission status. (Needs to be investigated yet)
    c. Poll permission status on a timmer (Not ideal, as there will be a delay, and/or will be inefficient)
  2. (Manual refresh) Add a public SDK method to tell the SDK to refresh it's permission state. Options:
    a. OneSignal.Notifications.refreshPermissionState()

The 1st (automatic) solution is highly preferred over 2 (manual refresh). Mostly because 1 is a landmine that could only be discovered after noticing the side effect (AKA this issue), or reading a part of the OneSignal documentation.

@kk-atom
Copy link

kk-atom commented Apr 16, 2024

@jkasten2 While you try to figure out how to listen for permission change directly let us do it ourselves. This issue is open for months, I dont belive you will solve it this year... Make refreshPermission() public and add a comment that usage of this method is discouraged.

@danielesegato
Copy link
Author

danielesegato commented Apr 16, 2024

I disagree option 2 (manual refresh) is the worst. In fact I think that is the way you should be doing it.

It just needs to be part of the documentation for integrating OneSignal.
And I agree with @kk-atom on the "let us do it ourselves". This issue shouldn't have existed in the first place. Anyone that worked as an Android developer would know this was needed.

@jkasten2 about your automatic research:

  1. no there's no API
  2. the permission can change outside the app, so when the app is started it should be checked again there's no way around that
  3. agree it's no ideal, but it could be a fallback IF you provide the manual option - possible with a flag to disable it if someone is sure they integrated it correctly

(imho this is a BUG it's not an enhancement or a feature request)

@sebastinto
Copy link

sebastinto commented Apr 16, 2024

While you try to figure out how to listen for permission change directly let us do it ourselves. This issue is open for months, I dont belive you will solve it this year... Make refreshPermission() public and add a comment that usage of this method is discouraged.

Agreed.

@jkasten2 I'd rather have the option to manually refresh the permission state even if it's to be considered a temporary and maybe not the most elegant solution. Please let us know if exposing such a function is being considered.

I'd also definitely consider @kk-atom 's suggestion of adding an @Discouraged annotation with a comment.

@sebastinto
Copy link

sebastinto commented Jun 3, 2024

Any update on exposing a way to refresh permission status?

We use Accompanist to request multiple permissions at the same time. Requesting multiple permissions this way means that the permission request dialog is re-used and smoothly transitions from one permission requested to another:

native.mp4

In contrast, requesting the notification permission with a separate invocation of OneSignal.requestPermission() will result in multiple dialogs being created and destroyed which definitely does not look as good in comparison:

mixed.mp4

@danielesegato
Copy link
Author

We've migrated away from OneSignal due to this and the inability to receive silent notifications without the notification permission.

We waited several months but we just couldn't rely on this SDK and opted to have more control and just built our one solution with direct usage of Firebase Cloud Messages.

I don't see how these two things aren't top priority to be fixed in an SDK about notifications.

If someone from OneSignal is reading this: take it as feedback. I'll also add that your native SDKs (cannot speak for cross platform ones) feel unnatural to use. And it is a shame because I kinda liked everything else about OneSignal. I believe the server-side API and modelling is spot on.

@raffaelgyr
Copy link

@jkasten2 Could you elaborate on why the 1st (automatic) solution is highly preferred over the 2nd (manual refresh) solution? Personally, I see it the other way around, but maybe I have not thought of something crucial that makes the 2nd solution much worse or the 1st solution much better.

@sebastinto
Copy link

sebastinto commented Jun 4, 2024

@jkasten2 Could you elaborate on why the 1st (automatic) solution is highly preferred over the 2nd (manual refresh) solution? Personally, I see it the other way around, but maybe I have not thought of something crucial that makes the 2nd solution much worse or the 1st solution much better.

@raffaelgyr I think @jkasten2 's comment may have meant to read:

Mostly because 2 is a landmine that could only be discovered after noticing the side effect (AKA this issue), or reading a part of the OneSignal documentation.

Which would answer your question.

I don't necessarily agree with this take though. Why not document it as part of the OneSignal Android SDK Setup doc, in the Initialization section?

import android.app.Application
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch

import com.onesignal.OneSignal
import com.onesignal.debug.LogLevel

// NOTE: Replace the below with your own ONESIGNAL_APP_ID
const val ONESIGNAL_APP_ID = "########-####-####-####-############"
  
class ApplicationClass : Application() {
   override fun onCreate() {
      super.onCreate()
        
      // Verbose Logging set to help debug issues, remove before releasing your app.
      OneSignal.Debug.logLevel = LogLevel.VERBOSE

      // OneSignal Initialization
      OneSignal.initWithContext(this, ONESIGNAL_APP_ID)

      // requestPermission will show the native Android notification permission prompt.
      // NOTE: It's recommended to use a OneSignal In-App Message to prompt instead. 
      // WARNING: If the notification permission is not requested with requestPermission or 
      //   OneSignal In-App Message, you will need to call OneSignal.Notifications.refreshPermission 
      //   when handling the permission request result. See {relevant section} for more details.
      CoroutineScope(Dispatchers.IO).launch {
         OneSignal.Notifications.requestPermission(false)
      }
   }
}

And use a banner (as already found throughout the documentation) as a prominent warning:

Screenshot 2024-06-04 at 5 41 48 PM

I suppose a refreshPermission() function could then use ContextCompat.checkSelfPermission internally to infer if an update to a user's Subscription Status is needed.

@jkasten2
Copy link
Member

@danielesegato

We've migrated away from OneSignal due to this and the inability to receive silent notifications without the notification permission.

Sorry to hear that. OneSignal's model doesn't have this as a first class state. If this SDK were to mark them as subscribed when they can't display notification it will have side effects on your CTR (click through rate) being lower as an example.

I'll also add that your native SDKs (cannot speak for cross platform ones) feel unnatural to use. And it is a shame because I kinda liked everything else about OneSignal. I believe the server-side API and modelling is spot on.

Could you explain in more detail about "feel unnatural to use"?

@jkasten2
Copy link
Member

@danielesegato @michael-winkler @souravpalitrana @kk-atom @sebastinto @raffaelgyr As of the 5.1.15 release the SDK now automatically detects changes to the notification permission when prompting outside of OneSignal.

@sebastinto
Copy link

Just tested 5.1.15 and can confirm the issue is fixed. Thanks @jkasten2!

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

No branches or pull requests

9 participants