Nicole Yee (:nicoleyee) Reporter
	Description • 7 years ago

Logic to give Tour Overlay with "what's new with photon" content instead of new user content to all existing users to showcase new photon features.

	Nicole Yee (:nicoleyee) Reporter
	Comment 1 • 7 years ago

If new user downloads fx 56 then updates to 57, please follow this logic: https://bugzilla.mozilla.org/show_bug.cgi?id=1360474

	gasolin@mozilla.com Assignee
	Comment 2 • 7 years ago

Logic described in bug 1360474 seems fine since we do the validation separately for each tour.

Here is what I think about how to describe the actionable task for this issue:

Should show the overlay
  * When user got a new uncompleted tour (ex: tour57)
  * Even when user was checked the `Mark all as complete` checkbox.
    * should unchecked the `Mark all as complete` checkbox in overlay

	gasolin@mozilla.com Assignee
	Comment 3 • 7 years ago

This bug need to be done after v56 merge day (8/2), only need add tour id into `browser.onboarding.updatetour` and set `browser.onboarding.tourset-version` to 2

	gasolin@mozilla.com Assignee
	Comment 4 • 7 years ago

we should do this after visual asset update as well, or user will file related known-bugs

	gasolin@mozilla.com Assignee
	Comment 6 • 7 years ago

The patch follow UX requirement in bug 1382376 and bug 1381351 to update the new tours and update tours.
should postpone the review until we updated the visual asset (bug 1382520)

	gasolin@mozilla.com Assignee
	Comment 9 • 7 years ago

Since bug 1382520 is landed we can reveal new tours to nighly users.

The patch did 3 things:

1. update the new tourset to follow v57 spec described in bug 1381351
2. add update tourset to follow v57 spec described in bug 1382376
3. remove unused search tour

And there are 2 things to notice:

* I've added screenshots in pref though its not available yet, but that will still work (overlay wll show only 5 tours) because we already filter out the unmatched tour in `init`. Once screenshots tour is available, the overlay will show 6 tours again.
* Related to Bug 1390042, putting `performance` tour as the default fist tour will lead some test timeout(because the tour will set as complete instantly when it's shown as default tour, therefore the update observer in test wont able to observe the pref update), the related unit test error also fixed in this bug.

	Fischer [:Fischer]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8893184 [details]
Bug 1366056 - showing v57 tourset for new user tour and update user tour;

https://reviewboard.mozilla.org/r/164206/#review173880

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:39
(Diff revision 4)
> +    promisePrefUpdated("browser.onboarding.notification.finished", true)
> +  ];
> +  tourIds.forEach((id, idx) => {
> +    // if the fitst tour tour tagged completed instantly upon showing
> +    // we need to check if the pref already set to true
> +    if (idx === 0 && SHOWING_COMPLETE_TOUR.includes(id)) {

We could reduce this special condition if moved all the expected pref updated promises to the top before opening tabs. With that change, would still be able to obeserve the updates of the "SHOWING_COMPLETE_TOUR" tours.

::: browser/extensions/onboarding/test/browser/head.js:12
(Diff revision 4)
>  const ABOUT_NEWTAB_URL = "about:newtab";
>  const URLs = [ABOUT_HOME_URL, ABOUT_NEWTAB_URL];
>  const TOUR_IDs = [
> +  "onboarding-tour-performance",
>    "onboarding-tour-private-browsing",
> +  // "onboarding-tour-screenshots",

nit: Probably could do "TODO: do onboarding-tour-screenshots"

::: browser/extensions/onboarding/test/browser/head.js:20
(Diff revision 4)
>    "onboarding-tour-default-browser",
> +];
> +const UPDATE_TOUR_IDs = [
> +  "onboarding-tour-performance",
> +  "onboarding-tour-library",
> +  // "onboarding-tour-screenshots",

nit: Probably could do "TODO: do onboarding-tour-screenshots"

	Fischer [:Fischer]
	Comment 12 • 7 years ago
mozreview-review-reply

Comment on attachment 8893184 [details]
Bug 1366056 - showing v57 tourset for new user tour and update user tour;

https://reviewboard.mozilla.org/r/164206/#review173880

> nit: Probably could do "TODO: do onboarding-tour-screenshots"

Type too fast. Should be "TODO: add onboarding-tour-screenshots"

> nit: Probably could do "TODO: do onboarding-tour-screenshots"

Type too fast. Should be "TODO: add onboarding-tour-screenshots"

	gasolin@mozilla.com Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8893184 [details]
Bug 1366056 - showing v57 tourset for new user tour and update user tour;

https://reviewboard.mozilla.org/r/164206/#review173880

> We could reduce this special condition if moved all the expected pref updated promises to the top before opening tabs. With that change, would still be able to obeserve the updates of the "SHOWING_COMPLETE_TOUR" tours.

good idea! updated

> Type too fast. Should be "TODO: add onboarding-tour-screenshots"

updated

> Type too fast. Should be "TODO: add onboarding-tour-screenshots"

updated

	KM Lee [:rexboy] (inactivate after Sep. 22)
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8893184 [details]
Bug 1366056 - showing v57 tourset for new user tour and update user tour;

https://reviewboard.mozilla.org/r/164206/#review173868

::: browser/extensions/onboarding/content/onboarding.js:532
(Diff revision 4)
> +
> +    switch (tourId) {
> +      // These tours are tagged completed instantly upon showing.
> +      case "onboarding-tour-default-browser":
> +      case "onboarding-tour-sync":
> +      case "onboarding-tour-performance":

These changes are already done in another patch; Remember to do rebase before landing.

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:36
(Diff revision 4)
> +  tourIds.forEach((id, idx) => {
> +    // if the fitst tour tour tagged completed instantly upon showing

typo: `if the first tour is tagged completed`

::: browser/extensions/onboarding/test/browser/browser_onboarding_tours.js:23
(Diff revision 4)
>  
> -add_task(async function test_hide_onboarding_tours() {
> +add_task(async function test_set_right_tour_completed_style_on_overlay() {
>    resetOnboardingDefaultState();
>  
>    let tourIds = TOUR_IDs;
> +  // Make the tours of even number as completed

nit: `Mark`

::: browser/extensions/onboarding/test/browser/head.js:12
(Diff revision 4)
>  const ABOUT_NEWTAB_URL = "about:newtab";
>  const URLs = [ABOUT_HOME_URL, ABOUT_NEWTAB_URL];
>  const TOUR_IDs = [
> +  "onboarding-tour-performance",
>    "onboarding-tour-private-browsing",
> +  // "onboarding-tour-screenshots",

`// TODO: add "onboarding-tour-screenshots" after landing screenshot tour.`

::: browser/extensions/onboarding/test/browser/head.js:20
(Diff revision 4)
>    "onboarding-tour-default-browser",
> +];
> +const UPDATE_TOUR_IDs = [
> +  "onboarding-tour-performance",
> +  "onboarding-tour-library",
> +  // "onboarding-tour-screenshots",

`// TODO: add "onboarding-tour-screenshots" after landing screenshot tour.`

::: browser/extensions/onboarding/test/browser/head.js:26
(Diff revision 4)
> +  "onboarding-tour-singlesearch",
> +  "onboarding-tour-customize",
> +  "onboarding-tour-sync",
> +];
> +// These tours are tagged completed instantly upon showing.
> +const SHOWING_COMPLETE_TOUR = [

nit: `INSTANTLY_COMPLETE_TOURS`

	Dave Townsend [:mossop]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8893184 [details]
Bug 1366056 - showing v57 tourset for new user tour and update user tour;

https://reviewboard.mozilla.org/r/164206/#review174092

	Fischer [:Fischer]
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8893184 [details]
Bug 1366056 - showing v57 tourset for new user tour and update user tour;

https://reviewboard.mozilla.org/r/164206/#review174338

	Pulsebot
	Comment 22 • 7 years ago

Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f25c91cca4c3
showing v57 tourset for new user tour and update user tour;r=Fischer,mossop,rexboy

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 23 • 7 years ago

Backed out for failing browser_all_files_referenced.js:

https://hg.mozilla.org/integration/autoland/rev/406de5596351ef8ec50adda30f240e2641b90270

First push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9e66c5c8468329ea2c5cea0807cd59d3e6a4dc4c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123539901&repo=autoland

[task 2017-08-16T10:27:43.351116Z] 10:27:43     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 2, expected 0
[task 2017-08-16T10:27:43.353064Z] 10:27:43     INFO - Stack trace:
[task 2017-08-16T10:27:43.354909Z] 10:27:43     INFO - chrome://mochikit/content/browser-test.js:test_is:1002
[task 2017-08-16T10:27:43.357127Z] 10:27:43     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:600
[task 2017-08-16T10:27:43.359603Z] 10:27:43     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-08-16T10:27:43.365314Z] 10:27:43     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://onboarding/img/figure_search.svg - 
[task 2017-08-16T10:27:43.367482Z] 10:27:43     INFO - Stack trace:
[task 2017-08-16T10:27:43.370040Z] 10:27:43     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:602
[task 2017-08-16T10:27:43.373125Z] 10:27:43     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-08-16T10:27:43.383522Z] 10:27:43     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://onboarding/img/icons_search-notification.svg -

	gasolin@mozilla.com Assignee
	Comment 25 • 7 years ago

removed unused files, thanks for catching that

	Pulsebot
	Comment 26 • 7 years ago

Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fbd55be05b2
showing v57 tourset for new user tour and update user tour;r=Fischer,mossop,rexboy

	Carsten Book [:Tomcat]
	Comment 27 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9fbd55be05b2

	Justin [:JW_SoftvisionQA]
	Comment 28 • 7 years ago

I have verified that this is fixed on today's nightly.

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com
	Comment 29 • 7 years ago

I can confirm Fx 57.0b8 has the intended behavior, I tested on Ubuntu 14.04 LTS, mac OS X 10.13 and Windows 10 x64.