Should be able to make all the tours completed by checking "Mark all as complete" checkbox

VERIFIED FIXED in Firefox 56

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: Fischer, Assigned: Fischer)

Tracking

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

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

4 months ago
Should be able to make all the tours completed by checking "Mark all as complete" checkbox
(Assignee)

Updated

4 months ago
Whiteboard: [photon-onboarding]

Updated

4 months ago
Target Milestone: --- → Firefox 55

Updated

4 months ago
Assignee: nobody → rexboy

Updated

4 months ago
Status: NEW → ASSIGNED

Updated

4 months ago
Flags: qe-verify+
QA Contact: jwilliams

Updated

4 months ago
Priority: -- → P1

Updated

3 months ago
Priority: P1 → P2
Target Milestone: Firefox 55 → Firefox 57

Updated

3 months ago
Assignee: rexboy → gasolin

Comment 1

3 months ago
should handle this after we implement the first tour validation mechanism
Assignee: gasolin → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 2

2 months ago
(In reply to Fred Lin [:gasolin] from comment #1)
> should handle this after we implement the first tour validation mechanism
Hi Fred,
Would you mind to let me handle this bug?
I have some local patch already.
I originally would like to tackle it with Bug 1357020.
But because of lacking tours right now, I will send out r? afterwards.
Thanks
Flags: needinfo?(gasolin)

Comment 3

2 months ago
go ahead, thanks
Flags: needinfo?(gasolin)

Comment 4

2 months ago
For current requirement, I think we should not setComplete all the tours because UX want us to show uncompleted tours in update user tours. 

So maybe we can just set this item as invalid or wontfix?
Flags: needinfo?(fliu)
(Assignee)

Comment 5

2 months ago
Hi Verdi,
For the "Make all as complete, and hide the tour" checkbox,

Suppose there are tour A, B, C, D in the version 56.
Users manually completes tour A only and check "Make all as complete, and hide the tour" checkbox to hide the tours.
Later there are new version 57 and in the version 57 there are tour A, B, F, G.
We should re-open the hidden tours and show
  - Case 1: tour A, B both in completed state
  - Case 2: tour A in completed state, B in not yet completed state

Is Case 1 or Case 2 correct?

Thanks
Flags: needinfo?(fliu) → needinfo?(mverdi)

Comment 6

2 months ago
(In reply to Fischer [:Fischer] from comment #5) 
> Is Case 1 or Case 2 correct?

Case 1 would be correct.

Let's use a likely use-case. The new user tour in 56 will have:
Sync
Add-ons
One-click Search 
Private Browsing
Customize
Default Browser

Let's say they either complete these all or checks the box to mark them as complete.

The update tour in 57 will have:
Sync (same as 56 version)
Library
Screenshots
Customize (different copy from 56 version)
Single Search (different from 56 version)

The items in the update tour that are the same as the 56 new user tour should then be marked complete (and not get a notification). In this case it would only be Sync.
√ Sync
  Library
  Screenshots
  Customize
  Search
Flags: needinfo?(mverdi)

Comment 7

2 months ago
Thanks for clarification, so we still need to set all tour as complete when we checked the box and dismissed the overlay.
Assignee: nobody → fliu
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 months ago
(In reply to Fischer [:Fischer] from comment #8)
> Created attachment 8878976 [details]
> Bug 1357021 - Handle tour completed state
> 
> This commit
> - turn on the completed css style for completed tours
> - sets individual tour as completed when action button of that tour is
> clicked
> - sets all tours as completed when hide-the-tour checkbox is checked
> 
> Review commit: https://reviewboard.mozilla.org/r/150232/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/150232/
This is a wip patch
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1357055
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1357050
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1357045
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1357037
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1357033
(Assignee)

Comment 15

2 months ago
Created attachment 8879632 [details]
onboarding_complete_UI.png
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 months ago
(In reply to Fischer [:Fischer] from comment #16)
> Created attachment 8879636 [details]
> Bug 1357021 - Part 2: Add browser_onboarding_tours.js test,
> 
> This commit
> - renames browser_onboarding_hide_tours.js to browser_onboarding_tours.js
> - adds test_click_action_button_to_set_tour_completed and
> test_set_right_tour_completed_style_on_overlay test cases
> - updates test_hide_onboarding_tours test case to test the changes of prefs
> of tours complete state
> 
> Review commit: https://reviewboard.mozilla.org/r/150896/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/150896/
Hi Mossop,

Please see [1] for the UI screenshot.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef447991c9e616ac4ce60cc1762f554d8c62d657&selectedJob=108567726

[1] attachment 8879632 [details]: onboarding_complete_UI.png

Thanks
(Assignee)

Updated

2 months ago
Priority: P2 → P1

Comment 19

2 months ago
mozreview-review
Comment on attachment 8878976 [details]
Bug 1357021 - Part 1: Handle tours completed state,

https://reviewboard.mozilla.org/r/150232/#review156336

::: browser/extensions/onboarding/content/onboarding.js:291
(Diff revision 2)
> +    if (params.length > 0) {
> +      this.sendMessageToChrome("set-prefs", params);
> +    }
> +  }
> +
> +  handleTourCompleteState(tourId) {

A better name here would be `markTourCompletionState`
Attachment #8878976 - Flags: review?(dtownsend) → review+

Comment 20

2 months ago
mozreview-review
Comment on attachment 8879636 [details]
Bug 1357021 - Part 2: Add the browser_onboarding_tours.js test,

https://reviewboard.mozilla.org/r/150896/#review156364
Attachment #8879636 - Flags: review?(dtownsend) → review+

Updated

2 months ago
Blocks: 1375775
(Assignee)

Comment 21

2 months ago
mozreview-review-reply
Comment on attachment 8878976 [details]
Bug 1357021 - Part 1: Handle tours completed state,

https://reviewboard.mozilla.org/r/150232/#review156336

> A better name here would be `markTourCompletionState`

Updated, thanks
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 months ago
(In reply to Fischer [:Fischer] from comment #22)
> Comment on attachment 8878976 [details]
> Bug 1357021 - Part 1: Handle tours completed state,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/150232/diff/2-3/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ef72c5411e99539afb1b1cf53e15a602a67ce1
Keywords: checkin-needed

Comment 25

2 months ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a78b750352f
Part 1: Handle tours completed state, r=mossop
https://hg.mozilla.org/integration/autoland/rev/3a2a088b9465
Part 2: Add the browser_onboarding_tours.js test, r=mossop
Keywords: checkin-needed

Comment 26

2 months ago
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
(Assignee)

Comment 27

2 months ago
(In reply to Mozilla Autoland from comment #26)
> We're sorry - something has gone wrong while rewriting or rebasing your
> commits. The commits being pushed no longer match what was requested. Please
> file a bug.
Hi Tomcat,
I am receiving this https://bugzilla.mozilla.org/show_bug.cgi?id=1357021#c26. First time seeing this. What dose this mean? Need me to do any action?
thank you.
Flags: needinfo?(cbook)
(In reply to Fischer [:Fischer] from comment #27)
> (In reply to Mozilla Autoland from comment #26)
> > We're sorry - something has gone wrong while rewriting or rebasing your
> > commits. The commits being pushed no longer match what was requested. Please
> > file a bug.
> Hi Tomcat,
> I am receiving this
> https://bugzilla.mozilla.org/show_bug.cgi?id=1357021#c26. First time seeing
> this. What dose this mean? Need me to do any action?
> thank you.

Honestly i don't know - i filed bug 1377065 to get this error message addressed (to file a bug as requested) and we will see there if you/me need to do any action
Flags: needinfo?(cbook)

Comment 29

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a78b750352f
https://hg.mozilla.org/mozilla-central/rev/3a2a088b9465
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

2 months ago
Target Milestone: Firefox 57 → Firefox 56
This issue has been verified on Win 10 x64.
Status: RESOLVED → VERIFIED

Updated

2 months ago
Depends on: 1378465
I've managed to reproduce this bug on Nightly 55.0a1 (2017-04-17) (64-bit) from Linux x64!

This issue is now verified as fixed on Latest Firefox Nightly 56.0a1 (2017-07-06) (64-bit)

Build ID: 20170706130309
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
OS: Linux 4.8.0-58-generic
QA Whiteboard: [bugday-20170705]
You need to log in before you can comment on or make changes to this bug.