1375775 - Allow reuse existing tours

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[[e-mail]]

convert existing onboardingTours from list to dictionary. And form the actual tour from a  predefined id list. 
So we can load userTours as new user tour or update user tour based on the predefined list.

The patch is r+'ed in bug 1367696
https://reviewboard.mozilla.org/r/150408/diff/5/

but we need to think twice on this due to the newly introduced notification also share the same list.


from fischer's comment
```
Please switch the usage between userTours and onboardingTours. That is, let userTours be the tours map and let onboardingTours still be the tours array as originally. This is for backward compatibilty, because we have other codes which is using onboardingTours. This code compatibility handling is a bit tricky, maybe we could handle when rebasing code and have a discussion about how to approach then.

And because the tour notification would need the tours to prompt notification, we need to decide tours in the init stage or the tour notification couldn't find any tours to prompt.
```

Comment 1

[[e-mail]]

In this issue we can also save the new/update tour order in pref, so PM could change the pref later with shield and analysis for the best combination.

Comment 2

[[e-mail]]

Attachment: Bug 1375775 - Allow reuse existing tours;

Review commit: https://reviewboard.mozilla.org/r/152718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152718/

Comment 3

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/1-2/

Comment 4

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/2-3/

Comment 5

[[e-mail]]

The patch is based on Bug 1357641 - Add onboarding tour notification and Bug 1367696 - determine the tour type, so please test with these 2 patches.

You can change the tour order by change "browser.onboarding.newtour" pref and test updating tour via add tour id in "browser.onboarding.updatetour" and turn "browser.onboarding.tour-type" to "update".


I'd like do some refactor to remove the global `onboardingTours` and add tests in the PART II patch.

Comment 6

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/3-4/

Comment 7

[Fischer [:Fischer]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

https://reviewboard.mozilla.org/r/152718/#review157842

::: browser/extensions/onboarding/content/onboarding.js:199
(Diff revision 2)
>    async init(contentWindow) {
>      this._window = contentWindow;
>      this._tourItems = [];
>      this._tourPages = [];
>  
> +    let userTours = [];

Please rename `userTours` to such as `tourIds` or `shortTourIds` to align with what it carrys.

::: browser/extensions/onboarding/content/onboarding.js:201
(Diff revision 2)
>      this._tourItems = [];
>      this._tourPages = [];
>  
> +    let userTours = [];
>      // we only support the new user tour at this moment
> -   if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") !== "new") {
> +    if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") === "new") {

You could call a shorter of  `Preferences.get("browser.onboarding.tour-type", "update")` since Preferences.jsm has been imprted already. Up to you.

::: browser/extensions/onboarding/content/onboarding.js:242
(Diff revision 2)
>      this._initNotification();
>    }
>  
> +  _getTourList(tourType) {
> +    let tours = Services.prefs.getStringPref(`browser.onboarding.${tourType}tour`, "");
> +    return tours.split(",").filter(word => word !== "" && word.trim());

Since we are calling `trim` here, looks like we are assuming there could be extra whitespace there. Please also return trimmed words to outside in case of "browser.onboarding.newtour" = "private, addons, customize, search, default".

Comment 8

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/4-5/

Comment 9

[Fischer [:Fischer]]

Hi Fred,
Would you like to postpone this bug to Thursday? Because the bug 1357021 which is going to be checked-in today also need code compatibility handling. And the tests should also need code compatibility handling. Thanks.

Comment 10

[[e-mail]]

Sure, I'll also make the change depends on bug 1357021

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

https://reviewboard.mozilla.org/r/152718/#review158060

::: browser/extensions/onboarding/content/onboarding.js:180
(Diff revision 5)
>            <button id="onboarding-tour-default-browser-button" data-l10n-id="${defaultBrowserButtonId}"></button>
>          </aside>
>        `;
>        return div;
>      },
>    },

Just remember to rebase and do the same change with the sync tour before landing. If you're going to postpone it to Thursday, sync tour should be in central.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

https://reviewboard.mozilla.org/r/152718/#review158294

::: browser/app/profile/firefox.js:1712
(Diff revision 5)
>  pref("browser.onboarding.hidden", false);
>  // On the Activity-Stream page, the snippet's position overlaps with our notification.
>  // So use `browser.onboarding.notification.finished` to let the AS page know
>  // if our notification is finished and safe to show their snippet.
>  pref("browser.onboarding.notification.finished", false);
> +pref("browser.onboarding.newtour", "private,addons,customize,search,default");

Why does this need to be a preference?

::: browser/extensions/onboarding/content/onboarding.js:199
(Diff revision 5)
>      this._tourItems = [];
>      this._tourPages = [];
> +    this._tours = [];
>  
> +    let tourIds = [];
>      // we only support the new user tour at this moment

This comment is outdated

::: browser/extensions/onboarding/content/onboarding.js:200
(Diff revision 5)
> -   if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") !== "new") {
> +    if (Services.prefs.getStringPref("browser.onboarding.tour-type", "update") === "new") {
> +      tourIds = this._getTourList("new");
> +    } else {
> +      tourIds = this._getTourList("update");
> +    }

The list you get always matches the pref value so maybe just use that instead of the if statement.

::: browser/extensions/onboarding/content/onboarding.js:240
(Diff revision 5)
>  
>      this._initPrefObserver();
>      this._initNotification();
>    }
>  
> +  _getTourList(tourType) {

Call this _getTourIDList

Comment 13

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

https://reviewboard.mozilla.org/r/152718/#review158304

::: browser/app/profile/firefox.js:1712
(Diff revision 5)
>  pref("browser.onboarding.hidden", false);
>  // On the Activity-Stream page, the snippet's position overlaps with our notification.
>  // So use `browser.onboarding.notification.finished` to let the AS page know
>  // if our notification is finished and safe to show their snippet.
>  pref("browser.onboarding.notification.finished", false);
> +pref("browser.onboarding.newtour", "private,addons,customize,search,default");

1. PM want to change the order of tours remotely with Shield study and like to change the different combinations from the tourset.
2. We can enable/disable/change order of update tours when we set the `updatetour` pref.

So put the tours order into preference might be a good idea.

Comment 14

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/5-6/

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

https://reviewboard.mozilla.org/r/152718/#review158802

::: browser/extensions/onboarding/content/onboarding.js:268
(Diff revision 6)
>      this._initNotification();
>    }
>  
> +  _getTourIDList(tourType) {
> +    let tours = Services.prefs.getStringPref(`browser.onboarding.${tourType}tour`, "");
> +    return tours.split(",").filter(word => word !== "");

Let's do the trimming here with a

    .map(word => word.trim())

Comment 16

[[e-mail]]

Attachment: Bug 1375775 - test cases for reusable tourset;

Review commit: https://reviewboard.mozilla.org/r/155072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155072/

Comment 17

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/6-7/

Comment 18

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/7-8/

Comment 19

[[e-mail]]

Comment on attachment 8884157 [details]
Bug 1375775 - test cases for reusable tourset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155072/diff/1-2/

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8884157 [details]
Bug 1375775 - test cases for reusable tourset;

https://reviewboard.mozilla.org/r/155072/#review160898

Comment 21

[[e-mail]]

Comment on attachment 8881547 [details]
Bug 1375775 - Allow reuse existing tours;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152718/diff/8-9/

Comment 22

[[e-mail]]

Comment on attachment 8884157 [details]
Bug 1375775 - test cases for reusable tourset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155072/diff/2-3/

Comment 23

[[e-mail]]

thanks!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3e948f381b5
Allow reuse existing tours;r=mossop
https://hg.mozilla.org/integration/autoland/rev/3915337ce78e
test cases for reusable tourset;r=mossop

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3e948f381b5
https://hg.mozilla.org/mozilla-central/rev/3915337ce78e

Comment 27

[Justin [:JW_SoftvisionQA]]

I can confirm that this pref change has been made.