Closed Bug 1276054 Opened 5 years ago Closed 5 years ago

Uplift Pocket 1.0.4 changes

Categories

(Firefox :: Pocket, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox48 --- verified
firefox49 --- verified
firefox50 --- verified

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(2 files)

Uplifting Pocket 1.0.4 changes for partner.
This is the same patch as:

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

with whitespace cleanup.
Attachment #8757352 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8757352 - Flags: review?(gijskruitbosch+bugs) → review-
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 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)
Attachment #8757352 - Flags: review- → review?(gijskruitbosch+bugs)
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.
Attachment #8757352 - Flags: review?(gijskruitbosch+bugs) → review-
(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.
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
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.
Attachment #8764643 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8764643 - Flags: review?(gijskruitbosch+bugs) → 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...

I'll talk to Shane about that after he gets back.
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]
https://hg.mozilla.org/mozilla-central/rev/e08bba68cf47
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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]:
Attachment #8764643 - Flags: approval-mozilla-beta?
Attachment #8764643 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(mozilla)
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...
Flags: needinfo?(mozilla)
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.
Attachment #8764643 - Flags: approval-mozilla-beta?
Attachment #8764643 - Flags: approval-mozilla-beta+
Attachment #8764643 - Flags: approval-mozilla-aurora?
Attachment #8764643 - Flags: approval-mozilla-aurora+
Blocks: 1285978
Need to be verified in beta.
Flags: qe-verify+
(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?
Flags: needinfo?(mozilla)
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".
I'm seeing the two page problem. Investigating. Thanks for finding it.
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(mozilla)
> 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.
Flags: needinfo?(mozilla)
Depends on: 1287476
Verified fixed FX 48b7, 49.0a2 (2016-07-19), 50.0a1 (2016-07-19) considering the follow up bug 1287476.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.