-
Notifications
You must be signed in to change notification settings - Fork 812
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
base: trunk
Are you sure you want to change the base?
Conversation
@@ -139,6 +177,7 @@ public function prepare_jitms( $screen ) { | |||
* @return bool True if the current page is a Jetpack or WooCommerce admin page, else false. | |||
*/ | |||
public function is_a8c_admin_page() { | |||
return true; |
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.
Short-circuiting this for testing purposes.
$.get( window.jitm_config.api_root + 'jetpack/v4/jitm', { | ||
// Original code | ||
// const url = jitm_config.api_root + 'jetpack/v4/jitm'; | ||
|
||
// make an WP Ajax request to local site as REST API is not initialized on Simple sites. | ||
const dotComURL = window.ajaxurl; |
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.
Obviously this will need to be made conditional based on whether run on Simple or not.
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
f6a951b
to
ce3b588
Compare
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
|
Note to self. Ensure that changes to the |
$query = $request['query']; | ||
if ( ! empty( $request['s'] ) ) { | ||
$query['s'] = $request['s']; |
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.
Let's bring in changes outlined in #41542 once that PR is merged.
...ugins/jetpack/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-jitm-v2.php
Show resolved
Hide resolved
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 after reviewing and testing. I added some minor comments.
( externalLink ? EXTERNAL_LINK_ICON : '' ) | ||
'</a>'; | ||
( externalLink ? EXTERNAL_LINK_ICON : '' ); | ||
( '</a>' ); |
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.
All whitespace changes can be shipped independently.
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.
See #41745
$.ajax( { | ||
url: window.jitm_config.api_root + 'jetpack/v4/jitm', | ||
method: 'POST', // using DELETE without permalinks is broken in default nginx configuration | ||
beforeSend: function ( xhr ) { | ||
xhr.setRequestHeader( 'X-WP-Nonce', window.jitm_config.nonce ); | ||
}, | ||
apiFetch( { | ||
path: '/wpcom/v2/jitm-v2', | ||
method: 'POST', |
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.
Change to apiFetch
can be shipped independently.
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.
See #41745
@@ -351,8 +357,11 @@ public function get_messages( $message_path, $query, $full_jp_logo_exists ) { | |||
|
|||
$dismissed_feature = isset( $hidden_jitms[ $envelope->feature_class ] ) && is_array( $hidden_jitms[ $envelope->feature_class ] ) ? $hidden_jitms[ $envelope->feature_class ] : null; | |||
|
|||
// If ignore_dismissible is true, skip the dismiss check | |||
$ignore_dismissible = apply_filters( 'jetpack_jitm_should_ignore_dismissals', false ); |
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.
Let's ship this as a standalone PR.
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.
See #41746
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.
I closed that PR. Let's remove this code entirely as we can work around it using filters and providing a fake JITM for testing purposes.
5d489c6
to
246ac3d
Compare
@@ -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 ) { |
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.
@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.
@@ -0,0 +1,151 @@ | |||
<?php | |||
/** | |||
* REST API endpoint for retrieving JITMs from the WPCOM API. |
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.
@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.
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.
Yeah good idea
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.
This tests well for me! I left a few minor remarks below.
Sorry for the late review! I had forgotten to submit my review after adding the comments!
@@ -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 ) { |
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.
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.
@@ -351,8 +357,11 @@ public function get_messages( $message_path, $query, $full_jp_logo_exists ) { | |||
|
|||
$dismissed_feature = isset( $hidden_jitms[ $envelope->feature_class ] ) && is_array( $hidden_jitms[ $envelope->feature_class ] ) ? $hidden_jitms[ $envelope->feature_class ] : null; | |||
|
|||
// If ignore_dismissible is true, skip the dismiss check | |||
$ignore_dismissible = apply_filters( 'jetpack_jitm_should_ignore_dismissals', false ); |
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.
Let's add a docblock for this filter, so it can be picked up by the codex.
In that docblock, you can use $$next-version$$
for the version number:
jetpack/projects/packages/README.md
Lines 114 to 123 in d3f5823
### Package Version Annotations | |
When needing to add a package version number inside a DocBlock, please use `$$next-version$$` as such: | |
- `@since $$next-version$$` | |
- `@deprecated $$next-version$$` | |
- `@deprecated since $$next-version$$` | |
- `_deprecated_function( __METHOD__, 'package-$$next-version$$' );` (other WordPress deprecation functions also work, but note it must be all on one line). | |
The `$$next-version$$` specifier will be automatically replaced with the correct package version number the next time a new version of that package is released. |
@@ -1,8 +1,13 @@ | |||
import jQuery from 'jquery'; |
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.
At this point, maybe we could get rid of jQuery in a follow-up PR? :)
|
||
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. |
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.
// 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. |
CHECKMARK_ICON + | ||
text + | ||
'</li>'; | ||
html += '<li>' + CHECKMARK_ICON + text + '</li>'; |
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.
If we're to make changes, maybe we could switch to template strings, here and below, so it would be more readable?
message_path: message_path, | ||
query: query, | ||
full_jp_logo_exists: full_jp_logo_exists, |
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.
Since the keys and values are the same, can't we use shorthand here and avoid duplication?
if ( 'object' === typeof response && response[ '1' ] ) { | ||
response = [ response[ '1' ] ]; | ||
if ( 'object' === typeof response && response[ 'data' ] ) { | ||
response = [ response[ 'data' ][ 0 ] ]; |
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.
Do we need to account for situations when response[ 'data' ]
would not be an array?
@@ -0,0 +1,167 @@ | |||
<?php | |||
/** | |||
* REST API endpoint for retrieving JITMs from the WPCOM API. |
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.
Maybe we could ship this endpoint in the JITM package instead, so things stay close together?
$jitm = Automattic\Jetpack\JITMS\JITM::get_instance(); | ||
|
||
if ( ! $jitm->jitms_enabled() ) { | ||
return rest_ensure_response( array() ); |
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.
Would it make sense to return more than just an empty array in those situations, to make debugging easier?
Fixes Automattic/wp-calypso#95651
Related to: 170707-ghe-Automattic/wpcom
Proposed changes:
Enables JITMs on Simple sites in WPAdmin (i.e. outside of Calypso). Must be partnered with 170707-ghe-Automattic/wpcom.
wpcom/v2/jitm-v2
endpoint which replaces the currentjetpack/v4/jitm
endpoint. This makes the endpoint available for both Simple and Jetpack Connected sites (including WoA).is_jetpack_authorized_for_site
check on the existingwpcom/v2/jitm
(not to be confused with the new endpoint above!) is disabled for sites running on the Dotcom Simple codebase (via a filter).Client::wpcom_json_api_request_as_blog
means there is no additional network request when running on Dotcom Simple)Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
wpcom/v2/jitm-v2
endpoint on the Dotcom Simple Public API.message_path
to target/wp:themes:admin_notices/
(display in "WPAdmin" on the "Themes" path in the "admin_notices" action).Simple site
bin/jetpack-downloader
command (see Github bot comment below) to apply this PR to your sandbox.https://{YOUR_SANDBOXED_SIMPLE_SITE}.wordpress.com/wp-admin/themes.php
Let us build your dream website
JITM at the top of the page.Atomic site 🤦
Jetpack
entry and click "Manage"try/jitm-on-simple
and enable it.https://wordpress.com/hosting-config/MY_WoA_SITE_HERE
and enable SSH.wp-config.php
file in the root and add thedefine( 'JETPACK__SANDBOX_DOMAIN', 'MY_SANDBOX_HERE' );
. This will ensure the WoA site uses your sandbox for the Public API. This will ensure that the changes from 170707-ghe-Automattic/wpcom are used by the WoA site.