Skip to content

Fix/28759 billing shipping addresses #29605

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
roykho merged 4 commits into from
Apr 7, 2021
Merged

Fix/28759 billing shipping addresses #29605

roykho merged 4 commits into from
Apr 7, 2021

Conversation

@barryhughes
Copy link
Member

@barryhughes barryhughes commented Apr 7, 2021 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

In some countries the state (county) field is optional—the UK is one such example. That means we might have a saved billing address with a county, and a saved shipping address without (or similar).

In these cases we should not copy the billing state/county to the shipping state/county: that action is really only appropriate if we don't already have a shipping address. Note that this problem also applies to the country field (though, it is easiest to see in relation to the state/county field).

Closes #28759.

How to test the changes in this Pull Request:

  1. Start by confirming you can see the original problem (checkout current trunk code, or else the 5.1.0 tag).
    1. Create a customer account and ensure shipping is set up.
    2. Add and save billing and shipping address, supplying a county for the billing address but not for the shipping address.
    3. Now purchase a product (it's easiest to see if you buy a simple physical product).
    4. Note that the shipping address appearing on the cart page includes the billing county.
  2. Now checkout this branch.
    1. Start by clearing/deleting the user session† and logging back in, otherwise the shipping address will be inherited from the first test (complete with inappropriately added state/county).
    2. Then, simply repeat the test. The billing county is not copied over to the shipping address.

† If you have your own snippet to kill the user session and dump the cart contents that's probably fine, or you could try using this snippet (updating the user ID, and if necessary the table prefix, accordingly) then flushing the cache.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Fix - If we have a non-empty shipping address then do not overwrite the state or country fields with billing address data.

@barryhughes barryhughes requested review from a team and roykho and removed request for a team April 7, 2021 00:32
@roykho
Copy link
Contributor

roykho commented Apr 7, 2021

In my testing it is still being copied when I go to checkout.

And here is my addresses for billing and shipping

@barryhughes
Copy link
Member Author

barryhughes commented Apr 7, 2021

Hey @roykho ... apologies, I missed a key step in my testing instructions (now updated). Can you try again?

@roykho
Copy link
Contributor

roykho commented Apr 7, 2021

I used incognito and thought that would be a cleared session but maybe not. Ok let me test again. And BTW you can use this tool to clear the session

@barryhughes
Copy link
Member Author

barryhughes commented Apr 7, 2021

I'm not sure incog by itself is enough, if you then log back into the same account, because of the cart persistence that happens via stored user meta data. Also, cool—was totally unaware of that tool. TIL 🙃

roykho
roykho reviewed Apr 7, 2021
View reviewed changes
includes/class-wc-customer.php
* Note that this does not indicate if the customer's shipping address
* is complete, only that one or more fields are populated.
*
* @return bool
Copy link
Contributor

@roykho roykho Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be good to add @since 5.3.0 versioning.

Copy link
Member Author

@barryhughes barryhughes Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

roykho
roykho approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@roykho roykho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested well and thanks for writing the tests!

@roykho roykho merged commit 8052fbe into trunk Apr 7, 2021
@roykho roykho deleted the fix/28759-billing-shipping-addresses branch April 7, 2021 21:34
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Apr 7, 2021
@roykho roykho added this to the 5.3.0 milestone Apr 7, 2021
@tammullen tammullen added testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Apr 20, 2021
@barryhughes barryhughes mentioned this pull request Aug 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@roykho roykho roykho approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release: add changelog Mark all PRs that have not had their changelog entries added. [auto]

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

Billing County is copied into Shipping Address

5 participants

@barryhughes @roykho @tammullen @woocommercebot