Skip to content
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

WooCommerce Services JITM #6183

Merged
merged 19 commits into from
Jan 31, 2017
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3e7ed4a
Start implementing class for installing WooCommerce Hosted Services
nabsul Jan 23, 2017
54f0f33
Add WooCommerce Services JITM for US and Canada-based merchants.
jeffstieler Jan 23, 2017
2be7eea
Remove check for testing class.
jeffstieler Jan 23, 2017
014ba86
Remove “Hello World” notice.
jeffstieler Jan 23, 2017
e318a8d
Hook up “Install WooCommerce Services” button.
jeffstieler Jan 23, 2017
d863d11
Add WooCommerce-specific JITM emblem.
jeffstieler Jan 25, 2017
752dfdc
Add a nonce to the WC Services install link, use “safe” redirect meth…
jeffstieler Jan 26, 2017
f7c1aa2
Show an error message if the WooCommerce Services install fails, chan…
jeffstieler Jan 26, 2017
9da1e76
Don’t open a new tab when installing WooCommerce Services from JITM.
jeffstieler Jan 27, 2017
f5328c4
Remove usage of `filter_input()` in favor of the `$_GET` super global.
jeffstieler Jan 27, 2017
e581bfd
Ensure that JITMs are only shown to users who can install plugins.
jeffstieler Jan 27, 2017
c383dac
Add a truthy return to WC Services installation method.
jeffstieler Jan 27, 2017
fdf129d
Prune unnecessary variable usage.
jeffstieler Jan 27, 2017
55dc750
No need to explicitly call Jetpack::do_stats().
jeffstieler Jan 27, 2017
caff508
Use the singleton pattern to allow for easier unit testing. Move hook…
jeffstieler Jan 27, 2017
68e5e75
Use a switch statement for WC Services JITM country detection, allowi…
jeffstieler Jan 27, 2017
8c09a93
Explicitly base the post-install redirect on the referring URL.
jeffstieler Jan 27, 2017
8594906
Update WooCommerce Services plugin slug for install and activation.
jeffstieler Jan 31, 2017
6a84c14
Add PHPDoc comments to WooCommerce Services installer class.
jeffstieler Jan 31, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions 3rd-party/3rd-party.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@

// We can't load this conditionally since polldaddy add the call in class constuctor.
require_once( JETPACK__PLUGIN_DIR . '3rd-party/polldaddy.php' );
require_once( JETPACK__PLUGIN_DIR . '3rd-party/woocommerce-services.php' );
88 changes: 88 additions & 0 deletions 3rd-party/woocommerce-services.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<?php

if ( ! defined( 'ABSPATH' ) ) {
exit;
}

class WC_Services_Installer {

/**
* @var WC_Services_Installer
**/
private static $instance = null;

static function init() {
if ( is_null( self::$instance ) ) {
self::$instance = new WC_Services_Installer();
}
return self::$instance;
}

public function __construct() {
add_action( 'admin_init', array( $this, 'add_error_notice' ) );
add_action( 'admin_init', array( $this, 'try_install' ) );
}

public function try_install() {
if ( isset( $_GET['wc-services-action'] ) && ( 'install' === $_GET['wc-services-action'] ) ) {
check_admin_referer( 'wc-services-install' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security concern: Nonces are for verifying intent, not capabilities. This doesn't seem to in any way test for the install_plugins capability. :\

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - we'll add the check for the install_plugins capability as well.


if ( ! current_user_can( 'install_plugins' ) ) {
return;
}

$result = $this->install();
$redirect = wp_get_referer();

if ( false === $result ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be easier to just do if ( ! $this->install() ) { -- no need for the variable to store it for a couple lines.

$redirect = add_query_arg( 'wc-services-install-error', true, $redirect );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume true works correctly here? I know false removes it, but I don't think I've ever tried passing in a boolean of true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - the value end up going through urlencode() which converts the boolean true to an integer 1.

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add an else clause so we could append a get argument confirming the installation happened? Or do we not care about showing a confirmation message on the page we're directing them to (which is fine)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not concerned initially since the first activation of the plugin will cause a welcome/TOS message.


wp_safe_redirect( $redirect );

exit;
}
}

public function add_error_notice() {
if ( ! empty( $_GET['wc-services-install-error'] ) ) {
add_action( 'admin_notices', array( $this, 'error_notice' ) );
}
}

public function error_notice() {
?>
<div class="notice notice-error is-dismissible">
<p><?php _e( 'There was an error installing WooCommerce Services.', 'jetpack' ); ?></p>
</div>
<?php
}

private function install() {
include_once( ABSPATH . '/wp-admin/includes/admin.php' );
include_once( ABSPATH . '/wp-admin/includes/plugin-install.php' );
include_once( ABSPATH . '/wp-admin/includes/plugin.php' );
include_once( ABSPATH . '/wp-admin/includes/class-wp-upgrader.php' );
include_once( ABSPATH . '/wp-admin/includes/class-plugin-upgrader.php' );

$api = plugins_api( 'plugin_information', array( 'slug' => 'connect-for-woocommerce' ) );

if ( is_wp_error( $api ) ) {
return false;
}

$upgrader = new Plugin_Upgrader( new Automatic_Upgrader_Skin() );
$result = $upgrader->install( $api->download_link );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work on environments where WordPress doesn't have write access to the filesystem? Is it just the catchall error in error_notice ?

I know we have a lot of caveats in our plugin installation rest api endpoint -- https://github.com/Automattic/jetpack/blob/master/json-endpoints/jetpack/class.jetpack-json-api-plugins-install-endpoint.php#L12-L54 // cc: @roccotripaldi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this, and it is caught by our generic error handler.


if ( true !== $result ) {
return false;
}

$result = activate_plugin( 'connect-for-woocommerce/woocommerce-connect-client.php' );

// activate_plugin() returns null on success
return is_null( $result );
}
}

WC_Services_Installer::init();
66 changes: 66 additions & 0 deletions class.jetpack-jitm.php
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ function prepare_jitms( $screen ) {
add_action( 'admin_enqueue_scripts', array( $this, 'jitm_enqueue_files' ) );
add_action( 'admin_notices', array( $this, 'backups_updates_msg' ) );
}
elseif ( ! Jetpack::is_plugin_active( 'woocommerce-services/woocommerce-services.php' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be on the previous line, as per WP Coding Standards -- https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#brace-style

(really, so should all the prior conditionals above -- but as you're just matching style, this is less critical to fix)

$pages_to_display = array(
'woocommerce_page_wc-settings', // WooCommerce > Settings
'edit-shop_order', // WooCommerce > Orders
'shop_order', // WooCommerce > Edit Order
);

if ( in_array( $screen->id, $pages_to_display ) ) {
add_action( 'admin_enqueue_scripts', array( $this, 'jitm_enqueue_files' ) );
add_action( 'admin_notices', array( $this, 'woocommerce_services_msg' ) );
}
}
}

/*
Expand Down Expand Up @@ -414,6 +426,60 @@ function videopress_media_upload_warning_msg() {
<?php
}

/**
* Display message prompting user to install WooCommerce Services.
*
* @since 4.6
*/
function woocommerce_services_msg() {
if ( ! current_user_can( 'manage_woocommerce' ) || ! current_user_can( 'install_plugins' ) ) {
return;
}

if ( isset( self::$jetpack_hide_jitm['woocommerce_services'] ) ) {
return;
}

if ( ! function_exists( 'wc_get_base_location' ) ) {
return;
}

$base_location = wc_get_base_location();

switch ( $base_location['country'] ) {
case 'US':
$message = __( 'Try our new service for USPS shipping & label-printing.', 'jetpack' );
break;
case 'CA':
$message = __( 'Try our new Canada Post shipping service.', 'jetpack' );
break;
default:
return;
}

$install_url = wp_nonce_url( add_query_arg( array( 'wc-services-action' => 'install' ) ), 'wc-services-install' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should provide a base page to the link here, instead of just coasting off current.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. okay - after a quick discussion, I think we'll take the user to the plugins page upon installation.

Copy link
Member

@georgestephanis georgestephanis Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also do wp_get_referer() to send them back after


?>
<div class="jp-jitm woo-jitm">
<a href="#" data-module="woocommerce_services" class="dismiss"><span class="genericon genericon-close"></span></a>
<div class="jp-emblem">
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" id="Layer_1" x="0" y="0" viewBox="0 0 24 24" enable-background="new 0 0 24 24" xml:space="preserve">
<path d="M18,8h-2V7c0-1.105-0.895-2-2-2H4C2.895,5,2,5.895,2,7v10h2c0,1.657,1.343,3,3,3s3-1.343,3-3h4c0,1.657,1.343,3,3,3s3-1.343,3-3h2v-5L18,8z M7,18.5c-0.828,0-1.5-0.672-1.5-1.5s0.672-1.5,1.5-1.5s1.5,0.672,1.5,1.5S7.828,18.5,7,18.5z M4,14V7h10v7H4z M17,18.5c-0.828,0-1.5-0.672-1.5-1.5s0.672-1.5,1.5-1.5s1.5,0.672,1.5,1.5S17.828,18.5,17,18.5z" />
</svg>
</div>
<p class="msg">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be wrapped in a <?php if ( ! empty( $message ) ) : ?> ? Otherwise outside of the US and Canada, it would print an empty p.msg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the most straightforward logic perhaps, but if you don't fall into either the US or CA cases, the default simply returns. See line 457.

<?php echo esc_html( $message ); ?>
</p>
<p>
<a href="<?php echo esc_url( $install_url ); ?>" title="<?php esc_attr_e( 'Install WooCommerce Services', 'jetpack' ); ?>" data-module="woocommerce_services" class="button button-jetpack launch show-after-enable"><?php esc_html_e( 'Install WooCommerce Services', 'jetpack' ); ?></a>
</p>
</div>
<?php
//jitm is being viewed, track it
$jetpack = Jetpack::init();
$jetpack->stat( 'jitm', 'woocommerce_services-viewed-' . JETPACK__VERSION );
}

/*
* Function to enqueue jitm css and js
*/
Expand Down
5 changes: 5 additions & 0 deletions scss/jetpack-admin-jitm.scss
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@
fill: #8cc258;
}

// WooCommerce jitm
&.woo-jitm path {
fill: #96588a;
}

.dismiss {
margin: 0;
text-decoration: none;
Expand Down