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

Preload validation counts data if AMP submenu is expanded #6770

Merged
merged 8 commits into from
Dec 15, 2021
89 changes: 66 additions & 23 deletions assets/src/amp-validation/counts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,56 @@ import domReady from '@wordpress/dom-ready';
import './style.css';

/**
* Updates a menu item with its count.
* Sets the loading state on a menu item.
*
* If the count is not a number or is `0`, the element that contains the count is instead removed (as it would be no longer relevant).
* @param {string} itemId Menu item ID.
*/
function setMenuItemIsLoading( itemId ) {
const itemEl = document.getElementById( itemId );

if ( ! itemEl || itemEl.querySelector( '.amp-count-loading' ) ) {
return;
}

const loadingEl = document.createElement( 'span' );

loadingEl.classList.add( 'amp-count-loading' );
itemEl.classList.add( 'awaiting-mod' );

itemEl.append( loadingEl );
}

/**
* Sets a menu item count value.
*
* @param {HTMLElement} itemEl Menu item element.
* @param {number} count Count to set.
* If the count is not a number or is `0`, the element that contains the count is instead removed (as it would be no
* longer relevant).
*
* @param {string} itemId Menu item ID.
* @param {number} count Count to set.
*/
function updateMenuItem( itemEl, count ) {
function setMenuItemCountValue( itemId, count ) {
const itemEl = document.getElementById( itemId );

if ( ! itemEl ) {
return;
}

if ( isNaN( count ) || count === 0 ) {
itemEl.parentNode.removeChild( itemEl );
} else {
itemEl.textContent = count.toLocaleString();
}
}

/**
* Initializes the 'Validated URLs' and 'Error Index' menu items.
*/
function initializeMenuItemCounts() {
setMenuItemIsLoading( 'new-error-index-count' );
setMenuItemIsLoading( 'new-validation-url-count' );
}

/**
* Updates the 'Validated URLs' and 'Error Index' menu items with their respective unreviewed count.
*
Expand All @@ -36,15 +71,23 @@ function updateMenuItem( itemEl, count ) {
function updateMenuItemCounts( counts ) {
const { validated_urls: newValidatedUrlCount, errors: newErrorCount } = counts;

const errorCountEl = document.getElementById( 'new-error-index-count' );
if ( errorCountEl ) {
updateMenuItem( errorCountEl, newErrorCount );
}
setMenuItemCountValue( 'new-error-index-count', newErrorCount );
setMenuItemCountValue( 'new-validation-url-count', newValidatedUrlCount );
}

const validatedUrlsCountEl = document.getElementById( 'new-validation-url-count' );
if ( validatedUrlsCountEl ) {
updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount );
}
/**
* Requests validation counts.
*/
function fetchValidationCounts() {
apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } ).then( ( counts ) => {
updateMenuItemCounts( counts );
} ).catch( ( error ) => {
updateMenuItemCounts( { validated_urls: 0, errors: 0 } );

const message = error?.message || __( 'An unknown error occurred while retrieving the validation counts', 'amp' );
// eslint-disable-next-line no-console
console.error( `[AMP Plugin] ${ message }` );
} );
}

/**
Expand All @@ -68,15 +111,7 @@ function createObserver( root ) {

observer.unobserve( target );

apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } ).then( ( counts ) => {
updateMenuItemCounts( counts );
} ).catch( ( error ) => {
updateMenuItemCounts( { validated_urls: 0, errors: 0 } );

const message = error?.message || __( 'An unknown error occurred while retrieving the validation counts', 'amp' );
// eslint-disable-next-line no-console
console.error( `[AMP Plugin] ${ message }` );
} );
fetchValidationCounts();
}, { root } );

observer.observe( target );
Expand All @@ -90,5 +125,13 @@ domReady( () => {
return;
}

createObserver( ampMenuItem );
initializeMenuItemCounts();

// If the AMP submenu is opened, fetch validation counts as soon as possible. Thanks to the preload middleware for
// `wp.apiFetch`, the validation count data should be available right away, so no actual HTTP request will be made.
if ( ampMenuItem.classList.contains( 'wp-menu-open' ) ) {
fetchValidationCounts();
} else {
createObserver( ampMenuItem );
}
} );
2 changes: 1 addition & 1 deletion includes/validation/class-amp-validated-url-post-type.php
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ public static function update_validated_url_menu_item() {

if ( ValidationCounts::is_needed() ) {
// Append markup to display a loading spinner while the unreviewed count is being fetched.
$submenu_item[0] .= ' <span id="new-validation-url-count" class="awaiting-mod"><span class="amp-count-loading"></span></span>';
$submenu_item[0] .= ' <span id="new-validation-url-count"></span>';
}

break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1745,7 +1745,7 @@ public static function add_admin_menu_validation_error_item() {

if ( ValidationCounts::is_needed() ) {
// Append markup to display a loading spinner while the unreviewed count is being fetched.
$menu_item_label .= ' <span id="new-error-index-count" class="awaiting-mod"><span class="amp-count-loading"></span></span>';
$menu_item_label .= ' <span id="new-error-index-count"></span>';
}

$post_menu_slug = 'edit.php?post_type=' . AMP_Validated_URL_Post_Type::POST_TYPE_SLUG;
Expand Down
38 changes: 37 additions & 1 deletion src/Admin/ValidationCounts.php
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
<?php
/**
* Class ValidatedUrlCounts.
* Class ValidationCounts.
*
* @package AmpProject\AmpWP
*/

namespace AmpProject\AmpWP\Admin;

use AMP_Options_Manager;
use AMP_Validated_URL_Post_Type;
use AmpProject\AmpWP\Infrastructure\Conditional;
use AmpProject\AmpWP\Infrastructure\Delayed;
use AmpProject\AmpWP\Infrastructure\HasRequirements;
Expand All @@ -30,6 +32,22 @@ final class ValidationCounts implements Service, Registerable, Conditional, Dela
*/
const ASSETS_HANDLE = 'amp-validation-counts';

/**
* RESTPreloader instance.
*
* @var RESTPreloader
*/
private $rest_preloader;

/**
* ValidationCounts constructor.
*
* @param RESTPreloader $rest_preloader An instance of the RESTPreloader class.
*/
public function __construct( RESTPreloader $rest_preloader ) {
$this->rest_preloader = $rest_preloader;
}

/**
* Get the action to use for registering the service.
*
Expand Down Expand Up @@ -93,5 +111,23 @@ public function enqueue_scripts() {
false,
$version
);

$this->maybe_add_preload_rest_paths();
}

/**
* Adds REST paths to preload.
*
* Preload validation counts data on an admin screen that has the AMP Options page as a parent or on any admin
* screen related to `amp_validation_error` post type (which includes the `amp_validation_error` taxonomy).
*/
protected function maybe_add_preload_rest_paths() {
if (
AMP_Options_Manager::OPTION_NAME === get_admin_page_parent()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't find a way to cover this condition with a unit test. I'm not sure how can I override get_admin_page_parent() in the test suite.

Copy link
Member

Choose a reason for hiding this comment

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

I've found a (hacky) way to do so in 7981cb0.

||
AMP_Validated_URL_Post_Type::POST_TYPE_SLUG === get_current_screen()->post_type
) {
$this->rest_preloader->add_preloaded_path( '/amp/v1/unreviewed-validation-counts' );
}
}
}
62 changes: 61 additions & 1 deletion tests/php/src/Admin/ValidationCountsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use AMP_Options_Manager;
use AMP_Theme_Support;
use AMP_Validated_URL_Post_Type;
use AmpProject\AmpWP\Admin\RESTPreloader;
use AmpProject\AmpWP\Admin\ValidationCounts;
use AmpProject\AmpWP\DevTools\UserAccess;
use AmpProject\AmpWP\Infrastructure\Conditional;
Expand All @@ -19,6 +20,7 @@
use AmpProject\AmpWP\Infrastructure\Service;
use AmpProject\AmpWP\Option;
use AmpProject\AmpWP\Services;
use AmpProject\AmpWP\Tests\Helpers\PrivateAccess;
use AmpProject\AmpWP\Tests\TestCase;

/**
Expand All @@ -28,6 +30,8 @@
*/
class ValidationCountsTest extends TestCase {

use PrivateAccess;

/**
* Test instance.
*
Expand All @@ -43,9 +47,13 @@ class ValidationCountsTest extends TestCase {
public function setUp() {
parent::setUp();

$this->instance = new ValidationCounts();
set_current_screen( 'edit' );
get_current_screen()->post_type = 'post';

$this->instance = new ValidationCounts( new RESTPreloader() );
}

/** @covers ::__construct() */
public function test__construct() {
$this->assertInstanceOf( ValidationCounts::class, $this->instance );
$this->assertInstanceOf( Delayed::class, $this->instance );
Expand Down Expand Up @@ -132,4 +140,56 @@ public function test_is_needed() {
delete_user_meta( $admin_user->ID, UserAccess::USER_FIELD_DEVELOPER_TOOLS_ENABLED );
$this->assertTrue( ValidationCounts::is_needed() );
}

/** @return array */
public function maybe_add_preload_rest_paths_data() {
return [
'no_type_post' => [
'screen' => 'user',
'post_type' => '',
'should_preload_path' => false,
],
'post_type_post' => [
'screen' => 'edit',
'post_type' => 'post',
'should_preload_path' => false,
],
'post_type_page' => [
'screen' => 'edit',
'post_type' => 'page',
'should_preload_path' => false,
],
'post_type_amp_validated_url' => [
'screen' => 'edit',
'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG,
'should_preload_path' => true,
],
];
}

/**
* @dataProvider maybe_add_preload_rest_paths_data
* @covers ::maybe_add_preload_rest_paths()
*/
public function test_maybe_add_preload_rest_paths( $screen, $post_type, $should_preload_path ) {
if ( ! function_exists( 'rest_preload_api_request' ) ) {
$this->markTestIncomplete( 'REST preload is not available so skipping.' );
}

set_current_screen( $screen );
if ( ! empty( $post_type ) ) {
get_current_screen()->post_type = $post_type;
}

$this->call_private_method( $this->instance, 'maybe_add_preload_rest_paths' );

$rest_preloader = $this->get_private_property( $this->instance, 'rest_preloader' );
$paths = $this->get_private_property( $rest_preloader, 'paths' );

if ( $should_preload_path ) {
$this->assertContains( '/amp/v1/unreviewed-validation-counts', $paths );
} else {
$this->assertNotContains( '/amp/v1/unreviewed-validation-counts', $paths );
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public function test_update_validated_url_menu_item() {

AMP_Validated_URL_Post_Type::update_validated_url_menu_item();
if ( Services::get( 'dependency_support' )->has_support() ) {
$this->assertSame( 'Validated URLs <span id="new-validation-url-count" class="awaiting-mod"><span class="amp-count-loading"></span></span>', $submenu[ AMP_Options_Manager::OPTION_NAME ][2][0] );
$this->assertSame( 'Validated URLs <span id="new-validation-url-count"></span>', $submenu[ AMP_Options_Manager::OPTION_NAME ][2][0] );
} else {
$this->assertSame( 'Validated URLs', $submenu[ AMP_Options_Manager::OPTION_NAME ][2][0] );
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1034,8 +1034,8 @@ public function test_add_admin_menu_validation_error_item() {
'Error Index',
];
if ( Services::get( 'dependency_support' )->has_support() ) {
$expected_submenu[0] .= ' <span id="new-error-index-count" class="awaiting-mod"><span class="amp-count-loading"></span></span>';
$expected_submenu[3] .= ' <span id="new-error-index-count" class="awaiting-mod"><span class="amp-count-loading"></span></span>';
$expected_submenu[0] .= ' <span id="new-error-index-count"></span>';
$expected_submenu[3] .= ' <span id="new-error-index-count"></span>';
}
$amp_options = $submenu[ AMP_Options_Manager::OPTION_NAME ];
$this->assertEquals( $expected_submenu, end( $amp_options ) );
Expand Down