1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Metrics, defect, P1)

Description

[Jean Collings]

Create a user attribute in Leanplum that tells us that a user has opted out of Pocket in their Top Sites.

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Attachment: bug_patch

Comment 2

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8964295 [details] [diff] [review]
bug_patch

Added a new user attribute in LP: "Top Sites by Pocket" to indicate if the user has the feature enabled or not, which will stored in LP along with the other attributes when the app first starts.

Because that same code would be duplicated in two other places I also refactored by replacing that with the newly created static method.

Comment 3

[:sdaswani]

Comment on attachment 8964295 [details] [diff] [review]
bug_patch

Mike I know this is a Leanplum change but it's really an application logic change that just sends a LP event, so I'm hoping you can ably review this. Let me know if not.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Attachment: bug_patch_and_small_refactoring

Because that same code would be duplicated in two other places I also refactored by replacing that with the newly created static method.

Comment 5

[:sdaswani]

We need this converted to mozreview right Petru?

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

Added a new user attribute in LP: "Top Sites by Pocket" to indicate if the user has the feature enabled or not, which will stored in LP along with the other attributes when the app first starts.

Review commit: https://reviewboard.mozilla.org/r/233654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233654/

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

Small refactoring to avoid duplicated code and use newly created way of checking if Pocket is recommending Top Sites.

Review commit: https://reviewboard.mozilla.org/r/233656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233656/

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Michael, I pushed two commits to review (one with just the needed change, second with minor refactoring), for the same bug and I see that the first review request's attachment was made obsolete by the second.
Both should be reviewed though.

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8964917 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233656/diff/1-2/

Comment 10

[:sdaswani]

Comment on attachment 8964917 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

https://reviewboard.mozilla.org/r/233652/#review239360

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:139
(Diff revision 3)
>          lm.destroyLoader(LOADER_ID_POCKET);
>  
>          load(lm);
>      }
>  
> +    public static boolean isPocketRecommendingTopSites(final Context context) {

I see you refactored the code from 116 to this method. It makes a lot of sense but I wonder is there any sort of unit test you can write to ensure the refactoring doesn't break the original determination?

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Petru-Mugurel Lingurar[:petru] from comment #8)
> Michael, I pushed two commits to review (one with just the needed change,
> second with minor refactoring), for the same bug and I see that the first
> review request's attachment was made obsolete by the second.
> Both should be reviewed though.

I don't understand the relationship of these patches: are they both supposed to land? It looks like their changes overlap one another, which means they'll conflict if they both land, i.e. the current branching scheme is:

Patch 1  Patch 2
|         |
|        /
|       /
|      /
|     /
|    /
|   /
|  /
| /
|/
master

In this case, I can see why one obsoleted the other: on mozreview, there should only be one changeset series per bug. Did you mean to make two separate changesets that land on top of each other? e.g.:

Patch 2: add LP probe
|
Patch 1: refactor
|
master

This should work correctly in mozreview.

As it is, it seems like I should just review the second patch, https://reviewboard.mozilla.org/r/233656/diff/2#index_header, which seems like the combination of the two patches.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8964917 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

https://reviewboard.mozilla.org/r/233656/#review239508

r+ w/ nits. Thanks for the refactor: that code duplication has irked me since it was written. :)

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:139
(Diff revision 2)
>          lm.destroyLoader(LOADER_ID_POCKET);
>  
>          load(lm);
>      }
>  
> +    public static boolean isPocketRecommendingTopSites(final Context context) {

nit: move this to `ActivityStreamConfiguration`

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:140
(Diff revision 2)
>  
>          load(lm);
>      }
>  
> +    public static boolean isPocketRecommendingTopSites(final Context context) {
> +        return ActivityStreamConfiguration.isPocketEnabledByLocale(context) &&

This method makes no guarantees about its thread safety (it's called from a background thread for Leanplum) but analyzing it, it appears to be thread safe: it accesses SharedPreferences (thread-safe) and it calls into BrowserLocaleManager (which also makes no guarantees) which appears to be thread safe.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:59
(Diff revision 2)
>      public static final String USER_ATT_FOCUS_INSTALLED = "Focus Installed";
>      public static final String USER_ATT_KLAR_INSTALLED = "Klar Installed";
>      public static final String USER_ATT_POCKET_INSTALLED = "Pocket Installed";
>      public static final String USER_ATT_DEFAULT_BROWSER = "Default Browser";
>      public static final String USER_ATT_SIGNED_IN = "Signed In Sync";
> +    public static final String USER_ATT_TOP_SITES_POCKET = "Top Sites by Pocket";

nit: "Pocket in Top Sites"?

"Top Sites by Pocket" sounds like the whole top sites is run by Pocket, rather than it being a section of top sites but it probably doesn't matter than much.

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:107
(Diff revision 2)
>          attributes.put(USER_ATT_FOCUS_INSTALLED, ContextUtils.isPackageInstalled(context, PACKAGE_NAME_FOCUS));
>          attributes.put(USER_ATT_KLAR_INSTALLED, ContextUtils.isPackageInstalled(context, PACKAGE_NAME_KLAR));
>          attributes.put(USER_ATT_POCKET_INSTALLED, ContextUtils.isPackageInstalled(context, PACKAGE_NAME_POCKET));
>          attributes.put(USER_ATT_DEFAULT_BROWSER, isDefaultBrowser(context));
>          attributes.put(USER_ATT_SIGNED_IN, FirefoxAccounts.firefoxAccountsExist(context));
> +        attributes.put(USER_ATT_TOP_SITES_POCKET, ActivityStreamPanel.isPocketRecommendingTopSites(context));

We're sending the boolean opposite of what was requested: "Create a user attribute in Leanplum that tells us that a user has opted out of Pocket in their Top Sites."

But I'm guessing that's fine?

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8964917 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

https://reviewboard.mozilla.org/r/233656/#review239584

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:139
(Diff revision 2)
>          lm.destroyLoader(LOADER_ID_POCKET);
>  
>          load(lm);
>      }
>  
> +    public static boolean isPocketRecommendingTopSites(final Context context) {

tx!

::: mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java:107
(Diff revision 2)
>          attributes.put(USER_ATT_FOCUS_INSTALLED, ContextUtils.isPackageInstalled(context, PACKAGE_NAME_FOCUS));
>          attributes.put(USER_ATT_KLAR_INSTALLED, ContextUtils.isPackageInstalled(context, PACKAGE_NAME_KLAR));
>          attributes.put(USER_ATT_POCKET_INSTALLED, ContextUtils.isPackageInstalled(context, PACKAGE_NAME_POCKET));
>          attributes.put(USER_ATT_DEFAULT_BROWSER, isDefaultBrowser(context));
>          attributes.put(USER_ATT_SIGNED_IN, FirefoxAccounts.firefoxAccountsExist(context));
> +        attributes.put(USER_ATT_TOP_SITES_POCKET, ActivityStreamPanel.isPocketRecommendingTopSites(context));

We are sending to LP an user attribute with a boolean value, depending on if Pocket suggests Top Sites or not.

In case of that feature being disabled, in LP this attribute will appear as:
Pocket in Top Sites : False

Which I think fits the requirements.

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

Small refactoring to avoid duplicated code and use newly created way of checking if Pocket is recommending Top Sites.

Review commit: https://reviewboard.mozilla.org/r/233962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233962/

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8964917 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233656/diff/2-3/

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8965254 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

https://reviewboard.mozilla.org/r/233962/#review239726

r+: these are the nits I suggested.

One note regarding commit messages: the commit messages for two commits should never be the same (and we generally don't squash them unless the developer does). In this case, the second commit might be, `Bug 1445799 - review: move isPocketRecommended... to ASConfiguration; update attr name.`

As an alternative to two commits, you can also squash your nits into the first commit (reviewboard provides an interdiff which makes it easy for reviews to see what's changed) and push that for review, but I generally don't do that because it's more work than coming up with a good commit message. :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73df2d0026bb
User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites; r=mcomella
https://hg.mozilla.org/integration/autoland/rev/6419ebd66757
User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites; r=mcomella

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/73df2d0026bb
https://hg.mozilla.org/mozilla-central/rev/6419ebd66757

Comment 19

[:sdaswani]

Ioana or Bogdan, can you verify this change in nightly please? Should be as easy as disabling pocket and then seeing the property on Leanplum.

Comment 20

[:sdaswani]

Comment on attachment 8965254 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

Approval Request Comment
[Feature/Bug causing the regression]: Per the description https://bugzilla.mozilla.org/show_bug.cgi?id=1445799#c0, this is a feature request to track when a user opts out of pocket in their top sites.
[User impact if declined]: Users may see messaging about pocket even though they expressed interest to not see pocket content.
[Is this code covered by automated tests?]: The refactor of some common code into one place should be covered by existing tests.
[Has the fix been verified in Nightly?]: Verification requested: https://bugzilla.mozilla.org/show_bug.cgi?id=1445799#c19
[Needs manual test from QE? If yes, steps to reproduce]: https://bugzilla.mozilla.org/show_bug.cgi?id=1445799#c19
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple centralization of some code used in various places, and then just updating the leanplum attributes, which is not a user facing change.
[String changes made/needed]:

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Chenxia, do leanplum probes need data review?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8965254 [details]
Bug 1445799 - User Attribute for Leanplum for Android Users that have Opted Out of Pocket in their Top Sites;

Leanplum fix needed for product reasons in Fx60. Approved for 60.0b11.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

In the future, please try to use commit messages which summarize what each patch is actually doing rather than restating the problem being solved (and being the same for multiple commits).

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/50977d34a015
https://hg.mozilla.org/releases/mozilla-beta/rev/dc9b50d160e1

Comment 25

[:sdaswani]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> In the future, please try to use commit messages which summarize what each
> patch is actually doing rather than restating the problem being solved (and
> being the same for multiple commits).

Will do, thanks Ryan!

Comment 26

[Chenxia Liu [:liuche]]

Kat, I see that Jean has deferred Leanplum to you, can you update the Leanplum wiki with documentation of this? And also fill out a data review request (this should be pretty simple: https://github.com/mozilla/data-review/blob/master/request.md )

Comment 27

[Ioana Chiorean]

(In reply to :sdaswani from comment #19)
> Ioana or Bogdan, can you verify this change in nightly please? Should be as
> easy as disabling pocket and then seeing the property on Leanplum.

Bogdan is gona work on it as he previously owned Leanplum features. Thanks

Comment 28

[Bogdan Surd, QA [:bsurd, NI]]

Device:
 - Samsung Galaxy S8 (Android 8.0)

 The "Pocket in Top Sites" setting is not visible in the LeanPlum dashboard after enabling LeanPlum on my device in the latest beta, all the other previous settings are still available.
 Tried with Recommended by Pocket enabled and disabled from the Settings menu. 

Note:
 Could not get LeanPlum to activate on the latest Nightly build in order to verify if its the same issue there as well.

Comment 29

[:sdaswani]

Petru, can you please talk to Bogdan about this ASAP? thanks!

Comment 30

[Chenxia Liu [:liuche]]

I talked briefly with Devyani today, and she said that there isn't an active campaign for "Pocket in Top Sites", but I'm not familiar with how the LP dashboard works (can you see events even if there isn't a campaign for them?).

Comment 31

[:sdaswani]

(In reply to Chenxia Liu [:liuche] - not actively working on Fennec from comment #30)
> I talked briefly with Devyani today, and she said that there isn't an active
> campaign for "Pocket in Top Sites", but I'm not familiar with how the LP
> dashboard works (can you see events even if there isn't a campaign for
> them?).

Yes, each user has an event / attributes profile, so future campaigns can run against those profiles.

Comment 32

[Bogdan Surd, QA [:bsurd, NI]]

After some investigation on Beta there seems to be a issue when updating from a .multi to a .multi build. Using a .en-US build and then updating to a .multi seems to fix the problem.

The same issue does not exist on Nightly builds since updating from a .multi to a .multi worked without any issues and the attribute was properly displayed.

For more information please see Bug 1429386 Comment 29.

Comment 33

[kplam]

(In reply to Chenxia Liu [:liuche] - not actively working on Fennec from comment #26)
> Kat, I see that Jean has deferred Leanplum to you, can you update the
> Leanplum wiki with documentation of this? And also fill out a data review
> request (this should be pretty simple:
> https://github.com/mozilla/data-review/blob/master/request.md )

I apologize for the delay. I'm not sure how to update Leanplum wiki or fill out a data review request. I've asked for help from Michele Warther and am waiting to hear back on next steps.

Comment 34

[Chenxia Liu [:liuche]]

Devyani (:dgupta) reached out to me yesterday and we discussed what the steps are so feel free to ask her (or me!) if you need more info.

Comment 35

[Bogdan Surd, QA [:bsurd, NI]]

As per Bug 1429386 Comment 30 closing and marking this bug as verified as well. I will log a separate issue after for the update problem after I finish investigating the issue.

Comment 36

[Petru-Mugurel Lingurar [:petru]]

Kat, Chenxia, I have made a data collection review request for this ticket - bug 1453985

The wiki documentation still needs to be updated.

Comment 37

[Chenxia Liu [:liuche]]

Copying the data review request and duping bug here. (Data reviews should be comments in the bug adding the data probe, not a separate bug.)

Request for data collection review for when the user has an app feature enabled or not

>1) What questions will you answer with this data?
If the feature related to Pocket recommending Top Sites is enabled or not.

>2) Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? 
Provide essential information needed for better MMA targeting (based on how the user uses the app)
    
>3) What alternative methods did you consider to answer these questions? Why were they not sufficient?
The proposed implementation is the only way to get the needed info.

>4) Can current instrumentation answer these questions?
No, this is the first time we are logging this kind of data.

>5) List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the Mozilla wiki.
 - Measurement Description: We only need to know if the user has Pocket recommending Top Sites feature enabled or not. This data is collected when the app starts.	
 - Data Collection Category: Category 1 “Technical data”  	
 - Tracking Bug #1445799
		
>6) How long will this data be collected? Choose one of the following:
I want to permanently monitor this data.

>7) What populations will you measure?
Firefox for Android users which have the LeanPlum feature available and have enabled Firefox Health Report.
 - Which release channels?: All
 - Which locales?: LeanPlum feature is currently available for users using "eng|zho|deu|fra|ita|ind|por|spa|pol|rus"

>8) If this data collection is default on, what is the opt-out mechanism for users?
Data collection is default on if the users have the LeanPlum feature available.
Users can opt-out of this by turning off Firefox Health Report.

>9) Please provide a general description of how you will analyze this data.
Data will be used to better understand how the apps features are used for better MMA targeting.

>10) Where do you intend to share the results of your analysis?
Data will be available only to specific persons within Mozilla with LeanPlum dashboard access.

Comment 39

[Chenxia Liu [:liuche]]

data review+

> 1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Pending documentation, I'm in conversation with :kplam and :dgupta about it.
https://wiki.mozilla.org/Leanplum_Contextual_Hints

> 2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, Firefox data controls

> 3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Permanent, to track engagement. Jean Collings ([e-mail]) will monitor over time. :kplam will monitor in the meantime.

There is a hypothesis, but currently no campaign that will immediately use this data. :kplam and :dgupta have it in their roadmap to add it to a campaign.

> 4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Type 2, whether Pocket stories is enabled (can be turned off by user)

> 5) Is the data collection request for default-on or default-off?

Default on

> 6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No

> 7) Is the data collection covered by the existing Firefox privacy notice? **If unsure: escalate to legal if:**

Yes

> 8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

No, follows other LeanPlum campaign/data collection