1276054 - Uplift Pocket 1.0.4 changes

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Mike Kaply [:mkaply]]

Uplifting Pocket 1.0.4 changes for partner.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Patch from partner

This is the same patch as:

https://github.com/mozilla-partners/pocket/pull/1/files

with whitespace cleanup.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8757352 [details] [diff] [review]
Patch from partner

Review of attachment 8757352 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know what this patch is doing and what the idea behind it is, so I can't review it. What's the aim of the patch?

Comment 3

[Mike Kaply [:mkaply]]

Sorry. Should have posted the info from the pull request:

This update makes two changes to the Pocket system add-on:

    It removes an old test that is still running where 10% of Firefox users did not get a “learn more” link in the logged-out panel.
    It adds a new test that when 10% logged-out users click the Pocket button, it opens a sign-in screen in a new tab, instead of the current panel. This will allow Pocket and Firefox to start testing some better ways to increase sign-ups rates.

As you’ll note, this is a minor set of revisions, intended for us to be able to iterate faster on the sign-up experience (#2) above, but also to keep the first “submission” of an update to the add-on light-weight and low-risk so that we can all test/better understand/document what the process is for getting an update out to the system add-on.

Some notes for those who are interested in testing. In order to force which experience you get when clicking the button:

    Make sure you are logged out of Pocket and that you’ve clicked the button at least once (the preference will not appear until then).
    In about:config locate: extensions.pocket.settings.test.panelTab
    Set the value to “tab” to open in a new tab. (Setting the value to anything else will open the normal panel)

Comment 4

[Mike Kaply [:mkaply]]

Comment on attachment 8757352 [details] [diff] [review]
Patch from partner

Rerequesting review. Sorry for leaving out the pull info:

This update makes two changes to the Pocket system add-on:

    It removes an old test that is still running where 10% of Firefox users did not get a “learn more” link in the logged-out panel.
    It adds a new test that when 10% logged-out users click the Pocket button, it opens a sign-in screen in a new tab, instead of the current panel. This will allow Pocket and Firefox to start testing some better ways to increase sign-ups rates.

As you’ll note, this is a minor set of revisions, intended for us to be able to iterate faster on the sign-up experience (#2) above, but also to keep the first “submission” of an update to the add-on light-weight and low-risk so that we can all test/better understand/document what the process is for getting an update out to the system add-on.

Some notes for those who are interested in testing. In order to force which experience you get when clicking the button:

    Make sure you are logged out of Pocket and that you’ve clicked the button at least once (the preference will not appear until then).
    In about:config locate: extensions.pocket.settings.test.panelTab
    Set the value to “tab” to open in a new tab. (Setting the value to anything else will open the normal panel)

Comment 5

[:Gijs (he/him)]

Comment on attachment 8757352 [details] [diff] [review]
Patch from partner

Review of attachment 8757352 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/pocket/content/main.js
@@ +131,5 @@
> +
> +        // AB test: Direct logged-out users to tab vs panel
> +        if (pktApi.getSignupPanelTabTestVariant() == 'tab')
> +        {
> +            openTabWithUrl('https://getpocket.com/firefox_learnmore?src=ff_ext&s=ffi&t=buttonclick', true);

This should use a pref for the URL. We should also make sure that, inasmuch as we have tests, that pref is set to some non-remote host so that we don't start failing 10% of mochitests.

@@ +136,5 @@
> +
> +            // force the panel closed before it opens
> +            // wrapped in setTimeout to avoid race condition after logging out
> +            // if this test goes to 100%, we should move this logic up before the panel is actually opened
> +            setTimeout(function(){getPanel().hidePopup();},0);

What are our ideas about code review here? If we're treating this as Firefox code (rather than "code Pocket sends to us and expects to land as-is"), this should really come with some tests, and the styling needs fixing in a lot of places (double quotes for strings, spaces around operators and after commas, actual sentences in comments, cuddly braces instead of braces-on-their-own-lines).

At a minimum, please indent this particular setTimeout call sanely. It's not really clear what 'race condition' this is avoiding, and it would be useful to clarify this. I'm assuming you've tested this sufficiently, Mike.

::: browser/extensions/pocket/content/pktApi.jsm
@@ +613,5 @@
> +        return getSimpleTestOption('panelTab', 0.1, 'tab');
> +    }
> +
> +    function getSimpleTestOption(testName, percent, testOptionName) {
> +

Nit: Useless empty line

@@ +624,2 @@
>          {
>              var rand = (Math.floor(Math.random()*100+1));

This is pretty hard to read.

I don't understand why we need to add 1 and then floor (you could just use Math.ceil).

In fact, I'm not sure why we're truncating to integers at all.

Even more so: as noted below, the input is a fraction (of 1), so you could just compare directly to the output of Math.random() without all the multiplying to 100 etc.

@@ +624,4 @@
>          {
>              var rand = (Math.floor(Math.random()*100+1));
> +
> +            if (rand <= 100*percent) {

This code is confusing.

"Percent" means "per/of 100". But the argument to this function is not a percentage (in which case its value here should be 10), it is a fraction.

Please either fix the name of the argument or fix the code to reflect the argument's name (and update the one callsite).

@@ +650,5 @@
>          getTags: getTags,
>          isPremiumUser: isPremiumUser,
>          getSuggestedTagsForItem: getSuggestedTagsForItem,
>          getSuggestedTagsForURL: getSuggestedTagsForURL,
> +        getSignupPanelTabTestVariant: getSignupPanelTabTestVariant

Nit: add trailing comma.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #5)

> @@ +136,5 @@
> > +
> > +            // force the panel closed before it opens
> > +            // wrapped in setTimeout to avoid race condition after logging out
> > +            // if this test goes to 100%, we should move this logic up before the panel is actually opened
> > +            setTimeout(function(){getPanel().hidePopup();},0);
> 
> What are our ideas about code review here? If we're treating this as Firefox
> code (rather than "code Pocket sends to us and expects to land as-is"), this
> should really come with some tests, and the styling needs fixing in a lot of
> places (double quotes for strings, spaces around operators and after commas,
> actual sentences in comments, cuddly braces instead of
> braces-on-their-own-lines).

Regarding styling issues...changes should be properly styled, however, these files (in content/ at least, but possibly anything other than bootstrap) are from Pocket and were landed in the original built-in version.  A followup bug to get them properly formatted should be enough.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Cleanup pull request here: https://github.com/mozilla-partners/pocket/pull/2

QA steps are:


1. Make sure you are logged out of Pocket and that you’ve clicked the button at least once (the preference will not appear until then).
2. In about:config locate: extensions.pocket.settings.test.panelTab
3. Set the value to “tab” to open in a new tab. (Setting the value to anything else will open the normal panel)

Expected: If in test group (10% or set above pref), show learn-more page in tab. If in control (90%), show panel as normal

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Updated patch from partner

This is the updated patch from the partner that was reviewed in github.

I've cleaned up the whitespace issues and verified that it passes eslint.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8764643 [details] [diff] [review]
Updated patch from partner

Review of attachment 8764643 [details] [diff] [review]:
-----------------------------------------------------------------

This looks better. Can we get some automated testing for this prioritized? It'd make me a lot more confident stuff works the way it's supposed to...

Comment 10

[Mike Kaply [:mkaply]]

> This looks better. Can we get some automated testing for this prioritized? It'd make me a lot more confident stuff works the way it's supposed to...

I'll talk to Shane about that after he gets back.

Comment 11

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/0b20196061d7a3162d7ee6aa0dc470e0e2b24236
Bug 1276054 - Uplift pocket changes from partner. r=gijs

Comment 12

[Mike Kaply [:mkaply]]

This is being backed out for causing test failures.

pulsebot> Check-in: https://hg.mozilla.org/integration/fx-team/rev/f3e9898727b0 - Ryan VanderMeulen - Backed out changeset 0b20196061d7 (bug 1276054) for browser_UITour_pocket.js failures.

See:

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=0b20196061d7a3162d7ee6aa0dc470e0e2b24236

In particular:

FATAL ERROR: Non-local network connections are disabled and a connection attempt to getpocket.com (52.200.48.115) was made.
[Child 3892] ###!!! ABORT: Aborting on channel error.: file c:/builds/moz2_slave/fx-team-w32-000000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 2053
TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour_pocket.js | application terminated with exit code 1
    1163932 Intermittent browser_UITour_pocket.js | leaked 2 window(s) until shutdown [url = about:blank]
PROCESS-CRASH | browser/components/uitour/test/browser_UITour_pocket.js | application crashed [@ mozilla::net::nsSocketTransport::InitiateSocket()]
    1163932 Intermittent browser_UITour_pocket.js | leaked 2 window(s) until shutdown [url = about:blank]

Comment 13

[Mike Kaply [:mkaply]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a8e112fe65a

Comment 14

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/e08bba68cf4700aefa9c6b6e79750a396e921268
Bug 1276054 - Uplift pocket changes from partner. r=gijs

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e08bba68cf47

Comment 16

[Mike Kaply [:mkaply]]

Comment on attachment 8764643 [details] [diff] [review]
Updated patch from partner

Approval Request Comment
[Feature/regressing bug #]: Pocket Partner Changes
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]:  Low. Partner specific changes.
[String/UUID change made/needed]:

Comment 17

[Gerry Chang [:gchang]]

Hi Mike,
If we let this ride the train on 49/50 only and won't fix in 48, will this be a big impact for user?

Comment 18

[Mike Kaply [:mkaply]]

We're looking to update this as part of GoFaster (because it's a system add-on).

My understanding is that for that to happen, the changes need to go into at least beta...

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8764643 [details] [diff] [review]
Updated patch from partner

Review of attachment 8764643 [details] [diff] [review]:
-----------------------------------------------------------------

Per comment #18, take it in 48 beta 7 and aurora.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/193762e3a279

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f52ccfac4ff0

Comment 22

[Gerry Chang [:gchang]]

Need to be verified in beta.

Comment 23

[Paul Silaghi, QA [:pauly]]

(In reply to Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] from comment #7)
> QA steps are:
> 
> 1. Make sure you are logged out of Pocket and that you’ve clicked the button
> at least once (the preference will not appear until then).
> 2. In about:config locate: extensions.pocket.settings.test.panelTab
> 3. Set the value to “tab” to open in a new tab. (Setting the value to
> anything else will open the normal panel)
> 
> Expected: If in test group (10% or set above pref), show learn-more page in
> tab. If in control (90%), show panel as normal
Possible issue:
At a first try, this works fine.
But after login from the new tab, save a page to Pocket, logout -> pressing the Pocket button again opens 2 new "learn more" tabs instead of 1.
Thoughts?

Comment 24

[Paul Silaghi, QA [:pauly]]

Moreover, if already signed-in with FxA, the lear-more page doesn't provide option for "sign in with firefox", only for "sign up with firefox".

Comment 25

[Mike Kaply [:mkaply]]

I'm seeing the two page problem. Investigating. Thanks for finding it.

Comment 26

[Paul Silaghi, QA [:pauly]]

(In reply to Mike Kaply [:mkaply] from comment #25)
> I'm seeing the two page problem. Investigating. Thanks for finding it.
Hi Mike,
Any updates? Let me know if you want me to file a separate issue.

Comment 27

[Mike Kaply [:mkaply]]

> Any updates? Let me know if you want me to file a separate issue.

I have a fix. Need to get it reviewed. Thanks for the ping.

Comment 28

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 48b7, 49.0a2 (2016-07-19), 50.0a1 (2016-07-19) considering the follow up bug 1287476.