Skip to content
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

Fix #23870 - State field editing for orders in admin #24019

Closed
wants to merge 1 commit into from
Closed

Fix #23870 - State field editing for orders in admin #24019

wants to merge 1 commit into from

Conversation

harddy
Copy link
Contributor

@harddy harddy commented Jun 27, 2019 •
edited
Loading

Fixed #23870
This PR will fix the state name issue.

Please review it and let me know if any changes are required.

Changes proposed in this Pull Request:

Closes #23870 .

How to test the changes in this Pull Request:

  1. Go to your shop page, add product to the cart and check out using any shipping / payment method
  2. Once the order has been placed, go to WooCommerce > Orders > YOUR ORDER
  3. For the shipping address, notice the presense of the County/State field
  4. Click on the "edit" button
  5. Earlier the State/County field was empty. After these changes, the State/County will be filled with the correct value.

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

Changed assets/js/admin/meta-boxes-order.js file to fill the state/county field value using jquery

@codecov
Copy link

codecov bot commented Jun 27, 2019 •
edited
Loading

Codecov Report

Merging #24019 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted Files Coverage Δ Complexity Δ
includes/class-wc-session-handler.php 60.68% <0%> (+0.68%) 48% <0%> (+1%) ⬆️

@ghost ghost mentioned this pull request Jun 28, 2019
Closed
3 tasks
kloon
kloon requested changes Jul 10, 2019
View reviewed changes
Copy link
Member

@kloon kloon left a comment

Choose a reason for hiding this comment

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

Hi @harddy , thanks for the contribution, but could you please update the PR description with the complete PR template filled out as you were asked to do when opening the PR? This just helps us with seeing what you went through, testing instructions, a changelog and a clear description of what is changing.

https://github.com/woocommerce/woocommerce/blob/master/.github/PULL_REQUEST_TEMPLATE.md

@kloon kloon added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Jul 10, 2019
@harddy
Copy link
Contributor Author

harddy commented Jul 11, 2019

Hello @kloon . Thanks for the feedback.

I have updated the PR description. Please let me know if any more changes are required.

@harddy harddy changed the title Fixed #23870 Fix #23870 - State field editing for orders in admin Jul 15, 2019
@claudiosanches claudiosanches added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Jul 30, 2019
dtwist
dtwist reviewed Jul 31, 2019
View reviewed changes
assets/js/admin/meta-boxes-order.js
@@ -39,10 +39,11 @@ jQuery( function ( $ ) {
var $this = $( this ),
country = $this.val(),
$state = $this.parents( 'div.edit_address' ).find( ':input.js_field-state' ),
$stateValue = $state.val(),
Copy link

Choose a reason for hiding this comment

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

variable should not use dollar sign; vars prefixed with $ indicate jQuery objects

dtwist added a commit to dtwist/woocommerce that referenced this pull request Jul 31, 2019
@dtwist
Alternate PR for woocommerce#23870 — Maintains "State" value when edi…
2320244 
…ting addresses

This code is an improvement to PR woocommerce#24019:
https://github.com/woocommerce/woocommerce/pull/24019/files/622816f2dc6b8c6d41bc438efb3921c1880d8eb1

It differs in two ways:
1) This includes a fix for State select lists (for countries that employ them instead of a simple text input)
2) It modified the variable name added in woocommerce#24019 to omit the leading dollar sign, since the variable is not a jQuery object.
@dtwist
Copy link

dtwist commented Jul 31, 2019

I'm not sure of the etiquette and protocol for suggesting an amendment to a PR someone else submitted. I left a comment on this PR regarding variable naming, but I also want to point out an additional fix for countries that employ lists of states. I've forked and committed changes here:
dtwist@2320244#diff-bdbde136beda4c33be12dd83d1bcdff2

@kloon kloon added this to the 3.7.1 milestone Jul 31, 2019
@nekranox
Copy link

nekranox commented Aug 1, 2019

This issue is causing the postmeta for this field to be deleted when orders are saved on the Edit Order screen. The issue only affects languages which have no predefined states in il8n -> states. Tested using UK English on stock install WC 3.6.5 / WP 5.2.2. Suggest that this is escalated and delivered sooner than 3.7.1 as it is causing data loss.

@dtwist
Copy link

dtwist commented Aug 1, 2019

The issue only affects languages which have no predefined states in il8n -> states.

This does affect countries with predefined states. See my comment on the bug report:
#23870 (comment)

@kloon kloon mentioned this pull request Aug 2, 2019
Merged
6 tasks
@kloon
Copy link
Member

kloon commented Aug 2, 2019

Thanks for the contribution @harddy and @dtwist, I have opened a new PR with the changes and we will review it there since we need to release this a bit urgently.

@kloon kloon closed this Aug 2, 2019
@kloon kloon removed this from the 3.7.1 milestone Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtwist dtwist dtwist left review comments

@kloon kloon kloon requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

State field editing for orders in admin
5 participants
@harddy @dtwist @nekranox @kloon @claudiosanches