Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 15 commits into from
Oct 22, 2019
Merged
Show file tree
Hide file tree
Changes from 9 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 @@ -4,7 +4,6 @@ import {
signIn,
signOut,
getUserDetails,
createAccount,
resetPassword
} from '../asyncActions';

Expand All @@ -23,45 +22,6 @@ const credentials = {
password: 'PASSWORD'
};

const accountInfo = {
customer: {
email: 'EMAIL'
},
password: 'PASSWORD'
};

describe('#createAccount', () => {
test('it returns a thunk', () => {
expect(createAccount()).toBeInstanceOf(Function);
});

test('its thunk dispatches createAccount', async () => {
await createAccount(accountInfo)(...thunkArgs);

expect(dispatch).toHaveBeenCalledWith(actions.createAccount.request());
});

test('its thunk dispatches signIn', async () => {
await createAccount(accountInfo)(...thunkArgs);

expect(dispatch).toHaveBeenNthCalledWith(2, expect.any(Function));
});

test('its thunk dispatches createAccount on invalid account info', async () => {
const error = new TypeError('ERROR');
request.mockRejectedValueOnce(error);

try {
await createAccount({})(...thunkArgs);
} catch (e) {}

expect(dispatch).toHaveBeenNthCalledWith(
2,
actions.createAccount.receive(error)
);
});
});

describe('signIn', () => {
test('it returns a thunk', () => {
expect(signIn()).toBeInstanceOf(Function);
Expand Down
25 changes: 0 additions & 25 deletions packages/peregrine/lib/store/actions/user/asyncActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,31 +78,6 @@ export const getUserDetails = () =>
}
};

export const createAccount = accountInfo => async dispatch => {
Copy link
Contributor Author

@sirugh sirugh Oct 17, 2019

Choose a reason for hiding this comment

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

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!

dispatch(actions.createAccount.request());

try {
await request('/rest/V1/customers', {
method: 'POST',
body: JSON.stringify(accountInfo)
});

await dispatch(
signIn({
username: accountInfo.customer.email,
password: accountInfo.password
})
);
} catch (error) {
dispatch(actions.createAccount.receive(error));

/*
* Throw error again to notify async action which dispatched handleCreateAccount.
*/
throw error;
}
};

export const resetPassword = ({ email }) =>
async function thunk(...args) {
const [dispatch] = args;
Expand Down
24 changes: 0 additions & 24 deletions packages/peregrine/lib/store/reducers/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ const initialState = {
firstname: '',
lastname: ''
},
createAccountError: null,
getDetailsError: null,
isCreatingAccount: false,
isGettingDetails: false,
isResettingPassword: false,
isSignedIn: isSignedIn(),
Expand Down Expand Up @@ -74,28 +72,6 @@ const reducerMap = {
isGettingDetails: false
};
},
[actions.createAccount.request]: state => {
return {
...state,
createAccountError: null,
isCreatingAccount: true
};
},
[actions.createAccount.receive]: (state, { payload, error }) => {
if (error) {
return {
...state,
createAccountError: payload,
isCreatingAccount: false
};
}

return {
...state,
createAccountError: null,
isCreatingAccount: false
};
},
[actions.resetPassword.request]: state => ({
...state,
isResettingPassword: true
Expand Down
12 changes: 4 additions & 8 deletions packages/peregrine/lib/talons/AuthModal/useAuthModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const useAuthModal = props => {
} = props;

const [username, setUsername] = useState('');
const [{ currentUser }, { createAccount, signOut }] = useUserContext();
const [{ currentUser }, { signOut }] = useUserContext();

// If the user is authed, the only valid view is "MY_ACCOUNT".
// view an also be `MENU` but in that case we don't want to act.
Expand All @@ -49,13 +49,9 @@ export const useAuthModal = props => {
closeDrawer();
}, [closeDrawer, showMainMenu]);

const handleCreateAccount = useCallback(
async values => {
await createAccount(values);
showMyAccount();
},
[createAccount, showMyAccount]
);
const handleCreateAccount = useCallback(() => {
showMyAccount();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}, [showMyAccount]);

const handleSignOut = useCallback(() => {
// TODO: Get history from router context when implemented.
Expand Down
58 changes: 48 additions & 10 deletions packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,61 @@
import { useMemo } from 'react';
import { useCallback, useMemo, useState } from 'react';
import { useMutation } from '@apollo/react-hooks';
import { useUserContext } from '@magento/peregrine/lib/context/user';

/**
* Returns props necessary to render CreateAccount component.
* Returns props necessary to render CreateAccount component. In particular this
* talon handles the submission flow by first doing a pre-submisson validation
* and then, on success, invokes the `onSubmit` prop, which is usually the action.
*
* @param {Object} props.initialValues initial values to sanitize and seed the form
* @param {Function} props.onSubmit the post submit callback
* @param {String} query the graphql query for creating the account
* @returns {{
* hasError: boolean,
* errors: array,
* handleSubmit: function,
* isDisabled: boolean,
* isSignedIn: boolean,
* initialValues: object
* }}
*/
export const useCreateAccount = props => {
const { initialValues = {} } = props;
const { initialValues = {}, onSubmit, query } = props;
const [isSubmitting, setIsSubmitting] = useState(false);

const [
{ createAccountError, isCreatingAccount, isSignedIn }
] = useUserContext();
const hasError = !!createAccountError;
const [{ isSignedIn }, { signIn }] = useUserContext();
const [createAccount, { error }] = useMutation(query);
Copy link
Contributor

Choose a reason for hiding this comment

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

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


const handleSubmit = useCallback(
async formValues => {
setIsSubmitting(true);
try {
// Try to create an account with the mutation.
await createAccount({
variables: {
email: formValues.customer.email,
firstname: formValues.customer.firstname,
lastname: formValues.customer.lastname,
password: formValues.password
}
});

// Then sign the user in.
await signIn({
username: formValues.customer.email,
password: formValues.password
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// Finally, invoke the post-submission callback.
onSubmit();
} catch (error) {
if (process.env.NODE_ENV === 'development') {
console.error(error);
}
setIsSubmitting(false);
}
},
[createAccount, onSubmit, signIn]
);

const sanitizedInitialValues = useMemo(() => {
const { email, firstName, lastName, ...rest } = initialValues;
Expand All @@ -30,8 +67,9 @@ export const useCreateAccount = props => {
}, [initialValues]);

return {
hasError,
isDisabled: isCreatingAccount,
errors: (error && error.graphQLErrors) || [],
handleSubmit,
isDisabled: isSubmitting,
isSignedIn,
initialValues: sanitizedInitialValues
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { useCallback, useMemo } from 'react';
import { useUserContext } from '@magento/peregrine/lib/context/user';

const validCreateAccountParams = ['email', 'firstName', 'lastName'];

Expand All @@ -22,17 +21,12 @@ const getCreateAccountInitialValues = search => {
* }}
*/
export const useCreateAccountPage = props => {
const [, { createAccount }] = useUserContext();
// TODO replace with useHistory in React Router 5.1
const { history } = props;

const handleCreateAccount = useCallback(
async accountInfo => {
await createAccount(accountInfo);
history.push('/');
},
[createAccount, history]
);
const handleCreateAccount = useCallback(() => {
history.push('/');
}, [history]);

const initialValues = useMemo(
() => getCreateAccountInitialValues(window.location.search),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,25 @@ import { Form } from 'informed';
import { createTestInstance } from '@magento/peregrine';

import CreateAccount from '../createAccount';
import { useMutation } from '@apollo/react-hooks';

jest.mock('@apollo/react-hooks', () => ({
useMutation: jest.fn().mockImplementation(() => [
jest.fn(),
{
error: null
}
])
}));
jest.mock('../../../util/formValidators');
jest.mock('@magento/peregrine/lib/context/user', () => {
const userState = {
createAccountError: null,
isCreatingAccount: false,
isSignedIn: false
};
const userApi = {};
const userApi = {
signIn: jest.fn()
};
const useUserContext = jest.fn(() => [userState, userApi]);

return { useUserContext };
Expand All @@ -37,18 +47,32 @@ test('attaches the submit handler', () => {
});

test('calls onSubmit if validation passes', async () => {
useMutation.mockImplementationOnce(() => [
jest.fn(),
{
called: true,
loading: false,
error: null,
data: {}
}
]);

const { root } = createTestInstance(<CreateAccount {...props} />);

const form = root.findByType(Form);
const { formApi } = form.instance;

// touch fields, call validators, call onSubmit
await act(() => {
formApi.submitForm();
const { controller } = form.instance;
await act(async () => {
await form.props.onSubmit({
customer: {
email: 'test@example.com',
firstname: 'tester',
lastname: 'guy'
},
password: 'foo'
});
expect(props.onSubmit).toHaveBeenCalledTimes(1);
});

const { errors } = root.findByType(Form).instance.controller.state;
const { errors } = controller.state;

expect(Object.keys(errors)).toHaveLength(0);
expect(props.onSubmit).toHaveBeenCalledTimes(1);
});
22 changes: 0 additions & 22 deletions packages/venia-ui/lib/components/CreateAccount/asyncValidators.js

This file was deleted.

22 changes: 17 additions & 5 deletions packages/venia-ui/lib/components/CreateAccount/createAccount.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,31 @@ import {
} from '../../util/formValidators';
import defaultClasses from './createAccount.css';
import { useCreateAccount } from '@magento/peregrine/lib/talons/CreateAccount/useCreateAccount';
import CREATE_ACCOUNT_MUTATION from '../../queries/createAccount.graphql';

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@supernova-at supernova-at Oct 21, 2019

Choose a reason for hiding this comment

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

👍 pending @jimbo 's verdict 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made #1928 :D

onSubmit: props.onSubmit
});

const { hasError, isDisabled, isSignedIn, initialValues } = talonProps;
const {
errors,
handleSubmit,
isDisabled,
isSignedIn,
initialValues
} = talonProps;

const errorMessage = hasError
? 'An error occurred. Please try again.'
// Map over any errors we get and display an appropriate error.
const errorMessage = errors.length
? errors
.map(({ message }) => message)
.reduce((acc, msg) => acc + '\n' + msg, '')
: null;

if (isSignedIn) {
Expand All @@ -43,7 +55,7 @@ const CreateAccount = props => {
<Form
className={classes.root}
initialValues={initialValues}
onSubmit={props.onSubmit}
onSubmit={handleSubmit}
>
<p className={classes.lead}>{LEAD}</p>
<Field label="First Name" required={true}>
Expand Down
Loading