Add checkout payment form #48

jrodger · 2019-04-16T09:56:40Z

First pass at implementing a payment form for this gateway. It's based on the minimal payments API branch which is still to be merged in, but I thought I'd get this up now anyway.

Closes #11.

Some parts that I'm not too sure on:

Stripe.js can be loaded twice if the Stripe extension is also active. This causes warnings in the console, and could cause conflicts where different versions are loaded. How do we want to handle this, if at all? Created issue Prevent stripe.js being loaded more than once #53
~~What browsers are we targeting? I'm assuming as old as reasonably possible for this part?~~ Follow WordPress Core policy: https://make.wordpress.org/core/handbook/best-practices/browser-support/
~~Are we planning to use any build or transpiling steps?~~ Handle in a separate issue (Configure build steps for public facing JavaScript #54)
Things to create new issues for:
- ~~Define a plugin version and use this to version the JavaScript when calling wp_register_script~~ Let's follow the same convention used in WC Admin (Add support for the SCRIPT_DEBUG constant #49)
- Create or wire-up an existing logger for sending messages to the server's logs (Debug logging #9)
- ~~Create plugin specific exception classes so that we can be smarter about what we create notices and log messages for (thinking about the logic in process_payments here).~~ There are some TODO notes in the code for this.
- ~~Card details get lost in the event form validation fails. I think I saw an issue for this on another gateway?~~ Behaviour is consistent with the existing Stripe extension.
- Properly style the UI (Style customer facing payment form #57)
- ~~The JavaScript is very basic. It'll be worth looking through what we have in the Stripe gateway and seeing what should be implemented here as well. Can create an issue for each piece of functionality.~~ This should fall out of work on other issues

To Test

Add a product to your basket
Proceed to the checkout and select "Credit Card (WooCommerce Payments)"
Enter a test credit card (e.g. 4242 4242 4242 4242 with any expiry and CVC)
Open your browser's network monitor and then click the "Place Order" button
The request to /?wc-ajax=checkout should contain a post variable wc-payment-token populated with a token value.
In the admin dashboard, the order notes should show a payment using WooCommerce Payments and a randomly generated transaction ID.

allendav · 2019-04-24T22:49:01Z

Stripe.js can be loaded twice if the Stripe extension is also active. This causes warnings in the console, and could cause conflicts where different versions are loaded. How do we want to handle this, if at all?

I propose we don't worry about this in the context of this PR, but please open an issue about this. I propose we avoid activating this plugin if we detect the Stripe extension is active and display a message prompting the merchant to disable that plugin.

jrodger · 2019-04-25T11:19:27Z

What browsers are we targeting? I'm assuming as old as reasonably possible for this part? Are we planning to use any build or transpiling steps?

We're about to merge @v18's work on the admin JavaScript, which is built with Webpack. Is anyone aware of any other projects that build both admin and client JavaScript with the same tools? Might be nice?

DanReyLop · 2019-04-26T11:44:45Z

Is anyone aware of any other projects that build both admin and client JavaScript with the same tools? Might be nice?

My 2 cents: We shouldn't need transpiling, polyfills, or Webpack for the customer-side Javascript. Looking at the Stripe extension, it just needs 2 (big) JS client-side files: https://github.com/woocommerce/woocommerce-gateway-stripe/tree/96ba18711819d1d5e73a630c4cc6950980811105/assets/js

Those files use jQuery and are mindful of old browser compatibility (using var instead of const, using function instead of () => {}, etc). If we follow the same guidelines, the only "tooling" we'll need for the customer-facing JS is a minifier.

Transpiling can help us develop and maybe reduce the amount of code we need to write, but it will bloat the JS bundle that needs to be shipped to all the customers, so I (barely) don't think it's worth it. What do you folks think?

jrodger · 2019-04-29T13:15:11Z

Transpiling can help us develop and maybe reduce the amount of code we need to write, but it will bloat the JS bundle that needs to be shipped to all the customers, so I (barely) don't think it's worth it. What do you folks think?

I've done a quick test using the stripe.js file from the Stripe extension:

Raw: 22,703 bytes
Uglify-ed: 10,984 bytes
Webpack-ed: 11,914 bytes

So at least at that size there is about 1Kb in it. With our settings and testing on smaller files that 1Kb seems to be a constant flat addition.

I think there might be some value in considering using Webpack for all our JavaScript building, especially if our non-admin JavaScript starts getting larger than a few Kb. I'll bump the discussion of how to minify this to another issue though.

For now I can re-work the JavaScript so it'll work in our target browsers without needing transpiling, that keeps our options open and then we can tackle this later.

DanReyLop · 2019-04-29T15:26:42Z

Webpack-ed: 11,914 bytes

It depends on how did you configure Webpack. If we start adding Babel transpilation and polyfills, that's when those kb start to add up. The thing with the stripe.js file is that it doesn't use any fancy modern JS, so even if you added a transpilation step it won't add much bloat.

DanReyLop · 2019-04-29T18:14:17Z

includes/class-wc-payment-gateway-wcpay.php

+	/**
+	 * Enqueues scripts for the checkout.
+	 */
+	public function payment_scripts() {


Why can't we just enqueue the scripts in payment_fields()? We wouldn't need to check for is_cart() || is_checkout() etc.

That makes sense, and it looks like some of the other payment integrations do the same thing.

DanReyLop · 2019-04-29T18:15:31Z

includes/class-wc-payment-gateway-wcpay.php

 	 * @return array|null
+	 * @throws Exception - PHPCS seems convinced this is possible, but it isn't.


LOL, this is funny but also sad

I really don't want to merge this bit. I don't suppose you can see any reason PHPCS is throwing this up!? Or worst case, a way to re-write the code so it's happier? This function will change a lot as we add functionality, so I'm not too worried about changing it to satisfy PHPCS at this point.

Bad news: squizlabs/PHP_CodeSniffer#2149

Throwing and cacthing exceptions in the same function is a code-smell/antipattern or whatever you want to call it. It doesn't matter because this is temporary code, but I guess the bug in PHPCS is indirectly enforcing that :)

Ah ha, thanks for the link. I'll leave a TODO for now so we can revisit this when implementing the function "properly".

I've reworked this a little and moved the code which was throwing an exception into a separate function. It also helped with muting some of the PHPCS warnings we want to suppress.

DanReyLop · 2019-04-29T18:32:04Z

includes/class-wc-payment-gateway-wcpay.php

+			$transaction_id = '';
+
+			if ( $amount > 0 ) {
+				// TODO: Do we need to check for a nonce at this point?


There's already a woocommerce-process-checkout-nonce built-in, so I don't think we need another here. Not sure why the Stripe extension is using its own stripe_checkout_return_handler.

DanReyLop · 2019-04-29T18:32:38Z

includes/class-wc-payment-gateway-wcpay.php

+					wp_unslash( $_POST['wc-payment-token'] )
+				); // wpcs: CSRF ok.
+
+				// TODO: implement the actual payment (that's the easy part, right?).


There's no need for this TODO anymore 🎉

DanReyLop · 2019-04-29T18:49:06Z

includes/class-wc-payment-gateway-wcpay.php

-			// TODO: implement the actual payment (that's the easy part, right?).
-			try {
-				$charge = $this->payments_api_client->create_charge( $amount, 'dummy-source-id' );
+				// TODO: Check sanitization.


I think wc_clean is more than enough. I wouldn't even call wp_unslash.

DanReyLop · 2019-04-29T18:55:44Z

includes/class-wc-payment-gateway-wcpay.php

+		wp_register_script(
+			'wc-payment-checkout',
+			plugins_url( 'assets/js/wc-payment-checkout.js', WCPAY_PLUGIN_FILE ),
+			array( 'wc-payment-stripe', 'wc-checkout' ),


Since you changed the previous wp_register_script, this line needs to be:

Suggested change

array( 'wc-payment-stripe', 'wc-checkout' ),

array( 'stripe', 'wc-checkout' ),

🤦‍♂ thanks

Adds a test mode setting along with settings for public and private keys for both live and test mode.

The payment UI shown to the store's customer is made up of four parts: * A description of the payment method * Somewhere for our JavaScript to create and place the payment fields * An area for displaying validation errors * An input to be submitted back with a payment token

jrodger · 2019-05-01T10:50:38Z

Thanks for all the pointers @DanReyLop. I think the last 4 commits should address them.

If they look OK then I'll tidy up the commits and merge this in.

DanReyLop

Cool, code looks good, and now it works :)

Pass the payment token to the payments API client.

This includes outputting the JavaScript. We also rework the creation of the payment gateway to make it easier to access from the other setup steps.

There won't be any PHP files in this folder, so it doesn't need to be checked.

jrodger added the pr: needs review label Apr 16, 2019

jrodger requested a review from a team April 16, 2019 09:56

jrodger self-assigned this Apr 16, 2019

jrodger force-pushed the add/checkout-payment-form branch from f6ccbee to b2dd249 Compare April 17, 2019 10:16

RadoslavGeorgiev self-requested a review April 24, 2019 15:28

jrodger force-pushed the add/minimal-payments-api-client branch from 05a2082 to 612442f Compare April 25, 2019 10:03

jrodger force-pushed the add/checkout-payment-form branch from b2dd249 to 33af2d4 Compare April 25, 2019 10:19

jrodger changed the base branch from add/minimal-payments-api-client to master April 25, 2019 10:20

jrodger mentioned this pull request Apr 25, 2019

Prevent stripe.js being loaded more than once #53

Closed

jrodger mentioned this pull request Apr 29, 2019

Configure build steps for public facing JavaScript #54

Closed

DanReyLop reviewed Apr 29, 2019

View reviewed changes

jrodger added 2 commits April 30, 2019 14:24

Add settings for Stripe keys

152a729

Adds a test mode setting along with settings for public and private keys for both live and test mode.

jrodger force-pushed the add/checkout-payment-form branch from c085092 to 1d59356 Compare May 1, 2019 09:34

DanReyLop approved these changes May 2, 2019

View reviewed changes

jrodger added 3 commits May 2, 2019 15:07

Accept payment token when processing payment

fd100bd

Pass the payment token to the payments API client.

Add JavaScript for rendering and controlling payment fields

527d110

This includes outputting the JavaScript. We also rework the creation of the payment gateway to make it easier to access from the other setup steps.

Exclude the assets folder from PHPCS

b676571

There won't be any PHP files in this folder, so it doesn't need to be checked.

jrodger force-pushed the add/checkout-payment-form branch from 3b4fb18 to b676571 Compare May 2, 2019 14:35

jrodger merged commit 4dd7f6f into master May 2, 2019

jrodger deleted the add/checkout-payment-form branch May 2, 2019 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checkout payment form #48

Add checkout payment form #48

jrodger commented Apr 16, 2019 •

edited

Loading

allendav commented Apr 24, 2019

jrodger commented Apr 25, 2019

DanReyLop commented Apr 26, 2019

jrodger commented Apr 29, 2019

DanReyLop commented Apr 29, 2019

DanReyLop Apr 29, 2019

jrodger Apr 30, 2019

DanReyLop Apr 29, 2019

jrodger Apr 30, 2019

DanReyLop Apr 30, 2019

jrodger Apr 30, 2019

jrodger May 1, 2019

DanReyLop Apr 29, 2019

DanReyLop Apr 29, 2019

DanReyLop Apr 29, 2019

DanReyLop Apr 29, 2019

jrodger Apr 30, 2019

jrodger commented May 1, 2019

DanReyLop left a comment

		* @return array\|null
		* @throws Exception - PHPCS seems convinced this is possible, but it isn't.

	array( 'wc-payment-stripe', 'wc-checkout' ),
	array( 'stripe', 'wc-checkout' ),

Add checkout payment form #48

Add checkout payment form #48

Conversation

jrodger commented Apr 16, 2019 • edited Loading

To Test

allendav commented Apr 24, 2019

jrodger commented Apr 25, 2019

DanReyLop commented Apr 26, 2019

jrodger commented Apr 29, 2019

DanReyLop commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrodger commented May 1, 2019

DanReyLop left a comment

Choose a reason for hiding this comment

jrodger commented Apr 16, 2019 •

edited

Loading