Guided Tours: Find & show tours based on state selectors #6417

mcsf · 2016-06-29T16:03:43Z

Previously, GuidedTours was able to trigger only a single tour -- main. This was based solely on the presence of tour=main in the query arguments. This change introduces a selectors-based approach to triggering tours that is based on the current context and tour-specific requirements for that context.

Duplicate of #6335, but based on master this time.
Product of exploration in #6146. See that PR for context.

Before merging

Tests
For consistency, settle on name: tour or tourName (punted)
Assimilate relevantFeatures into config, or leave that to next PR? (next PR)
Don't actually show dummy tours in production
Misc. cleanup, squash
Consider only saving certain ROUTE_SET actions in state, for performance (punted, later when we need it)

Testing

get into the guidedTours abtest***, create a new user account, and make sure after signup you are redirected to a ?tour=main URL
make sure the tour works fine
make sure you can visit other parts of Calypso without throwing an error (especially try clicking on themes)
make sure you can still summon the tour by appending the ?tour=main bit

*** localStorage.setItem( 'ABTests', '{"guidedTours_20160603":"guided"}' );

Test live: https://calypso.live/?branch=add/guided-tours-log-selectors-rebased

mcsf · 2016-06-30T00:03:37Z

One thing I noticed worries me.

The following breaks the tests, as it should:

diff --git a/client/state/ui/guided-tours/selectors.js b/client/state/ui/guided-tours/selectors.js
index 74af0d5..e7e3d1b 100644
--- a/client/state/ui/guided-tours/selectors.js
+++ b/client/state/ui/guided-tours/selectors.js
@@ -92,7 +92,7 @@ export const findEligibleTour = createSelector(
            return context( state );
        } );
    },
-   [ getActionLog, getToursHistory ]
+   getActionLog
 );

 /**

because the findEligibleTour selector gets called multiple times in that test suite and needs to know that it depends on getToursHistory to work properly.

However, the following does not break the tests, when I believe it should be functionally equivalent to the previous diff:

diff --git a/client/state/ui/guided-tours/selectors.js b/client/state/ui/guided-tours/selectors.js
index 74af0d5..2c4059f 100644
--- a/client/state/ui/guided-tours/selectors.js
+++ b/client/state/ui/guided-tours/selectors.js
@@ -92,7 +92,7 @@ export const findEligibleTour = createSelector(
            return context( state );
        } );
    },
-   [ getActionLog, getToursHistory ]
+   [ getActionLog ]
 );

 /**

@aduth, since we've been discussing selectors and have just merged #6391, I'm cc'ing you for a sanity check. It's getting late here.

Edit: this happens regardless of using shorthand, thus [ getActionLog ] and state => [ getActionLog( state ) ] lead to the same result.

aduth · 2016-06-30T13:12:07Z

Quite possibly because memoization cache is not reset between tests. See posts tests as an example.

mcsf · 2016-06-30T13:37:46Z

Yes, but I expect memoization to be working. However, I'm surprised that getActionLog( state ) is treated differently from [ getActionLog( state ) ].

aduth · 2016-06-30T13:45:36Z

Hmm, equally confusing, I find all tests passing when applying your first diff, but resetting selectors cache between tests.

diff --git a/client/state/ui/guided-tours/selectors.js b/client/state/ui/guided-tours/selectors.js
index 74af0d5..e279a62 100644
--- a/client/state/ui/guided-tours/selectors.js
+++ b/client/state/ui/guided-tours/selectors.js
@@ -45,7 +45,7 @@ const relevantFeatures = [
        },
 ];

-const getToursFromFeaturesReached = createSelector(
+export const getToursFromFeaturesReached = createSelector(
        state => reverse( uniq(
                getActionLog( state )
                        .filter( ( { type } ) => type === ROUTE_SET )
@@ -63,7 +63,7 @@ const getToursFromFeaturesReached = createSelector(
        getActionLog
 );

-const getToursSeen = createSelector(
+export const getToursSeen = createSelector(
        state => uniq(
                getToursHistory( state )
                        .map( ( { tourName } ) => tourName )
@@ -92,7 +92,7 @@ export const findEligibleTour = createSelector(
                        return context( state );
                } );
        },
-       [ getActionLog, getToursHistory ]
+       getActionLog
 );

 /**
diff --git a/client/state/ui/guided-tours/test/selectors.js b/client/state/ui/guided-tours/test/selectors.js
index 94b4557..bb1b0c4 100644
--- a/client/state/ui/guided-tours/test/selectors.js
+++ b/client/state/ui/guided-tours/test/selectors.js
@@ -7,12 +7,21 @@ import { expect } from 'chai';
  * Internal dependencies
  */
 import {
+       getToursFromFeaturesReached,
+       getToursSeen,
        getGuidedTourState,
        findEligibleTour,
 } from '../selectors';
 import guidedToursConfig from 'layout/guided-tours/config';

 describe( 'selectors', () => {
+       beforeEach( () => {
+               getToursFromFeaturesReached.memoizedSelector.cache.clear();
+               getToursSeen.memoizedSelector.cache.clear();
+               getGuidedTourState.memoizedSelector.cache.clear();
+               findEligibleTour.memoizedSelector.cache.clear();
+       } );
+
        describe( '#getGuidedTourState()', () => {
                it( 'should return an empty object if no state is present', () => {
                        const tourState = getGuidedTourState( {

aduth · 2016-06-30T13:55:54Z

Aside: I contemplated the idea that we could check in lib/create-selector whether the current environment is test and disabling cache, if it becomes a pain to deal with memoization reset in tests (cc @blowery ).

mcsf · 2016-06-30T14:59:23Z

Hmm, equally confusing, I find all tests passing when applying your first diff, but resetting selectors cache between tests.

That one actually sounds correct, because — even though we're setting the wrong dependencies by only mentioning getActionLog — by busting the cache we make sure that the selectors recompute everything. Since the state and the implementations are good enough, the tests then pass.

mcsf · 2016-06-30T15:02:44Z

Aside: I contemplated the idea that we could check in lib/create-selector whether the current environment is test and disabling cache, if it becomes a pain to deal with memoization reset in tests (cc @blowery ).

That's an interesting idea. I bet there's a higher probability of a bug not being spotted due to a selector's cache not being cleared than the opposite — even though, in this particular instance, I spotted the need to add the getToursHistory dependency thanks to the cached result not making sense.

mcsf · 2016-07-02T15:00:58Z

Rebased against master. An external fix, #6435, improves performance when browsing /themes.

aduth · 2016-07-05T15:22:19Z

client/state/notices/actions.js

@@ -36,10 +36,11 @@ export function createNotice( status, text, options = {} ) {
 	};
 }

-export function setRoute( path ) {
+export function setRoute( path, query = {} ) {


In #6306 (to be merged shortly), this action creator is moved to state/ui/actions.js

mcsf · 2016-07-13T12:45:49Z

Pushed a large rebase, with a lot of the feedback addressed right into pre-existing (repackaged) commits. I'll do some more squashing soon in a way that keeps the history sound.

@ehg: maybe wait until the second rebase before updating #6472 :)

mcsf · 2016-07-14T15:17:08Z

Thanks for tackling those outstanding items, @lsinger! The logic after the removal of the dummy tours seems good.

ehg · 2016-07-14T16:14:19Z

We should test whether the shouldDelay stuff still works with checkout before we consider merging :)

lsinger · 2016-07-15T08:32:29Z

We should test whether the shouldDelay stuff still works with checkout before we consider merging :)

Hgrmpf.

@mcsf

GuidedTours needs access to query arguments to determine whether a tour is being forced (e.g. .../?tour=main). This puts the query arguments into the redux state tree. This avoids having to read from interfaces like `location`, which are impure and make testing more difficult. Props @mcsf.

Previously, GuidedTours was able to trigger only a single tour -- ` main`. This was based solely on the presence of `tour=main` in the query arguments. This change introduces a selectors-based approach to triggering tours that is based on the current context and tour-specific requirements for that context.

ehg · 2016-07-18T15:09:46Z

Rebased.

ehg · 2016-07-18T15:18:46Z

Let's merge this.

ockham · 2017-11-12T17:07:53Z

Stumbled across the initial and current qargs reducers/selectors while looking at #19036. This adds considerable complexity and confusion for me TBH -- I don't know what initial vs current means for qargs without having any context, and when working with them elsewhere, I don't know how to deal with that distinction.

Do we really need both?

mcsf · 2017-11-28T13:20:01Z

tl;dr: initial means these are the qargs with which Calypso booted, current means these are the actual latest qargs, as seen in location.

IIRC, the following would happen:

Follow a link to //calypso/?tour=main
a. Either during init, Calypso adjusts the route and in that process the qarg is lost,
b. or the tour starts, but as the user follows tour instructions and navigates, qarg is lost
Loss of qarg messes up decision process in state selectors to keep on with the tour

This led to the need for initial. I can't remember if current was ever needed for us, or if it was introduced in opposition to initial. The rationale would be that, if there were only a single reducer qarg, a dev would expect it to reflect the latest browser state, not the initial one.

lsinger · 2017-12-05T10:21:56Z

I noticed today that triggering tours using ?tour=tourName doesn't work anymore — well, it does work, but only once until you clear your redux state.

Something @markryall found:

the first value of that query string is stored in the redux state and subsequent values are ignored

Does anyone have any clue how this might have come to be?

ockham · 2017-12-08T22:30:17Z

@lsinger Could this be related? #20255

lsinger · 2017-12-13T11:56:40Z

Yes, thanks @ockham. Documented here: #20553 (comment)

mcsf added the Guided Tours label Jun 29, 2016

mcsf added this to the GuidedTours M3: Multitours milestone Jun 29, 2016

mcsf self-assigned this Jun 29, 2016

mcsf mentioned this pull request Jun 29, 2016

Guided Tours: Find & show tours based on state selectors #6335

Closed

3 tasks

mcsf force-pushed the add/guided-tours-log-selectors-rebased branch 3 times, most recently from cdab56f to 36369be Compare June 29, 2016 23:39

mcsf added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] In Progress labels Jun 30, 2016

ehg mentioned this pull request Jul 1, 2016

GuidedTours: Add themes tour #6472

Merged

1 task

mcsf force-pushed the add/guided-tours-log-selectors-rebased branch 3 times, most recently from bd6694a to afc2dd7 Compare July 2, 2016 14:58

aduth reviewed Jul 5, 2016
View reviewed changes

mcsf force-pushed the add/guided-tours-log-selectors-rebased branch from afc2dd7 to 047e9ae Compare July 13, 2016 12:41

mcsf force-pushed the add/guided-tours-log-selectors-rebased branch 2 times, most recently from 56ae182 to c99b1c8 Compare July 13, 2016 16:36

lsinger force-pushed the add/guided-tours-log-selectors-rebased branch 3 times, most recently from c93fdce to 35cdd84 Compare July 14, 2016 14:52

lsinger added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] In Progress labels Jul 14, 2016

mcsf mentioned this pull request Jul 14, 2016

Guided Tours: Guarantee a tour's completion before selecting another #6785

Closed

ehg mentioned this pull request Jul 14, 2016

Add a First View introduction overlay to the Stats section #6717

Merged

lsinger force-pushed the add/guided-tours-log-selectors-rebased branch from 35cdd84 to 8cf91cb Compare July 15, 2016 08:19

lsinger added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 15, 2016

mcsf mentioned this pull request Jul 15, 2016

ui.queryArguments: update state less aggressively #6818

Merged

lsinger and others added 2 commits July 18, 2016 16:03

ehg force-pushed the add/guided-tours-log-selectors-rebased branch from 8cf91cb to fbb5cdb Compare July 18, 2016 15:06

ehg added [Status] Ready to Merge and removed [Status] In Progress labels Jul 18, 2016

mcsf merged commit 9e64cde into master Jul 18, 2016

mcsf deleted the add/guided-tours-log-selectors-rebased branch July 18, 2016 15:20

matticbot removed the [Status] Ready to Merge label Jul 18, 2016

ockham mentioned this pull request Nov 12, 2017

Fix wrong page title and missing head tags on login #19036

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guided Tours: Find & show tours based on state selectors #6417

Guided Tours: Find & show tours based on state selectors #6417

mcsf commented Jun 29, 2016 •

edited by lsinger

Loading

mcsf commented Jun 30, 2016 •

edited

Loading

aduth commented Jun 30, 2016

mcsf commented Jun 30, 2016

aduth commented Jun 30, 2016 •

edited

Loading

aduth commented Jun 30, 2016

mcsf commented Jun 30, 2016

mcsf commented Jun 30, 2016

mcsf commented Jul 2, 2016

aduth Jul 5, 2016

mcsf commented Jul 13, 2016

mcsf commented Jul 14, 2016

ehg commented Jul 14, 2016

lsinger commented Jul 15, 2016

ehg commented Jul 18, 2016

ehg commented Jul 18, 2016

ockham commented Nov 12, 2017

mcsf commented Nov 28, 2017

lsinger commented Dec 5, 2017

ockham commented Dec 8, 2017

lsinger commented Dec 13, 2017 •

edited

Loading

Guided Tours: Find & show tours based on state selectors #6417

Guided Tours: Find & show tours based on state selectors #6417

Conversation

mcsf commented Jun 29, 2016 • edited by lsinger Loading

Before merging

Testing

mcsf commented Jun 30, 2016 • edited Loading

aduth commented Jun 30, 2016

mcsf commented Jun 30, 2016

aduth commented Jun 30, 2016 • edited Loading

aduth commented Jun 30, 2016

mcsf commented Jun 30, 2016

mcsf commented Jun 30, 2016

mcsf commented Jul 2, 2016

aduth Jul 5, 2016

Choose a reason for hiding this comment

mcsf commented Jul 13, 2016

mcsf commented Jul 14, 2016

ehg commented Jul 14, 2016

lsinger commented Jul 15, 2016

ehg commented Jul 18, 2016

ehg commented Jul 18, 2016

ockham commented Nov 12, 2017

mcsf commented Nov 28, 2017

lsinger commented Dec 5, 2017

ockham commented Dec 8, 2017

lsinger commented Dec 13, 2017 • edited Loading

mcsf commented Jun 29, 2016 •

edited by lsinger

Loading

mcsf commented Jun 30, 2016 •

edited

Loading

aduth commented Jun 30, 2016 •

edited

Loading

lsinger commented Dec 13, 2017 •

edited

Loading