-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Stepper: calypso_signup_complete as in Start #94706
Changes from 3 commits
fabcdf2
54f2da0
cfddcb9
0a37653
a43fab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,68 +3,81 @@ import { useSelect } from '@wordpress/data'; | |
import { useCallback } from 'react'; | ||
import { USER_STORE, ONBOARD_STORE } from 'calypso/landing/stepper/stores'; | ||
import { SIGNUP_DOMAIN_ORIGIN, recordSignupComplete } from 'calypso/lib/analytics/signup'; | ||
import { useSelector } from 'calypso/state'; | ||
import isUserRegistrationDaysWithinRange from 'calypso/state/selectors/is-user-registration-days-within-range'; | ||
import { useSite } from './use-site'; | ||
import type { UserSelect, OnboardSelect } from '@automattic/data-stores'; | ||
|
||
export const useRecordSignupComplete = ( flow: string | null ) => { | ||
const site = useSite(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @escapemanuele is this where the issue stems? It looks like it grabs site ID/slug from URL params, which would obviously be Isn't there a store/state slice that gets populated with site ID when one is created? We should probably look at create-site step and do that if not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I ended up doing is passing the data from the processing step directly when calling the useRecordSignupComplete function here. At that point we have all the data needed. Do you think that should be changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. Sounds good (you obviously know more about these concepts)
I'm not sure. It might not be a bad idea to populate the onboard store with it once a site is created (or pending). It would make it available in anything that follows and needs it. I will comment up above on something related if you can also take a look pls |
||
const siteId = site?.ID || null; | ||
const theme = site?.options?.theme_slug || ''; | ||
const { domainCartItem, planCartItem, siteCount, selectedDomain } = useSelect( ( select ) => { | ||
return { | ||
siteCount: ( select( USER_STORE ) as UserSelect ).getCurrentUser()?.site_count, | ||
domainCartItem: ( select( ONBOARD_STORE ) as OnboardSelect ).getDomainCartItem(), | ||
planCartItem: ( select( ONBOARD_STORE ) as OnboardSelect ).getPlanCartItem(), | ||
selectedDomain: ( select( ONBOARD_STORE ) as OnboardSelect ).getSelectedDomain(), | ||
}; | ||
}, [] ); | ||
const { domainCartItem, planCartItem, siteCount, selectedDomain, currentUser } = useSelect( | ||
( select ) => { | ||
return { | ||
siteCount: ( select( USER_STORE ) as UserSelect ).getCurrentUser()?.site_count, | ||
domainCartItem: ( select( ONBOARD_STORE ) as OnboardSelect ).getDomainCartItem(), | ||
planCartItem: ( select( ONBOARD_STORE ) as OnboardSelect ).getPlanCartItem(), | ||
selectedDomain: ( select( ONBOARD_STORE ) as OnboardSelect ).getSelectedDomain(), | ||
currentUser: ( select( USER_STORE ) as UserSelect ).getCurrentUser(), | ||
}; | ||
}, | ||
[] | ||
); | ||
|
||
return useCallback( () => { | ||
// FIXME: once moving to the Stepper version of User step, | ||
// wire the value of `isNewUser()` from the user store. | ||
const isNewUser = ! siteCount; | ||
const isNewishUser = useSelector( ( state ) => | ||
isUserRegistrationDaysWithinRange( state, null, 0, 7 ) | ||
); | ||
|
||
// FIXME: | ||
// currently it's impossible to derive this data since it requires | ||
// the length of registration, so I use isNewUser here as an approximation | ||
const isNew7DUserSite = isNewUser; | ||
return useCallback( | ||
( signupCompletionState: Record< string, unknown > ) => { | ||
const siteSlug = site?.slug ?? signupCompletionState?.siteSlug; | ||
|
||
// Domain product slugs can be a domain purchases like dotcom_domain or dotblog_domain or a mapping like domain_mapping | ||
// When purchasing free subdomains the product_slugs is empty (since there is no actual produce being purchased) | ||
// so we avoid capturing the product slug in these instances. | ||
const domainProductSlug = domainCartItem?.product_slug ?? undefined; | ||
const isNewUser = !! currentUser?.username; | ||
|
||
// Domain cart items can sometimes be included when free. So the selected domain is explicitly checked to see if it's free. | ||
// For mappings and transfers this attribute should be empty but it needs to be checked. | ||
const hasCartItems = !! ( domainProductSlug || planCartItem ); // see the function `dependenciesContainCartItem() | ||
const isNew7DUserSite = !! ( | ||
isNewUser || | ||
( isNewishUser && siteSlug && siteCount && siteCount <= 1 ) | ||
); | ||
|
||
// When there is no plan put in the cart, `planCartItem` is `null` instead of `undefined` like domainCartItem. | ||
// It worths a investigation of whether the both should behave the same. | ||
const planProductSlug = planCartItem?.product_slug ?? undefined; | ||
// To have a paid domain item it has to either be a paid domain or a different domain product like mapping or transfer. | ||
const hasPaidDomainItem = | ||
( selectedDomain && ! selectedDomain.is_free ) || !! domainProductSlug; | ||
// Domain product slugs can be a domain purchases like dotcom_domain or dotblog_domain or a mapping like domain_mapping | ||
// When purchasing free subdomains the product_slugs is empty (since there is no actual produce being purchased) | ||
// so we avoid capturing the product slug in these instances. | ||
const domainProductSlug = domainCartItem?.product_slug ?? undefined; | ||
|
||
recordSignupComplete( | ||
{ | ||
flow, | ||
siteId, | ||
isNewUser, | ||
hasCartItems, | ||
isNew7DUserSite, | ||
theme, | ||
intent: flow, | ||
startingPoint: flow, | ||
isBlankCanvas: theme?.includes( 'blank-canvas' ), | ||
planProductSlug, | ||
domainProductSlug, | ||
isMapping: | ||
hasPaidDomainItem && domainCartItem ? isDomainMapping( domainCartItem ) : undefined, | ||
isTransfer: | ||
hasPaidDomainItem && domainCartItem ? isDomainTransfer( domainCartItem ) : undefined, | ||
signupDomainOrigin: SIGNUP_DOMAIN_ORIGIN.NOT_SET, | ||
}, | ||
true | ||
); | ||
}, [ domainCartItem, flow, planCartItem, selectedDomain, siteCount, siteId, theme ] ); | ||
// Domain cart items can sometimes be included when free. So the selected domain is explicitly checked to see if it's free. | ||
// For mappings and transfers this attribute should be empty but it needs to be checked. | ||
const hasCartItems = !! ( domainProductSlug || planCartItem ); // see the function `dependenciesContainCartItem() | ||
|
||
// When there is no plan put in the cart, `planCartItem` is `null` instead of `undefined` like domainCartItem. | ||
// It worths a investigation of whether the both should behave the same. | ||
const planProductSlug = planCartItem?.product_slug ?? undefined; | ||
// To have a paid domain item it has to either be a paid domain or a different domain product like mapping or transfer. | ||
const hasPaidDomainItem = | ||
( selectedDomain && ! selectedDomain.is_free ) || !! domainProductSlug; | ||
|
||
recordSignupComplete( | ||
{ | ||
flow, | ||
siteId: siteId ?? signupCompletionState?.siteId, | ||
isNewUser, | ||
hasCartItems, | ||
isNew7DUserSite, | ||
theme, | ||
intent: flow, | ||
startingPoint: flow, | ||
isBlankCanvas: theme?.includes( 'blank-canvas' ), | ||
planProductSlug, | ||
domainProductSlug, | ||
isMapping: | ||
hasPaidDomainItem && domainCartItem ? isDomainMapping( domainCartItem ) : undefined, | ||
isTransfer: | ||
hasPaidDomainItem && domainCartItem ? isDomainTransfer( domainCartItem ) : undefined, | ||
signupDomainOrigin: SIGNUP_DOMAIN_ORIGIN.NOT_SET, | ||
}, | ||
true | ||
); | ||
}, | ||
[ domainCartItem, flow, planCartItem, selectedDomain, siteCount, siteId, theme ] | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just roughly thinking. I get the impression that a specific step should not control and record the signup-complete event. I would expect this to be triggered via a flow unmount effect (where we equally observe for some "success" flag) or to actually mark a step as the "signup completion" step (so any step can be used for closing). WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same as well! But having found the
recordSignupComplete
I doubted that maybe there was some big decision behind this while I was not involved in Stepper work.Moreover,
useSignUpStartTracking
is in FlowRenderer. Logically, it makes sense to have the signup complete at a framework, and not step, level too. But I wonder if that's overcomplicated. It would be better to open an issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely something for a follow-up issue. It does make sense to have this near/in
useSignUpStartTracking
. It will probably lead to more holistic refactor, but we want these things to be perfect! :D