Closed Bug 1375775 Opened 4 years ago Closed 4 years ago

Allow reuse existing tours

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox56 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 files)

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.
```
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 57
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.
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 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".
Attachment #8881547 - Flags: review?(fliu)
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.
Flags: needinfo?(gasolin)
Attachment #8881547 - Flags: review?(fliu)
Sure, I'll also make the change depends on bug 1357021
Depends on: 1357021, 1357641
Flags: needinfo?(gasolin)
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.
Attachment #8881547 - Flags: review?(rexboy)
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 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 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())
Attachment #8881547 - Flags: review?(dtownsend) → review+
Blocks: 1372067
Comment on attachment 8884157 [details]
Bug 1375775 - test cases for reusable tourset;

https://reviewboard.mozilla.org/r/155072/#review160898
Attachment #8884157 - Flags: review?(dtownsend) → review+
thanks!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e3e948f381b5
https://hg.mozilla.org/mozilla-central/rev/3915337ce78e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1366056
Duplicate of this bug: 1357058
I can confirm that this pref change has been made.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.