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

VERIFIED FIXED in Firefox 60

Status

()

defect
P1
normal
VERIFIED FIXED
Last year
Last year

People

(Reporter: jcollings, Assigned: petru)

Tracking

Trunk
Firefox 61
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

(Whiteboard: [Leanplum] [61] )

Attachments

(2 attachments, 3 obsolete attachments)

Create a user attribute in Leanplum that tells us that a user has opted out of Pocket in their Top Sites.
Assignee: nobody → petru.lingurar
Status: NEW → ASSIGNED
Posted patch bug_patch (obsolete) — Splinter Review
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.
Attachment #8964295 - Flags: review?(michael.l.comella)
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.
Flags: needinfo?(michael.l.comella)
Attachment #8964295 - Attachment description: bug_patch_and_small_refactoring → bug_patch
Because that same code would be duplicated in two other places I also refactored by replacing that with the newly created static method.
Attachment #8964482 - Flags: review?(michael.l.comella)
We need this converted to mozreview right Petru?
Flags: needinfo?(petru.lingurar)
Attachment #8964916 - Attachment is obsolete: true
Attachment #8964916 - Flags: review?(michael.l.comella)
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.
Flags: needinfo?(petru.lingurar)
Attachment #8964916 - Attachment is obsolete: false
Attachment #8964916 - Flags: review?(michael.l.comella)
Attachment #8964295 - Attachment is obsolete: true
Attachment #8964295 - Flags: review?(michael.l.comella)
Attachment #8964482 - Attachment is obsolete: true
Attachment #8964482 - Flags: review?(michael.l.comella)
Attachment #8964916 - Flags: review?(michael.l.comella) → review?(sdaswani)
Attachment #8964917 - Flags: review?(michael.l.comella) → review?(sdaswani)
Attachment #8964917 - Flags: review?(michael.l.comella) → review?(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?
Attachment #8964917 - Flags: review?(michael.l.comella)
(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.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(petru.lingurar)
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?
Attachment #8964917 - Flags: review?(michael.l.comella) → review+
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.
Attachment #8964916 - Attachment is obsolete: true
Attachment #8964916 - Flags: review?(sdaswani)
Flags: needinfo?(petru.lingurar)
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. :)
Attachment #8965254 - Flags: review+
Pushed by michael.l.comella@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/73df2d0026bb
https://hg.mozilla.org/mozilla-central/rev/6419ebd66757
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.
Flags: needinfo?(chiorean.ioana)
Flags: needinfo?(bogdan.surd)
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]:
Attachment #8965254 - Flags: approval-mozilla-release?
Attachment #8965254 - Flags: approval-mozilla-beta?
Chenxia, do leanplum probes need data review?
Flags: needinfo?(liuche)
Attachment #8965254 - Flags: approval-mozilla-release?
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.
Attachment #8965254 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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).
Flags: needinfo?(petru.lingurar)
(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!
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 )
Flags: needinfo?(liuche)
Flags: needinfo?(kplam)
Flags: needinfo?(jcollings)
Flags: needinfo?(petru.lingurar)
(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
Flags: needinfo?(chiorean.ioana)
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(bogdan.surd)
Resolution: FIXED → ---
Petru, can you please talk to Bogdan about this ASAP? thanks!
Flags: needinfo?(petru.lingurar)
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?).
(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.
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.
OS: Unspecified → Android
Hardware: Unspecified → ARM
Version: unspecified → Trunk
(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.
Flags: needinfo?(kplam)
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.
Flags: needinfo?(petru.lingurar)
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.
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Kat, Chenxia, I have made a data collection review request for this ticket - bug 1453985

The wiki documentation still needs to be updated.
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.
Duplicate of this bug: 1453985
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 (jcollings@mozilla.com) 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
Flags: needinfo?(jcollings)
You need to log in before you can comment on or make changes to this bug.