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

VERIFIED FIXED in Firefox 56

Status

()

P1
normal
VERIFIED FIXED
2 years ago
a year 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])

Attachments

(3 attachments)

(Assignee)

Description

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

Updated

2 years ago
Whiteboard: [photon-onboarding]

Updated

2 years ago
Target Milestone: --- → Firefox 55
Assignee: nobody → rexboy

Updated

2 years ago
Status: NEW → ASSIGNED

Updated

2 years ago
Flags: qe-verify+
QA Contact: jwilliams

Updated

2 years ago
Priority: -- → P1
Priority: P1 → P2
Target Milestone: Firefox 55 → Firefox 57

Updated

2 years ago
Assignee: rexboy → gasolin

Comment 1

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

Comment 2

a year 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)
go ahead, thanks
Flags: needinfo?(gasolin)
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

a year 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

a year 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)
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

a year 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

a year ago
Duplicate of this bug: 1357055
(Assignee)

Updated

a year ago
Duplicate of this bug: 1357050
(Assignee)

Updated

a year ago
Duplicate of this bug: 1357045
(Assignee)

Updated

a year ago
Duplicate of this bug: 1357037
(Assignee)

Updated

a year ago
Duplicate of this bug: 1357033
(Assignee)

Comment 15

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

Comment 18

a year 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

a year ago
Priority: P2 → P1

Comment 19

a year 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

a year 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

a year ago
Blocks: 1375775
(Assignee)

Comment 21

a year 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

a year 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

a year 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

a year 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

a year 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

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

Updated

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

Updated

a year 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.