Closed Bug 1380850 Opened 3 years ago Closed 2 years ago

Drop an event when User installs Focus (from Leanplum contextual hints)

Categories

(Firefox for Android :: Metrics, defect, P2)

Other
iOS
defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jcollings, Assigned: petru)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Leanplum] [61])

Attachments

(1 file)

Looking to create an event when a user actually installs Focus. This will help us figure which message creates higher conversions.
This will require we keep some state about whether the user has Klar or Focus installed in user prefs. When you see the switch from false to true, drop the event.
We can use Leanplum's "State" feature. And fire a when the state has changed. But this will need to land code and ride the train.
Component: General → Metrics
Whiteboard: [FNC][SPT#57.1][MVP]
Whiteboard: [FNC][SPT#57.1][MVP] → [FNC][SPT#57.1][BL]
Whiteboard: [FNC][SPT#57.1][BL] → [FNC][SPT57.1][BL]
Whiteboard: [FNC][SPT57.1][BL]
We don't have time for this release. But need to do it in the future.
Priority: -- → P2
Should probably include Pocket/Rocket as well for future campaigns
[triage] Bulk edit: product thinks leanplum is high priority: P1 rank 1.
Assignee: cnevinchen → nobody
Rank: 1
Priority: P2 → P1
Priority: P1 → P2
Whiteboard: [Leanplum] [61]
Assignee: nobody → petru.lingurar
Kate, as Joe said in comment 4, should we also include Pocket beside Focus and Klar?

Currently the Android app saves as user attributes if Focus/Klar/Pocket is installed or not when the application first starts.
But this ticket only references Klar and Focus as the apps for which we should inform when they get installed.

Also, we do not inform LeanPlum about Rocket. Maybe we need another ticket for that?
Status: NEW → ASSIGNED
Flags: needinfo?(kplam)
Comment on attachment 8970146 [details]
Bug 1380850 - Track when Focus or Klar get installed;

https://reviewboard.mozilla.org/r/238948/#review245960

This seems reasonable to me. A few things I didn't verify but if you tested, sounds like it should work and wfm! :)
- Do we still get the intents if our BroadcastReceiver is `exported=false`?
- What format does the package installed intent dataString come in?

NI me when you fix the nits and I can land it!

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:137
(Diff revision 1)
>          }
>  
>          sharedPreferences.edit().putBoolean(KEY_ANDROID_PREF_BOOLEAN_FENNEC_IS_DEFAULT, isFennecDefaultBrowser).apply();
>      }
>  
> +    static void trackInstalledPackages(@NonNull final Context context, @NonNull final String packageName,

nit: trackPackageInstalled. "InstalledPackages" indicates that we're measuring which packages are installed, rather than responding to a package just installed.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:168
(Diff revision 1)
> +    /**
> +     * Method that allows tracking specific events to LeanPlum.
> +     * Assumes the caller guarantees LeanPlum is enabled and avoids checking for this.
> +     * @param event event to be tracked to LeanPlum
> +     */
> +    private static void trackSafely(String event) {

Why do we need this method? Is it because:
1) Performance reasons? If so, I think the check is trivial.
2) Init hasn't been called? I'm not sure init actually needs an Activity (maybe just a Context) – maybe you can refactor it accordingly? Otherwise, I'd rather just call track directly from trackInstalledPackages (and you can add a comment saying it's okay because we already checked if enabled).

I'd prefer not to have this method because someone may accidentally call it and not check `isMmaEnabled`: I'd rather make redundant checks than miss a check.

::: mobile/android/base/java/org/mozilla/gecko/mma/PackageAddedReceiver.java:26
(Diff revision 1)
> +                    getIfFirstTimeInstall(context, updatedPackage));
> +        }
> +    }
> +
> +    private String getInstalledPackageName(@NonNull final String intentData) {
> +        return intentData.split(INTENT_PACKAGE_SEPARATOR)[1];

Can you comment what you expect the intentData to look like?

Also, can we verify the split length before indexing into it so we don't accidentally crash if it doesn't appear as we expect it to (since the data is external)?

::: mobile/android/base/java/org/mozilla/gecko/mma/PackageAddedReceiver.java:31
(Diff revision 1)
> +        return intentData.split(INTENT_PACKAGE_SEPARATOR)[1];
> +    }
> +
> +    private boolean getIfFirstTimeInstall(@NonNull Context context, @NonNull final String packageName) {
> +        try {
> +            long firstInstallTime =   context.getPackageManager().getPackageInfo(packageName, 0).firstInstallTime;

nit: it's unclear how long `getPackageManager()...` will take so ideally you cache that in a local var.

::: mobile/android/base/java/org/mozilla/gecko/mma/PackageAddedReceiver.java:35
(Diff revision 1)
> +        try {
> +            long firstInstallTime =   context.getPackageManager().getPackageInfo(packageName, 0).firstInstallTime;
> +            long lastUpdateTime = context.getPackageManager().getPackageInfo(packageName, 0).lastUpdateTime;
> +            return firstInstallTime == lastUpdateTime;
> +        } catch (PackageManager.NameNotFoundException e) {
> +            return true;

Why do we return true here? Add a comment.
Attachment #8970146 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8970146 [details]
Bug 1380850 - Track when Focus or Klar get installed;

https://reviewboard.mozilla.org/r/238948/#review245960

> Why do we need this method? Is it because:
> 1) Performance reasons? If so, I think the check is trivial.
> 2) Init hasn't been called? I'm not sure init actually needs an Activity (maybe just a Context) – maybe you can refactor it accordingly? Otherwise, I'd rather just call track directly from trackInstalledPackages (and you can add a comment saying it's okay because we already checked if enabled).
> 
> I'd prefer not to have this method because someone may accidentally call it and not check `isMmaEnabled`: I'd rather make redundant checks than miss a check.

Indeed, I just wanted to avoid a superfluous check even though performance would not get visibly affected.
Removed trackSafely() and went with calling the tracking method directly. 

Regarding 2), MmaDelegate.init() needs an Activity because that will be passed to MmaLeanplumImp().init() which passes it to LeanPlum to be used internally - https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/mma/MmaLeanplumImp.java#69

> Can you comment what you expect the intentData to look like?
> 
> Also, can we verify the split length before indexing into it so we don't accidentally crash if it doesn't appear as we expect it to (since the data is external)?

Added a comment to explicitly state the expected data format and a cleaner way to extract the package name.
Thanks Michael for the review!

I have added better comments, better naming for the MmaDelegate method and a better way to extract the package name from the receiver's intent.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8970146 [details]
Bug 1380850 - Track when Focus or Klar get installed;

https://reviewboard.mozilla.org/r/238948/#review246846

One last nit and then we're there! Thanks Petru.

::: mobile/android/base/java/org/mozilla/gecko/mma/PackageAddedReceiver.java:29
(Diff revisions 1 - 2)
>      }
>  
> -    private String getInstalledPackageName(@NonNull final String intentData) {
> -        return intentData.split(INTENT_PACKAGE_SEPARATOR)[1];
> +    // Our intent filter uses the "package" scheme
> +    // So the intent we receive would be in the form package:org.mozilla.klar
> +    private String getInstalledPackageName(@NonNull Uri intentData) {
> +        return intentData.getSchemeSpecificPart();

Nice improvement. They didn't name that method intuitively though, huh? :)

::: mobile/android/base/java/org/mozilla/gecko/mma/PackageAddedReceiver.java:38
(Diff revisions 1 - 2)
> -            long firstInstallTime =   context.getPackageManager().getPackageInfo(packageName, 0).firstInstallTime;
> -            long lastUpdateTime = context.getPackageManager().getPackageInfo(packageName, 0).lastUpdateTime;
> +            final PackageInfo packageInfo = context.getPackageManager().getPackageInfo(packageName, 0);
> +            return packageInfo.firstInstallTime == packageInfo.lastUpdateTime;
> -            return firstInstallTime == lastUpdateTime;
>          } catch (PackageManager.NameNotFoundException e) {
> +            // We know the package exists, the try-catch is just to make Lint happy
>              return true;

For correctness, if the package is missing (even if unlikely), we probably don't want to track the package as installed, right (rather than setting it true)? e.g. maybe it was uninstalled asynchronously or corrupted. I think you should probably throw from this method and then in onReceive:
```
try {
    MmaDelegate.trackJustInstalledPackage(context, updatedPackage, getIfFirstTimeInstall(...)
} catch (PackageManager.NameNotFoundException e) {
    /* Nothing to do */
}
```

What do you think?
Flags: needinfo?(michael.l.comella)
Comment on attachment 8970146 [details]
Bug 1380850 - Track when Focus or Klar get installed;

https://reviewboard.mozilla.org/r/238948/#review246846

> For correctness, if the package is missing (even if unlikely), we probably don't want to track the package as installed, right (rather than setting it true)? e.g. maybe it was uninstalled asynchronously or corrupted. I think you should probably throw from this method and then in onReceive:
> ```
> try {
>     MmaDelegate.trackJustInstalledPackage(context, updatedPackage, getIfFirstTimeInstall(...)
> } catch (PackageManager.NameNotFoundException e) {
>     /* Nothing to do */
> }
> ```
> 
> What do you think?

My though was that even if the unlikely event of an invalid package name is to occur, it should not matter because that same package name is compared to some static strings that contain the packages we are interested in and so, the event would still not be tracked.

But your solution is more robust, thanks!
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b2bbaf13b5eb
Track when Focus or Klar get installed; r=mcomella
https://hg.mozilla.org/mozilla-central/rev/b2bbaf13b5eb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: needinfo?(kplam) → needinfo?(cpark)
Clearing NI as this is FIXED for 61.
Flags: needinfo?(cpark)
You need to log in before you can comment on or make changes to this bug.