Closed Bug 1357021 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

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

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(3 files)

Bug 1357021 - Part 1: Handle tours completed state,
59 bytes, text/x-review-board-request
mossop
: review+
Details
onboarding_complete_UI.png
415.10 KB, image/png
Details
Bug 1357021 - Part 2: Add the browser_onboarding_tours.js test,
59 bytes, text/x-review-board-request
mossop
: review+
Details 
Should be able to make all the tours completed by checking "Mark all as complete" checkbox
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 55
Assignee: nobody → rexboy
Status: NEW → ASSIGNED
Flags: qe-verify+
QA Contact: jwilliams
Priority: -- → P1
Priority: P1 → P2
Target Milestone: Firefox 55 → Firefox 57
Assignee: rexboy → gasolin
should handle this after we implement the first tour validation mechanism
Assignee: gasolin → nobody
Status: ASSIGNED → NEW
(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)
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)
(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
(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

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
(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

    
  
Priority: P2 → P1

    
    

    
  
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 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+

    
  
Blocks: 1375775

    
    

    
  
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

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(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

    
    

    
  
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

    
    

    
  
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.

    
    

    
  
(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)

    
  
Target Milestone: Firefox 57 → Firefox 56

    
    

    
  
This issue has been verified on Win 10 x64.
Status: RESOLVED → VERIFIED

    
  
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.