Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guided Tours: Bail if fresh user prefs missing #10822

Merged
merged 4 commits into from
Jan 27, 2017

Conversation

mcsf
Copy link
Member

@mcsf mcsf commented Jan 20, 2017

The Guided Tours framework tracks which tours a user has seen (completed or dismissed) by persisting that history using the Preferences API (state.preferences). Up to this point, it was missing an explicit check that the preferences available locally (via getPreferences) have been successfully pulled from the server and aren't stale. Without the check, GT assumed the history was empty (and thus any tour was good to show again). The check should guard against two issues:

  • In the case where Guided Tours runs before the request for fresh preferences (typically in order to satisfy a <QueryPreferences /> element), we ensure that GT waits for the preferences before deciding on a tour. [fig 1]
  • In the case where something is consistently failing between the user and the preferences endpoint (e.g. users with 2-factor authentication enabled and a stale token, see Framework: Account for Preferences settings 2FA refresh response #202), Guided Tours will defensively not run at all. [fig 2]

fig 1:
waiting-for-prefs-1

fig 2:
waiting-for-prefs-2

To test

The best is likely to locally revert #10706, set localStorage.setItem( 'debug', 'calypso:guided*' ) and test with different accounts (without 2FA, with 2FA and fresh token, with 2FA and stale token) and different histories (having seen editorInsertMenu, having not seen it).

@mcsf mcsf added Guided Tours [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Bug labels Jan 20, 2017
@mcsf mcsf self-assigned this Jan 20, 2017
@matticbot
Copy link
Contributor

@lsinger
Copy link
Contributor

lsinger commented Jan 26, 2017

Testing revealed that the change works mostly as intended, but that it also keeps tour from starting that are forced using a query argument. Therefore, 0e9e146 moves the bailing code to the findTriggeredTour function. @marekhrabe can you review when @mcsf has fixed the failing test suite 😬?

To make the token stale, delete the twostep_auth cookie from the public-api.wordpress.com domain.

@mcsf
Copy link
Member Author

mcsf commented Jan 26, 2017

Test suite amended, approval requested, @lsinger and @marekhrabe :)

@marekhrabe
Copy link
Contributor

I tested with token cookie removed and this PR worked as expected. :shipit:

Copy link
Contributor

@marekhrabe marekhrabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mcsf mcsf merged commit 831e6bb into master Jan 27, 2017
@mcsf mcsf deleted the fix/guided-tours-bail-if-stale-prefs branch January 27, 2017 15:40
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants