search

parse-community / Parse-SDK-Android

The Android SDK for Parse Platform
https://parseplatform.org/
Other
1.88k stars 739 forks source link

onPushOpen of push notification no longer able to start activity in Android 12 #1152

Open breumr opened 2 years ago

breumr commented 2 years ago

Issue Description

Notification trampoline restrictions have been introduced in Android 12 which restricts how activities can be started. As this article describes it:

To improve app performance and UX, apps that target Android 12 or higher can't start activities from services or broadcast receivers that are used as notification trampolines. In other words, after the user taps on a notification, or an action button within the notification, your app cannot call startActivity() inside of a service or broadcast receiver.

Steps to reproduce

Trigger a push notification and tap on the notification on the Android device. This triggers onPushOpen in the ParsePushBroadcastReceiver

Actual Outcome

Nothing happens. Logcat displays error: E/NotificationService: Indirect notification activity start (trampoline) from {insert package name} blocked

Expected Outcome

App should open

Environment

Parse Android SDK

azlekov commented 2 years ago

Hey @breumr thanks for opening this. I have introduced some changes in supporting push notifications for Android 12+ and I will be more than happy if you will find some time to open a PR for fixing this. I will glad to review it.

breumr commented 2 years ago

Hi @L3K0V thanks for responding. As much as I'd love to be able to fix it, I'm only a part time Android dev and am not actually confident of exactly how this should be fixed :(

mtrezza commented 2 years ago

This seems to be a critical bug, I'm adding a bounty to it.

bahaa-kallas commented 2 years ago

In order to solve it Google folks recommend removing the delegation of opening the notification from the broadcast receiver and do it directly from the Service (in our case from ParseFirebaseMessagingService). The question here. Should we follow what google says and refactor this part (will cause a breaking change) or should we glue-fix this issue by detecting the android api level at the service and run the app activity from there (Literally moving all the logic contained in onPushOpen and run it directly in ParseFirebaseMessagingService) @L3K0V You said that you have introduced some changes in supporting push notifications for Android 12+. What are these changes ? in which direction they go (Glue-fix or refactor) ? I am asking because I am researching how we can fix this.

bahaa-kallas commented 2 years ago

I just finished taking a deep look at the code and found where is the gotcha. @L3K0V I already saw your latest changes as well. https://github.com/parse-community/Parse-SDK-Android/blob/6fad701158950168fcfcb8a3730ae844513e99d4/parse/src/main/java/com/parse/ParsePushBroadcastReceiver.java#L422 The android studio shows a warning on this line contains the following message 

Notifications should only launch a `BroadcastReceiver` from notification actions (`addAction`) (This `BroadcastReceiver` intent is launched from a notification; this is discouraged except as notification actions)

What Google folks want us to do here is to pass our activity intent (Wrapped by PendingIntent) inside the notification So instead we will have to do the following 

if ( Build.VERSION.SDK_INT >= Build.VERSION_CODES.O){
            pContentIntent = PendingIntent.getActivity(context,
                contentIntentRequestCode,
                activityIntent, // The activity intent is the one we build inside the onPushOpen but now we should build it  in onPushReceive
                pendingIntentFlags
            );
        } else {
            pContentIntent = PendingIntent.getBroadcast(
                context,
                contentIntentRequestCode,
                contentIntent,
                pendingIntentFlags
            );
}

And then we pass that down to the notification builder.

https://github.com/parse-community/Parse-SDK-Android/blob/6fad701158950168fcfcb8a3730ae844513e99d4/parse/src/main/java/com/parse/ParsePushBroadcastReceiver.java#L447

What is the problem with this ? There is no way to utilize our custom broadcast event ACTION_PUSH_OPEN which means we can't trigger onPushOpen.

azlekov commented 2 years ago

@bahaa-kallas from what I remember and seeing now the Broadcast receiver was used to track push notifications open/dismiss for analytics like here: https://github.com/parse-community/Parse-SDK-Android/blob/6fad701158950168fcfcb8a3730ae844513e99d4/parse/src/main/java/com/parse/ParsePushBroadcastReceiver.java#L205 I'm not sure if these analytics are still used and working. To be honest I think the open ratio was broken and there was some issues on the parse-dashboard or parse-server repos.

https://github.com/parse-community/parse-dashboard/issues/1474

@mtrezza commented there:

Push opens are an Analytics feature. Analytics is not included in open source Parse Server. You'd have to implement your own solution for this.

And he can elaborate a little more if remember.

Everything until now means that actually the implementation can be adjusted to use activities directly instead of the Parse Broadcast Receiver if analytics are abandoned. A workaround also, if we switch to activity implementation for newer Android versions will be to tell developers if they want analytics to call ParseAnalytics.trackAppOpenedInBackground(intent); explicitly in their activities.

bahaa-kallas commented 2 years ago

As I am not part of the board here. It's not my decision to make (Whether to break push open tracking completely in this sdk Or putting the responsibility of tracking this event on the developers in their codebase but not at the sdk level) Imo. The fact that ParseAnalytics is not part of the open-source parse server is not enough to completely break analytics in the sdk. Users of open-source parse server can still enable analytics with their implementation.

I can open a PR right now that will include the fix above but I would vote for waiting a little bit. Maybe someone can come up with a better solution that won't introduce breaking changes (I have been disconnected for a while from the android eco-system so it's really likely that my naive solution above is not the only solution to the problem)

In a different context but relevant to our discussion here . Someone came up with (even worse than using broadcast receivers) idea from UI/UX perspective. https://stackoverflow.com/questions/69238026/android-12-notification-trampoline-restrictions

azlekov commented 2 years ago

As I am not part of the board here. It's not my decision to make (Whether to break push open tracking completely in this sdk Or putting the responsibility of tracking this event on the developers in their codebase but not at the sdk level) Imo.

Let's wait for @mtrezza what have to say about Analytics.

I can open a PR right now that will include the fix above but I would vote for waiting a little bit. Maybe someone can come up with a better solution that won't introduce breaking changes.

I'm thinking about push notification to open a special Parse Activity which handles the analytics. Like ParsePushActivity but again the developers should be guided to extends this ParsePushActivity.

bahaa-kallas commented 2 years ago

What you are thinking about is exactly what I have found in the stackoverflow link. I am not sure how bad is the UI/UX in that case (How much time it will take to reach the desired activity after clicking on the notification). But it seems like a solution for what we face here. If we are going in that direction. We won't require users to extend the activity in their code (ofc they can if they wan't to attach custom code to the callbacks) because they can simply add ParsePushActivity to their AndroidManifest.xml (like how we already add the broadcast receiver) and it will work.

azlekov commented 2 years ago

@bahaa-kallas I'm not taking about invisible activity, but Base activity which the developer should extend, so we can inject some logic. Of course it would be great to hear different options and opinions.

How do you imagine it, do you have some ideas after checking the code?

mtrezza commented 2 years ago

Regarding Analytics...

Parse Server:

Community:

Strategy:

Maybe we can think about removing any analytics legacy code from server and SDKs. We probably should consider it a breaking change, just in case someone has created their own analytics adapter based on the existing implementation.

More specifically regarding this issue; I think whatever solution we take, it should be possible to track push opens with popular 3rd party analytics tools, such as Google Analytics, AWS Mobile Analytics, or Mixpanel.

cbaker6 commented 2 years ago

REST API routes /events are implemented but there is no built-in (default) analytics adapter that would store analytics data in the DB. I'm not aware that anyone is providing a 3rd party analytics adapter for Parse Server, maybe for the strategic reasons mentioned below.

We probably should consider it a breaking change, just in case someone has created their own analytics adapter based on the existing implementation.

More specifically regarding this issue; I think whatever solution we take, it should be possible to track push opens with popular 3rd party analytics tools, such as Google Analytics, AWS Mobile Analytics, or Mixpanel.

Related to the analytics discussion, we have an open source analytics adapter we released recently that plugs into the Parse Server, https://github.com/netreconlab/parse-server-any-analytics-adapter

mtrezza commented 2 years ago

So maybe we should keep the analytics route? In any case, to me this seems to be a primitive implementation of analytics, which may be enough for certain use cases. In a more sophisticated app I think analytics is usually comprised of much more than tracking events. Also, in a pay-per-request setting, the Parse Server intermediary that just passes requests on from the client to a 3rd party may cost more than it's worth, especially when collection many data points.

cbaker6 commented 2 years ago

So maybe we should keep the analytics route?

My vote would be to keep as it's low maintenance from a Parse Server development perspective and isn't hurting anything. I think the usage of analytics on the server vs client side is a case-by-case situation for developers. For the apps my teams and collaborators build, we prefer the current server-side setup over the bloat (multiple dependencies) client side SDK's like Firebase provide to capture analytics. The beauty of Parse-Server is the flexibility it provides the developer for making such decisions and enables them to consider the tradeoffs of costs, requests, bloat, functionality, etc.

mtrezza commented 2 years ago

If we decide to keep it, I think we'd at least need to document it officially. In server docs and client SDK docs. Currently it appears like a half-way implemented feature artifact. We should probably also add the (only known?) Analytics adapter to our list of 3rd party adapters on our website.

cbaker6 commented 2 years ago

The PR thread that added the Analytics Adapter configuration may provide some insight for documentation, https://github.com/parse-community/parse-server/pull/2327. I currently don’t know of other adapters.

azlekov commented 2 years ago

Great discussion and most importantly I believe we are on the same page, so I propose this:

As I wrote somewhere above we could patch and for other versions of Android to leave the default old behaviour and for Android 12+ we will add support for Activities and we will document it, so if a developer wants to preserve the behaviour AND they are using analytics in some form, they should handle it by them selves in the Activity. No ParseActivity, no invisible activities. Who is using the analytics will must do additional small adjustments.

How this sounds to you @mtrezza, @bahaa-kallas? If it sounds nice - @bahaa-kallas do you still want to take this? Now we will have a clean plan how to do it. I can help.

mtrezza commented 2 years ago

Sounds good