Closed Bug 1395480 Opened 7 years ago Closed 7 years ago

Set a 2 week total tour notification timeout for 57 onboarding

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 + verified

People

(Reporter: chsiang, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding][photon-onboarding-newui])

User Story

For users who infrequently use firefox and do not use the tour. The tour will last way too long for those users. Let’s put a default tour time out to end the tour so that they don’t have the tour for a long time and we can show them a helpful snippet

Default time out: 2 weeks
Criteria:for all users
Notifications will stop showing after 2 weeks.

Attachments

(1 file)

      No description provided.
Update the bug title to reflect the session is for all tour notifications
Summary: Set a 2 week session timeout for 57 onboarding → Set a 2 week total tour notification timeout for 57 onboarding
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Flags: qe-verify+
QA Contact: jwilliams
Whiteboard: [photon-onboarding][triage] → [photon-onboarding][triage][photon-onboarding-newui]
Whiteboard: [photon-onboarding][triage][photon-onboarding-newui] → [photon-onboarding][photon-onboarding-newui]
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`.
(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
Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57
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 on attachment 8903108 [details]
Bug 1395480 - Set a 2 week total tour notification timeout for 57 onboarding,

https://reviewboard.mozilla.org/r/174906/#review180866
Attachment #8903108 - Flags: review?(rexboy) → review+
> 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.
This bug has code conflict with the bug 1392472 so wait for the bug 1392472 checked-in
Depends on: 1392472
Priority: -- → P1
Keywords: checkin-needed
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5424ca3a5c9c
Set a 2 week total tour notification timeout for 57 onboarding, r=rexboy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5424ca3a5c9c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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?
Flags: needinfo?(fliu)
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.
Blocks: 1392475
Flags: needinfo?(fliu)
(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?
User Story: (updated)
Flags: needinfo?(jwilliams)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jwilliams)
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.