Allow TOS agreement before Jetpack is fully active so we track the connection flow #14041

gravityrail · 2019-11-14T18:34:16Z

In #13763 we moved our ToS agreement code into a package, but we changed the logic at the same time. Now, instead of the user agreeing to the ToS when they click the connect button, it is not enabled until the site is fully active, which has essentially killed all the events we were getting during the connection flow.

Changes proposed in this Pull Request:

Allow sending events when user has agreed to ToS
As a fallback (on sites which may have been connected prior to the ToS flag being available) we also use is_active as a signal that the user has agreed to ToS
This also fixes a typo in an action name

Testing instructions:

It's helpful to add some debug code to the Tracks library so you can see if events are being sent
Go through the connect flow
Connection related events like jetpack_jpc_register_begin should now be sent. In 7.9 they were not sent.

…nnection flow

jetpackbot · 2019-11-14T18:36:37Z

	Warnings
⚠️	`pre-commit` hook was skipped for one or more commits
⚠️	"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against f9416f5

kraftbj

Have not tested live, but it reads well.

packages/terms-of-service/src/class-terms-of-service.php

dereksmart

Co-Authored-By: Derek Smart <smart@automattic.com>

lezama · 2019-11-14T22:50:27Z

@dereksmart I also noticed some consecutive events for jetpack_jpc_register_begin

We renamed the action lets also rename it in Jetpack.

jeherve

@enejb It seems you've edited jetpack_agreed_to_terms_of_service into jetpack_reject_terms_of_service. Did you mean to do that? We did not rename jetpack_agreed_to_terms_of_service.

… jetpack" This reverts commit e9c0f21.

enejb · 2019-11-15T11:09:18Z

Thanks for catching that @jeherve I reverted my last commit.

enejb · 2019-11-15T11:10:23Z

I tested this with both flows (original) and in place and it resulted in the same result.

dereksmart

Good again!

jeherve

Looking good to me. Merging.

…we track… (#14041) * Allow TOS agreement before Jetpack is fully active so we track the connection flow * Update packages/terms-of-service/src/class-terms-of-service.php Co-Authored-By: Derek Smart <smart@automattic.com> * Also update the action that is being called in jetpack We renamed the action lets also rename it in Jetpack. * [not verified] Revert "Also update the action that is being called in jetpack" This reverts commit e9c0f21. Co-authored-by: Brandon Kraft <public@brandonkraft.com> Co-authored-by: Enej Bajgoric <enej.bajgoric@gmail.com>

jeherve · 2019-11-15T16:04:08Z

Cherry-picked to branch-7.9 in ba4b09d

* Add products endpoint * [not verified] Add error code * Update _inc/lib/class.core-rest-api-endpoints.php Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Update _inc/lib/class.core-rest-api-endpoints.php Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Add QueryProducts data component and corresponding Redux and rest API calls * Update _inc/client/state/site/reducer.js Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * [not verified] Add SingleProductBackupBody component * [not verified] Rename plans-section__body to single_product_backup__body * [not verified] Give class name to h4 tag * [not verified] Remove single_product_backup__body * [not verified] Check for upgradeLinks and upgradeTitles before using them * [not verified] Switch function style * [not verified] Add key to PlanRadioButton * Remove duplicate definition of getProducts * Allow TOS agreement before Jetpack is fully active so we track… (#14041) * Allow TOS agreement before Jetpack is fully active so we track the connection flow * Update packages/terms-of-service/src/class-terms-of-service.php Co-Authored-By: Derek Smart <smart@automattic.com> * Also update the action that is being called in jetpack We renamed the action lets also rename it in Jetpack. * [not verified] Revert "Also update the action that is being called in jetpack" This reverts commit e9c0f21. Co-authored-by: Brandon Kraft <public@brandonkraft.com> Co-authored-by: Enej Bajgoric <enej.bajgoric@gmail.com> * Removed Jetpack references in the IXR client. (#14046) This change allows the package to be used outside of Jetpack, relying on the methods provided by the Manager, plus using the WP_Error class instead of the Jetpack_Error wrapper. * Full Sync: Don't allow more than one request to enqueue (#14039) * Spell checking CI integration (#13992) * Adds spell checking and fixes files. This implements a package to spellcheck files, and does a pass to ensure everything is green. * [not verified] Implements Travis check for spelling Adds Yarn script and Travis CI for spell checking. * [not verified] Fixes feedback. Updates docs to prevent spelling exceptions for certain files. * Export SingleProductBackupBody class * [not verified] No need to export single product backup body

Allow TOS agreement before Jetpack is fully active so we track the co…

b5cba34

…nnection flow

gravityrail requested a review from a team November 14, 2019 18:34

dereksmart added [Pri] BLOCKER [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Tracks labels Nov 14, 2019

dereksmart added this to the 7.9.1 milestone Nov 14, 2019

kraftbj reviewed Nov 14, 2019

View reviewed changes

packages/terms-of-service/src/class-terms-of-service.php Show resolved Hide resolved

packages/terms-of-service/src/class-terms-of-service.php Show resolved Hide resolved

dereksmart reviewed Nov 14, 2019

View reviewed changes

packages/terms-of-service/src/class-terms-of-service.php Outdated Show resolved Hide resolved

dereksmart previously approved these changes Nov 14, 2019

View reviewed changes

Update packages/terms-of-service/src/class-terms-of-service.php

57f5a85

Co-Authored-By: Derek Smart <smart@automattic.com>

kraftbj dismissed dereksmart’s stale review via 57f5a85 November 14, 2019 20:34

gravityrail added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 14, 2019

gravityrail requested review from kraftbj and dereksmart November 14, 2019 21:01

gravityrail added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 14, 2019

lezama previously approved these changes Nov 14, 2019

View reviewed changes

Also update the action that is being called in jetpack

e9c0f21

We renamed the action lets also rename it in Jetpack.

enejb dismissed lezama’s stale review via e9c0f21 November 15, 2019 09:57

jeherve requested changes Nov 15, 2019

View reviewed changes

jeherve assigned gravityrail Nov 15, 2019

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Cherry-Pick and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 15, 2019

[not verified] Revert "Also update the action that is being called in…

f9416f5

… jetpack" This reverts commit e9c0f21.

dereksmart approved these changes Nov 15, 2019

View reviewed changes

dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 15, 2019

jeherve approved these changes Nov 15, 2019

View reviewed changes

jeherve merged commit f275547 into master Nov 15, 2019

jeherve deleted the fix/tos-stats-broken branch November 15, 2019 16:03

matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 15, 2019

jeherve removed [Status] Needs Changelog [Status] Needs Cherry-Pick [Status] Needs Package Release This PR made changes to a package. Let's update that package now. labels Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow TOS agreement before Jetpack is fully active so we track the connection flow #14041

Allow TOS agreement before Jetpack is fully active so we track the connection flow #14041

gravityrail commented Nov 14, 2019 •

edited

Loading

jetpackbot commented Nov 14, 2019 •

edited

Loading

kraftbj left a comment

dereksmart left a comment

lezama commented Nov 14, 2019

jeherve left a comment

enejb commented Nov 15, 2019

enejb commented Nov 15, 2019

dereksmart left a comment

jeherve left a comment

jeherve commented Nov 15, 2019

Allow TOS agreement before Jetpack is fully active so we track the connection flow #14041

Allow TOS agreement before Jetpack is fully active so we track the connection flow #14041

Conversation

gravityrail commented Nov 14, 2019 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

jetpackbot commented Nov 14, 2019 • edited Loading

kraftbj left a comment

Choose a reason for hiding this comment

dereksmart left a comment

Choose a reason for hiding this comment

lezama commented Nov 14, 2019

jeherve left a comment

Choose a reason for hiding this comment

enejb commented Nov 15, 2019

enejb commented Nov 15, 2019

dereksmart left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

jeherve commented Nov 15, 2019

gravityrail commented Nov 14, 2019 •

edited

Loading

jetpackbot commented Nov 14, 2019 •

edited

Loading