1395480 - Set a 2 week total tour notification timeout for 57 onboarding

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Comment 1

[Fischer [:Fischer]]

Update the bug title to reflect the session is for all tour notifications

Comment 2

[Fischer [:Fischer]]

Attachment: Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

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

Comment 3

[Fischer [:Fischer]]

Comment on attachment 8903108 [details]
Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

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

Comment 4

[Fischer [:Fischer]]

Comment on attachment 8903108 [details]
Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

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

Comment 5

[Fischer [:Fischer]]

Comment on attachment 8903108 [details]
Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

https://reviewboard.mozilla.org/r/174906/#review180304

::: browser/extensions/onboarding/content/onboarding.js:754
(Diff revision 3)
>          completedText.remove();
>        }
>      }
>    }
>  
> -  _muteNotificationOnFirstSession() {
> +  _getLastTourChangeTime() {

Many places in `showNotification` need this time. Extract this out.
Not go for `get _lastTourChangeTime()` on purpose because `this._lastTourChangeTime` is quite easy for people to repeatedly call it just like a property. Quite a unnecessary performance burden. With an explict function name, we encourage people to do `let time = this._getLastTourChangeTime()` one and reuse that `time`.

Comment 6

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #4)
> Comment on attachment 8903108 [details]
> Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/174906/diff/2-3/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ad3b2366619e2bd86d22f37a2791964392c85ba

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57

Comment 8

[[e-mail]]

Comment on attachment 8903108 [details]
Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

https://reviewboard.mozilla.org/r/174906/#review180852

::: browser/app/profile/firefox.js:1749
(Diff revision 3)
>  // 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.notification.mute-duration-on-first-session-ms", 300000); // 5 mins
>  pref("browser.onboarding.notification.max-life-time-per-tour-ms", 432000000); // 5 days
> +pref("browser.onboarding.notification.max-life-time-all-tours-ms", 1209600000); // 14 days

We should also call clearUserPref("browser.onboarding.notification.last-time-of-changing-tour-sec") at `OnboardingTourType` for updated user tour case

Comment 9

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

Comment on attachment 8903108 [details]
Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

https://reviewboard.mozilla.org/r/174906/#review180866

Comment 10

[[e-mail]]

> We should also call
> clearUserPref("browser.onboarding.notification.last-time-of-changing-tour-
> sec") at `OnboardingTourType` for updated user tour case

Sorry, the new pref seems like to define a constant value, so no need change though.

Comment 11

[Fischer [:Fischer]]

This bug has code conflict with the bug 1392472 so wait for the bug 1392472 checked-in

Comment 12

[Fischer [:Fischer]]

Comment on attachment 8903108 [details]
Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5424ca3a5c9c
Set a 2 week total tour notification timeout for 57 onboarding, r=rexboy

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5424ca3a5c9c

Comment 15

[Justin [:JW_SoftvisionQA]]

Trying to Verify this issue but the firefox logo is not turning into a water mark as explained in the User Story. Any help here?

Comment 16

[[e-mail]]

Hi Justin, The patch just handled notification dismissed after 2 weeks.
The watermark part haven't implemented yet and should be fixed in bug 1392475, please wait a bit if you want to verify the full spec flow.

Comment 17

[Fischer [:Fischer]]

(In reply to Justin [:JW_SoftvisionQA] from comment #15)
> Trying to Verify this issue but the firefox logo is not turning into a water
> mark as explained in the User Story. Any help here?
Hi Justin,
The main purpose of this bug is to make tour notifications finished if user didn't use Firefox for more than 14 days.
So the verification steps would be like:
  1. Open the nightly with a clean profile
  2. Wait for the 1st notification until the 1st mute session (currently about 5 mins)
  3. Close FF after seeing the 1st notification
  4. Reopen FF after 14 days from the step 3 (maybe some system time adjustment is required)
  5. No more tour notifications seen

Just changed the user story to reflect what exactly this bug is doing. It's the duty of the bug 1392475 to turn the onboarding into the watermark mode when seeing tour notifications finished. Making things simpler and focused, let's test the watermark mode in the bug 1392475 and test 2-weeks timeout condition here. Is this ok for you?

Comment 18

[Justin [:JW_SoftvisionQA]]

I have verified that the notifications stop showing after 2 weeks. I will be tracking bug 1392475 so that we can test the watermark function after 2 weeks is finished. We will verify that with our pre-beta sign-off.

Comment 19

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the notifications stop showing after 2 weeks on beta as well(I changed the PC date). I verified using Fx 57.0b7, on Windows 10 x64, Ubuntu 14.04 LTS and mac OS X 10.12.6.