Improve login redirects and step tracking in stepper #91241

daledupreez · 2024-05-29T12:06:41Z

Related to:

Fixes #91243

Proposed Changes

This PR introduces some key concepts into the core Stepper framework to allow us to better handle logged out users and the redirects that stem from them. The key changes are as follows:
- Stepper steps now have an optional requiresLoggedInUser flag that should be specified in flow.useSteps()
  - The PR includes stepWithRequiredLogin() and stepsWithRequiredLogin() helpers to set this flag easily
- The pre-existing StepRoute component now checks whether the current step has a truthy requiresLoggedInUser value, and will not render any content if the user is also not logged in, as well as triggering a redirect to log the user in.
  - The login redirect relies on a new useLoginUrlForFlow() hook that builds on my explorations in Explore: new useFlowLoginUrl() hook for Stepper flows #90711
- The logic to trigger calypso_signup_step_start Tracks events has been moved from FlowRenderer to StepRoute, and we won't trigger the Tracks event if the StepRoute logic from above performs a login redirect for a step that requires a login

Why are these changes being made?

We've been having internal discussions around improvements to the way Stepper handles Tracks and logins, and one of the key items that was flagged is that the current implementation can send calypso_signup_step_start Tracks events even when the user doesn't see the step. This PR attempts to resolve this by "knowing" whether a specific step requires the user to be logged in, and performing a redirect when necessary, as well as skipping rendering and triggering a Tracks event when we know a redirect should occur.
- For now, the new skip logic only occurs when the step is "opted in" to the new requirement, but it should be relatively easy to roll out the behaviour on a flow-by-flow basis.

Testing Instructions

Run this branch locally or via Calypso.live
Open a window where you are not logged in
Ensure you have some way of seeing Tracks events
Navigate to any of the URLs at or below /setup/site-migration, including:
- /setup/site-migration/
- /setup/site-migration/site-migration-identify
- /setup/site-migration/site-migration-import-or-migrate
- /setup/site-migration/site-migration-upgrade-plan
- /setup/site-migration/site-migration-instructions-i2
Verify that you're redirected to the login page, and the redirect URL will return you to your original location. Also verify that no calypso_signup_step_start Tracks event was recorded for the site-migration flow.
Add URL parameters to the URLs above, and verify that you are still redirected to login and these URL parameters are included in the redirect URL used by the login page. You should still see no calypso_signup_step_start Tracks events for the site-migration flow.
Specify a valid locale URL parameter for some of the above URLs, and verify that you're redirected to the right login page with the locale in the path and the redirect URL for the login page includes the relevant non-English locale. You should still see no calypso_signup_step_start Tracks events for the site-migration flow.
In a second window where you are logged in, work through the same set of URLs above and verify that you're seeing the calypso_signup_step_start event being fired correctly for the site-migration flow
In the same logged-in window, work through some other flows and verify that you're seeing the calypso_signup_step_start event being fired correctly

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-05-29T12:06:52Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-109906

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-109906&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-109906&env=a8c-for-agencies

matticbot · 2024-05-29T12:08:13Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~588 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
entry-stepper      +2330 B  (+0.1%)     +588 B  (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~11655 bytes removed 📉 [gzipped])

name                           parsed_size            gzip_size
site-migration-flow                -2026 B   (-3.8%)     -694 B   (-7.3%)
hosted-site-migration-flow         -2026 B   (-3.8%)     -695 B   (-7.3%)
assembler-first-flow               -1494 B   (-2.0%)     -503 B   (-3.3%)
ai-assembler-flow                  -1494 B   (-2.0%)     -499 B   (-3.2%)
blog                               -1477 B  (-47.5%)     -557 B  (-40.4%)
tailored-ecommerce-flow            -1468 B  (-13.3%)     -514 B  (-14.8%)
start-writing-flow                 -1468 B   (-5.8%)     -470 B   (-7.3%)
migration-signup                   -1468 B   (-2.9%)     -487 B   (-5.7%)
free-flow                          -1468 B   (-2.7%)     -497 B   (-5.1%)
entrepreneur-flow                  -1468 B   (-1.3%)     -610 B   (-2.0%)
domain-user-transfer-flow          -1468 B   (-0.6%)     -486 B   (-1.0%)
design-first-flow                  -1468 B   (-5.6%)     -461 B   (-7.0%)
connect-domain                     -1468 B   (-5.2%)     -501 B   (-6.5%)
write-flow                         -1459 B   (-0.2%)     -451 B   (-0.2%)
reblogging-flow                    -1242 B  (-14.5%)     -454 B  (-16.1%)
newsletter-flow                    -1242 B   (-5.7%)     -422 B   (-6.9%)
new-hosted-site-flow               -1242 B  (-15.9%)     -428 B  (-16.2%)
link-in-bio-flow-domain            -1242 B   (-4.4%)     -456 B   (-6.0%)
link-in-bio-flow                   -1242 B   (-6.0%)     -415 B   (-7.0%)
import-hosted-site-flow            -1242 B   (-0.1%)     -346 B   (-0.1%)
hundred-year-plan                  -1242 B   (-3.2%)     -394 B   (-3.5%)
google-transfer                    -1242 B  (-19.6%)     -465 B  (-20.9%)
domain-transfer                    -1242 B  (-20.5%)     -466 B  (-21.3%)
link-in-bio-tld-flow               -1233 B   (-0.1%)     -373 B   (-0.1%)
trial-wooexpress-flow               -217 B   (-0.5%)      -65 B   (-0.9%)
transferring-hosted-site-flow        +18 B   (+0.0%)      +19 B   (+0.1%)
plugin-bundle-flow                   +18 B   (+0.0%)      +16 B   (+0.0%)
copy-site-flow                       +18 B   (+0.0%)      +15 B   (+0.0%)
build-flow                           +18 B   (+0.0%)      +19 B   (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

gabrielcaires · 2024-05-29T17:36:39Z

@daledupreez I started adding tests to your change and found a scenario I want to confirm.

Should we skip the tracking when the renderStep returns a null content?

I implemented this scenario on this commit 433160a

gabrielcaires · 2024-05-29T17:54:28Z

@daledupreez I committed this temporary change 355cce9 with some ideas to clean up the code based on this change.

alshakero

I can't tell you how much I appreciate this. Thank you so so much sir. Left a few comments here and there.

client/landing/stepper/declarative-flow/helpers/get-step-old-slug.ts

client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx

client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx

client/landing/stepper/declarative-flow/internals/index.tsx

alshakero · 2024-05-29T19:21:04Z

client/landing/stepper/declarative-flow/internals/types.ts

+	/**
+	 * Does the step require a logged-in user?
+	 */
+	requiresLoggedInUser?: boolean;


We should make a new type entirely that requires (requiresLoggedInUser) and deprecate the existing ones, so people get deprecation warnings from TS and the flows would organically upgrade to the new type.

I do something like that here.

I intentionally left this as optional for now, mostly because the login requirement is actually flow-specific. So I don't think we should define the login requirement as part of the core step definition. (More on that below.)

You're right. Maybe it should be requiresLoggedInUser?: true?

alshakero · 2024-05-29T19:32:17Z

client/landing/stepper/hooks/use-login-url-for-flow.ts

+	const redirectTo = addQueryArgs( location.pathname, {
+		...( locale && locale !== 'en' ? { locale } : {} ),
+		...( siteId ? { siteId } : {} ),
+		...( siteSlug ? { siteSlug } : {} ),
+		...Object.fromEntries( new URLSearchParams( location.search ).entries() ),
+	} );


100% a nit, but I would spread this into 4 ifs

Suggested change

const redirectTo = addQueryArgs( location.pathname, {

...( locale && locale !== 'en' ? { locale } : {} ),

...( siteId ? { siteId } : {} ),

...( siteSlug ? { siteSlug } : {} ),

...Object.fromEntries( new URLSearchParams( location.search ).entries() ),

} );

let redirectTo = location.pathname;

if ( locale && locale !== 'en' ) {

redirectTo = addQueryArgs( location.href, { locale } );

}

if ( siteId ) {

redirectTo = addQueryArgs( location.href, { siteId } );

}

if ( siteSlug ) {

redirectTo = addQueryArgs( location.href, { siteSlug } );

}

if ( siteSlug ) {

redirectTo = addQueryArgs( location.href, { siteSlug } );

}

And I think if we use location.href (vs pathname) we don't need to consider the pre-existing params manually.

client/landing/stepper/declarative-flow/site-migration-flow.ts

client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx

client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx

client/landing/stepper/declarative-flow/internals/index.tsx

client/landing/stepper/declarative-flow/site-migration-flow.ts

client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx

client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx

daledupreez

I have some feedback from inspection. I'll start some testing shortly.

client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx

client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx

client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx

client/landing/stepper/declarative-flow/internals/index.tsx

gabrielcaires · 2024-05-31T13:29:41Z

@daledupreez I created a second PR, based on this on just to address the renderStep issue. #91345

Co-authored-by: daledupreez <dale@automattic.com>

…tep-route/test/index.tsx Co-authored-by: daledupreez <dale@automattic.com>

client/landing/stepper/declarative-flow/site-migration-flow.ts

daledupreez · 2024-05-31T14:28:15Z

client/landing/stepper/declarative-flow/site-migration-flow.ts

 		if ( ! siteSlug && ! siteId && ! isHostedSiteMigrationFlow( this.variantSlug ?? FLOW_NAME ) ) {
 			window.location.assign( '/start' );
-			result = {
+
+			return {
 				state: AssertConditionState.FAILURE,
 				message: 'site-setup did not provide the site slug or site id it is configured to.',
 			};
 		}


I am wondering if we have two issues in this code block:

This should be inside useEffect()

Should this code also be checking for whether we have a logged-in user?

As a follow-up, I think this is another situation where we had an implicit dependency on the login redirect logic, which we've now removed.

Fix pushed in 4614bf4.

…ng-in-stepper

Dismissing to request a new one

alshakero · 2024-06-04T20:18:44Z

It seems the /setup/site-migration flow is redirecting me to /start. Am I missing something?

gabrielcaires · 2024-06-05T09:41:32Z

It seems the /setup/site-migration flow is redirecting me to /start. Am I missing something?

@alshakero
You can't access this flow directly without having a site defined &siteId=[SiteId], &siteSlug='someSlug'

daledupreez

@gabrielcaires, I have re-tested, and I think we are OK to get this shipped. I can't approve as the original PR author, but let me know if you have any concerns about shipping this as-is.

daledupreez added 2 commits May 29, 2024 13:32

Initial exploration: step Tracks and login checks in StepRoute

ab2c684

Add helpers to add requiresLoggedInUser: true

2186fdd

daledupreez requested review from gabrielcaires, travisw, MicBosi, alshakero and skim1220 May 29, 2024 12:06

daledupreez self-assigned this May 29, 2024

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 29, 2024

daledupreez requested a review from ddc22 May 29, 2024 12:15

gabrielcaires added 3 commits May 29, 2024 17:13

Use hooks to simplify the test setup

6b4644c

Add tests to step-route

4c49369

Add temporary rerenter step test

1615fc6

gabrielcaires self-assigned this May 29, 2024

gabrielcaires added 2 commits May 29, 2024 17:56

Add tests to cover re-entering case

9a1886e

Skip tracking when the step content is not rendered

433160a

Add temporary todo with improvement ideas

f07f3e2

gabrielcaires force-pushed the try/improve-login-redirects-and-step-tracking-in-stepper branch from 355cce9 to f07f3e2 Compare May 29, 2024 17:56

alshakero previously requested changes May 29, 2024

View reviewed changes

daledupreez commented May 29, 2024

View reviewed changes

client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx Outdated Show resolved Hide resolved

client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx Outdated Show resolved Hide resolved

daledupreez commented May 31, 2024

View reviewed changes

gabrielcaires and others added 4 commits May 31, 2024 14:30

Update client/landing/stepper/declarative-flow/internals/index.tsx

3f8f202

Co-authored-by: daledupreez <dale@automattic.com>

Update client/landing/stepper/declarative-flow/internals/components/s…

8b6a28c

…tep-route/test/index.tsx Co-authored-by: daledupreez <dale@automattic.com>

Remove extra comments

e576a62

Improve constant names on test file

d3aec1b

daledupreez commented May 31, 2024

View reviewed changes

client/landing/stepper/declarative-flow/site-migration-flow.ts Outdated Show resolved Hide resolved

gabrielcaires added 2 commits May 31, 2024 14:39

Add missing type keyword

5957f4e

Check user is logged in before redirect to start

7d080c5

gabrielcaires mentioned this pull request May 31, 2024

Stepper: Remove renderStep dependecy from step router #91345

Draft

7 tasks

daledupreez marked this pull request as ready for review May 31, 2024 14:15

daledupreez commented May 31, 2024

View reviewed changes

daledupreez changed the title ~~[WIP] Improve login redirects and step tracking in stepper~~ Improve login redirects and step tracking in stepper Jun 3, 2024

daledupreez and others added 2 commits June 3, 2024 17:33

Update redirection for missing site details

4614bf4

Merge branch 'trunk' into try/improve-login-redirects-and-step-tracki…

4e46d6a

…ng-in-stepper

gabrielcaires requested review from gabrielcaires and removed request for gabrielcaires June 4, 2024 17:04

daledupreez commented Jun 5, 2024

View reviewed changes

gabrielcaires approved these changes Jun 5, 2024

View reviewed changes

gabrielcaires merged commit 7fdd454 into trunk Jun 6, 2024
12 checks passed

gabrielcaires deleted the try/improve-login-redirects-and-step-tracking-in-stepper branch June 6, 2024 08:31

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 6, 2024

daledupreez mentioned this pull request Jun 7, 2024

Add Stepper framework login handling to ecommerce flow #91612

Merged

4 tasks

This was referenced Jun 25, 2024

Explore: new useFlowLoginUrl() hook for Stepper flows #90711

Closed

[WIP] Try to rethink Stepper and logins #91074

Closed

[WIP] Explore adding safety checks for Stepper signup flows #90323

Closed

gabrielcaires mentioned this pull request Jul 2, 2024

Stepper: Roll out explicit login requirements to all Stepper flows #92291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve login redirects and step tracking in stepper #91241

Improve login redirects and step tracking in stepper #91241

daledupreez commented May 29, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited by matticbot

Loading

matticbot commented May 29, 2024 •

edited

Loading

gabrielcaires commented May 29, 2024 •

edited

Loading

gabrielcaires commented May 29, 2024

alshakero left a comment

alshakero May 29, 2024

daledupreez May 29, 2024

alshakero May 29, 2024

alshakero May 29, 2024 •

edited

Loading

daledupreez left a comment

gabrielcaires commented May 31, 2024

daledupreez May 31, 2024

daledupreez Jun 3, 2024

daledupreez Jun 3, 2024

alshakero commented Jun 4, 2024 •

edited

Loading

gabrielcaires commented Jun 5, 2024 •

edited

Loading

daledupreez left a comment

-	const redirectTo = addQueryArgs( location.pathname, {
-		...( locale && locale !== 'en' ? { locale } : {} ),
-		...( siteId ? { siteId } : {} ),
-		...( siteSlug ? { siteSlug } : {} ),
-		...Object.fromEntries( new URLSearchParams( location.search ).entries() ),
-	} );
+       let redirectTo = location.pathname;
+       if ( locale && locale !== 'en' ) {
+           redirectTo = addQueryArgs( location.href, { locale } );
+       }
+       if ( siteId ) {
+           redirectTo = addQueryArgs( location.href, { siteId } );
+       }
+       if ( siteSlug ) {
+       	    redirectTo = addQueryArgs( location.href, { siteSlug } );
+       }
+       if ( siteSlug ) {
+       	    redirectTo = addQueryArgs( location.href, { siteSlug } );
+       }

Improve login redirects and step tracking in stepper #91241

Improve login redirects and step tracking in stepper #91241

Conversation

daledupreez commented May 29, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented May 29, 2024 • edited by matticbot Loading

matticbot commented May 29, 2024 • edited Loading

gabrielcaires commented May 29, 2024 • edited Loading

gabrielcaires commented May 29, 2024

alshakero left a comment

Choose a reason for hiding this comment

alshakero May 29, 2024

Choose a reason for hiding this comment

daledupreez May 29, 2024

Choose a reason for hiding this comment

alshakero May 29, 2024

Choose a reason for hiding this comment

alshakero May 29, 2024 • edited Loading

Choose a reason for hiding this comment

daledupreez left a comment

Choose a reason for hiding this comment

gabrielcaires commented May 31, 2024

daledupreez May 31, 2024

Choose a reason for hiding this comment

daledupreez Jun 3, 2024

Choose a reason for hiding this comment

daledupreez Jun 3, 2024

Choose a reason for hiding this comment

alshakero commented Jun 4, 2024 • edited Loading

gabrielcaires commented Jun 5, 2024 • edited Loading

daledupreez left a comment

Choose a reason for hiding this comment

daledupreez commented May 29, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited by matticbot

Loading

matticbot commented May 29, 2024 •

edited

Loading

gabrielcaires commented May 29, 2024 •

edited

Loading

alshakero May 29, 2024 •

edited

Loading

alshakero commented Jun 4, 2024 •

edited

Loading

gabrielcaires commented Jun 5, 2024 •

edited

Loading