Conversation
ksere
left a comment
ksere
left a comment
There was a problem hiding this comment.
Left some comments, mostly to make sure we have considered some things.
includes/class-wc-gateway-amazon-payments-advanced-abstract.php
Outdated
Show resolved
Hide resolved
includes/class-wc-gateway-amazon-payments-advanced-abstract.php
Outdated
Show resolved
Hide resolved
| <p> | ||
| <?php | ||
| esc_html_e( | ||
| 'Ranges covering thousands or more of postcodes may slow down your site, since Amazon Pay does not support postcode ranges we loop from the minimum until we reach the maximum in order to include them all. Try to use the asterisk (*) wildcard when possible for faster loading times.', |
There was a problem hiding this comment.
We should test with a large number of postcodes (99k ?) to make sure it works.
If it ends up clogging the payload & making API requests fail, it may be better to try something different.
There was a problem hiding this comment.
You are correct, when using 99k zip codes as range Amazon API fails. Will try to think of an alternative.
There was a problem hiding this comment.
Hmm, that's potentially a blocker.
One option would be to notify the user that their settings are "incompatible" with what Amazon supports, and not include those postcodes in the API call - so that it doesn't fail.
But again we would need to know/find what the threshold should be at which we are triggering that scenario.
There was a problem hiding this comment.
Implemented an algorithm where ranges are converted to wildcards using the '?' symbol supported by the Amazon API. Let me know your thoughts
ksere
left a comment
ksere
left a comment
There was a problem hiding this comment.
Looks good,
Left a comment about the new transient in use.
Also if possible the new stuff could be split into another file, maybe a helper class or somewhere else fitting better. Just to not add to the weight of the file.
Didn't get into how it actually works, the approach looks good, will leave it for QA to figure out where it breaks 🚀
| case 'postcode': | ||
| $postcode = $location->code; | ||
| if ( strstr( $postcode, '...' ) ) { | ||
| $pers_ps_array = get_transient( 'woocommerce_amazon_pa_ps_rules_' . self::RANGE_TO_WILDCARDS_ALGO_VERSION . '_' . md5( $postcode ) ); |
There was a problem hiding this comment.
Is the called method so heavy that we need a transient?
Usually we want to cache heavy DB queries or calls to remote resources.
Calculations, string operations etc can add up in some cases but here I am not sure a transient will be faster at least on setups without Object caching.
There was a problem hiding this comment.
Yea you are right. In most cases, running the method would be less load than querying the DB. I will ask for a extensive QA checks to find out how it works in various cases. Will remove the transient for the time being.
alexiglesias31
left a comment
alexiglesias31
left a comment
There was a problem hiding this comment.
Just a minot comment, not a blocker.
3 participants
All Submissions:
Changes proposed in this Pull Request:
Closes woo#117 .
How to test the changes in this Pull Request:
Other information:
Changelog entry