Add country-specific test card numbers for credit card testing #9688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timur27
changed the title
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
|
I've mentioned a couple of JN sites here, feel free to click around there but also it'd be great to setup a new one for a new country and test this out. |
timur27
requested review from
a team and
frosso
and removed request for
a team
frosso
approved these changes
Nov 8, 2024
| * @return string | ||
| */ | ||
| abstract public function get_testing_instructions(); | ||
| abstract public function get_testing_instructions( string $account_country = null ); |
There was a problem hiding this comment.
I find it a little bit confusing to type the argument's $account_country as string (both with PHP 7.0 typing and with the PHP docblock), but setting null as its default value.
A few questions:
- In which scenario would
$account_countrybenullor need a default value?get_account_country()will returnUSin case ofnullvalue: - Should
nullbe also typed in the docblock?
There was a problem hiding this comment.
I agree, no nullable default is needed. I removed it in 5712fbb
| * @return string Test card number | ||
| */ | ||
| public static function get_test_card_for_country( string $country_code ) { | ||
| return self::$country_test_cards[ $country_code ] ?? self::$country_test_cards['US']; |
There was a problem hiding this comment.
What do you think of using the country codes in Country_Code for consistency? (you'll probably need to flip the include_once in includes/class-wc-payments.php)
There was a problem hiding this comment.
Makes sense, applied in 11a914b but let me know if I misunderstood your comment and you meant something different.
There was a problem hiding this comment.
That's great, thank you! Just this one missing:
Suggested change
| return self::$country_test_cards[ $country_code ] ?? self::$country_test_cards['US']; | |
| return self::$country_test_cards[ $country_code ] ?? self::$country_test_cards[ Country_Code::UNITED_STATES ]; |
timur27
force-pushed
the
add/country-specific-test-cards
branch
from
d3a6bc0
to
5712fbb
Compare
timur27
force-pushed
the
add/country-specific-test-cards
branch
from
a1921e8
to
3fc0170
Compare
frosso
approved these changes
Nov 8, 2024
|
My only nitpick here would be that |
|
@aheckler good point, thank you. Looking at the list of country codes WooPayments implemented which contains all the countries added as part of the testing card addition and even more, I think it's fine to keep them. In terms of the code, it shouldn't be a big deal. If there is any concern about keeping non-supported countries in the plugin, we can remove them pretty quick, just let us know! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #9619
Changes proposed in this Pull Request
This PR adds support for country-specific test card numbers in test mode. When a merchant is testing payments, they will now see the appropriate test card number based on their account's country. The test card numbers are sourced from Stripe's documentation.
Testing instructions
Country_Test_Cardsis in line with countries list mentioned in Stripe docs$account_response['country'] = 'GB';before this line on the server4242 ...card number is usednpm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge