Closed Bug 1372067 Opened 7 years ago Closed 7 years ago

Should implement the prompt timing policy of the tour notification bar

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 files, 1 obsolete file)

Bug 1372067 - Part 1: Implement the prompt timing policy of the tour notification bar,
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1372067 - Part 2: Add the test cases,
59 bytes, text/x-review-board-request
mossop
: review+
Details
Attached file notification-logic.png (obsolete) — 
According to the bug 1357668 comments 3, we should implement the prompt timing policy of the tour notification bar. See the attached notification-logic.png.
Blocks: 1357008
Priority: -- → P2
Whiteboard: [photon-onboaridng]
Comment on attachment 8876550 [details]
notification-logic.png

Check the latest spec at attachment 8877010 [details].
Attachment #8876550 - Attachment is obsolete: true
Whiteboard: [photon-onboaridng] → [photon-onboarding]
Flags: qe-verify+
QA Contact: jwilliams
Target Milestone: --- → Firefox 56
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8881489 [details]
Bug 1372067 - Part 1: Implement the prompt timing policy of the tour notification bar,

https://reviewboard.mozilla.org/r/152630/#review157760

::: browser/extensions/onboarding/bootstrap.js:15
(Diff revision 1)
>  
>  const PREF_WHITELIST = [
>    "browser.onboarding.enabled",
>    "browser.onboarding.hidden",
>    "browser.onboarding.notification.finished",
> -  "browser.onboarding.notification.lastPrompted"
> +  "browser.onboarding.notification.last-prompted",

Other places use "-" style so we modify this one
(In reply to Fischer [:Fischer] from comment #3)
> Created attachment 8881490 [details]
> Bug 1372067 - Part 2: Add test cases,
> 
> This commit
> - adds the test_mute_notification_on_1st_session test case
> - adds the test_move_on_to_next_notification_when_reaching_max_prompt_count
> test case
> - add the test_move_on_to_next_notification_when_reaching_max_life_time test
> case
> - adds the
> test_move_on_to_next_notification_after_interacting_with_notification test
> case
> - updates the existing test cases for the new notification timing policy
> 
> Review commit: https://reviewboard.mozilla.org/r/152632/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/152632/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2026044452335de6a8fc7e2bfcda03fb4e0db22
Comment on attachment 8881489 [details]
Bug 1372067 - Part 1: Implement the prompt timing policy of the tour notification bar,

Hi mossop,
The timing policy is having some update so cancel the review request, thanks.
Attachment #8881489 - Flags: review?(dtownsend)
Attachment #8881490 - Flags: review?(dtownsend)
Target Milestone: Firefox 56 → Firefox 57
(In reply to Fischer [:Fischer] from comment #7)
> Comment on attachment 8881489 [details]
> Bug 1372067 - Part 1: Implement the prompt timing policy of the tour
> notification bar,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/152630/diff/1-2/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fff93e9ff3ce3cee4cfdb6353c6f1dbed4fd405
Comment on attachment 8881489 [details]
Bug 1372067 - Part 1: Implement the prompt timing policy of the tour notification bar,

https://reviewboard.mozilla.org/r/152630/#review158878

I'm not sure why NOTIFICATION_QUEUE_END_MARK and browser.onboarding.notification.last-prompted is needed here so I'd like to understand more before I do more on this review. I would expect that simply using browser.onboarding.notification.tour-ids-queue as the queue to show with the first item being the currently showing notification and an empty list meaning that we've completed would be a lot simpler.

::: commit-message-eaca7:9
(Diff revision 2)
> +- mutes tour notification for the 1st 5 mins on the 1st session
> +- moves on to next tour notification when
> +  a. previous tour has been prompted 8 times(8 impressions) or
> +  b. the last time of changing previous tour is 5 days ago
> +- removes tour from the notification queue forever after user clicked the close or the action button on notification bar to interact with that tour notification.
> +- makes each tour only has 2 chnaces to prompt with notification. Each chance includes 8 impressions and 5-days life time. After these 2 chances, no notification would be prompted for tour.

s/chnaces/chances/

::: browser/app/profile/firefox.js:1715
(Diff revision 2)
>  // if our notification is finished and safe to show their snippet.
>  pref("browser.onboarding.notification.finished", false);
> +pref("browser.onboarding.notification.mute-duration-on-first-session-ms", 300000); // 5 mins
> +pref("browser.onboarding.notification.max-life-time-per-tour-ms", 432000000); // 5 days
> +// Have to save in String because the int type is too small for this timestamp
> +pref("browser.onboarding.notification.last-time-of-changing-tour-ms", "0");

We'll solve a bunch of issues around parsing strings if we just store this in seconds. I'd also not set any value by default so we can simply detect when it hasn't been set.

::: browser/extensions/onboarding/content/onboarding.js:455
(Diff revision 2)
> +
> +    return false;
> +  }
> +
> +  _removeTourFromNotificationQueue(tourId) {
> +    let queue =  this._getNotificationQueue();

There is extra spacing here
Comment on attachment 8881489 [details]
Bug 1372067 - Part 1: Implement the prompt timing policy of the tour notification bar,

https://reviewboard.mozilla.org/r/152630/#review160170

::: browser/extensions/onboarding/content/onboarding.js:425
(Diff revision 2)
> +    }
> +
> +    // Reuse the `last-time-of-changing-tour-ms` to save the time that
> +    // we try to prompt on the 1st session.
> +    let lastTime = parseInt(
> +      Preferences.get("browser.onboarding.notification.last-time-of-changing-tour-ms"));

use Preferences.get(..., "0") to make sure parseInt work correctly

::: browser/extensions/onboarding/content/onboarding.js:426
(Diff revision 2)
> +
> +    // Reuse the `last-time-of-changing-tour-ms` to save the time that
> +    // we try to prompt on the 1st session.
> +    let lastTime = parseInt(
> +      Preferences.get("browser.onboarding.notification.last-time-of-changing-tour-ms"));
> +    if (!(lastTime > 0)) {

if (lastTime <= 0)

::: browser/extensions/onboarding/content/onboarding.js:439
(Diff revision 2)
> +    return Date.now() - lastTime <= muteDuration;
> +  }
> +
> +  _isTimeForNextTourNotification() {
> +    let promptCount = Preferences.get("browser.onboarding.notification.prompt-count");
> +    let maxCount = Preferences.get("browser.onboarding.notification.max-prompt-count-per-tour");

we can move `maxTourImpressions` as a class level constant, so we don't need to query it everytime

::: browser/extensions/onboarding/content/onboarding.js:445
(Diff revision 2)
> +    if (promptCount >= maxCount) {
> +      return true;
> +    }
> +
> +    let lastTime = parseInt(
> +      Preferences.get("browser.onboarding.notification.last-time-of-changing-tour-ms"));

Preferences.get("...", "0")

::: browser/extensions/onboarding/content/onboarding.js:459
(Diff revision 2)
> +  _removeTourFromNotificationQueue(tourId) {
> +    let queue =  this._getNotificationQueue();
> +    queue = queue.filter(id => id != tourId);
> +    this.sendMessageToChrome("set-prefs", [{
> +      name: "browser.onboarding.notification.tour-ids-queue",
> +      value: queue.join(",")

can cascade in single line `queue.filter(id => id != tourId).join(",")`

::: browser/extensions/onboarding/content/onboarding.js:473
(Diff revision 2)
> +      // if user never interact with it.
> +      // Assume there are tour #0 ~ #5. Here would form the queue as
> +      // "#0,#1,#2,#3,#4,#5,#0,#1,#2,#3,#4,#5,NOTIFICATION_QUEUE_END_MARK".
> +      // Then we would loop through this queue and remove prompted tour from the queue
> +      // until reaching the end of the queue.
> +      let ids = onboardingTours.map(tour => tour.id);

Please use `this._tours` instead of onboardingTours after Bug 1375775 is landed

::: browser/extensions/onboarding/content/onboarding.js:474
(Diff revision 2)
> +      // Assume there are tour #0 ~ #5. Here would form the queue as
> +      // "#0,#1,#2,#3,#4,#5,#0,#1,#2,#3,#4,#5,NOTIFICATION_QUEUE_END_MARK".
> +      // Then we would loop through this queue and remove prompted tour from the queue
> +      // until reaching the end of the queue.
> +      let ids = onboardingTours.map(tour => tour.id);
> +      queue = ids.join(",");

can do the `.map(tour => tour.id).join(",");` in same line

::: browser/extensions/onboarding/content/onboarding.js:475
(Diff revision 2)
> +      // "#0,#1,#2,#3,#4,#5,#0,#1,#2,#3,#4,#5,NOTIFICATION_QUEUE_END_MARK".
> +      // Then we would loop through this queue and remove prompted tour from the queue
> +      // until reaching the end of the queue.
> +      let ids = onboardingTours.map(tour => tour.id);
> +      queue = ids.join(",");
> +      queue += "," + queue + "," + NOTIFICATION_QUEUE_END_MARK;

we can use iteral string for quicker string composing. 
ex: 
queue = `${ids},${ids},${NOTIFICATION_QUEUE_END_MARK}`

::: browser/extensions/onboarding/content/onboarding.js:497
(Diff revision 2)
>  
> +    let queue =  this._getNotificationQueue();
> +    let targetTourId = queue[0];
> +    let lastPromptedId = Preferences.get("browser.onboarding.notification.last-prompted", "");
> +    // See if need to move on to the next tour
> +    if (targetTourId != NOTIFICATION_QUEUE_END_MARK &&

can remove this NOTIFICATION_QUEUE_END_MARK condition check after move `if (targetTourId == NOTIFICATION_QUEUE_END_MARK) {` section up

::: browser/extensions/onboarding/content/onboarding.js:504
(Diff revision 2)
> +        this._isTimeForNextTourNotification()) {
> +      queue.shift();
> +      targetTourId = queue[0];
> +    }
> +    // We don't want to prompt completed tour.
> +    while (targetTourId != NOTIFICATION_QUEUE_END_MARK && this.isTourCompleted(targetTourId)) {

why use `while`?

::: browser/extensions/onboarding/content/onboarding.js:504
(Diff revision 2)
> +        this._isTimeForNextTourNotification()) {
> +      queue.shift();
> +      targetTourId = queue[0];
> +    }
> +    // We don't want to prompt completed tour.
> +    while (targetTourId != NOTIFICATION_QUEUE_END_MARK && this.isTourCompleted(targetTourId)) {

can remove this NOTIFICATION_QUEUE_END_MARK condition check after move `if (targetTourId == NOTIFICATION_QUEUE_END_MARK) {` section up

::: browser/extensions/onboarding/content/onboarding.js:509
(Diff revision 2)
> +    while (targetTourId != NOTIFICATION_QUEUE_END_MARK && this.isTourCompleted(targetTourId)) {
> +      queue.shift();
> +      targetTourId = queue[0];
> +    }
>  
> -    if (!targetTour) {
> +    if (targetTourId == NOTIFICATION_QUEUE_END_MARK) {

can moved this section up after get the targetTourId
Depends on: 1375775
Comment on attachment 8881490 [details]
Bug 1372067 - Part 2: Add the test cases,

https://reviewboard.mozilla.org/r/152632/#review160176

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:57
(Diff revision 2)
>    await SpecialPowers.pushPrefEnv({set: [["browser.onboarding.enabled", true]]});
> +  Preferences.set("browser.onboarding.notification.max-prompt-count-per-tour", 1);
> +  skipMuteNotificationOnFirstSession();
>  
>    let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
>    await BrowserTestUtils.loadURI(tab.linkedBrowser, ABOUT_NEWTAB_URL);

please also run the test on ABOUT_HOME_URL if possible
(In reply to Fred Lin [:gasolin] from comment #11)
> Comment on attachment 8881489 [details]
> Bug 1372067 - Part 1: Implement the prompt timing policy of the tour
> notification bar,
> 
> https://reviewboard.mozilla.org/r/152630/#review160170
> 
> ::: browser/extensions/onboarding/content/onboarding.js:439 (Diff revision 2)
> > +  _isTimeForNextTourNotification() {
> > +    let promptCount = Preferences.get("browser.onboarding.notification.prompt-count");
> > +    let maxCount = Preferences.get("browser.onboarding.notification.max-prompt-count-per-tour");
> 
> we can move `maxTourImpressions` as a class level constant, so we don't need to query it everytime
> 
Since tour notification only prompt once per open of about:newtab so no case for multiple tomes of query it.
Plus no need to query it if tour notifications are finished. Hence better to query it when necessary.

> 
> ::: browser/extensions/onboarding/content/onboarding.js:459 (Diff revision 2)
> > +    this.sendMessageToChrome("set-prefs", [{
> > +      name: "browser.onboarding.notification.tour-ids-queue",
> > +      value: queue.join(",")
> 
> can cascade in single line `queue.filter(id => id != tourId).join(",")`
> 
Updated

> ::: browser/extensions/onboarding/content/onboarding.js:473
> (Diff revision 2)
> > +      // if user never interact with it.
> > +      // Assume there are tour #0 ~ #5. Here would form the queue as
> > +      // "#0,#1,#2,#3,#4,#5,#0,#1,#2,#3,#4,#5,NOTIFICATION_QUEUE_END_MARK".
> > +      // Then we would loop through this queue and remove prompted tour from the queue
> > +      // until reaching the end of the queue.
> > +      let ids = onboardingTours.map(tour => tour.id);
> 
> Please use `this._tours` instead of onboardingTours after Bug 1375775 is
> landed
> 
Yes, this should be taken care when rebasing.

> ::: browser/extensions/onboarding/content/onboarding.js:475 (Diff revision 2)
> > +      queue += "," + queue + "," + NOTIFICATION_QUEUE_END_MARK;
> 
> we can use iteral string for quicker string composing. 
> ex: 
> queue = `${ids},${ids},${NOTIFICATION_QUEUE_END_MARK}`
> 
Updated

> ::: browser/extensions/onboarding/content/onboarding.js:504 (Diff revision 2)
> > +    // We don't want to prompt completed tour.
> > +    while (targetTourId != NOTIFICATION_QUEUE_END_MARK && this.isTourCompleted(targetTourId)) {
> 
> why use `while`?
> 
Consider we may encounter 2 or more completed tours.
(In reply to Dave Townsend [:mossop] from comment #10)
> Comment on attachment 8881489 [details]
> Bug 1372067 - Part 1: Implement the prompt timing policy of the tour
> notification bar,
> 
> https://reviewboard.mozilla.org/r/152630/#review158878
> 
> I'm not sure why NOTIFICATION_QUEUE_END_MARK and
> browser.onboarding.notification.last-prompted is needed here so I'd like to
> understand more before I do more on this review. I would expect that simply
> using browser.onboarding.notification.tour-ids-queue as the queue to show
> with the first item being the currently showing notification and an empty
> list meaning that we've completed would be a lot simpler.
> 
Removed the use of `browser.onboarding.notification.last-prompted`.

About `browser.onboarding.notification.tour-ids-queue`, consider the case that:
User clicks action button or close button to dismiss all notifications one by one and tours are removed from the queue one by one too.
When calling `_getNotificationQueue` to get the queue, the queue would be re-filled because of an empty queue.
Therefore, the tour notifications wouldn't finish but re-prompt again and again.
We may check if the queue is empty and set `browser.onboarding.notification.finished` inside `_removeTourFromNotificationQueue` to solve the re-prompt problem. Just that consider we assume a default empty queue inside `_getNotificationQueue` so adding an explicit `NOTIFICATION_QUEUE_END_MARK` maybe be clearer and explicit about we are done with tour notifications. Please let me know if `NOTIFICATION_QUEUE_END_MARK` is not really an necessary consideration, thanks. 

> ::: commit-message-eaca7:9 (Diff revision 2)
> > +- makes each tour only has 2 chnaces to prompt with notification. Each chance includes 8 impressions and 5-days life time. After these 2 chances, no notification would be prompted for tour.
> 
> s/chnaces/chances/
> 
Updated, thanks.

> ::: browser/app/profile/firefox.js:1715 (Diff revision 2)
> >  // if our notification is finished and safe to show their snippet.
> >  pref("browser.onboarding.notification.finished", false);
> > +pref("browser.onboarding.notification.mute-duration-on-first-session-ms", 300000); // 5 mins
> > +pref("browser.onboarding.notification.max-life-time-per-tour-ms", 432000000); // 5 days
> > +// Have to save in String because the int type is too small for this timestamp
> > +pref("browser.onboarding.notification.last-time-of-changing-tour-ms", "0");
> 
> We'll solve a bunch of issues around parsing strings if we just store this in seconds. I'd also not set any value by default so we can simply detect when it hasn't been set.
> 
Updated to use `notification.max-life-time-per-tour-sec` and to detect when prefs haven't been set.

> ::: browser/extensions/onboarding/content/onboarding.js:455 (Diff revision 2)
> > +  _removeTourFromNotificationQueue(tourId) {
> > +    let queue =  this._getNotificationQueue();
> 
> There is extra spacing here
Updated, thanks.
Flags: needinfo?(dtownsend)
(In reply to Fischer [:Fischer] from comment #16)
> (In reply to Dave Townsend [:mossop] from comment #10)
> > Comment on attachment 8881489 [details]
> > Bug 1372067 - Part 1: Implement the prompt timing policy of the tour
> > notification bar,
> > 
> > https://reviewboard.mozilla.org/r/152630/#review158878
> > 
> > I'm not sure why NOTIFICATION_QUEUE_END_MARK and
> > browser.onboarding.notification.last-prompted is needed here so I'd like to
> > understand more before I do more on this review. I would expect that simply
> > using browser.onboarding.notification.tour-ids-queue as the queue to show
> > with the first item being the currently showing notification and an empty
> > list meaning that we've completed would be a lot simpler.
> > 
> Removed the use of `browser.onboarding.notification.last-prompted`.
> 
> About `browser.onboarding.notification.tour-ids-queue`, consider the case
> that:
> User clicks action button or close button to dismiss all notifications one
> by one and tours are removed from the queue one by one too.
> When calling `_getNotificationQueue` to get the queue, the queue would be
> re-filled because of an empty queue.
> Therefore, the tour notifications wouldn't finish but re-prompt again and
> again.

This is only the case because you default the queue to the empty string. If you just didn't set it at all you would know to fill the queue if the pref didn't exist and if it did exist and was empty then the queue is finished.

> We may check if the queue is empty and set
> `browser.onboarding.notification.finished` inside
> `_removeTourFromNotificationQueue` to solve the re-prompt problem. Just that
> consider we assume a default empty queue inside `_getNotificationQueue` so
> adding an explicit `NOTIFICATION_QUEUE_END_MARK` maybe be clearer and
> explicit about we are done with tour notifications. Please let me know if
> `NOTIFICATION_QUEUE_END_MARK` is not really an necessary consideration,
> thanks. 

I think it's an added complication that can be removed.
Flags: needinfo?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #17)
> > We may check if the queue is empty and set
> > `browser.onboarding.notification.finished` inside
> > `_removeTourFromNotificationQueue` to solve the re-prompt problem. Just that
> > consider we assume a default empty queue inside `_getNotificationQueue` so
> > adding an explicit `NOTIFICATION_QUEUE_END_MARK` maybe be clearer and
> > explicit about we are done with tour notifications. Please let me know if
> > `NOTIFICATION_QUEUE_END_MARK` is not really an necessary consideration,
> > thanks. 
> 
> I think it's an added complication that can be removed.
Hi Mossop,
Updated, please have a look, thank you.
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend)
Comment on attachment 8881489 [details]
Bug 1372067 - Part 1: Implement the prompt timing policy of the tour notification bar,

https://reviewboard.mozilla.org/r/152630/#review160902

::: browser/extensions/onboarding/content/onboarding.js:424
(Diff revision 4)
> +    }
> +
> +    // Reuse the `last-time-of-changing-tour-sec` to save the time that
> +    // we try to prompt on the 1st session.
> +    let lastTime = 1000 * Preferences.get("browser.onboarding.notification.last-time-of-changing-tour-sec", 0);
> +    if (!(lastTime > 0)) {

if (lastTime)

::: browser/extensions/onboarding/content/onboarding.js:501
(Diff revision 4)
> -    // Or #3 would be the one to show if #4 ~ #2 are all completed.
> -    let toursToNotify = [ ...onboardingTours.slice(lastTourIndex + 1), ...onboardingTours.slice(0, lastTourIndex + 1) ];
> -    targetTour = toursToNotify.find(tour => !this.isTourCompleted(tour.id));
>  
> +    let queue = this._getNotificationQueue();
> +    let targetTourId = queue[0];

Please don't get values past the end of the array. Cache the array length to compare whether the array has changed or not later on.

::: browser/extensions/onboarding/content/onboarding.js:514
(Diff revision 4)
> +    while (queue.length > 0 && this.isTourCompleted(targetTourId)) {
> +      queue.shift();
> +      targetTourId = queue[0];
> +    }
>  
> -    if (!targetTour) {
> +    if (!targetTourId) {

if (queue.length == 0)

::: browser/extensions/onboarding/content/onboarding.js:545
(Diff revision 4)
>      let tourMessage = this._notificationBar.querySelector("#onboarding-notification-tour-message");
>      tourMessage.textContent = notificationStrings.message;
>      this._notificationBar.classList.add("onboarding-opened");
>  
> -    this.sendMessageToChrome("set-prefs", [{
> -      name: "browser.onboarding.notification.lastPrompted",
> +    let params = [];
> +    let promptCountPref = "browser.onboarding.notification.prompt-count";

Define this as a const at the top of the file.

::: browser/extensions/onboarding/content/onboarding.js:546
(Diff revision 4)
>      tourMessage.textContent = notificationStrings.message;
>      this._notificationBar.classList.add("onboarding-opened");
>  
> -    this.sendMessageToChrome("set-prefs", [{
> -      name: "browser.onboarding.notification.lastPrompted",
> -      value: targetTour.id
> +    let params = [];
> +    let promptCountPref = "browser.onboarding.notification.prompt-count";
> +    if (lastPromptedId != targetTour.id) {

Use queue.length to check if the queue has changed

::: browser/extensions/onboarding/content/onboarding.js:563
(Diff revision 4)
> +    params.push({
> +      name: "browser.onboarding.notification.tour-ids-queue",
> +      value: queue.join(",")
> +    });

You only need to send this in the event that the queue has changed
Attachment #8881489 - Flags: review?(dtownsend) → review-
Comment on attachment 8881490 [details]
Bug 1372067 - Part 2: Add the test cases,

https://reviewboard.mozilla.org/r/152632/#review160928

::: browser/base/content/test/newtab/browser_newtab_focus.js:63
(Diff revision 4)
> +  let maxTime = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms");
> +  let lastTime = Math.floor((Date.now() - maxTime - 1) / 1000);
> +  await SpecialPowers.pushPrefEnv({set: [["browser.onboarding.notification.last-time-of-changing-tour-sec", lastTime]]});

I think we'd be better off making it possible to opt-out of the first session muting, that way we can control it remotely if needed. Can you make it so if browser.onboarding.notification.mute-duration-on-first-session-ms == 0 then the first session isn't muted?

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:144
(Diff revision 4)
> +  await waitUntilWindowIdle(tab.linkedBrowser);
> +  let promptCount = Preferences.get("browser.onboarding.notification.prompt-count", 0);
> +  is(0, promptCount, "Should not prompt tour notification during the mute duration on the 1st session");
> +
> +  // Test notification prompted after the mute duration on the 1st session
> +  await new Promise(resolve => setTimeout(resolve, TEST_MUTE_DURATION + 1));

Please don't introduce delays into the test. This slows down test runs and could introduce intermittency. Instead manipulate the prefs to make the code think that the requisite time has passed.

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:203
(Diff revision 4)
> +  await BrowserTestUtils.loadURI(tab.linkedBrowser, ABOUT_NEWTAB_URL);
> +  await promiseOnboardingOverlayLoaded(tab.linkedBrowser);
> +  await promiseTourNotificationOpened(tab.linkedBrowser);
> +  let previousTourId = await getCurrentNotificationTargetTourId(tab.linkedBrowser);
> +
> +  await new Promise(resolve => setTimeout(resolve, TEST_MAX_LIFE_TIME + 1));

Same here

::: browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:307
(Diff revision 4)
> +  tab.linkedBrowser.reload();
> +  await reloadPromise;
> +  await promiseOnboardingOverlayLoaded(tab.linkedBrowser);
> +  await expectedPrefUpdate;
> +  let tourId = await getCurrentNotificationTargetTourId(tab.linkedBrowser);
> +  ok(!tourId, "Should not prompt tour notifcations any more after taking actions on all notifcations.");

*notifications
Attachment #8881490 - Flags: review?(dtownsend) → review-
Blocks: 1375793
It will be nice to describe how notification works in README.md#Architecture section.
(In reply to Dave Townsend [:mossop] from comment #21)
> Comment on attachment 8881489 [details]
> Bug 1372067 - Part 1: Implement the prompt timing policy of the tour
> notification bar,
> 
> https://reviewboard.mozilla.org/r/152630/#review160902
> 
> ::: browser/extensions/onboarding/content/onboarding.js:424
> (Diff revision 4)
> > +    if (!(lastTime > 0)) {
> 
> if (lastTime)
> 
Updated, thanks

> ::: browser/extensions/onboarding/content/onboarding.js:501
> (Diff revision 4)
> > +    let targetTourId = queue[0];
> 
> Please don't get values past the end of the array. Cache the array length to
> compare whether the array has changed or not later on.
> 
Updated, thanks

> ::: browser/extensions/onboarding/content/onboarding.js:514
> (Diff revision 4)
> > +    if (!targetTourId) {
> 
> if (queue.length == 0)
> 
Updated, thanks

> ::: browser/extensions/onboarding/content/onboarding.js:545
> (Diff revision 4)
> > +    let promptCountPref = "browser.onboarding.notification.prompt-count";
> 
> Define this as a const at the top of the file.
> 
Updated, thanks

> ::: browser/extensions/onboarding/content/onboarding.js:546
> (Diff revision 4)
> > +    if (lastPromptedId != targetTour.id) {
> 
> Use queue.length to check if the queue has changed
> 
Updated, thanks

> ::: browser/extensions/onboarding/content/onboarding.js:563
> (Diff revision 4)
> > +    params.push({
> > +      name: "browser.onboarding.notification.tour-ids-queue",
> > +      value: queue.join(",")
> > +    });
> 
> You only need to send this in the event that the queue has changed
Updated, thanks
(In reply to Dave Townsend [:mossop] from comment #22)
> Comment on attachment 8881490 [details]
> Bug 1372067 - Part 2: Add the test cases,
> 
> https://reviewboard.mozilla.org/r/152632/#review160928
> 
> ::: browser/base/content/test/newtab/browser_newtab_focus.js:63
> (Diff revision 4)
> > +  let maxTime = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms");
> > +  let lastTime = Math.floor((Date.now() - maxTime - 1) / 1000);
> > +  await SpecialPowers.pushPrefEnv({set: [["browser.onboarding.notification.last-time-of-changing-tour-sec", lastTime]]});
> 
> I think we'd be better off making it possible to opt-out of the first
> session muting, that way we can control it remotely if needed. Can you make
> it so if browser.onboarding.notification.mute-duration-on-first-session-ms
> == 0 then the first session isn't muted?
> 
Definitely. Updated, thanks.

> browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:144
> (Diff revision 4)
> > +  // Test notification prompted after the mute duration on the 1st session
> > +  await new Promise(resolve => setTimeout(resolve, TEST_MUTE_DURATION + 1));
> 
> Please don't introduce delays into the test. This slows down test runs and
> could introduce intermittency. Instead manipulate the prefs to make the code
> think that the requisite time has passed.
> 
Updated, thanks

> :::
> browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:203
> (Diff revision 4)
> > +  await new Promise(resolve => setTimeout(resolve, TEST_MAX_LIFE_TIME + 1));
> 
> Same here
Updated, thanks

> browser/extensions/onboarding/test/browser/browser_onboarding_notification.js:307
> (Diff revision 4)
> > +  ok(!tourId, "Should not prompt tour notifcations any more after taking actions on all notifcations.");
> 
> *notifications
Updated, thanks
Comment on attachment 8881489 [details]
Bug 1372067 - Part 1: Implement the prompt timing policy of the tour notification bar,

https://reviewboard.mozilla.org/r/152630/#review161354
Attachment #8881489 - Flags: review?(dtownsend) → review+
Comment on attachment 8881490 [details]
Bug 1372067 - Part 2: Add the test cases,

https://reviewboard.mozilla.org/r/152632/#review161356
Attachment #8881490 - Flags: review?(dtownsend) → review+
I am a bit confused. How is this not needed in Firefox 56?
Flags: needinfo?(fliu)
Target Milestone: Firefox 57 → Firefox 56
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30)
> I am a bit confused. How is this not needed in Firefox 56?
Thanks for updated this. I guess this was a mistake.
Flags: needinfo?(fliu)
Will rebase on the r+ed bug 1358970 and then check in or code conflict would happen.
Depends on: 1358970
(In reply to Fischer [:Fischer] from comment #34)
> Comment on attachment 8881490 [details]
> Bug 1372067 - Part 2: Add the test cases,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/152632/diff/5-6/
Rebased on the latest central and bug 1358970.
Also Split up the browser_onboarding_notification.js test btw because a bit too many test cases and tour notifications prompts in one test file would be more likely to cause the intermittent long-running test timeout issue.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4dfeb115d8d
Part 1: Implement the prompt timing policy of the tour notification bar, r=mossop
https://hg.mozilla.org/integration/autoland/rev/959d492b962f
Part 2: Add the test cases, r=mossop
Keywords: checkin-needed
Hi Kate,

After this bug landed, the onboarding tour notifications could go from the finished state to the prompting state.

The onboarding is having multiple version updates (at least the version for FF56 and the one for FF57).
Assume one user in 56 finished all notifications so "browser.onboarding.notification.finished" became true.
Then upgrading to 57 which has an update version tours, "browser.onboarding.notification.finished" would become true and the onboarding would start prompting new notifications for new tours.

Just let you the will be a case like this, thank you.
Flags: needinfo?(khudson)
Sorry correct the mistake:
(In reply to Fischer [:Fischer] from comment #39)
> Hi Kate,
> 
> After this bug landed, the onboarding tour notifications could go from the
> finished state to the prompting state.
> 
> The onboarding is having multiple version updates (at least the version for
> FF56 and the one for FF57).
> Assume one user in 56 finished all notifications so
> "browser.onboarding.notification.finished" became true.
> Then upgrading to 57 which has an update version tours,
"browser.onboarding.notification.finished" would become *false* and the
> onboarding would start prompting new notifications for new tours.
> 
> Just let you the will be a case like this, thank you.
Depends on: 1381075
Depends on: 1383070
Flags: needinfo?(khudson)
I have confirmed this fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: