Skip to content
This repository was archived by the owner on Dec 19, 2019. It is now read-only.

Create customer account functionality #254

Merged

Conversation

Stepa4man
Copy link
Contributor

@Stepa4man Stepa4man commented Nov 19, 2018

Description

Added the ability to create customer account through the GraphQL

Fixed Issues

  1. magento/graphql-ce/[My Account] Create customer account #248 Create a customer account

Manual testing scenarios (*)

Run in GraphiQL:
mutation { createCustomer ( input: { firstname: "Test" lastname: "Dev" email: "[email protected]" is_subscribed:true } ) { customer{ id, firstname, lastname, email, is_subscribed } } }

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@Stepa4man Stepa4man requested a review from naydav November 19, 2018 08:07
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 19, 2018

CLA assistant check
All committers have signed the CLA.

@Stepa4man
Copy link
Contributor Author

@naydav, I haven't written tests yet. Will be covered a bit later.

Copy link
Contributor

@naydav naydav left a comment

Choose a reason for hiding this comment

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

  • Need to add a possibility to set customer EAV attributes (include validation rules)
    Pay attention EAV attributes have own scope

$customerDataObject->setWebsiteId($store->getWebsiteId());
$customerDataObject->setStoreId($store->getId());

$password = array_key_exists('password', $args['input']) ? $args['input']['password'] : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

When do we not need a 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.

When we want to have registration and then send email to the customer with a link to set a password.
IN current state it works like that:
image

try {
$customer = $this->createUserAccount($args);
$this->setUpUserContext($context, $customer);
if (array_key_exists('is_subscribed', $args['input'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls, reuse \Magento\CustomerGraphQl\Model\Customer\ChangeSubscriptionStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

array $value = null,
array $args = null
) {
if (!isset($args['input']) || !is_array($args['input']) || empty($args['input'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to have more smarty validation (based on is_required values of Customer EAV attributes)

Copy link
Contributor Author

@Stepa4man Stepa4man Dec 3, 2018

Choose a reason for hiding this comment

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

I got it from the current \Magento\CustomerGraphQl\Model\Resolver\UpdateCustomer implementation. Is it ok for updating but not ok for creation?

Just question to think. We can have attribute requirement changed between account creation and account updating. So it also needs to be validated in customer update.

My implementation checks which fields are required

Copy link
Contributor

Choose a reason for hiding this comment

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

Magento\CustomerGraphQl\Model\Resolver\UpdateCustomer does not contain any validation related to settings in admin panel (there is only one rule need to provide password if you change email)

}
$data = $this->customerDataProvider->getCustomerById((int)$customer->getId());
} catch (LocalizedException $e) {
throw new GraphQlInputException(__($e->getMessage()));
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to highlight to client all of possible errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

return ['customer' => $data];
}

private function createUserAccount($args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls, add dockblcoks and type hinting to all of methods (including return values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Stepa4man
Copy link
Contributor Author

@naydav eav attributes are implemented

use Magento\Newsletter\Model\SubscriberFactory;
use Magento\Store\Model\StoreManagerInterface;

class CreateCustomer implements ResolverInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to split this class (an example is in other resolvers of this module)

use Magento\Newsletter\Model\SubscriberFactory;
use Magento\Store\Model\StoreManagerInterface;

class CreateCustomer implements ResolverInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to create dockblock for all of classes

* @var CustomerDataProvider
*/
private $customerDataProvider;
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed rmpty line

* @throws LocalizedException
* @throws NoSuchEntityException
*/
public function resolve(
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use inheritdoc

array $value = null,
array $args = null
) {
if (!isset($args['input']) || !is_array($args['input']) || empty($args['input'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Magento\CustomerGraphQl\Model\Resolver\UpdateCustomer does not contain any validation related to settings in admin panel (there is only one rule need to provide password if you change email)

@misha-kotov
Copy link

@Stepa4man @naydav Thanks for the demo today! After going back to check on the use cases, it does not look like a guest shopper can create an account without a password on storefront. The Customer configuration options do now allow this either. The only way to create an account without a password is in Admin, which is a use case we do not support. So the password field should be required when creating an account through GraphQL.

@Stepa4man
Copy link
Contributor Author

@misha-kotov
Misha, I still don't agree with you. You have to take into an account that it's just additional possibility for merchants to implement their own vision.
It's not a security issue.
Restricting this we will just restrict Magento's flexibility, I think.

Let me just give you some use case for example:
After a successful checkout guest customer will see a button to create an account. After hitting this button account will be created and the invitation to set a password will be sent to the customer email. So the customer can set this password at any time when it will be convenient for him... Not only right after order checkout.
Please consider my claim=)

@misha-kotov
Copy link

After discussing this further, we decided to make the password field not required to allow for alternative account creation flows. So out of the box behavior of the endpoint would be:

  • if the password is specified, customer account is created right away with the password
  • if the password is not specified, an email is sent to the customer with the set password link

@magento-engcom-team
Copy link
Contributor

@Stepa4man thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@magento-engcom-team magento-engcom-team merged commit 7c5bea3 into magento:2.3-develop Feb 1, 2019
@ghost
Copy link

ghost commented Feb 1, 2019

Hi @Stepa4man, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants