Allow reuse existing tours

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 attachments)

Assignee

Description

2 years ago
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

Updated

2 years ago
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 57
Assignee

Comment 1

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
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 hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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)
Comment hidden (mozreview-request)
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)

Updated

2 years ago
Attachment #8881547 - Flags: review?(fliu)
Assignee

Comment 10

2 years ago
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 12

2 years ago
mozreview-review
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
Assignee

Comment 13

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1372067
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 23

2 years ago
thanks!
Keywords: checkin-needed

Comment 24

2 years ago
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

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3e948f381b5
https://hg.mozilla.org/mozilla-central/rev/3915337ce78e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Blocks: 1366056

Updated

2 years ago
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.