Add WooCommerce password change action. #585

joshcanhelp · 2018-11-13T17:23:59Z

Changes

Add Auth0 password change functionality to WooCommerce edit account + tests.

References

Reported on the WP support forums.

Testing

This PR adds unit tests. It can also be tested manually by:

Start with Auth0 and WooCommerce installed and configured
Go to the edit account page for WooCommerce (front-end of the site)
Attempt to change the password to something invalid; expect a visible warning, no changes made, and an error in the error log
Attempt to change the password to something valid; expect the change to complete with no errors

I included manual testing steps above, if applicable
This change adds unit test coverage
This change has been tested on the latest version of the platform

Checklist

All existing and new tests complete without errors
All code quality tools/guidelines in the Contribution guide have been run/followed
All active GitHub CI checks have passed

lib/profile/WP_Auth0_Profile_Change_Password.php

tests/testApiChangePassword.php

tests/testProfileChangePassword.php

lbalmaceda · 2018-11-13T21:25:56Z

tests/testProfileChangePassword.php


- // Call should fail because of a missing password.
+ // Test core WP password field.


sounds like part of another test case

First part is just setup to show that it's succeeding. Then we remove pieces and show that it's failing. Each failure case kicks back false regardless of how it fails so this is the best way to make sure that what we're actually doing is causing the failure.

First part is just setup to show that it's succeeding

This does sound like overkill. No need to add a success scenario before each failure case. Have a test asserting the success use case, have others asserting the failure cases (without repeating what you're already testing on other tests). If the test says "fails when password property is not provided" just call unset("password_field") and that will suffice.

tests/testProfileChangePassword.php

joshcanhelp · 2018-11-13T22:11:33Z

@lbalmaceda - Responded and changes made. Thank you!

codecov-io · 2018-11-13T22:13:17Z

Codecov Report

Merging #585 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #585      +/-   ##
============================================
+ Coverage     28.51%   28.52%   +0.01%     
- Complexity     1306     1308       +2     
============================================
  Files            51       51              
  Lines          4202     4203       +1     
============================================
+ Hits           1198     1199       +1     
  Misses         3004     3004

Impacted Files	Coverage Δ	Complexity Δ
lib/profile/WP_Auth0_Profile_Delete_Mfa.php	`100% <ø> (ø)`	`12 <0> (ø)`	⬇️
lib/profile/WP_Auth0_Profile_Delete_Data.php	`100% <ø> (ø)`	`8 <0> (ø)`	⬇️
lib/profile/WP_Auth0_Profile_Change_Password.php	`100% <100%> (ø)`	`12 <0> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6126a34...803f6f5. Read the comment docs.

lbalmaceda

I don't agree with how the tests are written. You're writting 1 test like this:

1. prepare config
2. make method work fine / succeed
3. change something in the config
4. test that method now fails
5. repeat (3) and (5) for each property under test

while it should be

1. prepare successful config
2. test method works fine

1. prepare failure config A
2. test method fails with err a

1. prepare failure config B
2. test method fails with err b

etc.. I'm leaving a general comment because I found many tests like this on this PR. All marked as "belongs to some other test case". Please correct those.

lbalmaceda · 2018-11-14T17:47:59Z

tests/testProfileChangePassword.php


- // Call should fail because of a missing password.
+ // Test core WP password field.


First part is just setup to show that it's succeeding

This does sound like overkill. No need to add a success scenario before each failure case. Have a test asserting the success use case, have others asserting the failure cases (without repeating what you're already testing on other tests). If the test says "fails when password property is not provided" just call unset("password_field") and that will suffice.

lbalmaceda · 2018-11-14T17:51:42Z

tests/testProfileChangePassword.php

 $this->assertFalse( $change_password->validate_new_password( $errors, false ) );

- $_POST['pass1'] = uniqid();
+ $_POST['password_1'] = uniqid();
+ $this->assertTrue( $change_password->validate_new_password( $errors, false ) );


Please clarify if this returns true when the password was changed or when the password was not changed. I'm confused with this line and the 3 above.

lbalmaceda · 2018-11-14T17:52:39Z

tests/testProfileChangePassword.php

+ /**
+ * Test that empty user data will skip the password update.
+ */
+ public function testThatMissingUserDataSkipsUpdate() {


Test is called "test that <condition> skips (the 'password' I suppose) update". How do you know when it was or wasn't changed?

tests/testProfileChangePassword.php

lbalmaceda · 2018-11-14T19:02:32Z

tests/testProfileChangePassword.php

+ $this->storeAuth0Data( $user->ID, 'auth0' );
+
+ $this->assertTrue( $change_password->validate_new_password( $errors, $user ) );
+ $this->assertEquals( $password, $_POST['pass1'] );


can this change in anyway when validate_new_password is called? If that doesn't happen, what I see is:

var a = 1; assert(a).equals(1);

can this change in anyway when validate_new_password is called?

Yes, alters the global var

lbalmaceda · 2018-11-14T19:10:53Z

tests/testProfileChangePassword.php

 $this->assertFalse( $change_password->validate_new_password( $errors, false ) );
+ }


do you need to do $this->assertEmpty( $errors->get_error_messages() ); here, as done on the test above? applies to other tests as well

Good to make sure in some cases. I added it where I thought it was needed.

lbalmaceda · 2018-11-14T19:11:51Z

tests/testProfileChangePassword.php

 $_POST['user_id'] = $user_id;
+


should you call something more explicit like $this->storeAuth0Data( null );?

lbalmaceda · 2018-11-14T19:12:35Z

tests/testProfileChangePassword.php

+ /**
+ * Test that a user without Auth0 data skips update.
+ */
+ public function testThatNonAuth0UserSkipsUpdate() {


sug. rename to testThatMissingAuth0UserDataSkipsUpdate

lbalmaceda · 2018-11-14T19:13:56Z

tests/testProfileChangePassword.php

+ // Provide everything except a user record.
+ $change_password = $this->getStub( true );
+ $_POST['pass1'] = uniqid();
+ $this->storeAuth0Data( $user_id );


"missing user record" but here you're passing a user id, from a user that was created right at the beginning of the test. What I am missing?

What I am missing?

A lot 😆 ... $this->storeAuth0Data() creates meta data for some user, $_POST['user_id'] is a global POST var that clues the method in that there is a user to change.

Add WooCommerce password change action.

3b5c31c

joshcanhelp added CH: Fixed labels Nov 13, 2018

joshcanhelp added this to the 3.8.1 milestone Nov 13, 2018

lbalmaceda reviewed Nov 13, 2018

View reviewed changes

PR feedback

4bffc76

lbalmaceda reviewed Nov 14, 2018

View reviewed changes

PR feedback on tests

803f6f5

lbalmaceda reviewed Nov 14, 2018

View reviewed changes

lbalmaceda approved these changes Nov 14, 2018

View reviewed changes

joshcanhelp merged commit 64d6852 into master Nov 14, 2018

joshcanhelp deleted the fix-woocommerce-password branch November 14, 2018 19:29

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WooCommerce password change action. #585

Add WooCommerce password change action. #585

joshcanhelp commented Nov 13, 2018

lbalmaceda Nov 13, 2018

joshcanhelp Nov 13, 2018

lbalmaceda Nov 14, 2018

joshcanhelp commented Nov 13, 2018

codecov-io commented Nov 13, 2018 •

edited

Loading

lbalmaceda left a comment

lbalmaceda Nov 14, 2018

lbalmaceda Nov 14, 2018

lbalmaceda Nov 14, 2018

lbalmaceda Nov 14, 2018

joshcanhelp Nov 14, 2018

lbalmaceda Nov 14, 2018

joshcanhelp Nov 14, 2018

lbalmaceda Nov 14, 2018

joshcanhelp Nov 14, 2018

lbalmaceda Nov 14, 2018

lbalmaceda Nov 14, 2018

joshcanhelp Nov 14, 2018


		// Call should fail because of a missing password.
		// Test core WP password field.

		$this->assertFalse( $change_password->validate_new_password( $errors, false ) );
		}

Add WooCommerce password change action. #585

Add WooCommerce password change action. #585

Conversation

joshcanhelp commented Nov 13, 2018

Changes

References

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp commented Nov 13, 2018

codecov-io commented Nov 13, 2018 • edited Loading

Codecov Report

lbalmaceda left a 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

codecov-io commented Nov 13, 2018 •

edited

Loading