Closed Bug 1304142 Opened 8 years ago Closed 8 years ago

[gofaster] Pocket update for fx50 part 2, including strings

Categories

(Firefox :: Pocket, defect)

Product:
Firefox
Component:
Pocket
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox50 - verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file)

Bug 1304142 pocket a/b test updates,
58 bytes, text/x-review-board-request
Gijs
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details 
This is part 2 of an update for Pocket in Fx50.

Part 1 in bug 1301442 updated the build system to move l10n out of the tree, into github/pontoon, allowing for Pocket to uplift strings.  

This part includes code changes to test additional signup flows as well as 2 string changes.  Since part 1 accidentally uplifted a patch meant only for fx51, and since this needs to get uplifted through to beta, I would like to land the code changes and existing l10n updates (which looks like about 60% of previously complete locales have been updated, and includes major languages).

Later in the cycle (another week or two) we can uplift more locale updates, increasing the number of supported locales in the addon.
[Tracking Requested - why for this release]: pocket system addon update with enhanced user acquisition testing
tracking-firefox50: --- → ?
Blocks: 1304151
Comment on attachment 8793015 [details]
Bug 1304142 pocket a/b test updates,

https://reviewboard.mozilla.org/r/79830/#review78592

::: browser/extensions/pocket/content/panels/css/signup.css:147
(Diff revision 2)
> +.pkt_ext_signupdetail p.pkt_ext_tos{
> +    color: #777;

Eh, I guess I missed this in earlier review. Space before {, but also, hardcoding colors isn't a good idea unless we guarantee a hardcoded background color here. Do we?

::: browser/extensions/pocket/locale/jar.mn:26
(Diff revision 2)
>  [features/firefox@getpocket.com] @AB_CD@.jar:
>  % locale pocket @AB_CD@ %locale/@AB_CD@/
>    # For locales we support, include the file from the locale's directory in the
>    # source tree.
>    # For other locales (and en-US) fallback to the en-US directory.
> -#if AB_CD == ar || AB_CD == ast || AB_CD == bg || AB_CD == bn_BD || AB_CD == br || AB_CD == bs || AB_CD == ca || AB_CD == cs || AB_CD == cy || AB_CD == da || AB_CD == de || AB_CD == en_GB || AB_CD == en_US || AB_CD == en_ZA || AB_CD == eo || AB_CD == es_AR || AB_CD == es_CL || AB_CD == es_ES || AB_CD == es_MX || AB_CD == et || AB_CD == eu || AB_CD == fa || AB_CD == fi || AB_CD == fr || AB_CD == fy_NL || AB_CD == ga_IE || AB_CD == gd || AB_CD == hi_IN || AB_CD == hr || AB_CD == hu || AB_CD == hy_AM || AB_CD == id || AB_CD == is || AB_CD == it || AB_CD == ja || AB_CD == ja_JP_mac || AB_CD == ka || AB_CD == kk || AB_CD == km || AB_CD == kn || AB_CD == ko || AB_CD == lt || AB_CD == lv || AB_CD == mr || AB_CD == my || AB_CD == nb_NO || AB_CD == ne_NP || AB_CD == nl || AB_CD == nn_NO || AB_CD == pa_IN || AB_CD == pt_BR || AB_CD == rm || AB_CD == ro || AB_CD == ru || AB_CD == sk || AB_CD == sl || AB_CD == son || AB_CD == sq || AB_CD == sr || AB_CD == sv_SE || AB_CD == te || AB_CD == th || AB_CD == tr || AB_CD == uk || AB_CD == xh || AB_CD == zh_CN || AB_CD == zh_TW
> +#if AB_CD == ast || AB_CD == az || AB_CD == bg || AB_CD == bn_BD || AB_CD == cs || AB_CD == da || AB_CD == de || AB_CD == dsb || AB_CD == en_US || AB_CD == es_CL || AB_CD == es_ES || AB_CD == es_MX || AB_CD == et || AB_CD == fi || AB_CD == fr || AB_CD == fy_NL || AB_CD == hr || AB_CD == hsb || AB_CD == hu || AB_CD == it || AB_CD == ja || AB_CD == kab || AB_CD == lv || AB_CD == nl || AB_CD == nn_NO || AB_CD == or || AB_CD == pt_BR || AB_CD == pt_PT || AB_CD == rm || AB_CD == ro || AB_CD == ru || AB_CD == sk || AB_CD == sl || AB_CD == sq || AB_CD == sr || AB_CD == sv_SE || AB_CD == te || AB_CD == th || AB_CD == tr || AB_CD == uk || AB_CD == zh_CN || AB_CD == zh_TW

This "if" is now confusing. Shouldn't it only list either the locales we're shipping or the ones we're not?

::: browser/extensions/pocket/skin/osx/pocket.css:41
(Diff revision 2)
>      -moz-image-region: rect(36px, 36px, 72px, 0);
>    }
>  }
>  
>  #PanelUI-pocketView[mainview=true] > .panel-subview-body > #pocket-panel-iframe {
> -  border-radius: var(--arrowpanel-border-radius);
> +  border-radius: var(--panel-arrowcontent-border-radius);

What is this a diff against? I don't see this file currently having arrowpanel-border-radius in this rule - it's panel-arrowcontent-border-radius already... not on beta or aurora either.
Attachment #8793015 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8793015 [details]
Bug 1304142 pocket a/b test updates,

https://reviewboard.mozilla.org/r/79830/#review78592

> This "if" is now confusing. Shouldn't it only list either the locales we're shipping or the ones we're not?

It contains the locales that are in the locale directory, which are the ones we're shipping.  Some locales were removed in this patch as they are not fully translated yet.  There is another bug to catch up in a week or two with additional locales.  I wanted to get this reviewed/landed early for code changes.

> What is this a diff against? I don't see this file currently having arrowpanel-border-radius in this rule - it's panel-arrowcontent-border-radius already... not on beta or aurora either.

Hmm.  THe patch is against inbound.  I can change it to be against m-c
Comment on attachment 8793015 [details]
Bug 1304142 pocket a/b test updates,

https://reviewboard.mozilla.org/r/79830/#review78592

> Eh, I guess I missed this in earlier review. Space before {, but also, hardcoding colors isn't a good idea unless we guarantee a hardcoded background color here. Do we?

There is a hardcoded background color.  I'll fix the space issue.
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Comment on attachment 8793015 [details]
Bug 1304142 pocket a/b test updates,

https://reviewboard.mozilla.org/r/79830/#review78604

rs=me . I haven't checked all the locales, I'm assuming you're coordinating with flod and co.
Attachment #8793015 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8793015 [details]
Bug 1304142 pocket a/b test updates,

https://reviewboard.mozilla.org/r/79830/#review78628

::: .eslintignore:75
(Diff revision 4)
> +# generated or library files in pocket
> +browser/extensions/pocket/content/panels/js/tmpl.js
> +browser/extensions/pocket/content/panels/js/vendor/**

Nit: please keep in-order, ie before browser/locales/**
missed a minor error in signup.css that was caught by browser_parsable_css, fixing and pushing to review for autoland, will just carry forward r+
https://hg.mozilla.org/mozilla-central/rev/016f24cec86e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment on attachment 8793015 [details]
Bug 1304142 pocket a/b test updates,

Approval Request Comment
[Feature/regressing bug #]:pocket
[User impact if declined]:none, update is new a/b testing of user aquisition
[Describe test coverage new/current, TreeHerder]:manual testing
[Risks and why]: minor code changes and locale updates, while a lot seems to have changed it's relatively benign.
[String/UUID change made/needed]:yes, but are self contained in the addon

This patch has code changes in the addon as well as some l10n update.  A followup with additional locales (once translated) will be done.
Attachment #8793015 - Flags: approval-mozilla-beta?
Attachment #8793015 - Flags: approval-mozilla-aurora?
Flod, fyi, I know you were ok with part1 of this change. Assuming this is also ok, will approve the uplift.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8793015 [details]
Bug 1304142 pocket a/b test updates,

Planned fixes in Pocket add-on for Fx50 release, Aurora51+, Beta50+
Attachment #8793015 - Flags: approval-mozilla-beta?
Attachment #8793015 - Flags: approval-mozilla-beta+
Attachment #8793015 - Flags: approval-mozilla-aurora?
Attachment #8793015 - Flags: approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #20)
> Flod, fyi, I know you were ok with part1 of this change. Assuming this is
> also ok, will approve the uplift.

Yes, all Pocket changes are OK to uplift from now on. They contain strings, but they're not exposed for localization, so they don't have an actual l10n impact.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #22)
> (In reply to Ritu Kothari (:ritu) from comment #20)
> > Flod, fyi, I know you were ok with part1 of this change. Assuming this is
> > also ok, will approve the uplift.
> 
> Yes, all Pocket changes are OK to uplift from now on. They contain strings,
> but they're not exposed for localization, so they don't have an actual l10n
> impact.

Got it.
Flags: qe-verify+
Hi Shane,

We ran some smoke tests on FX 50.0b2 over the main functionality of Pocket. No issues were found, except that we noticed the new panel - http://imgur.com/a/rItPc is not always displayed, sometimes when starting Firefox with a new profile, the old panel appears. Moreover, the new panel doesn't seem to provide a "Sign Up" option. Is everything expected?

Also, please let me know if there are other specific scenarios related to the latest Pocket changes that we might have not covered during our smoke testing.
Flags: needinfo?(mixedpuppy)
Hi Paul,
Pocket is testing user adoption when a login is not required.  Users will be able to instantly use Pocket and choose to signup later.  Currently 3 panels are a/b tested in the addon, so you may see different panels at different times.  You can find some more detail here:

https://github.com/mozilla-partners/pocket/pull/6

There is one bug that has been fixed but not yet landed, which may make one of the panels not work properly, details here:

https://github.com/mozilla-partners/pocket/pull/8

Hugo, can you comment on any further information useful for testing?
Flags: needinfo?(mixedpuppy) → needinfo?(hugo)
Hello, 
We did some testing on our end as well, but It would be great to confirm that each variation of the experiment is working properly e.g. things look visually in shape and buttons/links are working. 

As mentioned in the pull request: https://github.com/mozilla-partners/pocket/pull/6

You can test the 3 variations by logging out of Pocket, then changing the “extensions.pocket.settings.test.panelSignUp” setting in about:config to one of the following values:

“control” = old sign-up panel
“v1” = new try it now panel
“v2” = opens to learn more tab

I'll have our QA lead chime in if needed.
Flags: needinfo?(hugo)
(In reply to Hugo Romano from comment #28)
> You can test the 3 variations by logging out of Pocket, then changing the
> “extensions.pocket.settings.test.panelSignUp” setting in about:config to one
> of the following values:
> 
> “control” = old sign-up panel
> “v1” = new try it now panel
> “v2” = opens to learn more tab

"control" and "v1" seem to work fine, but setting "extensions.pocket.settings.test.panelSignUp=v2" does nothing. Setting instead "extensions.pocket.settings.test.panelSignUp=tab" opens the "learn more" tab.
Testede on FX 50b2, 52.0a1 (2016-09-27) Win 7.
Thoughts?
Flags: needinfo?(hugo)
(In reply to Paul Silaghi, QA [:pauly] from comment #29)
> (In reply to Hugo Romano from comment #28)

> > “v2” = opens to learn more tab

v2 will not work until the next patch lands, which includes pull 8 (see my comment #27), instead I think tab does work as you found (but Hugo can verify that)
It's working as expected. Thank you for the smoke test!
Flags: needinfo?(hugo)
Thank you!
Verified fixed FX 50b2, 51.0a2 (2016-09-28), 52.0a1 (2016-09-28) Win 7, Ubuntu 14.04, OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox52: fixed → verified
Depends on: 1310719
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: