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

Stepper: calypso_signup_complete as in Start #94706

Merged
merged 5 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ const ProcessingStep: React.FC< ProcessingStepProps > = function ( props ) {
if ( availableFlows[ flow ] ) {
availableFlows[ flow ]().then( ( flowExport ) => {
if ( flowExport.default.isSignupFlow ) {
recordSignupComplete();
recordSignupComplete( { ...destinationState } );
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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

}
} );
}
Expand Down
115 changes: 64 additions & 51 deletions client/landing/stepper/hooks/use-record-signup-complete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The 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 undefined unless carried along?

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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Sounds good (you obviously know more about these concepts)

Do you think that should be changed?

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 ]
);
};
Loading