woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
47 stars 21 forks source link

Update the shipping time controllers to handle the maximum shipping time #2590

Closed jorgemd24 closed 1 month ago

jorgemd24 commented 2 months ago

Changes proposed in this Pull Request:

Part of pcTzPl-2qP-p2

This PR updates the shipping time controllers to allow for updating and fetching the maximum shipping time, which will be used in a second PR that I’m currently working on for the UI.

Screenshots:

Detailed test instructions:

Prerequisites

  1. Follow the steps mentioned in this PR to update the database: https://github.com/woocommerce/google-listings-and-ads/pull/2520

  1. Make a GET /wc/gla/mc/shipping/times request and check that the max_time property is now included in the response.
  2. Update one of the shipping times by making the following request: POST gla/mc/shipping/times and the following body: {"country_code":"ES","time":2, "max_time": 3}
  3. Make the following request for one specific country: GET gla/mc/shipping/times/YOUR_COUNTRY for example GET gla/mc/shipping/times/ES and check that the max_time property is now included.

Additional details:

Changelog entry

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 77.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.3%. Comparing base (8f3231f) to head (af40ca3). Report is 148 commits behind head on update/shippings-settings-phase-1.

Files with missing lines Patch % Lines
...trollers/MerchantCenter/ShippingTimeController.php 85.3% 5 Missing :warning:
src/DB/Table/ShippingTimeTable.php 0.0% 4 Missing :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590/graphs/tree.svg?width=650&height=150&src=pr&token=UROWUPF1LX&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) ```diff @@ Coverage Diff @@ ## update/shippings-settings-phase-1 #2590 +/- ## ===================================================================== + Coverage 65.2% 66.3% +1.0% - Complexity 4588 4598 +10 ===================================================================== Files 803 475 -328 Lines 23012 17927 -5085 Branches 1234 0 -1234 ===================================================================== - Hits 15012 11879 -3133 + Misses 7833 6048 -1785 + Partials 167 0 -167 ``` | [Flag](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [js-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `?` | | | [php-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `66.3% <77.5%> (+0.7%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [...ers/MerchantCenter/ShippingTimeBatchController.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590?src=pr&el=tree&filepath=src%2FAPI%2FSite%2FControllers%2FMerchantCenter%2FShippingTimeBatchController.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0FQSS9TaXRlL0NvbnRyb2xsZXJzL01lcmNoYW50Q2VudGVyL1NoaXBwaW5nVGltZUJhdGNoQ29udHJvbGxlci5waHA=) | `92.5% <100.0%> (+0.4%)` | :arrow_up: | | [src/DB/Table/ShippingTimeTable.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590?src=pr&el=tree&filepath=src%2FDB%2FTable%2FShippingTimeTable.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0RCL1RhYmxlL1NoaXBwaW5nVGltZVRhYmxlLnBocA==) | `50.0% <0.0%> (-3.8%)` | :arrow_down: | | [...trollers/MerchantCenter/ShippingTimeController.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590?src=pr&el=tree&filepath=src%2FAPI%2FSite%2FControllers%2FMerchantCenter%2FShippingTimeController.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0FQSS9TaXRlL0NvbnRyb2xsZXJzL01lcmNoYW50Q2VudGVyL1NoaXBwaW5nVGltZUNvbnRyb2xsZXIucGhw) | `93.6% <85.3%> (+53.4%)` | :arrow_up: | ... and [327 files with indirect coverage changes](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2590/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)
puntope commented 1 month ago

@jorgemd24 I see one potential issue in the controller where you can set -1 as value for time and 'max_time':

  1. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": -1,time: -1 }
  2. Request is successful
  3. Request GET wp-json/wc/gla/mc/shipping/times/es
  4. It returns max_time and time as -1
Screenshot 2024-09-09 at 11 24 58 Screenshot 2024-09-09 at 11 24 51
puntope commented 1 month ago

Another potential issue I see is that we don't require time and max_time which is overrides as 0 is you don't set them.

  1. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": 1, time: 1 }
  2. Request is successful
  3. Request GET wp-json/wc/gla/mc/shipping/times/es
  4. It returns max_time and time as 1
  5. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": 2 }
  6. Request is successful
  7. Request GET wp-json/wc/gla/mc/shipping/times/es
  8. It returns max_time 2 but time as 0 (it shouldn't be 1?)
puntope commented 1 month ago

Finally, one extra issue I see is that time can be set as a bigger number that max_time

  1. Request POST wp-json/wc/gla/mc/shipping/times with {"country_code":"es, "max_time": 1, time: 2 }
  2. Request is successful
  3. Request GET wp-json/wc/gla/mc/shipping/times/es
  4. It returns max_time 1 and time as 2
Screenshot 2024-09-09 at 11 39 21 Screenshot 2024-09-09 at 11 37 39
jorgemd24 commented 1 month ago

Thanks, @puntope, for your helpful review! I've made the changes based on your comments. Could you please take another look? Thanks!