-
Notifications
You must be signed in to change notification settings - Fork 9.4k
#17488 Fix Authenticating a customer via REST API does not update the last logged in data #17978
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
#17488 Fix Authenticating a customer via REST API does not update the last logged in data #17978
Conversation
Hi @prakashpatel07. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
__('You did not sign in correctly or your account is temporarily disabled.') | ||
); | ||
} | ||
$this->eventManager->dispatch('customer_login', ['customer' => $customerDataObject]); |
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.
Should this event be thrown in case when token used instead of when it is acquired?
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.
@paliarush Actually such an event should be thrown once a customer is logged id that is the case
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.
@slavvka Just to confirm I was on right track or should I use a @paliarush suggestion or not?
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.
@prakashpatel07 after discussing this, I think current implementation is good. Thanks
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 fix the issue
TokenCollectionFactory $tokenModelCollectionFactory, | ||
CredentialsValidator $validatorHelper | ||
CredentialsValidator $validatorHelper, | ||
ManagerInterface $eventManager |
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 in backward compatible way. Please refer to the guide https://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/
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.
Let me use the Backward Сompatibility Policy.
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.
Regarding Backward compatible development
Adding a constructor parameter
Add a new optional parameter to the constructor at the end of the arguments list instead of modifying the constructor.
In the constructor body, if the new dependency is not provided (i.e. the value of the introduced argument is null), fetch the dependency using Magento\Framework\App\ObjectManager::getInstance().
So
CredentialsValidator $validatorHelper,
ManagerInterface $eventManager = null
...
$this->eventManager = $eventManager ?: \Magento\Framework\App\ObjectManager::getInstance()->get(ManagerInterface::class);
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.
Extactly. Thank you @0m3r :)
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 long line 77 :(
77 | ERROR | Line exceeds maximum limit of 120 characters; contains 129
| | characters
Split on two
$this->eventManager = $eventManager ?: \Magento\Framework\App\ObjectManager::getInstance()
->get(ManagerInterface::class);
Hi @slavvka, thank you for the review. |
@prakashpatel07 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository. |
…ot update the last logged in data #17978
Hi @prakashpatel07. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
Description
Fixed Last Logged In date when we authenticate a customer via REST API.
Fixed Issues (if relevant)
#17488: Authenticating a customer via REST API does not update the last logged in data #17488
Manual testing scenarios
http://{{base_url}}/rest/V1/integration/customer/token/?username=[email protected]&password=examplel@123
Contribution checklist