Create pending sticker when loading the "how to migrate" step #95413

renatho · 2024-10-15T20:04:37Z

Related to #95288

Proposed Changes

Create the sticker when loading the "How to migrate" step.
Extract hook to simplify step code.

Why are these changes being made?

We want to register the migration-pending when the user is navigating through the migration flow. See more in p1728684758970459/1728480467.656839-slack-C07N53UNUQJ.

Testing Instructions

To check the status creation, you check the network tab of your browser filtering by /site-migration-status-sticker. To make sure the status was created, you can check it directly in your sandbox (p1729023356708799/1728480467.656839-slack-C07N53UNUQJ).

Test the following steps in /setup/migration and in the /setup/hosted-site-migration:

Navigate to the "How to migrate" step.
Check that it register the pending status.
Click on the options, and make sure it continues registering the pending status for the "DIY" and "DIFM".

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-10-15T20:04:49Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-10-15T20:15:23Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Imran92

The change looks very well done! Just for good measure, should we cover this particular behavior with a unit test?

m1r0 · 2024-10-21T14:31:53Z

I'm not sure if it's related to these changes, but when I select "I'll do it myself", I end up with 2 migration stickers:

It looks like 2 requests are sent at the same time when the button is clicked. Is this a bug?

renatho · 2024-10-21T18:23:09Z

I'm not sure if it's related to these changes, but when I select "I'll do it myself", I end up with 2 migration stickers:

It looks like 2 requests are sent at the same time when the button is clicked. Is this a bug?

Hi @m1r0! It's a bug. Since it's not related to this PR, I just created another issue to handle it: #95560

renatho · 2024-10-21T19:28:34Z

The change looks very well done! Just for good measure, should we cover this particular behavior with a unit test?

Added some tests here: 970ce0c

...low/internals/steps-repository/site-migration-how-to-migrate/use-pending-migration-status.ts

Co-authored-by: Gabriel Caires <[email protected]>

matticbot · 2024-10-23T14:59:56Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/migration-pending-status on your sandbox.

m1r0

Looks good. I found no issues while testing. 🚀

valterlorran · 2024-10-31T13:43:38Z

...low/internals/steps-repository/site-migration-how-to-migrate/use-pending-migration-status.ts

+const usePendingMigrationStatus = ( { onSubmit }: PendingMigrationStatusProps ) => {
+	const site = useSite();
+	const siteId = site?.ID;
+
+	const canInstallPlugins = site?.plan?.features?.active.find(
+		( feature ) => feature === 'install-plugins'
+	)
+		? true
+		: false;
+
+	const { updateMigrationStatus } = useUpdateMigrationStatus();
+
+	// Register pending migration status when loading the step.
+	useEffect( () => {
+		if ( siteId ) {
+			updateMigrationStatus( siteId, 'migration-pending' );
+		}
+	}, [ siteId, updateMigrationStatus ] );
+
+	const setPendingMigration = ( how: string ) => {
+		const destination = canInstallPlugins ? 'migrate' : 'upgrade';
+		if ( siteId ) {
+			const parsedHow = how === HOW_TO_MIGRATE_OPTIONS.DO_IT_MYSELF ? 'diy' : how;
+			updateMigrationStatus( siteId, `migration-pending-${ parsedHow }` );
+		}
+
+		if ( onSubmit ) {
+			return onSubmit( { how, destination } );
+		}
+	};
+
+	return { setPendingMigration };
+};


Hey folks! I stumbled upon this code today; I got really confused. I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook. Can you enlighten me on the reasoning for this? Even for me, who worked on this, I had difficulties finding where the status was being updated.

Especially since the hook is strictly tightened with the How to migrate step, as it won't work anywhere else.

I also disagree that the action on the How to migrate step is to "set a migration as pending", and that's the idea that it we give when we do:

onClick={ () => setPendingMigration( option.value ) }

For me, setting the pending state is a side effect of the submit action and not the other way around. I also feel that for future maintenance, removing the submit logic from the How to migrate step file just makes it harder to understand the code. We added a new layer for the onSubmit event, which by itself is already deep enough.

I agree that it seems not necessary to have the submit inside the usePendingMigrationStatus`

Hey folks! I stumbled upon this code today; I got really confused. I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook.

Another point is that we are not waiting for the request to be finished before calling the onSubmit.

I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook.

I am ok with the extraction because it removes business logic from the original component and prevents some components from re-rendering. For example, useEffects inside hooks don't trigger component rendering.

But I am unaware of why we need to use 2 states to set migration as pending.

But I am unaware of why we need to use 2 states to set migration as pending.

I think the first one is to flag when the user gets to the How to migrate step so we can redirect them back to that stage. The migration-pending-{difm/diy} means that the user has already submitted the intent.

For example, useEffects inside hooks don't trigger component rendering.

In this case, would it trigger the render? I asked this because we are not setting any states that would justify that.

I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook

The idea was to extract the portion of logic related to the sticker creation in the how to migrate. Otherwise, it would introduce a big complexity to the SiteMigrationHowToMigrate, which already has many other logic too. I thought having it in the same folder as the component would be enough, but maybe we could try to rename the hook to something else to avoid the confusion? I tried to think something to add the HowToMigrate in the name, but my ideas seem too verbose. Something like usePendingMigrationStatusHandlerForHowToMigrateStep?

I also disagree that the action on the How to migrate step is to "set a migration as pending", and that's the idea that it we give when we do:

onClick={ () => setPendingMigration( option.value ) }

For me, setting the pending state is a side effect of the submit action and not the other way around. I also feel that for future maintenance, removing the submit logic from the How to migrate step file just makes it harder to understand the code. We added a new layer for the onSubmit event, which by itself is already deep enough.

Hm! Looking at this again, I totally agree with you. Maybe we could refator it removing the onSubmit dependency from that and adding a clickHandler that will call the setPendingMigration and the navigation.submit?

Another point is that we are not waiting for the request to be finished before calling the onSubmit.

This is already being fixed as part of #95612
Also, notice that any refactor would be nice to do on top of this last PR to avoid conflicts. 👆 @m1r0, I think we could merge that, right?

I think the first one is to flag when the user gets to the How to migrate step so we can redirect them back to that stage. The migration-pending-{difm/diy} means that the user has already submitted the intent.

That's correct!

In this case, would it trigger the render? I asked this because we are not setting any states that would justify that.

That's less about it and more about the separation of concerns to simplify the component logic and avoid the components with hundreds of lines complex to maintain! 🤭

I think we could merge that, right?

Yep, I've merged it. 👍

Also, one important thing when moving the submit to the component, it needs to happen after the sticker is created. 😉

The #95612 does it fixing a race condition issue.

renatho self-assigned this Oct 15, 2024

renatho marked this pull request as ready for review October 15, 2024 20:17

renatho requested a review from a team October 15, 2024 20:17

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

renatho linked an issue Oct 15, 2024 that may be closed by this pull request

Post-migration experience: Add pending migration blog sticker #95288

Closed

renatho changed the title ~~Create pending sticker when loading the step~~ Create pending sticker when loading the "how to migrate" step Oct 15, 2024

renatho mentioned this pull request Oct 18, 2024

Replace overview when site is with a pending migration #95497

Merged

7 tasks

Imran92 reviewed Oct 21, 2024

View reviewed changes

renatho added 2 commits October 21, 2024 14:56

Create pending sticker when loading the step

5b7c3ec

Extract pending migration status hook

7023a65

renatho force-pushed the add/migration-pending-status branch from baff4fd to 7023a65 Compare October 21, 2024 17:56

renatho mentioned this pull request Oct 21, 2024

Multiple stickers being added simultaneously when navigating to the "DIY" option #95560

Closed

Add tests

970ce0c

renatho requested review from Imran92 and m1r0 October 21, 2024 19:59

gabrielcaires reviewed Oct 23, 2024

View reviewed changes

...low/internals/steps-repository/site-migration-how-to-migrate/use-pending-migration-status.ts Outdated Show resolved Hide resolved

Update function name to not be opinionated

4c81714

Co-authored-by: Gabriel Caires <[email protected]>

renatho force-pushed the add/migration-pending-status branch from 6cef19a to 4c81714 Compare October 23, 2024 14:51

m1r0 approved these changes Oct 25, 2024

View reviewed changes

renatho merged commit 2101023 into trunk Oct 25, 2024
11 checks passed

renatho deleted the add/migration-pending-status branch October 25, 2024 14:09

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 Oct 25, 2024

valterlorran reviewed Oct 31, 2024

View reviewed changes

m1r0 mentioned this pull request Oct 31, 2024

Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create pending sticker when loading the "how to migrate" step #95413

Create pending sticker when loading the "how to migrate" step #95413

renatho commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited by matticbot

Loading

matticbot commented Oct 15, 2024

Imran92 left a comment

m1r0 commented Oct 21, 2024

renatho commented Oct 21, 2024

renatho commented Oct 21, 2024

matticbot commented Oct 23, 2024

m1r0 left a comment •

edited

Loading

valterlorran Oct 31, 2024

gabrielcaires Oct 31, 2024 •

edited

Loading

valterlorran Oct 31, 2024

valterlorran Oct 31, 2024

renatho Oct 31, 2024

m1r0 Oct 31, 2024

renatho Oct 31, 2024

Create pending sticker when loading the "how to migrate" step #95413

Create pending sticker when loading the "how to migrate" step #95413

Conversation

renatho commented Oct 15, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Oct 15, 2024 • edited by matticbot Loading

matticbot commented Oct 15, 2024

Imran92 left a comment

Choose a reason for hiding this comment

m1r0 commented Oct 21, 2024

renatho commented Oct 21, 2024

renatho commented Oct 21, 2024

matticbot commented Oct 23, 2024

m1r0 left a comment • edited Loading

Choose a reason for hiding this comment

valterlorran Oct 31, 2024

Choose a reason for hiding this comment

gabrielcaires Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

valterlorran Oct 31, 2024

Choose a reason for hiding this comment

valterlorran Oct 31, 2024

Choose a reason for hiding this comment

renatho Oct 31, 2024

Choose a reason for hiding this comment

m1r0 Oct 31, 2024

Choose a reason for hiding this comment

renatho Oct 31, 2024

Choose a reason for hiding this comment

renatho commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited by matticbot

Loading

m1r0 left a comment •

edited

Loading

gabrielcaires Oct 31, 2024 •

edited

Loading