-
Notifications
You must be signed in to change notification settings - Fork 9.4k
33383 GraphQL: add new mutation 'deleteCustomer' #33411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
33383 GraphQL: add new mutation 'deleteCustomer' #33411
Conversation
Hi @khrystynastolbova. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @khrystynastolbova for working on this one. Could you please review the questions and suggestions below?
app/code/Magento/CustomerGraphQl/Model/Resolver/DeleteCustomer.php
Outdated
Show resolved
Hide resolved
dev/tests/api-functional/testsuite/Magento/GraphQl/Customer/DeleteCustomerTest.php
Outdated
Show resolved
Hide resolved
dev/tests/api-functional/testsuite/Magento/GraphQl/Customer/DeleteCustomerTest.php
Show resolved
Hide resolved
dev/tests/api-functional/testsuite/Magento/GraphQl/Customer/DeleteCustomerTest.php
Outdated
Show resolved
Hide resolved
dev/tests/api-functional/testsuite/Magento/GraphQl/Customer/DeleteCustomerTest.php
Outdated
Show resolved
Hide resolved
dev/tests/api-functional/testsuite/Magento/GraphQl/Customer/DeleteCustomerTest.php
Outdated
Show resolved
Hide resolved
dev/tests/api-functional/testsuite/Magento/GraphQl/Customer/DeleteCustomerTest.php
Outdated
Show resolved
Hide resolved
$isSecure = $this->registry->registry('isSecureArea'); | ||
|
||
$this->registry->unregister('isSecureArea'); | ||
$this->registry->register('isSecureArea', true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a concern regarding the deprecated \Magento\Framework\Registry
usage. Is there any alternative? Are we required to play with this configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is located in magento/lib/internal/Magento/Framework/Model/ActionValidator/RemoveAction.php
public function isAllowed(AbstractModel $model)
{
$isAllowed = true;
if ($this->registry->registry('isSecureArea')) {
$isAllowed = true;
} elseif (in_array($this->getBaseClassName($model), $this->protectedModels)) {
$isAllowed = false;
}
return $isAllowed;
}
ProtectedModels include the customer model too. In the file magento/app/code/Magento/Customer/etc/di.xml
<type name="Magento\Framework\Model\ActionValidator\RemoveAction">
<arguments>
<argument name="protectedModels" xsi:type="array">
<item name="customer" xsi:type="string">Magento\Customer\Model\Customer</item>
</argument>
</arguments>
</type>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid deprecated \Magento\Framework\Registry
, we can remove the Customer model from protected models in general or copy all protected models to graphQL/di.xml for all magento modules except the Customer one.
app/code/Magento/CustomerGraphQl/Model/Customer/DeleteCustomer.php
Outdated
Show resolved
Hide resolved
app/code/Magento/CustomerGraphQl/Model/Customer/DeleteCustomer.php
Outdated
Show resolved
Hide resolved
Hi @eduard13. Thank you for your suggestions. I made changes except the deprecated \Magento\Framework\Registry usage. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please review the below suggestions as well?
Thank you.
use Magento\Integration\Api\CustomerTokenServiceInterface; | ||
|
||
/** | ||
* Delete custom tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please adjust the description
*/ | ||
protected function setUp(): void | ||
{ | ||
parent::setUp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to call the parent method?
public function testDeleteCustomerIfAccountIsLocked(): void | ||
{ | ||
$this->expectException(\Exception::class); | ||
$this->expectExceptionMessage('The account sign-in was incorrect or your account is disabled temporarily. Please wait and try again later'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets avoid long code lines
$this->expectExceptionMessage('The account sign-in was incorrect or your account is disabled temporarily. Please wait and try again later'); | |
$this->expectExceptionMessage( | |
'The account sign-in was incorrect or your account is disabled temporarily. Please wait and try again later' | |
); |
) { | ||
$this->getCustomer = $getCustomer; | ||
$this->deleteCustomer = $deleteCustomer; | ||
$this->registry =$registry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->registry =$registry; | |
$this->registry = $registry; |
@magento run Static Tests, Magento Health Index |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
Since there are a couple of auto jobs are getting failed, and so moving the ticket to Extended Testing. Note: Before moving the ticket to Merge In Progress, we should have a devdoc updates request on this case! |
…L_delete_customer
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Functional Tests B2B, Integration Tests, Magento Health Index |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
The failing test of Functional B2B test is not part of this PR or not failing because of this PR changes. Related to failed Magento Health Index test, raised approval JIRA ticket. Waiting for approval now. Thank you! |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run Magento Health Index , Functional Tests B2B, Functional Tests EE |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
Since we got all the approvals for magento health index on JIRA moving this PR to Merge in Progress. Thank you! |
33383 GraphQL: add new mutation 'deleteCustomer' #33411
Description (*)
Related Pull Requests
This PR added new mutation 'deleteCustomer'
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)