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.

Sign up for GitHub

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

Jump to bottom

Signup: Alias anonUserId to WPCOM user id when user is created #11715

Merged
merged 2 commits into from
May 2, 2017
Merged

Signup: Alias anonUserId to WPCOM user id when user is created #11715

merged 2 commits into from
May 2, 2017

Conversation

bisko
Copy link
Contributor

@bisko bisko commented Mar 2, 2017

This PR aims to properly connect the anonUserId and the WPCOM user id.

Context: p3Ex-24k-p2

To test:

  1. Checkout branch
  2. Go through Signup without adding anything into Cart (domain or plan)
  3. Reach User step
  4. Fill the user step
  5. Submit
  6. Verify the _aliasUser event has been properly sent to the analytics backend

@bisko bisko added [Feature] Tracks Metrics & Monitoring Capturing analytics about user behavior on WordPress.com. [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. labels Mar 2, 2017
@bisko bisko self-assigned this Mar 2, 2017
@bisko bisko requested a review from mattm March 2, 2017 16:58
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/signup-analytics-send-user-alias-on-signup

@bisko bisko requested a review from michaeldcain March 2, 2017 16:58
@matticbot matticbot added the [Size] M Medium sized issue label Mar 2, 2017
@bisko bisko added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 2, 2017
@bisko bisko requested review from xyu and removed request for mattm March 6, 2017 11:13
michaeldcain
michaeldcain reviewed Apr 19, 2017
View reviewed changes
client/lib/analytics/index.js Outdated
@@ -382,17 +382,20 @@ const analytics = {
s.parentNode.insertBefore( wa, s );
}
},

identifyUser: function() {

Copy link
Member

Choose a reason for hiding this comment

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

L385: Trailing spaces not allowed

michaeldcain
michaeldcain requested changes Apr 19, 2017
View reviewed changes
client/lib/signup/step-actions.js Outdated
@@ -239,7 +239,7 @@ module.exports = {
createAccount( callback, dependencies, { userData, flowName, queryArgs, service, token }, reduxStore ) {
const surveyVertical = getSurveyVertical( reduxStore.getState() ).trim();
const surveySiteType = getSurveySiteType( reduxStore.getState() ).trim();

Copy link
Member

Choose a reason for hiding this comment

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

I bet you can guess ;)

@xyu
Copy link
Contributor

xyu commented Apr 21, 2017

This looks good to me but I don't actually know how the user object/state and signup actions are handled in Calypso. Perhaps someone with more Calypso experience can take a look? @rralian can you take a look or suggest someone?

@michaeldcain
Copy link
Member

but I don't actually know how the user object/state and signup actions are handled in Calypso.

I'm happy to take a look at it from here if you don't see any data blockers.

michaeldcain
michaeldcain requested changes Apr 21, 2017
View reviewed changes
client/lib/signup/step-actions.js Outdated
}

callback( errors, assign( {}, { username: userData.username }, bearerToken ) );
} );
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we lost an indentation level at L256, and thus lost the closing bracket for the conditional on L54.

@bisko bisko force-pushed the fix/signup-analytics-send-user-alias-on-signup branch from 91bf4b5 to ca26c4f Compare April 26, 2017 14:25
michaeldcain
michaeldcain reviewed Apr 28, 2017
View reviewed changes
client/signup/main.jsx
* The destination URL is not always inside Calypso.
* For these cases make sure that a proper redirect is issued.
*/
if ( /^https?:\/\// ) {
Copy link
Member

Choose a reason for hiding this comment

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

This works for me, and I can't think of a way that it'd currently mess up. No SSR to worry about. Might be worth adding a note to the destination explanation in the Signup readme, but other than that, 👍.

@michaeldcain michaeldcain added [Status] Ready to Merge and removed [Status] Needs Rebase [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 28, 2017
@bisko bisko merged commit fb6da67 into master May 2, 2017
@bisko bisko deleted the fix/signup-analytics-send-user-alias-on-signup branch May 2, 2017 15:23
bisko added a commit that referenced this pull request May 2, 2017
@bisko
Revert "Signup: Alias anonUserId to WPCOM user id when user is created (
435dcab 
#11715)"

This reverts commit fb6da67.
@bisko bisko mentioned this pull request May 2, 2017
Merged
bisko added a commit that referenced this pull request May 2, 2017
@bisko
Revert "Signup: Alias anonUserId to WPCOM user id when user is create…
a902e56 
…d" (#13563)

* Revert "Reader: Data-Layer for recommended sites (#13394)"

This reverts commit 516a24a.

* Revert "Add isRtl selector (#13333)"

This reverts commit d34cfc4.

* Revert "Signup: Alias anonUserId to WPCOM user id when user is created (#11715)"

This reverts commit fb6da67.
ramonjd added a commit that referenced this pull request May 7, 2019
@ramonjd
Reverting changes in #13563, which reverted changes in #11715
c42aa81
@ramonjd ramonjd mentioned this pull request May 7, 2019
Merged
ramonjd added a commit that referenced this pull request Jun 22, 2019
@ramonjd
Reverting changes in #13563, which reverted changes in #11715
762e7f1
ramonjd added a commit that referenced this pull request Jul 1, 2019
@ramonjd
Reverting changes in #13563, which reverted changes in #11715
ee3801c
ramonjd added a commit that referenced this pull request Jul 5, 2019
@ramonjd
Signup: Alias anonUserId to WPCOM user id when user is created (#32850)
1864958 
* Reverting changes in #13563, which reverted changes in #11715

* Added Arbitrary logging for the case when the user fetch fails during new user account creation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeldcain michaeldcain michaeldcain approved these changes

@xyu xyu Awaiting requested review from xyu

Assignees

@bisko bisko

Labels
[Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. [Feature] Tracks Metrics & Monitoring Capturing analytics about user behavior on WordPress.com. [Size] M Medium sized issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bisko @matticbot @xyu @michaeldcain