-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RPP: Minimal Checkout #7332
RPP: Minimal Checkout #7332
Conversation
This is required, because the currently used PSR-11 package defines interfaces for exceptions, but those interfaces do not implement the `Throwable` interface. On the other hand, the League container does extend the `Exception` class, so in practice, all exceptions will implement the Throwable interface.
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.42 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me and almost ready to be shipped. Two small suggestions below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either with or without the generic exception handling mentioned here but let's make sure we can handle errors properly in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my inline comments (most of which are nitpick), I've tried to test this PR in my local dev site. iI turns that there are some errors. @RadoslavGeorgiev - could you have a look at them?
-
After enabling enough factors in WCPay Dev Tools for my testing flow, I could test it, and face this error
Too few arguments to function WCPay\Internal\Payment\State\InitialState::__construct(), 4 passed and exactly 5 expected
. This can be easily fixed by updatingPaymentsServiceProvider
https://gist.github.com/htdat/5d15b253341e8c0e2e8f304ad40de47d -
After applying the diff in the gist link above, I got another error, which I have not been able to figure out yet.
PHP Fatal error: Uncaught TypeError: Argument 1 passed to WCPay\Core\Server\Request\Create_Intention::set_fingerprint() must be of the type string, null given, called in /var/www/html/wp-content/plugins/woocommerce-payments/src/Internal/Service/PaymentRequestService.php on line 41 and defined in /var/www/html/wp-content/plugins/woocommerce-payments/includes/core/server/request/class-create-intention.php:156
Stack trace:
#0 /var/www/html/wp-content/plugins/woocommerce-payments/src/Internal/Service/PaymentRequestService.php(41): WCPay\Core\Server\Request\Create_Intention->set_fingerprint(NULL)
#1 /var/www/html/wp-content/plugins/woocommerce-payments/src/Internal/Payment/State/InitialState.php(102): WCPay\Internal\Service\PaymentRequestService->create_intent(Object(WCPay\Internal\Payment\PaymentContext))
#2 /var/www/html/wp-content/plugins/woocommerce-payments/src/Internal/Service/PaymentProcessingService.php(69): WCPay\Internal\Payment\State\InitialState->process(Object(WCPay\Internal\Payment\PaymentRequest))
#3 /var/www/html/w in /var/www/html/wp-content/plugins/woocommerce-payments/includes/core/server/request/class-create-intention.php on line 156
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- LGTM, pre-approving! Only one minor comment, then it's good to ship 🚢.
- I've run another manual testing too. Everything works like a charm 🎉
Fixes #7497
Changes proposed in this Pull Request
This PR adds everything needed for a happy-path basic payment to the new checkout process.
SystemErrorState
andPaymentErrorState
classes are added.PaymentContext
receives support for the following props:currency
,manual_capture
,metadata
,level3_data
,metadata
,cvc_confirmation
,fingerprint
,user_id
andcustomer_id
PaymentRequest
:cvc_confirmation
andfingerprint
.process
method ofInitialState
now calls all needed services to process a simple payment:OrderService
is now responsible for both loading data, and post-processing. We will need to split it in a meaningful way, suggestions are welcome. Keep in mind that we'll probably work on them later through a separate issue.PaymentRequestService
is responsible for generating API/server requests, keeping things composed instead of inheriting or huge classes.WC_Payments_Customer_Service
class has a new method to create a customer based on order details. Should we keep it there, or add it to a new service?Testing instructions
As always, check tests first, and make sure that they are sound.
With this PR, checkout with a saved/new payment method should work, adding the necessary order status, data and notes. Sounds simple, but just try it.
npm run changelog
to add a changelog file, choosepatch
to 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.