Closed Bug 1386151 Opened 7 years ago Closed 7 years ago

Enable Push Notification feature for Leanplum

Categories

(Firefox for Android Graveyard :: Metrics, enhancement)

Product:
Firefox for Android Graveyard
Component:
Metrics
Version:
56 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: cnevinchen, Assigned: cnevinchen)

References

Details

(Whiteboard: [FNC][SPT57.2][MVP][LP_M2][SP=X, 13])

Attachments

(4 files)

Bug 1386151 - Enable Push Notification feature for Leanplum.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1386151 - Accept multiple sender ID for MMA. Waiting for Sender ID udpate and API Key request.
59 bytes, text/x-review-board-request
maliu
: review+
cnevinchen
: review+
Details
Bug 1386151 - Update document for Leanplum push notification.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1386151 - If leanplum is disabled, we skip the message.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Push Notification was disabled in 56. We need to enable it now for 57. There's a strong requirement from Marketing team to enable this feature so that Leanplum can send push Notification to those users who are set to enable Leanplum.
Comment on attachment 8892328 [details] Bug 1386151 - Enable Push Notification feature for Leanplum. https://reviewboard.mozilla.org/r/163292/#review169098 ::: commit-message-f1693:3 (Diff revision 2) > +Bug 1386151 - Enable Push Notification feature for Leanplum. r?maliu > + > +In this patch, I enable some Leaplum components and add back LeanplumPushListenerService to manifest(although it's not used) for two reasons Update commit log if you decide to redirect the message to original component in LeanplumSDK ::: mobile/android/base/GcmAndroidManifest_permissions.xml.in:5 (Diff revision 2) > <uses-permission android:name="com.google.android.c2dm.permission.RECEIVE" /> > <!-- Avoid a linter warning by not double-including WAKE_LOCK. > <uses-permission android:name="android.permission.WAKE_LOCK" /> > --> > + <permission android:name="${applicationId}.permission.C2D_MESSAGE" Placeholder:${applicationId} only works in gradle build. Please consider Mach build in the first place and try to use @ANDROID_PACKAGE_NAME@ ::: mobile/android/base/MmaAndroidManifest_services.xml.in:25 (Diff revision 2) > <!-- Leanplum GCM/FCM Registration Service --> > <service android:name="com.leanplum.LeanplumPushRegistrationService" android:exported="false" > - android:enabled="false" /> > + android:enabled="true" /> > + > + <!-- Leanplum GCM Message Handling Service --> > + <service android:name="com.leanplum.LeanplumPushListenerService" android:exported="false" Consider redirect the intent start command to the actual component in LeanplumSDK. Not prepare a mock one like you did here. ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:160 (Diff revision 2) > final String packageName = info.activityInfo.packageName; > return (TextUtils.equals(packageName, context.getPackageName())); > } > > + public static boolean handleGcmMessage(@NonNull Context context, @NonNull Bundle bundle) { > + mmaHelper.handleGcmMessage(context, bundle); Make sure this message is dropped if user disabled leanplum during receive. ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:111 (Diff revision 2) > + return false; > + } > + > + @Override > + public String getSenderId() { > + return "," + LeanplumPushService.LEANPLUM_SENDER_ID; Try not to add "," here. getSenderId should not imply a concatenation prefix with comma. Make it dumb, and let others to the combine. ::: mobile/android/base/java/org/mozilla/gecko/push/PushManager.java:293 (Diff revision 2) > return registration; > } > > + @NonNull > + public static String getSenderIds() { > + return AppConstants.MOZ_ANDROID_GCM_SENDERID + MmaDelegate.getMultipleSenderId(); Do the combine right here. ::: mobile/android/thirdparty/com/leanplum/LeanplumPushListenerService.java:45 (Diff revision 2) > * @param senderId Sender ID of the sender. > * @param data Data bundle containing the message data as key-value pairs. > */ > @Override > public void onMessageReceived(String senderId, Bundle data) { > - try { > + // do nothing. See bug 1386151 Redirect the message to here. ::: mobile/android/thirdparty/com/leanplum/LeanplumPushService.java:257 (Diff revision 2) > message.putString(Keys.PUSH_MESSAGE_ID, messageId); > } > return messageId; > } > > - static void handleNotification(final Context context, final Bundle message) { > + public static void handleNotification(final Context context, final Bundle message) { If we redirect the message, we don't need this to be public outside the pacakge scope.
Attachment #8892328 - Flags: review?(max) → review-
Comment on attachment 8892328 [details] Bug 1386151 - Enable Push Notification feature for Leanplum. https://reviewboard.mozilla.org/r/163292/#review169856 ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:160 (Diff revision 3) > final String packageName = info.activityInfo.packageName; > return (TextUtils.equals(packageName, context.getPackageName())); > } > > + public static boolean handleGcmMessage(@NonNull Context context, @NonNull Bundle bundle) { > + mmaHelper.handleGcmMessage(context, bundle); Why are we always return false and not using the result of mmaHelper.handleGcmMessage ? Would it help if we add a @CheckResult annotation? ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaInterface.java:28 (Diff revision 3) > > void event(String mmaEvent, double value); > > void stop(); > + > + boolean handleGcmMessage(Context context, Bundle bundle); Consider to add a @CheckResult annotation to help this api user use the boolean ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:13 (Diff revision 3) > +import android.graphics.Bitmap; > +import android.graphics.BitmapFactory; Do we need this import? ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:23 (Diff revision 3) > import com.leanplum.Leanplum; > import com.leanplum.LeanplumActivityHelper; > +import com.leanplum.LeanplumPushNotificationCustomizer; > +import com.leanplum.LeanplumPushService; > +import com.leanplum.internal.Constants; > +import com.leanplum.utils.BitmapUtil; Do we need this import? ::: mobile/android/base/java/org/mozilla/gecko/push/PushManager.java:15 (Diff revision 3) > import android.util.Log; > > import org.json.JSONObject; > import org.mozilla.gecko.AppConstants; > import org.mozilla.gecko.gcm.GcmTokenClient; > +import org.mozilla.gecko.mma.MmaDelegate; Do we need this import?
Attachment #8892328 - Flags: review?(max) → review-
Whiteboard: [FNC][SPT#57.1][BL]
Whiteboard: [FNC][SPT#57.1][BL] → [FNC][SPT57.1][BL]
Comment on attachment 8892328 [details] Bug 1386151 - Enable Push Notification feature for Leanplum. https://reviewboard.mozilla.org/r/163292/#review171136
Attachment #8892328 - Flags: review?(max) → review-
I expect there will be some data change about the gcm token we send to Laenplum’s backend. After I get the final approval of the architecture I'll update mma.rst.
Flags: needinfo?(liuche)
Comment on attachment 8894775 [details] Bug 1386151 - Accept multiple sender ID for MMA. Waiting for Sender ID udpate and API Key request. I'm waiting for the sender ID from Jeremy so it's not ready to land/review.
Comment on attachment 8895750 [details] Bug 1386151 - Update document for Leanplum push notification. Push notification is also a kind of message. So I added the document under Messages section. The current architecture is to add another Mozilla sender ID (with the corresponding API key) and monitor the traffice there as a kill switch. I don't think it will change in the future. So the data document should be okay to be reviewed.
Comment on attachment 8895750 [details] Bug 1386151 - Update document for Leanplum push notification. https://reviewboard.mozilla.org/r/167056/#review172986 ::: mobile/android/docs/mma.rst:172 (Diff revision 1) > as "Accept" and "Show"). A messages is a combination of an Event and a Deep Link. The combinations are downloaded from Leanplum > when Leanplum SDK is initialized. When the criteria is met (set in Leanplum backend, could be when an event happens a certain number of times, > and/or targeting certain user attribute ), a prompt message will show up. And there may be buttons for users to click. Those clicks > may trigger deep links. > > +We use another Mozilla GCM sender ID to send push notifications. These push notifications will looks like the notifications that Sync sends out. Can you talk a little bit about what this id is, and how it's anonymized?
Attachment #8895750 - Flags: review?(liuche) → review+
Comment on attachment 8896108 [details] Bug 1386151 - If leanplum is disabled, we skip the message. https://reviewboard.mozilla.org/r/167394/#review173210
Attachment #8896108 - Flags: review?(max) → review+
Blocks: 1351571
Whiteboard: [FNC][SPT57.1][BL] → [FNC][SPT57.1][BL][LP_M2]
Whiteboard: [FNC][SPT57.1][BL][LP_M2] → [FNC][SPT57.2][MVP][LP_M2]
Hi Max All the pathes are ready. Please help review them. Thanks!
Flags: needinfo?(max)
Comment on attachment 8894775 [details] Bug 1386151 - Accept multiple sender ID for MMA. Waiting for Sender ID udpate and API Key request. https://reviewboard.mozilla.org/r/165938/#review174894 Do we really want to hardcode the sender id in MmaConstants? Shouldn't this come from the configuration like other constants?
(In reply to Sebastian Kaspari (:sebastian) from comment #31) > Do we really want to hardcode the sender id in MmaConstants? Shouldn't this > come from the configuration like other constants? The reason why it's hard-coded is because we are not actually changing it. In <channel>/configure.sh they are all the same.
Comment on attachment 8894775 [details] Bug 1386151 - Accept multiple sender ID for MMA. Waiting for Sender ID udpate and API Key request. https://reviewboard.mozilla.org/r/165938/#review175178 ::: mobile/android/base/MmaConstants.java.in:31 (Diff revision 6) > "@MOZ_LEANPLUM_SDK_CLIENTID@"; > //#else > null; > //#endif > > +public static final String MOZ_MMA_SENDER_ID = "242693410970"; Please make the sender id as a configure variable just like GCM id did ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:167 (Diff revision 6) > - public static boolean handleGcmMessage(@NonNull Context context, @NonNull Bundle bundle) { > - return mmaHelper.handleGcmMessage(context, bundle); > + public static boolean handleGcmMessage(@NonNull Context context, String from, @NonNull Bundle bundle) { > + return mmaHelper.handleGcmMessage(context, from, bundle); > } > > + public static String getSenderIds() { > + return AppConstants.MOZ_ANDROID_GCM_SENDERID + mmaHelper.getMmaSenderId(); MmaDelegate should only provide the sender id from itself. Please do the concat in the caller. ::: mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java:130 (Diff revision 6) > return false; > } > > + @Override > + public String getMmaSenderId() { > + return "," + MmaConstants.MOZ_MMA_SENDER_ID; Remove the "comma" and do the concat in caller or above.
Attachment #8894775 - Flags: review?(max) → review+
Comment on attachment 8892328 [details] Bug 1386151 - Enable Push Notification feature for Leanplum. https://reviewboard.mozilla.org/r/163292/#review175186
Attachment #8892328 - Flags: review?(max) → review+
Blocks: 1391575
Comment on attachment 8894775 [details] Bug 1386151 - Accept multiple sender ID for MMA. Waiting for Sender ID udpate and API Key request. https://reviewboard.mozilla.org/r/165938/#review175258 ::: mobile/android/base/MmaConstants.java.in:31 (Diff revision 6) > "@MOZ_LEANPLUM_SDK_CLIENTID@"; > //#else > null; > //#endif > > +public static final String MOZ_MMA_SENDER_ID = "242693410970"; I'll try to use a follow up bug 1391575 to fix this
Attachment #8894775 - Flags: review+
Pushed by nechen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/23eb53d80421 Enable Push Notification feature for Leanplum. r=maliu https://hg.mozilla.org/integration/autoland/rev/613112a62f90 Accept multiple sender ID for MMA. Waiting for Sender ID udpate and API Key request. r=maliu https://hg.mozilla.org/integration/autoland/rev/efc3a3eaef79 Update document for Leanplum push notification. r=liuche https://hg.mozilla.org/integration/autoland/rev/4705d4f9fcdf If leanplum is disabled, we skip the message. r=maliu
Flags: needinfo?(max)
Flags: needinfo?(liuche)
Whiteboard: [FNC][SPT57.2][MVP][LP_M2] → [FNC][SPT57.2][MVP][LP_M2][SP=X, 13]
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: