1122101 - /whatsnew tour for Hello in 36 with an exception

Status: VERIFIED FIXED

(Firefox :: Tours, defect)

Description

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

[Tracking Requested - why for this release]:

We will be creating a /whatsnew tour for 36.

The exception for this release is that we _exclude_ a segment of users from the tour.

If a user has loop.gettingStarted.seen=true, they will NOT be shown the tour. The reasons for this is that the user has already engaged with Hello since Fx35, so we don't have anything new to show them.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

New "feature", tracking.

Cory, do you know when this is going to be ready? Thanks

Comment 2

[[e-mail] (:bhearsum)]

Thanks for the early heads up in this, it's really helpful. Rail - I think you're on the hook for 36 from our side?

Comment 3

[Rail Aliiev [:rail]]

Correct, I'm on the hook from releng.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

How will this work ? Does the pref check mean that it can only happen on the client-side ? eg using  nsBrowserContentHandler.js. Or are we going to tell everyone to show /whatsnew via the update information but then not show the tour if loop.gettingStarted.seen=true ?

Comment 5

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #1)
> Cory, do you know when this is going to be ready? Thanks
Here is our production schedule: https://app.smartsheet.com/b/publish?EQBCT=3135891d871b4fd6b85eca6070c9cc0e

(In reply to Nick Thomas [:nthomas] from comment #4)
> How will this work ? Does the pref check mean that it can only happen on the
> client-side ? eg using  nsBrowserContentHandler.js. 
The product will be handling the preference check; we won't be doing any checking/redirection within the tour itself. 

> Or are we going to tell
> everyone to show /whatsnew via the update information but then not show the
> tour if loop.gettingStarted.seen=true ?
As far as specifics, :MattN may have more information here.

Comment 6

[Matthew N. [:MattN]]

We're going to have the code in nsBrowserContentHandler.js decide whether to open the tour tab or not. This bug will likely get assigned during 38.2 (2 week sprint starting Tuesday).

I forget what we did the last few times where we handled it in the client: Should the update snippet still contain the URL or not? I know the update snippet shouldn't have "silent" set or it will prevent the nsBrowserContentHandler code from opening the tab. (I think it's useful to not bypass the "silent" option checking in the code so we can remotely shut off whatsnew if something goes wrong).

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for 36 beta

This patch clears the override page if Loop has been used already.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Matthew N. [:MattN] from comment #6)
> I forget what we did the last few times where we handled it in the client:
> Should the update snippet still contain the URL or not? I know the update
> snippet shouldn't have "silent" set or it will prevent the
> nsBrowserContentHandler code from opening the tab. (I think it's useful to
> not bypass the "silent" option checking in the code so we can remotely shut
> off whatsnew if something goes wrong).

Last time we had silent set, and that had to be removed because it was overriding special case code (bug 1093187). The current configuration is to not set it for future release builds.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

Re attachment 8556638 [details] [diff] [review], startup.homepage_override_url is currently set to "" in browser/branding/official/pref/firefox-branding.js, so no-one would get a whatsnew page.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Nick Thomas [:nthomas] from comment #9)
> Re attachment 8556638 [details] [diff] [review],
> startup.homepage_override_url is currently set to "" in
> browser/branding/official/pref/firefox-branding.js, so no-one would get a
> whatsnew page.

I was thinking the idea would be to set that pref to be the tour URL for Loop. and then the patch in this bug would disregard that URL if Loop had already been used.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8556638 [details] [diff] [review]
Patch for 36 beta

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

Regarding the pref "startup.homepage_override_url":
* If we specify the URL in firefox.js, we get reports like bug 1003924 from enterprise deployments where the page can display on every startup for that version.
* If we don't specify the URL in firefox.js, users who have Firefox updated outside of them running in their profile (e.g. enterprise deployment to all users) won't see the whatsnew page at all (since the default pref and the update with actions won't exist).

Both of the above are edge cases but I prefer that latter failure case since bug 1003924 is very frustrating to users. After seeing that Jared's patch is simpler than my proposal, and avoids the first case above, I think we should have the URL set in the update snippet for 36.0 and take this approach (with the code movement below).

Rail/Nick, is that fine with you? This matches the last version of comment 4.

r=me on the patch assuming we get the URL set in the update snippet.

::: browser/components/nsBrowserContentHandler.js
@@ +511,5 @@
>  
> +            // Temporary exclusion of the Loop tour page for users who have already used Loop.
> +            if (Services.prefs.getBoolPref("loop.gettingStarted.seen")) {
> +              overridePage = "";
> +              break;

As I mentioned in comment 6, I think allowing "silent" to still prevent the page from opening is useful so I think we should move this below the `overridePage = getPostUpdateOverridePage(overridePage);` line (then you don't need the `break` either).

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Matthew N. [:MattN] from comment #11)
> Rail/Nick, is that fine with you? This matches the last version of comment 4.

Yep, no problem, setting a global value is easy. 

Rail, we'd just revert http://hg.mozilla.org/build/tools/rev/9d9c3aa48db0, and change the 33.1 in the url to the current shipping version in config (it will get bumped when the next release runs).

Comment 13

[Rail Aliiev [:rail]]

+ https://wiki.mozilla.org/Release:Release_Automation_on_Mercurial:Updates_through_Shipping#Set-up_whatsnew_page I think.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for 36 beta (r+'d by MattN)

Approval Request Comment
[Feature/regressing bug #]: what's new tour for Loop on 36
[User impact if declined]: users who have used Loop already pre-36 will see the tour
[Describe test coverage new/current, TreeHerder]: will need manual testing
[Risks and why]: none expected
[String/UUID change made/needed]: none

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/24133abc4f8d

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Rail Aliiev [:rail] from comment #13)
> +
> https://wiki.mozilla.org/Release:Release_Automation_on_Mercurial:
> Updates_through_Shipping#Set-up_whatsnew_page I think.

Could do it that way, but adding it to the patcher config would make it automatic from them on.

Comment 17

[Catalin Varga [QA][:VarCat]]

Is there a test environment that contains the first run with the Hello step, that can be used to manually verify this bug?

Comment 18

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Catalin Varga [QA][:VarCat] from comment #17)
> Is there a test environment that contains the first run with the Hello step,
> that can be used to manually verify this bug?
Setup/instructions: https://hello-onboarding.etherpad.mozilla.org/36-tour-qa

Comment 19

[Catalin Varga [QA][:VarCat]]

I've updated from FF 35.0b8 to FF 36.0b8 via the beta channel. My loop.gettingStarted.seen pref was set as false. After the update the whatsnew page was not displayed. Is this due to the channel used for updating? or was my expected behavior off?

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

The update server isn't set up to include |actions="showURL" openURL="https://www.mozilla.org/..."| on beta. We could set up a test channel for beta if that would help. Looks like it would need to be pointed at
  https://www-demo2.allizom.org/en-US/firefox/36.0/whatsnew/?oldversion=35.0
based on the etherpad in comment #18.

Comment 21

[Catalin Varga [QA][:VarCat]]

Do we get an update server in order to verify this bug or is this a low priority?

Comment 22

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Chiming in to add a couple notes. I can't speak to the update server question.

(In reply to Catalin Varga [QA][:VarCat] from comment #21)
> is this a low priority?
This tour is launching with RC36.

The prod URLs will be live 2/23 (bug 1132947).

Comment 23

[Nick Thomas [:nthomas] (UTC+12)]

Catalin, I've set up an update test for you. Please use 36.0b8 or older, and modify defaults/pref/channel-prefs.js to use 'beta-bug1122101' instead of 'beta'. Instead of restarting at line 46 on the etherpad, please close Firefox, modify the pref, then start up. You should expect to get 36.0b9 (this won't change if we do a b10 etc).

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

Rail, looks like comment #13 was wrong. Instead we can set openURL in the release config and it gets into Balrog that way:
  http://hg.mozilla.org/build/tools/file/default/scripts/updates/balrog-release-pusher.py#l109

Comment 25

[Nick Thomas [:nthomas] (UTC+12)]

The update test in comment #23 turns out to be limited to 36b -> 36.0b9 only, and you need to make sure the browser.startup.homepage_override.mstone pref isn't '36.0' before you apply the update.

Comment 26

[Catalin Varga [QA][:VarCat]]

I verified this fix using the update channel provided by Nick on:

FF 36.0b9
OS: Win 7 x64, Ubuntu 14.04 x64 and Mac Os X 10.9.5

and if the update is done via an update channel the what's new page is linked if loop.gettingStarted.seen=false and is not displayed if loop.gettingStarted.seen=true.

However if the update is a pave over update, eg installing the new FF version over the old FF version directory using the setup file, than the what's new page is not displayed even if loop.gettingStarted.seen=false .

Comment 27

[Nick Thomas [:nthomas] (UTC+12)]

The paveover case would also needs the mstone pref step, if you are overwriting 36.0b with 36.0b. Should just work (AFAICT) if you're overwriting 35.0 or older. The firstrun pref is set [1] but to the official location rather than the tour.

[1]  https://hg.mozilla.org/releases/mozilla-beta/file/8597521cb8bd/browser/branding/official/pref/firefox-branding.js#l6

Comment 28

[Matthew N. [:MattN]]

The firstrun pref isn't used for a paveover upgrade though, startup.homepage_override_url is. Not showing whatsnew for the paveover upgrade is okay IMO since there are tradeoffs either way. See comment 11.

In this case it wouldn't be too bad to see the pref in product since we wouldn't continue to show it once the user actually does the tour since loop.gettingStarted.seen would become true but it would be annoying users affected by this issue until they figure that out.

Comment 29

[Catalin Varga [QA][:VarCat]]

Based on the above comment marking the bug as verified.