feat: GooglePay live account test mode notice #10429
Conversation
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: -256 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
| @@ -3,15 +3,6 @@ | |||
| */ | |||
| import { createContext } from 'react'; | |||
|
|
|||
| const WCPaySettingsContext = createContext( { | |||
There was a problem hiding this comment.
The main reason for all the changes is here - the default value of createContext for WCPaySettingsContext was incompatible with the values declared in the client/globals.d.ts file.
By assigning wcpaySettings as the default value of createContext, we can ensure that the two are at least in sync.
All I wanted was to ensure the isLive boolean was correctly typed...
frosso
requested review from
a team and
FangedParakeet
and removed request for
a team
frosso
changed the title
…ompatibility-notice
…ompatibility-notice
frosso
changed the title
There was a problem hiding this comment.
OK, first of all, cheers for all the changes here. It took me a while to pore through them all, but I think the tighter typing is a pretty large standard of life improvement.
FWIW, I tested this PR by simply mocking out the WC_Payments_Account class and hard-coding the value of the isLive and everything worked as expected. Implementation looks clean and tidy, so no issues there.
Sadly and truly fatefuly, just as I was about to approve this PR, for some reason I decided to re-read the changelog of all things and I noticed a typo that will either make your ribs tickle or make you want to punch mine. 😁
I'd normally allow it in just the changelog, but it's kind of scattered throughout these changes as well, so if you wouldn't mind updating this PR and removing that devilish "L" from wherever it has managed to sneak in, I think we'll be all good and I'll immediately approve. Apologies in advance for being cursed with these eagle eyes of mine. Cheers.
changelog/feat-google-play-test-mode-live-account-compatibility-notice
Outdated
Show resolved
Hide resolved
| expect( container.firstChild ).toBeNull(); | ||
| } ); | ||
|
|
||
| it( 'does not render when GooglePlay is not enabled', () => { |
There was a problem hiding this comment.
| it( 'does not render when GooglePlay is not enabled', () => { | |
| it( 'does not render when GooglePay is not enabled', () => { |
Aha, I almost missed this one, until I decided to double-check the changelog. Sorry, lol.
There was a problem hiding this comment.
Same goes for this filename.
There was a problem hiding this comment.
I think this filename should probably be google-pay-test-mode-compatibility-notice.tsk...and we probably need to amend all the places where this is imported too lol. I think this is the last occurence of this that I could find.
|
@FangedParakeet thank you for noticing - fixed in 5d170c6 . Honestly, I don't even know why my fingers continuously slipped on that L. You'll notice in the PR's title before your review comment, on the branch name, in the PR's description... I just don't know what possessed my fingers 😅 
|
There was a problem hiding this comment.
It's less critical than the other ones, but could we update this filename too please? 🙏
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import GooglePayTestModeCompatibilityNotice from '../google-play-test-mode-compatibility-notice'; |
There was a problem hiding this comment.
| import GooglePayTestModeCompatibilityNotice from '../google-play-test-mode-compatibility-notice'; | |
| import GooglePayTestModeCompatibilityNotice from '../google-pay-test-mode-compatibility-notice'; |
Ooh, this one is a bit more critical. Could we just update this filename and related imports too please? Ta. 👍
There was a problem hiding this comment.
Ugh, I thought I had caught everything in the last reivew. But no... she still plays when she should pay. My keyboard was never the faithful type 💔
Appreciate you catching that, thank you for making me see the truth, sorry about the multiple review rounds! I think (hope) I now fixed everything in ae90d7a . Time for me to move on...
|
There are a bunch of merchant E2E tests failing on WC latest. Taking a quick peek at them, it doesn't appear that any of them are really even on the Payments settings pages and in some of the failing multicurrency tests, it appears as if the multicurrency widget isn't even found on the E2E test site, which is particularly sus. It's possible that updating the branch and rerunning will resolve them, but I think some of the same E2E tests have been failing pretty consistently on the last couple commits. Can you just double-check this? If it appears that it is indeed something unrelated and the result of a flaky test, can we just open a quick cheeky new issue to capture resolving these failures? Cheers. |
…ompatibility-notice
…t. each line you type, a cruel mistake. feels like a game she loves to play, a never-ending push-and-pain. you press my keys but never stay, a line of code then walk away
Still checking this, since I don't see the failure on other PRs |
|
Ok, it should be solved in dd271c9 |
…ompatibility-notice
There was a problem hiding this comment.
Cheers for those pesky little edits! I think this LGTM now. 👍
There is one unit test failing and, while I can't see it failing on a couple other PRs that I glanced at, looking at the test I'm pretty sure it's not related to any of these changes and looking at the failure specifically, it appears to be an error that is off by one-second, so I'm leaning towards this being a flaky test that needs to be fixed (probably shouldn't be an assertSame on that time comparison, but some slightly broader window maybe).
Can you have a quick look at that failing test and, if you come to the same conclusion that I did, can you just pop up a quick issue to fix it (maybe remove the test for the time being--doesn't have to be in this PR), ping the relevant team, and then we can proceed with merging these changes? Cheers!
|
Looks like it passed on retry 💪 |
frosso
deleted the
feat/google-play-test-mode-live-account-compatibility-notice
branch
Fixes: #10430
Related to #4083
Asked here: paJDYF-gMH-p2
Changes proposed in this Pull Request
Adding a notice to GooglePay/ApplePay when:
I needed to make a few changes to the types of the
WCPaySettingsContextprovider. So I fixed a few areas here and there.I renamed some problematic tests from
tsxtojs, given the discussion here: paJDYF-f5D-p2Testing instructions
You'll need a live account for this.
As an alternative, you can just "mock" a "live account" by changing this line to return
true: https://github.com/Automattic/woocommerce-payments/blob/feat/google-play-test-mode-live-account-compatibility-notice/includes/class-wc-payments-account.php#L354npm 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