My Account > Change account information and Newsletter subscription #162

nuzil · 2018-08-28T08:19:34Z

Description

Implementing 2 issues:

Customer account edit data and customer newsletter subscription

https://app.zenhub.com/workspace/o/magento/graphql-ce/issues/56
https://app.zenhub.com/workspace/o/magento/graphql-ce/issues/55

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)

magento-cicd2 · 2018-08-28T08:19:36Z

All committers have signed the CLA.

naydav · 2018-09-04T16:06:19Z

app/code/Magento/CustomerGraphQl/Model/Resolver/Customer/CustomerDataProvider.php

+    /**
+     * @var \Magento\Newsletter\Model\SubscriberFactory
+     */
+    protected $subscriberFactory;


Pls, use private access

naydav · 2018-09-04T16:07:05Z

app/code/Magento/CustomerGraphQl/Model/Resolver/Customer/CustomerDataProvider.php

+     * @return CustomerInterface
+     * @throws LocalizedException
+     * @throws NoSuchEntityException
+     */


Why do we need mathod-wrapper over Repository?

Good catch, I planned to use it in another location, but currently it not needed anymore

naydav · 2018-09-04T16:08:30Z

app/code/Magento/CustomerGraphQl/Model/Resolver/Customer/CustomerDataProvider.php

@@ -13,6 +13,11 @@
 use Magento\Framework\Serialize\SerializerInterface;


Looks like this class has a lot of responsibility (looks like helper in Magento 1)
Need to split the class

naydav · 2018-09-04T16:09:10Z

app/code/Magento/CustomerGraphQl/Model/Resolver/Customer/CustomerDataProvider.php

+
+        $customerSecure = $this->customerRegistry->retrieveSecureData($customerId);
+        $hash = $customerSecure->getPasswordHash();
+        if (!$this->encryptor->validateHash($password, $hash)) {


You can simplify this condition

naydav · 2018-09-04T16:11:03Z

app/code/Magento/CustomerGraphQl/Model/Resolver/CustomerUpdate.php

+            return !empty($data) ? $data : [];
+        };
+
+        return $this->valueFactory->create($result);


Do we need to return something if it is 'command' operation?

naydav · 2018-09-04T16:12:09Z

app/code/Magento/CustomerGraphQl/etc/schema.graphqls

@@ -51,3 +51,13 @@ type CustomerAddressRegion @doc(description: "CustomerAddressRegion defines the
    region_id: Int @doc(description: "Uniquely identifies the region")
 }

+type Mutation {


Also, need to cover functionality with API-functional tests
Now we have a lot of examples and tests writing is very simple

…mutations

…mutations # Conflicts: # app/code/Magento/CustomerGraphQl/etc/schema.graphqls

-- fixes after merge with mainline

-- revert composer lock

-- fix static tests

keharper · 2018-10-09T16:21:58Z

app/code/Magento/CustomerGraphQl/Model/Customer/UpdateAccountInformation.php

+
+        if (isset($data['email']) && $customer->getEmail() !== $data['email']) {
+            if (!isset($data['password']) || empty($data['password'])) {
+                throw new GraphQlInputException(__('For changing "email" you should provide current "password".'));


Change exception message to 'Provide the current "password" to change "email".'

keharper · 2018-10-09T16:23:54Z

app/code/Magento/CustomerGraphQl/Model/Resolver/ChangePassword.php

+        array $args = null
+    ) {
+        if (!isset($args['currentPassword'])) {
+            throw new GraphQlInputException(__('"currentPassword" value should be specified'));


Change exception message to 'Specify the "currentPassword" value.`

keharper · 2018-10-09T16:24:31Z

app/code/Magento/CustomerGraphQl/Model/Resolver/ChangePassword.php

+        }
+
+        if (!isset($args['newPassword'])) {
+            throw new GraphQlInputException(__('"newPassword" value should be specified'));


Change exception message to 'Specify the "newPassword" value.'

keharper · 2018-10-09T16:25:23Z

app/code/Magento/CustomerGraphQl/Model/Resolver/GenerateCustomerToken.php

@@ -43,19 +44,19 @@ public function resolve(
        array $value = null,
        array $args = null
    ) {
+        if (!isset($args['email'])) {
+            throw new GraphQlInputException(__('"email" value should be specified'));


Change exception message to 'Specify the "email" value.'

keharper · 2018-10-09T16:25:39Z

app/code/Magento/CustomerGraphQl/Model/Resolver/GenerateCustomerToken.php

+        }
+
+        if (!isset($args['password'])) {
+            throw new GraphQlInputException(__('"password" value should be specified'));


Change exception message to 'Specify the "password" value.'

keharper · 2018-10-09T16:26:17Z

app/code/Magento/CustomerGraphQl/Model/Resolver/UpdateCustomer.php

+        array $args = null
+    ) {
+        if (!isset($args['input']) || !is_array($args['input']) || empty($args['input'])) {
+            throw new GraphQlInputException(__('"input" value should be specified'));


Change exception message to 'Specify the "input" value.'

keharper · 2018-10-09T16:27:41Z

app/code/Magento/CustomerGraphQl/etc/schema.graphqls

-    generateCustomerToken(email: String!, password: String!): CustomerToken @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\Customer\\Account\\GenerateCustomerToken") @doc(description:"Retrieve Customer token")
-    changeCustomerPassword(currentPassword: String!, newPassword: String!): Customer @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\Customer\\Account\\ChangePassword") @doc(description:"Changes password for logged in customer")
-    revokeCustomerToken: Boolean @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\Customer\\Account\\RevokeCustomerToken") @doc(description:"Revoke Customer token")
+    generateCustomerToken(email: String!, password: String!): CustomerToken @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\GenerateCustomerToken") @doc(description:"Retrieve Customer token")


Change the description text to "Retrieve the customer token"

keharper · 2018-10-09T16:29:06Z

app/code/Magento/CustomerGraphQl/etc/schema.graphqls

-    changeCustomerPassword(currentPassword: String!, newPassword: String!): Customer @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\Customer\\Account\\ChangePassword") @doc(description:"Changes password for logged in customer")
-    revokeCustomerToken: Boolean @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\Customer\\Account\\RevokeCustomerToken") @doc(description:"Revoke Customer token")
+    generateCustomerToken(email: String!, password: String!): CustomerToken @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\GenerateCustomerToken") @doc(description:"Retrieve Customer token")
+    changeCustomerPassword(currentPassword: String!, newPassword: String!): Customer @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\ChangePassword") @doc(description:"Changes password for logged in customer")


Change the description text to "Changes the password for the logged-in customer"

keharper · 2018-10-09T16:30:08Z

app/code/Magento/CustomerGraphQl/etc/schema.graphqls

-    revokeCustomerToken: Boolean @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\Customer\\Account\\RevokeCustomerToken") @doc(description:"Revoke Customer token")
+    generateCustomerToken(email: String!, password: String!): CustomerToken @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\GenerateCustomerToken") @doc(description:"Retrieve Customer token")
+    changeCustomerPassword(currentPassword: String!, newPassword: String!): Customer @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\ChangePassword") @doc(description:"Changes password for logged in customer")
+    updateCustomer (input: UpdateCustomerInput): UpdateCustomerOutput @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\UpdateCustomer") @doc(description:"Update customer personal information")


Change the description text to "Update the customer's personal information"

keharper · 2018-10-09T16:30:40Z

app/code/Magento/CustomerGraphQl/etc/schema.graphqls

+    generateCustomerToken(email: String!, password: String!): CustomerToken @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\GenerateCustomerToken") @doc(description:"Retrieve Customer token")
+    changeCustomerPassword(currentPassword: String!, newPassword: String!): Customer @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\ChangePassword") @doc(description:"Changes password for logged in customer")
+    updateCustomer (input: UpdateCustomerInput): UpdateCustomerOutput @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\UpdateCustomer") @doc(description:"Update customer personal information")
+    revokeCustomerToken: Boolean @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\RevokeCustomerToken") @doc(description:"Revoke Customer token")


Change the description text to the "Revoke the customer token"

paliarush · 2018-10-09T14:50:55Z

app/code/Magento/CustomerGraphQl/Model/Customer/ChangeSubscriptionStatus.php

+     */
+    public function execute(int $customerId, bool $subscriptionStatus): void
+    {
+        $subscriber = $this->subscriberFactory->create()->loadByCustomerId($customerId);


Does not make sense to load records from DB if the next two conditions fail.

paliarush · 2018-10-09T14:57:00Z

app/code/Magento/CustomerGraphQl/Model/Customer/CheckCustomerAccount.php

+    public function execute(?int $customerId, ?int $customerType): void
+    {
+        if (true === $this->isCustomerGuest($customerId, $customerType)) {
+            throw new GraphQlAuthorizationException(__('The current customer isn\'t authorized.'));


Usually it is better to use 'is not' instead of 'isn't' in error message.

paliarush · 2018-10-09T15:00:07Z

app/code/Magento/CustomerGraphQl/Model/Customer/CheckCustomerAccount.php

+     * @throws GraphQlNoSuchEntityException
+     * @throws GraphQlAuthenticationException
+     */
+    public function execute(?int $customerId, ?int $customerType): void


Why customer type is needed here? For guests $customerID will be 0.
Also, why these arguments are optional? We can just ask for (int)$customerId

paliarush · 2018-10-09T16:20:32Z

app/code/Magento/CustomerGraphQl/Model/Customer/CheckCustomerPassword.php

+/**
+ * Check customer password
+ */
+class CheckCustomerPassword


We should use nouns as class names. Please consider changing it to CustomerPasswordVerifier or CustomerAuthenticator.

Please fix the names of all affected variables which hold this object as well.

paliarush · 2018-10-09T16:43:43Z

app/code/Magento/CustomerGraphQl/Model/Resolver/ChangePassword.php

+use Magento\Framework\GraphQl\Schema\Type\ResolveInfo;
+
+/**
+ * @inheritdoc


We need explicit description for the purpose of this class.

paliarush · 2018-10-09T16:52:21Z

app/code/Magento/CustomerGraphQl/composer.json

@@ -6,7 +6,9 @@
    "php": "~7.1.3||~7.2.0",
    "magento/module-customer": "*",
    "magento/module-authorization": "*",
+    "magento/module-newsletter": "*",


Please remember to fix health index etalon tests

paliarush · 2018-10-09T16:53:16Z

app/code/Magento/CustomerGraphQl/etc/schema.graphqls

-    revokeCustomerToken: Boolean @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\Customer\\Account\\RevokeCustomerToken") @doc(description:"Revoke Customer token")
+    generateCustomerToken(email: String!, password: String!): CustomerToken @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\GenerateCustomerToken") @doc(description:"Retrieve Customer token")
+    changeCustomerPassword(currentPassword: String!, newPassword: String!): Customer @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\ChangePassword") @doc(description:"Changes password for logged in customer")
+    updateCustomer (input: UpdateCustomerInput): UpdateCustomerOutput @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\UpdateCustomer") @doc(description:"Update customer personal information")


Must be marked as required UpdateCustomerInput!

paliarush · 2018-10-09T16:55:49Z

app/code/Magento/CustomerGraphQl/etc/schema.graphqls

+    generateCustomerToken(email: String!, password: String!): CustomerToken @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\GenerateCustomerToken") @doc(description:"Retrieve Customer token")
+    changeCustomerPassword(currentPassword: String!, newPassword: String!): Customer @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\ChangePassword") @doc(description:"Changes password for logged in customer")
+    updateCustomer (input: UpdateCustomerInput): UpdateCustomerOutput @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\UpdateCustomer") @doc(description:"Update customer personal information")
+    revokeCustomerToken: Boolean @resolver(class: "\\Magento\\CustomerGraphQl\\Model\\Resolver\\RevokeCustomerToken") @doc(description:"Revoke Customer token")


Let's create a wrapper for the revoke result to allow for future extensibility.

paliarush · 2018-10-09T17:07:35Z

lib/internal/Magento/Framework/GraphQl/Exception/GraphQlAlreadyExistsException.php

+use Magento\Framework\Phrase;
+
+/**
+ * Class GraphQlAlreadyExistsException


More meaningful description is required for this and other newly introduced exceptions.

paliarush · 2018-10-09T17:08:28Z

lib/internal/Magento/Framework/GraphQl/Query/ResolverInterface.php

@@ -20,7 +20,7 @@
     * Fetches the data from persistence models and format it according to the GraphQL schema.
     *
     * @param \Magento\Framework\GraphQl\Config\Element\Field $field
-     * @param $context
+     * @param \Magento\Framework\GraphQl\Query\Resolver\ContextInterface $context


Is fixed in 2.3.0 performance improvements and will result in a conflict. Better revert it here.

…ubscription #162

naydav · 2018-10-10T20:32:24Z

All CR fixes will be provided in separate PR

nuzil added 2 commits August 21, 2018 19:44

Prototype of GraphQL mutations for customer update

09dc09c

Code alignments, formatting

ef7f579

Code refactoring

8963811

naydav suggested changes Sep 4, 2018

View reviewed changes

naydav added in-progress broken-tests labels Sep 13, 2018

nuzil added 3 commits September 13, 2018 14:54

Refactored Account Mutation

d0dd90f

Code formatting

b691d84

Code clean

d759867

naydav added Partner: Comwrap and removed broken-tests labels Sep 17, 2018

naydav added the partners-contribution label Sep 26, 2018

nuzil and others added 9 commits October 3, 2018 00:23

Merge branch '2.3-develop' into 55-56-account-mutations

a1dd683

Fix tests

cc8d328

Merge remote-tracking branch 'origin/2.3-develop' into 55-56-account-…

9200a16

…mutations

GraphQL-55: [Mutations] My Account > Change account information

2057ddf

GraphQL-55: [Mutations] My Account > Change account information

20e3393

GraphQL-55: [Mutations] My Account > Change account information

661988b

Merge remote-tracking branch 'origin/2.3-develop' into 55-56-account-…

706af91

…mutations # Conflicts: # app/code/Magento/CustomerGraphQl/etc/schema.graphqls

GraphQL-55: [Mutations] My Account > Change account information

a1c4d33

-- fixes after merge with mainline

GraphQL-55: [Mutations] My Account > Change account information

0cb378b

-- revert composer lock

magento-engcom-team assigned naydav Oct 9, 2018

magento-engcom-team added this to the Release: 2.3.1 milestone Oct 9, 2018

magento-engcom-team added the Release Line: 2.3 label Oct 9, 2018

Valeriy Nayda added 3 commits October 9, 2018 15:33

GraphQL-55: [Mutations] My Account > Change account information

6e6b5c3

-- fix static tests

GraphQL-55: [Mutations] My Account > Change account information

6f73325

-- fix static tests

GraphQL-55: [Mutations] My Account > Change account information

7ca6097

-- fix static tests

keharper suggested changes Oct 9, 2018

View reviewed changes

paliarush suggested changes Oct 9, 2018

View reviewed changes

magento-engcom-team merged commit 7ca6097 into magento:2.3-develop Oct 10, 2018

magento-engcom-team pushed a commit that referenced this pull request Oct 10, 2018

ENGCOM-3139: My Account > Change account information and Newsletter s…

a3de41d

…ubscription #162

magento-engcom-team added the Award: test coverage label Oct 10, 2018

naydav mentioned this pull request Oct 26, 2018

Apply changes from CR for PR-162 #222

Closed

This was referenced Nov 20, 2018

[Mutations] My Account > Change account information #55

Closed

[Mutations] My Account > Newsletter Subsciption #56

Closed

keharper added the requires-documentation label Nov 21, 2018

keharper mentioned this pull request Nov 21, 2018

GraphQL: Update customer endpoint docs for 2.3.1 magento/devdocs#3338

Closed

12 tasks

naydav removed this from the Release: 2.3.1 milestone Dec 7, 2018

keharper added documentation-completed and removed requires-documentation labels Mar 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

My Account > Change account information and Newsletter subscription #162

My Account > Change account information and Newsletter subscription #162

nuzil commented Aug 28, 2018

magento-cicd2 commented Aug 28, 2018 •

edited

Loading

naydav Sep 4, 2018

naydav Sep 4, 2018

nuzil Sep 6, 2018

naydav Sep 4, 2018

naydav Sep 4, 2018

naydav Sep 4, 2018

naydav Sep 4, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

keharper Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

paliarush Oct 9, 2018

naydav commented Oct 10, 2018

		@@ -13,6 +13,11 @@
		use Magento\Framework\Serialize\SerializerInterface;

My Account > Change account information and Newsletter subscription #162

My Account > Change account information and Newsletter subscription #162

Conversation

nuzil commented Aug 28, 2018

Description

Contribution checklist

magento-cicd2 commented Aug 28, 2018 • 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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav commented Oct 10, 2018

magento-cicd2 commented Aug 28, 2018 •

edited

Loading