-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
LoginAsCustomer GraphQL #30130
LoginAsCustomer GraphQL #30130
Conversation
Hi @KrishnaK-Z. 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 |
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.
Hi @KrishnaK-Z. Thank you for your collaboration. Please, check my comments below.
@@ -0,0 +1,11 @@ | |||
<?xml version="1.0" ?> | |||
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Module/etc/module.xsd"> | |||
<module name="Magento_LoginAsCustomerGraphQl" setup_version="1.0.0"> |
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.
You may omit the setup_version
since we don't introduce changes to the database. In case it will happen in the future, it's recommended to use declarative schema that does not require setup_version
either.
private $createCustomerToken; | ||
|
||
/** | ||
* RequestCustomerToken constructor. |
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, omit a short description of constructors. They don't provide useful information but use one more line of the file.
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, remove unnecessary comments
/** | ||
* Gets customer token | ||
* | ||
* Class RequestCustomerToken |
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, omit the class name in the DocBlock, but leave the short description.
) { | ||
$this->storeManager = $storeManager; | ||
$this->tokenModelFactory = $tokenModelFactory; | ||
$this->customer= $customer; |
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, do not inject models using DI. Use customerFactory
to create the corresponding instance instead.
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.
In case if you need to work with the existing customer instance, you may pass it as an argument to the execute
method.
$customerId = $this->customer->getId(); | ||
$customerToken = $this->tokenModelFactory->create(); | ||
return [ | ||
"customer_token" => $customerToken->createCustomerToken($customerId)->getToken() |
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.
We may have an unhandled exception upon creating customer token. For example if the system is unable to save a token to the database. I would recommend catching exception here.
"type": "magento2-module", | ||
"require": {}, | ||
"version": "1.0.0", | ||
"require": { |
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, provide all the dependencies you use within the scope of the module. Like LoginAsCustomerApi
, Customer
, Store
etc
private $adminTokenService; | ||
|
||
/** | ||
* Set Up |
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, avoid unnecessary comments
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 move tests under the LoginAsCustomerGraphQl module
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.
@cspruiell Moved tests under the LoginAsCustomerGraphQl module but getting as below error now so do I have to revert it as earlier, please suggest.
Passed: 31456, Failed: 1, Incomplete: 0, Skipped: 0.
Data set: dev/tests/api-functional/testsuite/Magento/GraphQl/LoginAsCustomerGraphQl/GenerateLoginCustomerTokenTest.php
Location of /var/www/html/dev/tests/api-functional/testsuite/Magento/GraphQl/LoginAsCustomerGraphQl/GenerateLoginCustomerTokenTest.php does not match formal namespace: Magento\GraphQl\LoginAsCustomerGraphQl
|
||
$mutation = $this->getQuery($customerEmail); | ||
|
||
$response = $this->graphQlMutation($mutation, [], '', $this->getAdminHeaderAuthentication('TestAdmin1', 'Zilker777')); |
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, make sure the line length does not exceed 120 characters
* @return string[] | ||
* @throws AuthenticationException | ||
*/ | ||
public function getAdminHeaderAuthentication(string $userName, string $password) |
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, make sure that you use the private
scope for the test methods that don't require public access.
try { | ||
$userModel->setData($adminInfo); | ||
$userModel->setRoleId(1); | ||
$userModel->save(); |
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, do not use deprecated methods for saving data. Use the resource model or repository instead.
@rogyar Update the PR as suggested. Please have a look. |
@magento run Static 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.
Hi @KrishnaK-Z. I've outlined some unresolved points. Please, address them when you will have a chance.
The failing functional tests are not related to your changes. But the failing static tests tell that you have 1 undeclared dependency. This point should be resolved as well.
Thank you!
Hi @rogyar for the Failing static test case I have removed the undeclared one from module.xml but still the same failed case. Can you suggest on this and let me know what can be done to get rid of this. I did follow https://devdocs.magento.com/guides/v2.4/architecture/archi_perspectives/components/modules/mod_depend.html
@magento run all tests |
Hi @nihara47, there are two changes you need that will address the static failures:
|
Hi @cspruiell When you say the root composer.json file needs to have this new module added |
Yes, magento2/composer.json |
@magento run Static Tests |
@magento run Static Tests |
@magento run all tests |
Hi @cspruiell thanks for the suggestion. It worked actually. @cspruiell and @rogyar the suggested changes has been updated. Currently there is only one static test case pending which adding this module to root composer.json. I believe @cpartica will take care for this. There are multiple test cases failing which belongs to some other modules like Magento_Checkout or Magento_PurchaseOrder etc and these are changing time to time. For ex. please check attached one. |
@magento run WebAPI Tests |
@magento run all tests |
__('Customer email provided does not exist') | ||
); | ||
} | ||
|
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.
We also need to validate that the 'allow_remote_shopping_assistance' setting is enabled for the customer. See https://github.com/magento/architecture/pull/414/files#diff-ccdc60253a5ea983cb68248c51500c3cdc077232bd52ded6cb08d4af9cba8d2bR39
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.
Done
} | ||
|
||
type Customer { | ||
allow_remote_shopping_assistance: Boolean! @doc(description: "Indicates whether the customer has enabled remote shopping assistance") |
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 GetCustomer resolver will need to be updated to return this new value
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.
Done
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.
Added a new fixture for this with setAssistanceAllowed enabled
|
||
input CustomerUpdateInput { | ||
allow_remote_shopping_assistance: Boolean @doc(description: "Indicates whether the customer has enabled remote shopping assistance") | ||
} |
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.
Resolvers using these inputs will need to be updated to ensure allow_remote_shopping_assistance is persisted.
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.
Done
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 don't see the changes for this. The createCustomer & updateCustomer mutations need to be updated so that allow_remote_shopping_assistance is persisted
mutation { createCustomerV2(input: { email: "test@example.com" firstname: "Test" lastname: "Magento" password: "T3stP4ssw0rd" allow_remote_shopping_assistance: true}) { customer { firstname lastname email allow_remote_shopping_assistance } } }
@magento run all tests |
Hi @KrishnaK-Z, thank you for your contribution! |
Description (*)
This PR covers the Login as customer GraphQl functionality.
Related Pull Requests
magento/architecture#414
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)