Simple Payments: CPTs and Shortcode #7409

artpi · 2017-06-30T09:12:51Z

This is Jetpack part of architecture described in p8RSuw-2E-code #comment-51

Introduces custom post type for orders and products (products being stored payment button configurations)
Enables these custom post types over /posts endpoints
describes order data structure
describes product data structre
introduces JS dependencies for displaying the checkout button
introduces a shortcode to make checkout button appear

After this is merged in JP, we will port it to wpcom.
We will communicate with JP sites using the code below and with wpcom sites via reqular wp_query.

Testing

Check out this branch to your JP sandbox
Create new product for your store using code below
(replace blog_id and domain with your JP sandbox)

function test_meta( $blog_id, $domain ) {
	$token = Jetpack_Data::get_access_token_by_blog_id_user_id( $blog_id, wpcom_get_blog_owner( $blog_id ) );
	$installed = Jetpack_Client::json_api_request( array(
	    'url' => 'https://public-api.wordpress.com/rest/v1.1/sites/'.$domain.'/posts/new',
	    'blog_id' => $blog_id,
	    'jetpack_token' => $token,
	    'method'=> 'POST',
	    'body' => json_encode(
	        array(
	        	'type' => 'jp_pay_product',
				'title'=>'Best potato in the whole universe',
				'content' => 'Buy our organic best potato. It is the best.',
	            'metadata' => array(
	                array(
	                	'key' => 'spay_price',
						'value' =>  '34.00'
	                ),
	                array(
	                	'key' => 'spay_currency',
	                	'value' => 'USD'
	                ),
	                array(
	                	'key' => 'spay_cta',
	                	'value' => 'Buy'
	                ),
	                array(
	                	'key' => 'spay_email',
	                	'value' => 'artur.piszek+test2@automattic.com'
	                ),
	                array(
	                	'key' => 'spay_multiple',
	                	'value' => '0'
	                ),
	                array(
	                	'key' => 'spay_status',
	                	'value' => 'enabled'
	                )
	            )
	        )
	    )
	) );

	print_r( $installed );	
}
test_meta( 118642378, 'arturpiszek.wpsandbox.me' );

note the outputted postID
go to your JP site, use the shortcode:

[simple-payment id=84]

(replace ID with your post id)

See the paypal button appear:

You can test a full flow if you sandbox D6278-code

CC @lamosty @sirbrillig @jhnstn @retrofox @jsnajdr

lamosty · 2017-06-30T20:30:48Z

modules/simple-payments/simple-payments.php

+		 * spay_currency - currency code
+		 * spay_cta - text with "Buy" or other CTA
+		 * spay_email - paypal email
+		 * spay_multiple - allow for multiple items


We should also probably store if a product is "enabled" or "disabled" (if it should show or show out of stock).

I added "status".

lamosty · 2017-06-30T20:34:38Z

Except for the one comment I left it looks good to me. We can always add/remove stuff if needed later on.

lamosty

I like the approach you took here! Great work.

Left a few smaller comments.

lamosty · 2017-07-03T16:28:08Z

modules/simple-payments/simple-payments.php

+	static $post_type_order = 'jp_pay_order';
+	static $post_type_product = 'jp_pay_product';
+
+	static $shortcode = 'simple-payment';


Maybe simple-payments? That's how we are calling it internally.

Yes, but the shortcode represents a single simple payment, so it would be weird to have:

[simple-payments id=43]

Not weird for me. It's a product name (like videopress), not a real English word. It would make sense in case of [paypal-button id...] but [simple-payment] is strange as the product is called simple-payments.

/cc @jhnstn

There is precedence for using the product name even if it's a plural form e.g. [8tracks ...] or [googleapps ...] for shortcodes available on wpcom https://en.support.wordpress.com/shortcodes/
But those are highly branded services that we can't change.

I can't find anything specific around best practices here https://developer.wordpress.org/plugins/shortcodes/#shortcode-best-practices
There is the prefix thing so something like [jetpack-simple-payments ] would make sense to use the full product name. Not saying we need the prefix however.

I do find it more natural to use the singular form but the plural form is more correct. That said I suggest we go with [simple-payment] since it's more user friendly.

lamosty · 2017-07-03T16:28:36Z

modules/simple-payments/simple-payments.php

+	}
+
+	private function register_scripts() {
+		wp_register_script( 'paypal-checkout-js', 'https://www.paypalobjects.com/api/checkout.js' );


Can we make a local copy of the file or is this the only way to load it?

Paypal heavily discourages it, but Im on the fence here.
maybe it is better to load this from server?

But we would have to keep it updated, so I dont know

https://developer.paypal.com/docs/integration/direct/express-checkout/integration-jsv4/add-paypal-button/

Alright, I see. Let's use it from their servers.

I'm not sure if we should delegate the source of this file straight to the Paypal server. I think we should keep a copy of the latest stable version and find a way to update it when it's needed.

Generally speaking I wouldn't want to source from a third party but in this case I think we should start with PayPal's recommendation to source the script from them.

🤔 At the very least we should probably leave a comment in the source here with a brief explanation and a link to that documentation.

lamosty · 2017-07-03T16:36:01Z

modules/simple-payments/simple-payments.php

+
+	private function register_scripts() {
+		wp_register_script( 'paypal-checkout-js', 'https://www.paypalobjects.com/api/checkout.js' );
+		wp_register_script( 'paypal-express-checkout', plugins_url( '/paypal-express-checkout.js', __FILE__ ) , array( 'paypal-checkout-js' ) );


Do we need this script? On https://developer.paypal.com/docs/integration/direct/express-checkout/integration-jsv4/server-side-REST-integration/, I see only the first one. Or is it our own JS script for this shortcode?

/paypal-express-checkout.js is our own JS script and we need it.

lamosty · 2017-07-03T16:47:08Z

modules/simple-payments/simple-payments.php

+			'dom_id' => uniqid( 'jp_simple_payments__button_' . $product->ID . '_' ),
+			'class' => 'jp_simple_payments__' . $product->ID,
+			'title' => get_the_title( $product ),
+			'description' => $product->post_content,


I think we should also run it though the the_content filter so it behaves the same like when rendering a post:

$content = $content_post->post_content; $content = apply_filters('the_content', $content);

lamosty · 2017-07-03T16:47:40Z

modules/simple-payments/simple-payments.php

+			'class' => 'jp_simple_payments__' . $product->ID,
+			'title' => get_the_title( $product ),
+			'description' => $product->post_content,
+			'cta' => get_post_meta( $product->ID, 'spay_cta', true ),


Do we still want this? Since Paypal won't allow to change the CTA text.

lamosty · 2017-07-03T16:48:41Z

modules/simple-payments/simple-payments.php

+			$data
+		);
+
+		wp_enqueue_script( 'paypal-express-checkout' );


I think we want to enqueue this only once per page -- what if there are multiple simple payments shortcodes on one page?

wp_enqueue_script works in a way that you can call it multiple times, but it will enqueue only once

While this is true, it's better and faster to use wp_script_is to avoid all the processing wp_enqueue_script does.

sirbrillig

Currently attempting to install a git version of jetpack on my jetpack sandbox; it's taking a while but I figured I'll leave my comments now and report on the testing later.

sirbrillig · 2017-07-03T21:09:04Z

modules/simple-payments/paypal-express-checkout.js

+		createPaymentEndpoint: '', //TODO: point to the actual endpoint
+		executePaymentEndpoint: '' //TODO: point to the actual endpoint
+	},
+	renderButton: function( id ) {


Just for defensive coding purposes (like if somehow this got included without the paypal global), do you think it would be helpful to add if ( ! paypal ) { throw new Error( 'paypal module is required by PaypalExpressCheckout' ) } or if ( ! paypal ) { console.error( 'paypal module is required by PaypalExpressCheckout' ) && return }?

sirbrillig · 2017-07-03T21:12:13Z

modules/simple-payments/paypal-express-checkout.js

+					// TODO: handle success, errors, messaging, etc, etc.
+					/* jshint ignore:start */
+					console.log( 'payment: ', payment );
+					alert( 'success!' );


Let's make sure to avoid committing this!

Why?
Nobody has access to that code either way

If nobody has access then it's one more reason to not commit it.

sirbrillig · 2017-07-03T21:14:27Z

modules/simple-payments/simple-payments.php

+	}
+
+	private function register_scripts() {
+		wp_register_script( 'paypal-checkout-js', 'https://www.paypalobjects.com/api/checkout.js' );


🤔 At the very least we should probably leave a comment in the source here with a brief explanation and a link to that documentation.

sirbrillig · 2017-07-03T21:16:38Z

modules/simple-payments/simple-payments.php

+			'description' => $product->post_content,
+			'cta' => get_post_meta( $product->ID, 'spay_cta', true ),
+		), $attrs );
+		$data[ 'price' ] = $this->format_price(


Not a big deal but the WP code guidelines say to not put spaces inside array reference brackets if the key is a string (which is annoying since they say the opposite for JavaScript). There's a few other instances of this syntax in this file. A code linter should notice them.

sirbrillig · 2017-07-03T21:31:02Z

modules/simple-payments/simple-payments.php

+	 * @return array
+	 */
+	function allow_rest_api_types( $post_types ) {
+		$post_types[] = self::$post_type_order;


Wasn't the []= shorthand introduced in php 5.3? Maybe I'm wrong; I've always thought it was, but I'm having a hard time finding evidence. (If we are following WP core coding standards, we can't use features newer than PHP 5.2.)

This shorthand was since 4 I think.
The one introduced later is $var = [];

jhnstn · 2017-07-04T00:10:40Z

modules/simple-payments/paypal-express-checkout.js

+				color: 'blue'
+			},
+			payment: function() {
+				return paypal.request.post( PaypalExpressCheckout.constants.createPaymentEndpoint ).then( function( data ) {


paypal.request.post accepts post data as the second argument: https://github.com/paypal/paypal-checkout/blob/master/docs/http.md#windowpaypalrequestposturl-data-options

I know we don't have the wpcom REST endpoints ready but might be a good time to add what we want to send to the createPaymentEndpoint.

I'm not sure what we can derive from the request but I'd imagine we'd want to send something like:

{ button_id: 123, post_id: 456, button_metadata: { ... } // for quantity etc }

Definitely out of the scope of this PR, lets add other stuff in another one.
This is monster already

iamtakashi · 2017-07-05T08:56:53Z

modules/simple-payments/simple-payments.php

+		}
+		$output = <<<TEMPLATE
+<div class="{$data[ 'class' ]} jp_simple_payments__wrapper">
+	<h2 class="jp_simple_payments__title">{$data['title']}</h2>


I'd avoid using headings here. Post titles are often h2 on posts and archive pages while they are h1 on single post page. Also themes have heading styles that are likely too big with a product image. I think using just div would be safer and would look ok in many themes.

iamtakashi · 2017-07-05T09:15:18Z

modules/simple-payments/simple-payments.php

+		       $items='<div class="jp_simple_payments__items" ><input type="number" value="1"></div>';
+		}
+		$output = <<<TEMPLATE
+<div class="{$data[ 'class' ]} jp_simple_payments__wrapper">


I think we should avoid using underscores for selectors: https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors

Wouldn't jetpack- be better than just jp-?

lamosty · 2017-07-12T09:50:18Z

modules/simple-payments/paypal-express-checkout.js

+		return 'https://public-api.wordpress.com/wpcom/v2/sites/' + blogId + '/simple-payments/paypal/payment';
+	},
+	getExecutePaymentEndpoint: function( blogId, paymentId ) {
+		return 'https://public-api.wordpress.com/wpcom/v2/sites/' + blogId + '/simple-payments/paypal/' + paymentId + '/execute';


Split to two lines pls.

lamosty · 2017-07-12T09:51:11Z

modules/simple-payments/paypal-express-checkout.js

+		if ( ! numberField ) {
+			return 1;
+		}
+		number = Number( numberField.value );


Or parseInt?

@lamosty depending on what he wants:

parseInt( '12' ) > 12 Number( '12' ) > 12 parseInt( '12ab' ) > 12 Number( '12ab' ) > NaN

he's using isNaN below

parseInt returns NaN as well in some cases:

An integer number parsed from the given string. If the first character cannot be converted to a number, NaN is returned. (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt)

But yeah, if parseInt( '12ab' ) still produces a number, I would consider that a bad behavior so let's stick with Number().

lamosty · 2017-07-12T09:52:23Z

modules/simple-payments/paypal-express-checkout.js

+	},
+	renderButton: function( blogId, buttonId, domId, enableMultiple ) {
+		if ( ! paypal ) {
+			throw new Error( 'PayPal module is required by PaypalExpressCheckout' );


Not sure we want to throw a JS error here. Wouldn't it stop other JS execution on a page? Maybe a bit destructive in that case.

lamosty · 2017-07-12T09:53:40Z

modules/simple-payments/paypal-express-checkout.js

+					number: PaypalExpressCheckout.getNumberOfItems( domId + '_number', enableMultiple ),
+					buttonId: buttonId
+				};
+				return paypal.request.post( PaypalExpressCheckout.getCreatePaymentEndpoint( blogId ), payload ).then( function( data ) {


Split to two lines pls.

eliorivero · 2017-07-12T16:39:10Z

modules/simple-payments/simple-payments.php

+			'dom_id' => uniqid( 'jetpack-simple-payments-' . $product->ID . '_' ),
+			'class' => 'jetpack-simple-payments-' . $product->ID,
+			'title' => get_the_title( $product ),
+			'description' => apply_filters( 'the_content', $product->post_content ),


Add

/** This filter is already documented in core/wp-includes/post-template.php */

above this line

We are dropping that filter since we don't want to have HTML tags in description field.

eliorivero

There are some changes to be made, but overall good work!

eliorivero · 2017-07-12T16:41:49Z

modules/simple-payments/simple-payments.php

+
+		// We allow for overriding the presentation labels
+		$data = shortcode_atts( array(
+			'blog_id' => $blog_id = Jetpack_Options::get_option( 'id' ),


The assignment to $blog_id is unnecessary, it's never used on its own.

eliorivero · 2017-07-12T16:43:02Z

modules/simple-payments/simple-payments.php

+		// We allow for overriding the presentation labels
+		$data = shortcode_atts( array(
+			'blog_id' => $blog_id = Jetpack_Options::get_option( 'id' ),
+			'dom_id' => uniqid( 'jetpack-simple-payments-' . $product->ID . '_' ),


uniqid might not return a unique value on some systems. At least the second parameter of uniqid should be set to true.

However, is there a better alternative? Maybe keeping an internal static index in the class? Or anything else that ensures a truly unique id=

We do not store this data. It used only while trying by mistake to use the same product button multiple times on a page. That really should not be a regular case

eliorivero · 2017-07-12T16:44:50Z

modules/simple-payments/simple-payments.php

+		$data['price'] = $this->format_price(
+			get_post_meta( $product->ID, 'spay_price', true ),
+			get_post_meta( $product->ID, 'spay_currency', true ),
+			$data


You're passing 3 parameters here, but format_price method only admits 2.

Added handling 3rd argument to the function.

eliorivero · 2017-07-12T16:50:21Z

modules/simple-payments/simple-payments.php

+			$data
+		);
+
+		wp_enqueue_script( 'paypal-express-checkout' );


While this is true, it's better and faster to use wp_script_is to avoid all the processing wp_enqueue_script does.

eliorivero · 2017-07-12T16:51:10Z

modules/simple-payments/simple-payments.php

+	}
+
+	function output_shortcode( $data ) {
+		$items="";


Add spacing around the operator and single quotes instead of double.

eliorivero · 2017-07-12T16:52:02Z

modules/simple-payments/simple-payments.php

+		if( $data['multiple'] ) {
+		       $items="<div class='jetpack-simple-payments-items'>
+		       <input class='jetpack-simple-payments-items-number' type='number' value='1' id='{$data['dom_id']}_number'>
+		       </div>";


Is this trailing space after </div> ok?

eliorivero · 2017-07-12T16:53:49Z

modules/simple-payments/simple-payments.php

+	{$items}
+	<div class="jetpack-simple-payments-button" id="{$data['dom_id']}_button"></div>
+</div>
+TEMPLATE;


Why this isn't a double quoted string like the previous one? I think it can be solved in the same way and be more consistent in the code.

eliorivero · 2017-07-12T16:54:53Z

modules/simple-payments/simple-payments.php

+
+	function format_price( $price, $currency ) {
+		// TODO: better price formatting logic. Extracting from woocmmerce is not a solution since its bound with woo site options.
+		return $price. " " . $currency;


Needs space before first . and empty strings should always be single quoted.
In any case, this can be written as return "$price $currency";

eliorivero · 2017-07-12T17:02:45Z

modules/simple-payments/simple-payments.php

+		);
+		$order_args = array(
+			'label'                 => __( 'Order', 'jetpack' ),
+			'description'           => __( 'Simple Payments orders', 'jetpack' ),


Please always use esc_html__, trust no one, including translations.

eliorivero · 2017-07-12T17:03:28Z

modules/simple-payments/simple-payments.php

+		);
+		$product_args = array(
+			'label'                 => __( 'Product', 'jetpack' ),
+			'description'           => __( 'Simple Payments products', 'jetpack' ),


Same, use esc_html__ please

artpi · 2017-07-14T11:26:41Z

I implemented review fixes

Attempt to make test pass in JS

Changes were made

artpi added the [Status] In Progress label Jun 30, 2017

artpi self-assigned this Jun 30, 2017

artpi requested review from retrofox, jsnajdr, jhnstn, sirbrillig and lamosty June 30, 2017 15:28

artpi added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jun 30, 2017

artpi changed the title ~~[WIP] Simple Payments: Introduce CPTs for /posts endpoints~~ Simple Payments: Introduce CPTs for /posts endpoints Jun 30, 2017

lamosty reviewed Jun 30, 2017

View reviewed changes

artpi changed the title ~~Simple Payments: Introduce CPTs for /posts endpoints~~ Simple Payments: CPTs and Shortcode Jul 3, 2017

lamosty suggested changes Jul 3, 2017

View reviewed changes

artpi mentioned this pull request Jul 3, 2017

Simple payments: redux data layer Automattic/wp-calypso#15801

Merged

sirbrillig reviewed Jul 3, 2017

View reviewed changes

jhnstn reviewed Jul 4, 2017

View reviewed changes

artpi force-pushed the simple-payments/cpt branch from b1f6e11 to ed752a8 Compare July 4, 2017 10:54

iamtakashi reviewed Jul 5, 2017

View reviewed changes

artpi force-pushed the simple-payments/cpt branch from 0a9a506 to 0049c0d Compare July 10, 2017 12:45

lamosty mentioned this pull request Jul 11, 2017

Simple Payments: add Editor shortcode views Automattic/wp-calypso#16028

Merged

lamosty reviewed Jul 12, 2017

View reviewed changes

artpi force-pushed the simple-payments/cpt branch from b5bbf30 to 269e5eb Compare July 12, 2017 15:05

eliorivero reviewed Jul 12, 2017

View reviewed changes

eliorivero previously requested changes Jul 12, 2017

View reviewed changes

eliorivero added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jul 12, 2017

artpi removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jul 14, 2017

Introduce Jetpack_Simple_Payments class, load it

96f72ba

artpi added 13 commits July 20, 2017 16:55

Make ids work for paypal code

8fe0d2d

repair init code

bae0e45

Introduce ID for form

fc05038

Add endpoint urls

2eb272e

Better domain

bb73e17

Endpoint urls with blog_id

4ecdfd1

Fix id

65df62a

Add version

a046b03

Change endpoint

e2059e0

Make sandbox

6c7fce2

Add post meta syncing

d3362e5

Sandbox management

62337bd

Review fixes

73ac723

artpi force-pushed the simple-payments/cpt branch from 456c39e to 893bbb3 Compare July 20, 2017 14:55

retrofox mentioned this pull request Jul 20, 2017

Simple payments/show messages response #7495

Merged

artpi added 2 commits July 20, 2017 17:46

remove console

d7b1e44

Attempt to make test pass in JS

Fir for missing product

5214211

artpi force-pushed the simple-payments/cpt branch from 893bbb3 to 5214211 Compare July 20, 2017 15:47

zinigor added [Feature] Custom Content Types Custom post or content types (usually for testimonials and portfolios) and their settings. [Feature] Shortcodes / Embeds [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Jul 20, 2017

lamosty approved these changes Jul 20, 2017

View reviewed changes

artpi changed the base branch from master to feature/simple-payments July 21, 2017 10:14

artpi merged commit d88e954 into feature/simple-payments Jul 21, 2017

jeherve added the Simple Payments label Sep 7, 2017

Viper007Bond deleted the simple-payments/cpt branch January 29, 2018 23:17

kraftbj removed the [Status] Needs Review This PR is ready for review. label Sep 29, 2021

matticbot added the Touches WP.com Files label Sep 29, 2021

jeherve added the [Feature] Pay with PayPal aka Simple Payments label Aug 2, 2023

Simple Payments: CPTs and Shortcode #7409

Simple Payments: CPTs and Shortcode #7409

Conversation

artpi commented Jun 30, 2017 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lamosty commented Jun 30, 2017

lamosty left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artpi Jul 3, 2017 • edited Loading

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

sirbrillig left a 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

jhnstn Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamtakashi Jul 5, 2017 • edited Loading

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

eliorivero Jul 12, 2017 • edited Loading

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

eliorivero left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artpi commented Jul 14, 2017

artpi commented Jun 30, 2017 •

edited

Loading

artpi Jul 3, 2017 •

edited

Loading

jhnstn Jul 4, 2017 •

edited

Loading

iamtakashi Jul 5, 2017 •

edited

Loading

eliorivero Jul 12, 2017 •

edited

Loading