Add Site data store to be used by Gutenboarding #39050

andrewserong · 2020-01-24T02:07:33Z

This PR adds a Site data store, responsible for making API calls to create a new site using the sites/new endpoint. It follows the same basic pattern as the User store added in #38823.

Changes proposed in this Pull Request

Add a Site data store to fire off requests to create a new site via the sites/new endpoint
Store information about the created site
Support passing in a bearer token to create the site (allowing site creation after a new user is created but before they're logged in / before they've reloaded the page)

Testing instructions

Make sure you're in an incognito browser / you are not already logged in to WP.com. Also ensure that you do not reload the page while doing the following steps, to test that it works without requiring a page reload.

Create a new user at: http://calypso.localhost:3000/gutenboarding/#
Open up the console and enter the following, one by one: ( replace blog_name with a unique name for the blog's subdomain )

const { bearerToken } = wp.data.select( 'automattic/user' ).getNewUser();
console.log( wp.data.dispatch( 'automattic/site' ).createSite( { blog_name: 'myfakesite202001241229', authToken: bearerToken } ) );
console.log( wp.data.select( 'automattic/site' ).getNewSite() );

Related to: #38707

matticbot · 2020-01-24T02:07:35Z

Test live: https://calypso.live/?branch=add/site-data-store

ramonjd · 2020-01-24T02:17:02Z

packages/data-stores/src/site/controls.ts

+				apiVersion: '1.1',
+				method: 'post',
+				body: mergedParams,
+				token: authToken || undefined,


I'm not sure this is needed right now. It's passed to wpcomUndocumented if config.isEnabled( 'oauth' ), but I think it's related to the OAuth login page.

See: https://github.com/Automattic/wp-calypso/blob/master/client/lib/oauth-store/README.md

I think we definitely won't need it thanks to your reloadWpcomProxy PR in #39049! But I used token here just because I found the key in the wpcomFactory here: https://github.com/automattic/wp-calypso/blob/2de5e432fc90d3793d8bac4f8aa3e7a7dfb5432c/client/lib/wpcom-undocumented/index.js#L41 and it seemed to get it working for me in testing 🤷‍♂

But I think your idea is much better, and will allow the site creation to be properly separate from User creation (so we don't have to go passing around bearer tokens).

Good to know. 👍

matticbot · 2020-01-24T02:22:08Z

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.

packages/data-stores/src/site/controls.ts

ramonjd · 2020-01-24T02:28:10Z

packages/data-stores/src/site/types.ts

+	blog_details: object;
+}
+
+export interface NewSiteErrorResponse {


Just a reminder: check the API for the varying error responses. I looked at the callback in the API code and yes, they vary :)

packages/data-stores/src/site/selectors.ts

ramonjd · 2020-01-24T02:32:20Z

packages/data-stores/src/site/reducer.ts

+	return state;
+};
+
+const isFetchingNewUser: Reducer<


isFetchingSite

I haven't quite worked out how isResolving works, and whether we need to pass in the last known params, e.g.,

wp-calypso/packages/data-stores/src/domain-suggestions/selectors.ts

Lines 36 to 38 in d4ac5e4

return select( 'core/data' ).isResolving( STORE_KEY, '__internalGetDomainSuggestions', [

normalizedQuery,

] );

Maybe @sirreal could school us on that magic

That's the gist as I recall. You pass the store key, the resolver name, and an array of the arguments you passed to the resolver. More than the last known params, consider that the same resolver may be resolving different requests, or the same resolver may be resolving the same request multiple times. The params serve to the same resolvers differentiate between the same and different resolution.

ramonjd · 2020-01-24T02:32:42Z

packages/data-stores/src/site/types.ts

+	RECEIVE_NEW_SITE = 'RECEIVE_NEW_SITE',
+	RECEIVE_NEW_SITE_SUCCESS = 'RECEIVE_NEW_SITE',
+	RECEIVE_NEW_SITE_FAILED = 'RECEIVE_NEW_SITE_FAILED',
+	FETCH_NEW_SITE = 'FETCH_NEW_SITE',


What's the difference between FETCH_ and CREATE_?

I'm just wondering, mostly out of ignorance, why we'd need fetchNewSite() at all when we have the createSite() action.

Is for the isFetching selector?

Thanks, that's a good question! It's (currently) just used for the isFetching selector so we can manage a loading state in the UI. I'm not fully certain of how @wordpress/data deals with controls, but from Redux DevTools it looks like the CREATE_SITE action never hits the reducer. Taking a walk through the redux-routine code in Gutenberg, it looks to me like for this Redux middleware, if it encounters an action type for which we have a control (CREATE_SITE), then the control is called, but the action isn't then dispatched as well.

So I think this means that the middleware effectively blocks the action at that point to perform whatever we want it to do in the control (in this case make an API call), but it doesn't hit the reducer. If it encounters actions for which we don't have a matching control, they're dispatched as normal. To set a fetching state, we then need an additional action for that (which I've called here FETCH_NEW_SITE but maybe it needs to be a bit more descriptive, because "fetch" sounds like we're retrieving something rather than submitting a POST request).

Unless of course there's a solution in using isResolving instead for the isFetching state. I haven't quite worked out how isResolving works, either, so I'm curious to learn about that, too!

And of course please let me know if I'm way off on my understanding of this! 😄

andrewserong · 2020-01-24T05:19:05Z

Thanks for reviewing @ramonjd — looking back over my typos there, I'm very glad I added "hacky" to the title of this PR!

andrewserong · 2020-01-24T06:44:08Z

I'll likely be pausing work on this PR over the course of the next week (or only dipping in occasionally), so here's a to-do list to ensure I don't forget:

Use reloadProxy after a user account has been created (introduced in [Gutenboarding]: Add reloadProxy to our wpcom wrapper #39049) so that passing in token isn't required for creating an account. (Though maybe it's nice to keep the functionality?)
Clean up reducers, assuming Gutenboarding: data-store reducer function signatures don't need to list which actions they handle #38932 gets merged
The generic controls and refactorings in Data-stores: Generic WpcomProxyControls #38959 become even more valuable with another store added to data-stores, so it might be worth revisiting a bit more refactoring before getting this PR to a mergeable state
Remove hard-coded params passed in to the new site call, allowing data to be provided by the consuming code (Gutenboarding) as in Ramon's comment: Add Site data store to be used by Gutenboarding #39050 (comment)

sirreal · 2020-01-29T12:34:33Z

packages/data-stores/src/site/controls.ts

+				{ ...providedParams },
+				{ validate: false } // Set to false because account validation should be a separate action
+			);
+			const newUser = await wpcomRequest( {


Not newUser, this is for sites 🙂

If we're not processing the response here, we should be able to just return the promise. An async function is going to return a promise anyways, so unwrapping to return the result wrapped in a promise should be the same as return wpcomRequest( /* … */ )

sirreal · 2020-01-29T12:40:04Z

packages/data-stores/src/site/reducer.ts

+	return state;
+};
+
+const isFetchingSite: Reducer<


I'd discourage this manual request tracking. We should be able to find core-provided functionality for this.

The way I understand it so far (very limited), is that isResolving is a method limited to resolvers (as side-effect for selectors).

At the moment we've designed createSite as an action, and not a selector.

Intuitively this seems correct as we are making changes and not retrieving data.

Do you think there's an elegant way to go about checking for fetching state other than updating the state in the reducer?

Middleware using redux-routine maybe?

@andrewserong I think we could park the isFetching debate until we work out the best way to do it.

We'll definitely need some sort of isRequesting flag, particularly in light of #39268

For now, we could probably open this PR up for proper review and get it merged.

After that we can decide how to integrate it into Frankenflow.

👍

andrewserong · 2020-02-05T07:45:27Z

Thanks for cleaning up my various hacks @ramonjd! I've switched this over to ready for review to get some more input, but I think we're pretty close to a (hopefully) minimally mergeable version of this, and we can continue to tweak as we go. I did a quick test of creating a site with an existing user, and it's working correctly. I bumped into an AuthorizationRequiredError when attempting to create a site via token on a newly created user. Not quite sure what's going on there, but I'll dig into it further tomorrow!

andrewserong · 2020-02-06T07:08:36Z

I bumped into an AuthorizationRequiredError when attempting to create a site via token on a newly created user.

This is working again now. Just had to move the token key out of the body object and be top-level in the object passed to wpcomRequest, which I've pushed in a84f832. Ready for another review.

ramonjd · 2020-02-06T07:25:09Z

Just had to move the token key out of the body object and be top-level in the object passed to wpcomRequest,

That was probably my bad. Thanks for fixing.

… anyways, so unwrapping to return the result wrapped in a promise should be the same as return wpcomRequest( /* … */ ) See comment: https://github.com/Automattic/wp-calypso/pull/39050/files#r372356504

…token` as an optional property

…ew endpoint :)

…auth for /sites/new call

andrewserong · 2020-02-06T21:59:12Z

Rebased.

ramonjd

Works! We're not pulling this in anywhere yet, so I think it's safe to merge.

We can optimize later as mentioned in the PR's comments.

Thank you @andrewserong !

ramonjd reviewed Jan 24, 2020

View reviewed changes

packages/data-stores/src/site/controls.ts Show resolved Hide resolved

ramonjd reviewed Jan 24, 2020

View reviewed changes

packages/data-stores/src/site/selectors.ts Outdated Show resolved Hide resolved

ramonjd reviewed Jan 24, 2020

View reviewed changes

ramonjd mentioned this pull request Jan 24, 2020

[Gutenboarding]: Frontend site creation request/response handling #38707

Closed

andrewserong self-assigned this Jan 24, 2020

sirreal reviewed Jan 29, 2020

View reviewed changes

ramonjd force-pushed the add/site-data-store branch from bec0789 to 41e0217 Compare February 4, 2020 02:29

andrewserong changed the title ~~[WIP] Add Site data store and hacky initial implementation~~ Add Site data store to be used by Gutenboarding Feb 5, 2020

andrewserong marked this pull request as ready for review February 5, 2020 07:41

andrewserong requested a review from a team as a code owner February 5, 2020 07:41

andrewserong requested a review from a team February 5, 2020 07:42

andrewserong added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 5, 2020

sirreal requested review from jeraldjuice and Addison-Stavlo February 5, 2020 10:44

sirreal mentioned this pull request Feb 5, 2020

Gutenboarding - Add editor loading placeholder #39266

Merged

andrewserong and others added 5 commits February 7, 2020 08:57

Add Site data store and hacky initial implementation

e3ad1df

Fix typos

d1ac82b

Fixed some typos (carried on from the copy paste from User). Adding `…

8ae92d7

…token` as an optional property

Updated type properties based on the varying responses of the sites n…

e90a13d

…ew endpoint :)

Move token key out of body object of wpcomRequest to fix token-based …

34482a0

…auth for /sites/new call

andrewserong force-pushed the add/site-data-store branch from a84f832 to 34482a0 Compare February 6, 2020 21:58

ramonjd added the [Goal] New Onboarding previously called Gutenboarding label Feb 7, 2020

ramonjd approved these changes Feb 10, 2020

View reviewed changes

andrewserong merged commit cd878e1 into master Feb 10, 2020

andrewserong deleted the add/site-data-store branch February 10, 2020 02:47

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 10, 2020

razvanpapadopol mentioned this pull request Feb 11, 2020

Gutenboarding: use Site data-store to create site #39391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Site data store to be used by Gutenboarding #39050

Add Site data store to be used by Gutenboarding #39050

andrewserong commented Jan 24, 2020 •

edited

Loading

matticbot commented Jan 24, 2020

ramonjd Jan 24, 2020

andrewserong Jan 24, 2020

ramonjd Jan 24, 2020 •

edited

Loading

matticbot commented Jan 24, 2020

ramonjd Jan 24, 2020

ramonjd Jan 24, 2020

ramonjd Jan 24, 2020 •

edited by sirreal

Loading

sirreal Jan 24, 2020

ramonjd Jan 24, 2020

andrewserong Jan 24, 2020

andrewserong commented Jan 24, 2020

andrewserong commented Jan 24, 2020

sirreal Jan 29, 2020

sirreal Jan 29, 2020

ramonjd Feb 4, 2020 •

edited

Loading

ramonjd Feb 5, 2020

andrewserong commented Feb 5, 2020

andrewserong commented Feb 6, 2020

ramonjd commented Feb 6, 2020

andrewserong commented Feb 6, 2020

ramonjd left a comment

	return select( 'core/data' ).isResolving( STORE_KEY, '__internalGetDomainSuggestions', [
	normalizedQuery,
	] );

Add Site data store to be used by Gutenboarding #39050

Add Site data store to be used by Gutenboarding #39050

Conversation

andrewserong commented Jan 24, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

matticbot commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Jan 24, 2020 • edited by sirreal Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewserong commented Jan 24, 2020

andrewserong commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewserong commented Feb 5, 2020

andrewserong commented Feb 6, 2020

ramonjd commented Feb 6, 2020

andrewserong commented Feb 6, 2020

ramonjd left a comment

Choose a reason for hiding this comment

andrewserong commented Jan 24, 2020 •

edited

Loading

ramonjd Jan 24, 2020 •

edited

Loading

ramonjd Jan 24, 2020 •

edited by sirreal

Loading

ramonjd Feb 4, 2020 •

edited

Loading