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

Enable JITMs in WPAdmin on Simple sites #41252

Open
wants to merge 27 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a7397f2
Testing JITM on simple
getdave Jan 22, 2025
4ae1aec
changelog
getdave Jan 22, 2025
cce91e9
Remove redundant function in JS
getdave Jan 22, 2025
8a2ae76
Hack around some things
getdave Feb 1, 2025
66436d7
Remove logging from JS
getdave Feb 2, 2025
767acc0
Improve ajax handler security and allow themes screen for jitms
getdave Feb 2, 2025
2fe8d3e
Remove ajax handling for no-logged in users
getdave Feb 2, 2025
a07724c
Make it easier to see which screens jitms are enabled on
getdave Feb 2, 2025
d1de75e
Use correct nonce
getdave Feb 2, 2025
f631378
Remove redundant code
getdave Feb 10, 2025
39dcb07
Remove Ajax handling but retain original Jetpack endpoint
getdave Feb 10, 2025
2c01a40
changelog
getdave Feb 10, 2025
c587e35
Add ability to dismiss JITMs
getdave Feb 10, 2025
1686b8c
Move WP_COM code fork into JP class to preserve all functionality
getdave Feb 11, 2025
ff8382a
Add helpful comment to explain code fork
getdave Feb 11, 2025
ac7af97
Remove code fork and use filter to bypass Jetpack user auth check
getdave Feb 11, 2025
1f7b336
Tidy Post_Connection class
getdave Feb 11, 2025
4398240
Improve auth filter to run only on Dotcom Simple
getdave Feb 11, 2025
d9b3081
Allow dismissals to bypass JP auth check on Simple
getdave Feb 11, 2025
a76cbc4
Correction
getdave Feb 11, 2025
0cdabb2
Apply changes from https://github.com/Automattic/jetpack/pull/41542/ …
getdave Feb 12, 2025
1259953
Provide context on true type
getdave Feb 12, 2025
246ac3d
Reuse endpoint URL
getdave Feb 12, 2025
6a8565a
remove addition of new filter for dissmissable JITMs
getdave Feb 13, 2025
beb9503
Tidy whitespace changes
getdave Feb 13, 2025
25e198b
Update permissions checks in endpoint to match existing endpoint
getdave Feb 13, 2025
5323a50
Add Themes page to test case
getdave Feb 13, 2025
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
4 changes: 4 additions & 0 deletions projects/packages/jitm/changelog/try-jitm-on-simple
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Enable JITM on Simple sites
23 changes: 20 additions & 3 deletions projects/packages/jitm/src/class-jitm.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ class JITM {

const PACKAGE_VERSION = '4.0.7';

/**
* List of screen IDs where JITMs are allowed to display.
*
* @var string[]
*/
const APPROVED_SCREEN_IDS = array(
'jetpack',
'woo',
'shop',
'product',
'themes',
Copy link
Contributor Author

@getdave getdave Feb 12, 2025

Choose a reason for hiding this comment

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

Let's pull all these changes out into a separate PR, and remove the addition of themes to the list until we're ready to ship.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #41748

);

/**
* The configuration method that is called from the jetpack-config package.
*/
Expand All @@ -36,7 +49,7 @@ public static function configure() {
* @return Post_Connection_JITM|Pre_Connection_JITM JITM instance.
*/
public static function get_instance() {
if ( ( new Connection_Manager() )->is_connected() ) {
if ( ( new Connection_Manager() )->is_connected() || defined( 'IS_WPCOM' ) && IS_WPCOM ) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we have the Status package as a requirement for the JITM package, we can use ( new Automattic\Jetpack\Status\Host() )->is_wpcom_simple() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scruffian You asked about whether the IS_WPCOM check should be part of Connection_Manager. The answer is I have no idea. It could be.

We'd need input from the Jetpack team on this I think.

$jitm = new Post_Connection_JITM();
} else {
$jitm = new Pre_Connection_JITM();
Expand Down Expand Up @@ -127,6 +140,7 @@ public function prepare_jitms( $screen ) {
add_action( 'admin_enqueue_scripts', array( $this, 'jitm_enqueue_files' ) );
add_action( 'admin_notices', array( $this, 'ajax_message' ) );
add_action( 'edit_form_top', array( $this, 'ajax_message' ) );

}
}

Expand Down Expand Up @@ -156,7 +170,10 @@ public function is_a8c_admin_page() {
return (
$current_screen
&& $current_screen->id
&& (bool) preg_match( '/jetpack|woo|shop|product/', $current_screen->id )
&& (bool) preg_match(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggesting for now we gate this with IS_WPCOM to ensure the JITM code will not run on Simple until we're ready to deploy.

'/' . implode( '|', self::APPROVED_SCREEN_IDS ) . '/',
$current_screen->id
)
);
}

Expand Down Expand Up @@ -226,7 +243,7 @@ public function ajax_message() {
return;
}

// Only add this to Jetpack or Woo admin pages.
// Only add this to specifically whitelisted pages.
if ( ! $this->is_a8c_admin_page() ) {
return;
}
Expand Down
6 changes: 5 additions & 1 deletion projects/packages/jitm/src/class-post-connection-jitm.php
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,11 @@ public function dismiss( $id, $feature_class ) {
}

/**
* Asks the wpcom API for the current message to display keyed on query string and message path
* Asks the wpcom API for the current message to display keyed on query string and message path.
*
* For sites running on the Dotcom Simple codebase, the network request is bypassed
* via Client::wpcom_json_api_request_as_blog allowing for the JITM\Engine to be called
* directly.
*
* @param string $message_path The message path to ask for.
* @param string $query The query string originally from the front end.
Expand Down
25 changes: 16 additions & 9 deletions projects/packages/jitm/src/js/jetpack-jitm.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import jQuery from 'jquery';
Copy link
Member

Choose a reason for hiding this comment

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

At this point, maybe we could get rid of jQuery in a follow-up PR? :)

import apiFetch from '@wordpress/api-fetch';
import { addQueryArgs } from '@wordpress/url';

import '../css/jetpack-admin-jitm.scss';

jQuery( document ).ready( function ( $ ) {
// Site ID will be automatically added to the request.// Site ID will be automatically added to the request.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Site ID will be automatically added to the request.// Site ID will be automatically added to the request.
// Site ID will be automatically added to the request.

const JITM_ENDPOINT_URL = `/wpcom/v2/jitm-v2`;

var templates = {
default: function ( envelope ) {
const EXTERNAL_LINK_ICON = `
Expand Down Expand Up @@ -155,8 +160,8 @@ jQuery( document ).ready( function ( $ ) {
$my_template.hide();

apiFetch( {
path: '/jetpack/v4/jitm',
method: 'POST', // using DELETE without permalinks is broken in default nginx configuration
path: JITM_ENDPOINT_URL,
method: 'POST',
data: {
id: response.id,
feature_class: response.feature_class,
Expand Down Expand Up @@ -300,14 +305,16 @@ jQuery( document ).ready( function ( $ ) {

var full_jp_logo_exists = $( '.jetpack-logo__masthead' ).length ? true : false;

$.get( window.jitm_config.api_root + 'jetpack/v4/jitm', {
message_path: message_path,
query: query,
full_jp_logo_exists: full_jp_logo_exists,
_wpnonce: $el.data( 'nonce' ),
apiFetch( {
path: addQueryArgs( JITM_ENDPOINT_URL, {
message_path: message_path,
query: query,
full_jp_logo_exists: full_jp_logo_exists,
Comment on lines +310 to +312
Copy link
Member

Choose a reason for hiding this comment

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

Since the keys and values are the same, can't we use shorthand here and avoid duplication?

} ),
method: 'GET',
} ).then( function ( response ) {
if ( 'object' === typeof response && response[ '1' ] ) {
response = [ response[ '1' ] ];
if ( 'object' === typeof response && response[ 'data' ] ) {
response = [ response[ 'data' ][ 0 ] ];
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to account for situations when response[ 'data' ] would not be an array?

}

// properly handle the case of an empty array or no content set
Expand Down
1 change: 1 addition & 0 deletions projects/packages/jitm/tests/php/test_JITM.php
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ public function data_test_is_a8c_admin_page() {
'WooCommerce admin page' => array( 'woocommerce_page_wc-admin', true ),
'WooCommerce order management' => array( 'edit-shop_order', true ),
'WooCommerce product management' => array( 'edit-product', true ),
'Themes page' => array( 'themes', true ),
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
<?php
/**
* REST API endpoint for retrieving JITMs from the WPCOM API.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could ship this endpoint in the JITM package instead, so things stay close together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scruffian Do you agree we should pull this out into a separate PR?

That way the code can be available on Simple, Atomic and JP and we can enable it on Simple by way of 170707-ghe-Automattic/wpcom when we are ready.

Also we can write some tests for it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah good idea

*
* Available on:
* - Simple - via Dotcom Public API (https://public-api.wordpress.com/wpcom/v2/sites/{site_id}/jitm-v2).
* - WoA and Jetpack connected sites - via local site REST API (https://myjetpackconnectedsite.com/wp-json/wpcom/v2/jitm-v2)
*
* Utilises Jetpack classes to orchestrate the request and response handling.
* All JITM configuration happens on the Dotcom Simple codebase.
*
* @package automattic/jetpack
*/

use Automattic\Jetpack\Connection\REST_Connector;

/**
* Class WPCOM_REST_API_V2_Endpoint_JITM_V2ssssss
*/
class WPCOM_REST_API_V2_Endpoint_JITM_V2 extends WP_REST_Controller {

/**
* Namespace prefix.
*
* @var string
*/
public $namespace = 'wpcom/v2';

/**
* Endpoint base route.
*
* @var string
*/
public $rest_base = 'jitm-v2';

/**
* WPCOM_REST_API_V2_Endpoint_JITM_V2 constructor.
*/
public function __construct() {
add_action( 'rest_api_init', array( $this, 'register_routes' ) );
}

/**
* Register routes.
*/
public function register_routes() {
register_rest_route(
$this->namespace,
$this->rest_base . '/',
array(
array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_item' ),
'permission_callback' => '__return_true',
),
array(
'methods' => WP_REST_Server::CREATABLE,
'callback' => array( $this, 'dismiss_item' ),
'permission_callback' => array( $this, 'dismiss_item_permissions_check' ),
),
)
);
}

/**
* Retrieves the JITMs.
*
* @param WP_REST_Request $request Full details about the request.
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function get_item( $request ) {
$jitm = Automattic\Jetpack\JITMS\JITM::get_instance();

if ( ! $jitm->jitms_enabled() ) {
return rest_ensure_response( array() );
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 return more than just an empty array in those situations, to make debugging easier?

}

// add the search term to the query params if it exists
$query_param = $request['query'] ?? '';

// Disable the jetpack_user_auth_check filter on Dotcom Simple codebase.
// This allows the wpcom/v2/jitm endpoint to work for Simple sites.
// See fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Serfg%2Qncv%2Qcyhtvaf%2Sraqcbvagf%2Swrgcnpx.cuc%3Se%3Q4580oq59%2374-og.
if ( defined( 'IS_WPCOM' ) && IS_WPCOM ) {
add_filter( 'rest_api_jitm_jetpack_user_auth_check', '__return_true' );
}

$messages = $jitm->get_messages(
$request['message_path'],
urldecode_deep( array( 'query' => $query_param ) ),
'true' === $request['full_jp_logo_exists']
);

if ( defined( 'IS_WPCOM' ) && IS_WPCOM ) {
remove_filter( 'rest_api_jitm_jetpack_user_auth_check', '__return_true' );
}

return rest_ensure_response( $messages );
}

/**
* Checks if a given request has access to dismiss JITMs.
*
* @return true|WP_Error True if the request has permission to dismiss, WP_Error object otherwise.
*/
public function dismiss_item_permissions_check() {
if ( ! current_user_can( 'read' ) ) {
return new WP_Error(
'invalid_user_permission_jetpack_delete_jitm_message',
REST_Connector::get_user_permissions_error_msg(),
array( 'status' => rest_authorization_required_code() )
);
}

return true;
}

/**
* Dismisses a JITM message.
*
* @param WP_REST_Request $request Full details about the request.
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function dismiss_item( $request ) {
$jitm = Automattic\Jetpack\JITMS\JITM::get_instance();

if ( ! $jitm->jitms_enabled() ) {
// Boolean return matches return type of $jitm->dismiss().
// Not returning a WP_Error avoids a 400 response code
// and allows the dismiss action to be silently ignored.
return rest_ensure_response( true );
}

// Disable the jetpack_user_auth_check filter on Dotcom Simple codebase.
// This allows the wpcom/v2/jitm endpoint to work for Simple sites.
// See fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Serfg%2Qncv%2Qcyhtvaf%2Sraqcbvagf%2Swrgcnpx.cuc%3Se%3Q4580oq59%2374-og.
if ( defined( 'IS_WPCOM' ) && IS_WPCOM ) {
add_filter( 'rest_api_jitm_jetpack_user_auth_check', '__return_true' );
}

$result = $jitm->dismiss( $request['id'], $request['feature_class'] );

if ( defined( 'IS_WPCOM' ) && IS_WPCOM ) {
remove_filter( 'rest_api_jitm_jetpack_user_auth_check', '__return_true' );
}

return rest_ensure_response( $result );
}
}

wpcom_rest_api_v2_load_plugin( 'WPCOM_REST_API_V2_Endpoint_JITM_V2' );
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/try-jitm-on-simple
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: other

Adds new WPCom v2 endpoint for handling JITMs
Loading