Skip to content
This repository has been archived by the owner on Jun 15, 2022. It is now read-only.

issue fix of order_billing_index storing in database,issue id:#109 #110

Closed
wants to merge 13 commits into from

Conversation

ronakganatra9
Copy link

@ronakganatra9 ronakganatra9 commented Feb 7, 2019

Fixed Issue in updating the billing, shipping address index when we edit anything without the editing first name #109
It was only checking that the billing first name is changed or not, if it was changed then the billing_index column will be updated so if anyone has changed postal code only then database was not updating. So, Instead of checking billing_first_name is set or not I have changed it to that if billing index is changed then it will update the whole billing _index. It has fixed the issue and test on it.

##Reproduce steps:

  • Edit any order
  • Update Billing index zipcode only not name or anything.
  • save the order check the database.
  • Now the billing index column is updated with the new zip. Before it was not updaing

@coveralls
Copy link

coveralls commented Feb 7, 2019

Pull Request Test Coverage Report for Build 247

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.1%) to 96.798%

Totals Coverage Status
Change from base Build 225: 2.1%
Covered Lines: 514
Relevant Lines: 531

💛 - Coveralls

@stevegrunwell stevegrunwell self-assigned this Feb 7, 2019
@stevegrunwell stevegrunwell self-requested a review February 7, 2019 15:41
Copy link
Contributor

@stevegrunwell stevegrunwell left a comment

Choose a reason for hiding this comment

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

Hey @ronakganatra9, thank you for the PR! The change itself looks good, but could you please include tests covering the change with your pull request? As the plugin (and WooCommerce itself) evolves, we want to make sure we don't see regressions down the road 😄

Thanks again!

@ronakganatra9
Copy link
Author

Hi Steven,

Greetings of the day...!!!

Thanks for the quick action. I am not able to generate a test coverage report on my ubuntu. Do you have any reference link where I can get the steps to generate the test coverage?

Let me know the details so I can generate it and complete my PR.

Thanks & Regards,
Ronak

@stevegrunwell
Copy link
Contributor

@ronakganatra9, you don't need to generate the test coverage (Coveralls will handle that for us), but rather add a test case to reproduce the original issue. This helps prevent regressions if, for instance, your change were ever reversed.

For instance, your test might capture that changes to any of the fields make up the billing index (first name, last name, address, city, etc.) will result in the database column row being updated as well.

If you need help, please feel free to reach back out! 😄

@ronakganatra9
Copy link
Author

@ronakganatra9, you don't need to generate the test coverage (Coveralls will handle that for us), but rather add a test case to reproduce the original issue. This helps prevent regressions if, for instance, your change were ever reversed.

For instance, your test might capture that changes to any of the fields make up the billing index (first name, last name, address, city, etc.) will result in the database column row being updated as well.

If you need help, please feel free to reach back out!

I have added the reproduce steps to my description:

Let me know anything else should I have to do?

@ronakganatra9
Copy link
Author

ronakganatra9 commented Apr 4, 2019

When I run test coverage by composer test-coverage it gives me follwing error:
Script phpunit --testsuite=plugin --coverage-html=tests/coverage handling the test-coverage event returned with error code 255

Can you please guide me If I am missing it anything?
order_table_test_coverage

@erikdemarco
Copy link

Any update about this pull request? I also experience the same bug. This pull request can fix the bug.

@lukecav lukecav closed this Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants