[chore?]: Use createCustomer gql mutation instead of create-account REST endpoint #1898

sirugh · 2019-10-16T16:28:15Z

Description

This change replaces the old, unused email validation function and the createAccount redux/rest action with usage of the createCustomer mutation.

Related Issue

Closes #1210.

Acceptance

Verification Steps

Go to /create-account and try to create an account with an email that is already used. You should see the new error.
Go to the create account view in left nav and do the same. You should see the error.
Now, in both views, try to create accounts with valid emails and it should succeed.

Screenshots / Screen Captures (if appropriate)

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

… endpoint

PWAStudioBot · 2019-10-16T16:32:23Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 71b461b

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js

… onSubmit handlers. Add local submitting state to disable button during submission

… error state better than using a specific isEmailAvailable query.

sirugh · 2019-10-17T19:12:24Z

packages/peregrine/lib/store/actions/user/asyncActions.js

@@ -78,31 +78,6 @@ export const getUserDetails = () =>
        }
    };

-export const createAccount = accountInfo => async dispatch => {


It didn't make sense that the CreateAccount page took a callback that was just a reference to this action. In all scenarios, it was pulled off context and passed in anyways. I can't imagine a scenario where we would want to change the action passed in to the component. So now, the useCreateAccount talon uses the createCustomer mutation (no more REST!) And components that want to use the CreateAccount form can pass it a "post submit" callback.

Bye Felicia!

sirugh · 2019-10-17T19:15:25Z

packages/peregrine/lib/talons/AuthModal/useAuthModal.js

-        [createAccount, showMyAccount]
-    );
+    const handleCreateAccount = useCallback(() => {
+        showMyAccount();


See how this callback no longer cares about the createAccount action. So clean!

sirugh · 2019-10-17T19:18:09Z

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js

+                await signIn({
+                    username: formValues.customer.email,
+                    password: formValues.password
+                });


This logic used to exist within the redux action but now lives in the talon!

packages/venia-ui/lib/queries/createAccount.graphql

supernova-at

Dang, code looks great!

Haven't run the verification steps yet

packages/venia-ui/lib/queries/createAccount.graphql

supernova-at · 2019-10-18T19:06:19Z

packages/venia-ui/lib/queries/createAccount.graphql

@@ -0,0 +1,19 @@
+mutation createAccount(


Since this is the first mutation, I'm wondering if we shouldn't create a lib/mutations/ folder. It's a bit weird to read the import:

import CREATE_ACCOUNT_MUTATION from '../../queries/createAccount.graphql';

A mutation is just a type of query. I don't think we should do this.

Left more detail below.

Maybe "a mutation is just a type of request"?

Queries and Mutations are separate, special types that serve as entry points for the respective operations.

GraphQL and the Magento GraphQL schema separates Queries from Mutations, here's GraphiQL:

Whether or not the front end splits these things up into queries and mutations directories is a discussion worth having, but GraphQL and our Magento GraphQL schema does separate them, so I assumed we would want to too, if only to reduce confusion when going back and forth.

Is it the worst thing in the world if a .graphql file in the queries/ directory contained addConfigurableProductsToCart? Probably not. 🤷‍♂

supernova-at · 2019-10-18T19:07:51Z

packages/venia-ui/lib/components/CreateAccount/createAccount.js


 const LEAD =
    'Check out faster, use multiple addresses, track orders and more by creating an account!';

 const CreateAccount = props => {
    const talonProps = useCreateAccount({
-        initialValues: props.initialValues
+        initialValues: props.initialValues,
+        query: CREATE_ACCOUNT_MUTATION,


Only because in GraphQL parlance "queries" and "mutations" are two distinctly separate things, this reads funny.

Yea -- I thought about changing the import name. I don't really think the UI needs to care about whether the query is a mutation/query/subscription.

I'd like to get @jimbo's take on naming here.

👍 pending @jimbo 's verdict 😄

Whether or not the front end splits these things up into queries and mutations directories is a discussion worth having, but GraphQL and our Magento GraphQL schema does separate them, so I assumed we would want to too, if only to reduce confusion when going back and forth.

I'm pretty convinced by @supernova-at's argument here. As long as we have a queries folder, it makes sense have a mutations folder, since they're separate entities.

Since this is the first mutation, I'm wondering if we shouldn't create a lib/mutations/ folder.

That said, this is a reminder that there is no good reason to have a queries folder in the first place. Queries (and mutations) are supposed to be highly tailored to the component using them; it's not considered an optimization to reuse a query, so there's no value in keeping them in a shared folder.

Ideally, queries and mutations would reside in the component folder alongside .js and .css component files. If we're not going to do that, a mutations directory is fine.

That said, this is a reminder that there is no good reason to have a queries folder in the first place.

I agree. I think we moved them into the queries/ folder maybe to make it easier on some tooling? I'm not sure.

How about this compromise - we merge this as is and I open an issue to move all the queries/mutations to the component folders where they are used.

Made #1928 :D

packages/venia-ui/lib/components/CreateAccount/createAccount.js

jimbo · 2019-10-22T16:08:25Z

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js

-    ] = useUserContext();
-    const hasError = !!createAccountError;
+    const [{ isSignedIn }, { signIn }] = useUserContext();
+    const [createAccount, { error }] = useMutation(query);


I'm so glad we didn't have mutations prior to official hooks. 😅

Validate email through graphql before submissission to create account…

7004853

… endpoint

sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Oct 16, 2019

sirugh added the enhancement New feature or request label Oct 16, 2019

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Oct 16, 2019

Merge branch 'develop' into rugh/gql-validate-email

1d2705a

supernova-at self-assigned this Oct 16, 2019

m2-community-project bot added the Progress: review label Oct 16, 2019

supernova-at reviewed Oct 16, 2019

View reviewed changes

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js Show resolved Hide resolved

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js Outdated Show resolved Hide resolved

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js Outdated Show resolved Hide resolved

m2-community-project bot assigned sirugh Oct 16, 2019

sirugh added 3 commits October 17, 2019 13:14

Moving actual createAccount call into the talon rather than passed in…

e5ec0b2

… onSubmit handlers. Add local submitting state to disable button during submission

Use createCustomer mutation instead of old rest API. Mutation returns…

929dc7b

… error state better than using a specific isEmailAvailable query.

Fix tests

474c65a

sirugh commented Oct 17, 2019

View reviewed changes

packages/venia-ui/lib/queries/createAccount.graphql Show resolved Hide resolved

sirugh added 2 commits October 17, 2019 14:22

add jsdoc

5636f1e

Remove old createAccount redux tests

21585ef

sirugh changed the title ~~Validate email through graphql before submissission to create account endpoint~~ [chore?]: Use createCustomer gql mutation instead of create-account REST endpoint Oct 17, 2019

sirugh added 2 commits October 17, 2019 14:36

fixup comments

7a8f799

Clean up old state from reducer

4544f8f

sirugh mentioned this pull request Oct 18, 2019

[chore?] Replace # REST with gql. #1904

Merged

supernova-at reviewed Oct 18, 2019

View reviewed changes

Merge branch 'develop' into rugh/gql-validate-email

df60e02

sirugh commented Oct 18, 2019

View reviewed changes

packages/venia-ui/lib/components/CreateAccount/createAccount.js Outdated Show resolved Hide resolved

Reverse error message accumulation

7868bad

Merge branch 'develop' into rugh/gql-validate-email

6d5b362

supernova-at previously approved these changes Oct 21, 2019

View reviewed changes

Add comment :)

a1e5b8e

sirugh dismissed supernova-at’s stale review via a1e5b8e October 22, 2019 15:06

m2-community-project bot assigned jimbo Oct 22, 2019

jimbo approved these changes Oct 22, 2019

View reviewed changes

sirugh mentioned this pull request Oct 22, 2019

[chore]: Move .graphql files from queries/ directory to the component directory where they are used #1928

Closed

Merge branch 'develop' into rugh/gql-validate-email

5179f5c

supernova-at approved these changes Oct 22, 2019

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Oct 22, 2019

Merge branch 'develop' into rugh/gql-validate-email

71b461b

m2-community-project bot removed the Progress: approved label Oct 22, 2019

dpatil-magento merged commit bcfaf66 into develop Oct 22, 2019

sirugh deleted the rugh/gql-validate-email branch March 4, 2020 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chore?]: Use createCustomer gql mutation instead of create-account REST endpoint #1898

[chore?]: Use createCustomer gql mutation instead of create-account REST endpoint #1898

sirugh commented Oct 16, 2019 •

edited

Loading

PWAStudioBot commented Oct 16, 2019 •

edited

Loading

sirugh Oct 17, 2019 •

edited

Loading

sirugh Oct 17, 2019

sirugh Oct 17, 2019

supernova-at left a comment

supernova-at Oct 18, 2019

sirugh Oct 18, 2019

sirugh Oct 18, 2019

brendanfalkowski Oct 18, 2019

supernova-at Oct 21, 2019 •

edited

Loading

supernova-at Oct 18, 2019

sirugh Oct 18, 2019

supernova-at Oct 21, 2019 •

edited

Loading

jimbo Oct 22, 2019

sirugh Oct 22, 2019

sirugh Oct 22, 2019

jimbo Oct 22, 2019

[chore?]: Use createCustomer gql mutation instead of create-account REST endpoint #1898

[chore?]: Use createCustomer gql mutation instead of create-account REST endpoint #1898

Conversation

sirugh commented Oct 16, 2019 • edited Loading

Description

Related Issue

Acceptance

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Oct 16, 2019 • edited Loading

sirugh Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at left a comment

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

Choose a reason for hiding this comment

supernova-at Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at Oct 21, 2019 • edited 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

Choose a reason for hiding this comment

sirugh commented Oct 16, 2019 •

edited

Loading

PWAStudioBot commented Oct 16, 2019 •

edited

Loading

sirugh Oct 17, 2019 •

edited

Loading

supernova-at Oct 21, 2019 •

edited

Loading

supernova-at Oct 21, 2019 •

edited

Loading