#26288 Throw an exception when a customer does not exist requests password reset #27269

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.

Sign up for GitHub

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

Jump to bottom

Merged

magento-engcom-team merged 11 commits into magento:2.4-develop from Bal2018:26288-Missing-Throw

Sep 4, 2020

Contributor

Bal2018 commented Mar 12, 2020 •

edited by sdzhepa

Loading

Description (*)

When a customer (who does not exist) requests a password reset - Magento will now throw an exception error.

Fixed Issues (if relevant)

fixes missing Throw in copy pasted code #26288: missing throw in copy-pasted code

Manual testing scenarios (*)

...created a customer
...checked email sent for new customer
...deleted customer
...from email clicked on "reset password"
...when email entered, then error message thrown

Questions or comments

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 are green)


          magento#26288 Will now throw an exception when a customer does not ex…

de530f7

…ist who wants to reset password

m2-assistant bot commented Mar 12, 2020

Hi @Bal2018. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

magento-engcom-team added Component: Customer Release Line: 2.4 labels

ghost added the Progress: pending review label

Contributor

lbajsarowicz commented Mar 12, 2020

Nice catch. Let me help you with the description, but also I need you to improve code style (Static Tests)

lbajsarowicz changed the title ~~#26288 Will now throw an exception when a customer does not exist wh…~~ #26288 Throw an exception when a customer does not exist requests password reset

lbajsarowicz suggested changes

View reviewed changes

Contributor

lbajsarowicz left a comment

Please correct the static tests.
Could you also add Unit Tests for that case, to avoid "losing that" in future*?

Test coverage is part of Definition of Done.

ghost assigned lbajsarowicz

ghost added Progress: needs update and removed Progress: pending review labels

Bal2018 added 2 commits

March 13, 2020 10:37


          magento#26288 Throw an exception when a customer does not exist reque…

f44add2

…sts password reset


          Fixed format of file: GetCustomerByToken.php

1bd7d1e

ghost added Priority: P3 Severity: S3 labels

engcom-Echo self-assigned this

Contributor

engcom-Echo commented May 7, 2020

I will take care of test coverage.

engcom-Echo added 2 commits

May 7, 2020 14:57


          Cover Unit test

5d11fdc


          Merge branch '2.4-develop' of http://github.com/magento/magento2 into…

… 26288-Missing-Throw

lbajsarowicz suggested changes

View reviewed changes

Contributor

lbajsarowicz left a comment

@engcom-Echo Please review the comments.

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

Comment on lines 23 to 25

+              /**
+               * Test GetCustomerByToken class
+               */

Contributor

lbajsarowicz May 7, 2020

This annotation is not necessary :-)
You can also use @covers if you really need to express anything as class annotation.

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

+               */
+              class GetCustomerByTokenTest extends TestCase
+              {
+                  protected const RESET_PASSWORD = 'resetPassword';

Contributor

lbajsarowicz May 7, 2020

Suggested change

      
                protected const RESET_PASSWORD = 'resetPassword';
          
                private const RESET_PASSWORD = 'resetPassword';

Unit Tests are not expected to be inherited, use private instead

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

+                  /**
+                   * @var SearchCriteriaBuilder|MockObject
+                   */
+                  protected $searchCriteriaBuilderMock;

Contributor

lbajsarowicz May 7, 2020

Suggested change

      
                protected $searchCriteriaBuilderMock;
          
                private $searchCriteriaBuilderMock;

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

+                  /**
+                   * @var SearchCriteria|MockObject
+                   */
+                  protected $searchCriteriaMock;

Contributor

lbajsarowicz May 7, 2020

Suggested change

      
                protected $searchCriteriaMock;
          
                private $searchCriteriaMock;

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

+                  /**
+                   * @var CustomerRepositoryInterface|MockObject
+                   */
+                  protected $customerRepositoryMock;

Contributor

lbajsarowicz May 7, 2020

Suggested change

      
                protected $customerRepositoryMock;
          
                private $customerRepositoryMock;

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

Comment on lines 95 to 97

+                      $this->searchResultMock->expects($this->any())
+                          ->method('getTotalCount')
+                          ->willReturn($totalCount);

Contributor

lbajsarowicz May 7, 2020

If it does not matter how many the method is being called, this means it's stub, not mock. Then you should avoid ->expects() at all.

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

Comment on lines 114 to 116

+                      $this->searchResultMock->expects($this->any())
+                          ->method('getTotalCount')
+                          ->willReturn($totalCount);

Contributor

lbajsarowicz May 7, 2020

If it does not matter how many the method is being called, this means it's stub, not mock. Then you should avoid ->expects() at all.

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

Comment on lines 108 to 110

+                  /**
+                   * @return void
+                   */

Contributor

lbajsarowicz May 7, 2020

Suggested change

      
                /**
          
                 * @return void
          
                 */

This provides no additional value

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

Comment on lines 127 to 129

+                  /**
+                   * @return void
+                   */

Contributor

lbajsarowicz May 7, 2020

Suggested change

      
                /**
          
                 * @return void
          
                 */

No additional value provided by docblock

app/code/Magento/Customer/Test/Unit/Model/ForgotPasswordToken/GetCustomerByTokenTest.php Outdated

Comment on lines 133 to 135

+                      $this->searchResultMock->expects($this->any())
+                          ->method('getTotalCount')
+                          ->willReturn($totalCount);

Contributor

lbajsarowicz May 7, 2020

Suggested change

      
                    $this->searchResultMock->expects($this->any())
          
                        ->method('getTotalCount')
          
                        ->willReturn($totalCount);
          
                    $this->searchResultMock->method('getTotalCount')->willReturn($totalCount);

If it does not matter how many the method is being called, this means it's stub, not mock. Then you should avoid ->expects() at all.


          refactor unit test

d30e5e2

lbajsarowicz approved these changes

View reviewed changes

Contributor

lbajsarowicz left a comment

Awesome. Thank you!

ghost added Progress: ready for testing and removed Progress: needs update labels

Contributor

magento-engcom-team commented May 7, 2020

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7527 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

engcom-Echo added the Auto-Tests: Covered label

engcom-Alfa self-assigned this

ghost added Progress: testing in progress and removed Progress: ready for testing labels

Contributor

engcom-Alfa commented May 8, 2020

✔️ QA Passed

ghost unassigned engcom-Echo

Contributor

engcom-Echo commented Jun 26, 2020

@magento run all tests

Contributor

engcom-Echo commented Jun 26, 2020

Hi @Bal2018 I mistakenly closed PR.

engcom-Echo assigned engcom-Alfa, engcom-Delta, engcom-Echo and lbajsarowicz

sidolov removed this from the 2.4.1 milestone

sidolov approved these changes

View reviewed changes

ghost assigned sidolov

ghost added Progress: ready for testing and removed Progress: pending review labels

Contributor

magento-engcom-team commented Aug 27, 2020

Hi @sidolov, thank you for the review.
ENGCOM-7527 has been created to process this Pull Request

ghost added Progress: testing in progress and removed Progress: ready for testing labels

Contributor

engcom-Delta commented Sep 1, 2020

✔️ QA passed
Result:
No exception error is shown

Contributor

engcom-Delta commented Sep 1, 2020

Note: Functional Tests EE and Integration Tests are failed

ghost added Progress: extended testing and removed Progress: testing in progress labels


          Merge branch '2.4-develop' into 26288-Missing-Throw

bcc29ed

Contributor

engcom-Echo commented Sep 1, 2020

@magento run all tests

ghost added Progress: accept and removed Progress: extended testing labels

magento-engcom-team mentioned this pull request

missing Throw in copy pasted code #26288

Closed

magento-engcom-team pushed a commit that referenced this pull request


          ENGCOM-7527: #26288 Throw an exception when a customer does not exist…

8d893d5

… requests password reset #27269

magento-engcom-team merged commit ebede20 into magento:2.4-develop

m2-assistant bot commented Sep 4, 2020

Hi @Bal2018, 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.

engcom-Alfa mentioned this pull request

Fixed the customer rp token invalid issue. #28704

Closed

1 task

engcom-Charlie mentioned this pull request

Reset customer password bug. Fixed retrieving first item from Search Results #29770

Closed

5 tasks

engcom-Alfa mentioned this pull request

[Issue] Fixed the customer rp token invalid issue. #29663

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Auto-Tests: Covered Component: Customer Priority: P3 Progress: accept QA: Added to Regression Scope Release Line: 2.4 Severity: S3