From 8ba7980e786d21e8e100ccff92352308a1d8e601 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 18 Feb 2021 21:59:14 -0500 Subject: [PATCH 01/14] Remove 'At a Glance' filter for showing validation URL count summary --- .../class-amp-validated-url-post-type.php | 72 ------------------- ...test-class-amp-validated-url-post-type.php | 29 -------- 2 files changed, 101 deletions(-) diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index febf7df7a94..e98d0ce3321 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -5,10 +5,8 @@ * @package AMP */ -use AmpProject\AmpWP\DevTools\UserAccess; use AmpProject\AmpWP\Admin\OptionsMenu; use AmpProject\AmpWP\Icon; -use AmpProject\AmpWP\PluginRegistry; use AmpProject\AmpWP\Option; use AmpProject\AmpWP\QueryVar; use AmpProject\AmpWP\Services; @@ -270,13 +268,6 @@ public static function handle_plugin_update( $old_version ) { public static function add_admin_hooks() { add_action( 'admin_enqueue_scripts', [ __CLASS__, 'enqueue_post_list_screen_scripts' ] ); - $dev_tools_user_access = Services::get( 'dev_tools.user_access' ); - - if ( $dev_tools_user_access->is_user_enabled() ) { - add_filter( 'dashboard_glance_items', [ __CLASS__, 'filter_dashboard_glance_items' ] ); - add_action( 'rightnow_end', [ __CLASS__, 'print_dashboard_glance_styles' ] ); - } - // Edit post screen hooks. add_action( 'admin_enqueue_scripts', [ __CLASS__, 'enqueue_edit_post_screen_scripts' ] ); add_action( 'add_meta_boxes', [ __CLASS__, 'add_meta_boxes' ], PHP_INT_MAX ); @@ -2937,69 +2928,6 @@ public static function get_recheck_url( $url_or_post ) { ); } - /** - * Filter At a Glance items add AMP Validation Errors. - * - * @param array $items At a glance items. - * @return array Items. - */ - public static function filter_dashboard_glance_items( $items ) { - $count = self::get_validation_error_urls_count(); - - if ( 0 !== $count ) { - $items[] = sprintf( - '%s', - esc_url( - admin_url( - add_query_arg( - [ - 'post_type' => self::POST_TYPE_SLUG, - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_STATUS_QUERY_VAR => [ - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS, - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS, - ], - ], - 'edit.php' - ) - ) - ), - esc_html( - sprintf( - /* translators: %s is the validation error count */ - _n( - '%s URL w/ new AMP errors', - '%s URLs w/ new AMP errors', - $count, - 'amp' - ), - number_format_i18n( $count ) - ) - ) - ); - } - return $items; - } - - /** - * Print styles for the At a Glance widget. - */ - public static function print_dashboard_glance_styles() { - ?> - - user->create( [ 'role' => 'administrator' ] ) ); AMP_Validated_URL_Post_Type::add_admin_hooks(); - $this->assertEquals( 10, has_filter( 'dashboard_glance_items', [ self::TESTED_CLASS, 'filter_dashboard_glance_items' ] ) ); - $this->assertEquals( 10, has_action( 'rightnow_end', [ self::TESTED_CLASS, 'print_dashboard_glance_styles' ] ) ); - $this->assertEquals( 10, has_action( 'admin_enqueue_scripts', [ self::TESTED_CLASS, 'enqueue_edit_post_screen_scripts' ] ) ); $this->assertEquals( PHP_INT_MAX, has_action( 'add_meta_boxes', [ self::TESTED_CLASS, 'add_meta_boxes' ] ) ); $this->assertEquals( 10, has_action( 'edit_form_top', [ self::TESTED_CLASS, 'print_url_as_title' ] ) ); @@ -1654,32 +1651,6 @@ public function test_get_recheck_url() { $this->assertStringContains( wp_create_nonce( AMP_Validated_URL_Post_Type::NONCE_ACTION ), $link ); } - /** - * Test for filter_dashboard_glance_items() - * - * @covers \AMP_Validated_URL_Post_Type::filter_dashboard_glance_items() - */ - public function test_filter_dashboard_glance_items() { - - // There are no validation errors, so this should return the argument unchanged. - $this->assertEmpty( AMP_Validated_URL_Post_Type::filter_dashboard_glance_items( [] ) ); - - // Create validation errors, so that the method returns items. - $post_id = self::factory()->post->create(); - AMP_Validated_URL_Post_Type::store_validation_errors( - [ - [ 'code' => 'accepted' ], - [ 'code' => 'rejected' ], - [ 'code' => 'new' ], - ], - get_permalink( $post_id ) - ); - $items = AMP_Validated_URL_Post_Type::filter_dashboard_glance_items( [] ); - $this->assertStringContains( '1 URL w/ new AMP errors', $items[0] ); - $this->assertStringContains( AMP_Validated_URL_Post_Type::POST_TYPE_SLUG, $items[0] ); - $this->assertStringContains( AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_STATUS_QUERY_VAR, $items[0] ); - } - /** * Test for get_validated_url_title() * From 8b7baf5015c2c19a46800242159b5a0af4653b13 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 18 Feb 2021 22:13:57 -0500 Subject: [PATCH 02/14] Fetch unreviewed counts of validated URLs & errors via AJAX call; update respective menu items with counts --- .phpstorm.meta.php | 2 + assets/src/amp-validation/counts/index.js | 62 +++++++++ assets/src/amp-validation/counts/style.css | 24 ++++ .../class-amp-validated-url-post-type.php | 9 +- .../class-amp-validation-error-taxonomy.php | 10 +- src/Admin/ValidationCounts.php | 82 +++++++++++ src/AmpWpPlugin.php | 2 + src/DevTools/UserAccess.php | 2 - .../ValidationCountsRestController.php | 130 ++++++++++++++++++ webpack.config.js | 1 + 10 files changed, 308 insertions(+), 16 deletions(-) create mode 100644 assets/src/amp-validation/counts/index.js create mode 100644 assets/src/amp-validation/counts/style.css create mode 100644 src/Admin/ValidationCounts.php create mode 100644 src/Validation/ValidationCountsRestController.php diff --git a/.phpstorm.meta.php b/.phpstorm.meta.php index cd9765b85cc..2e9a22ea927 100644 --- a/.phpstorm.meta.php +++ b/.phpstorm.meta.php @@ -14,6 +14,7 @@ 'admin.options_menu' => \AmpProject\AmpWP\Admin\OptionsMenu::class, 'admin.polyfills' => \AmpProject\AmpWP\Admin\Polyfills::class, 'admin.paired_browsing' => \AmpProject\AmpWP\Admin\PairedBrowsing::class, + 'admin.validation_counts' => \AmpProject\AmpWP\Admin\ValidationCounts::class, 'amp_slug_customization_watcher' => \AmpProject\AmpWP\AmpSlugCustomizationWatcher::class, 'css_transient_cache.ajax_handler' => \AmpProject\AmpWP\Admin\ReenableCssTransientCachingAjaxAction::class, 'css_transient_cache.monitor' => \AmpProject\AmpWP\BackgroundTask\MonitorCssTransientCaching::class, @@ -33,6 +34,7 @@ 'plugin_suppression' => \AmpProject\AmpWP\PluginSuppression::class, 'reader_theme_loader' => \AmpProject\AmpWP\ReaderThemeLoader::class, 'rest.options_controller' => \AmpProject\AmpWP\OptionsRESTController::class, + 'rest.validation_counts_controller' => \AmpProject\AmpWP\Validation\ValidationCountsRestController::class, 'server_timing' => \AmpProject\AmpWP\Instrumentation\ServerTiming::class, 'site_health_integration' => \AmpProject\AmpWP\Admin\SiteHealth::class, 'validated_url_stylesheet_gc' => \AmpProject\AmpWP\BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class, diff --git a/assets/src/amp-validation/counts/index.js b/assets/src/amp-validation/counts/index.js new file mode 100644 index 00000000000..00a473aab16 --- /dev/null +++ b/assets/src/amp-validation/counts/index.js @@ -0,0 +1,62 @@ +/** + * External dependencies + */ +import { isPlainObject } from 'lodash'; + +/** + * WordPress dependencies + */ +import apiFetch from '@wordpress/api-fetch'; +import domReady from '@wordpress/dom-ready'; + +/** + * Internal dependencies + */ +import './style.css'; + +/** + * Updates a menu item with its count. + * + * 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 {HTMLElement} itemEl Menu item element. + * @param {number} count Count to set. + */ +function updateMenuItem( itemEl, count ) { + count = Math.abs( count ); + + if ( isNaN( count ) || count === 0 ) { + itemEl.parentNode.parentNode.removeChild( itemEl.parentNode ); + } else { + itemEl.classList.remove( 'loading' ); + itemEl.textContent = count.toLocaleString(); + } +} + +/** + * Updates the 'Validated URLs' and 'Error Index' menu items with their respective unreviewed count. + * + * @param {Object} counts Counts for menu items. + * @param {number} counts.validated_urls Unreviewed validated URLs count. + * @param {number} counts.errors Unreviewed validation errors count. + */ +function updateMenuItemCounts( counts ) { + if ( ! isPlainObject( counts ) ) { + // eslint-disable-next-line no-console + console.error( '[AMP Plugin] An error occurred while retrieving unreviewed validation counts. Received: ' + JSON.stringify( counts ) ); + counts = {}; + } + + const { validated_urls: newValidatedUrlCount, errors: newErrorCount } = counts; + + const errorCountEl = document.getElementById( 'new-error-index-count' ); + const validatedUrlsCountEl = document.getElementById( 'new-validation-url-count' ); + + updateMenuItem( errorCountEl, newErrorCount ); + updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount ); +} + +domReady( async () => { + const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } ); + updateMenuItemCounts( counts ); +} ); diff --git a/assets/src/amp-validation/counts/style.css b/assets/src/amp-validation/counts/style.css new file mode 100644 index 00000000000..c328d626768 --- /dev/null +++ b/assets/src/amp-validation/counts/style.css @@ -0,0 +1,24 @@ +@keyframes rotate-forever { + + 0% { + transform: rotate(0deg); + } + + 100% { + transform: rotate(360deg); + } +} + +#toplevel_page_amp-options > ul > li span.loading { + animation-duration: 0.75s; + animation-iteration-count: infinite; + animation-name: rotate-forever; + animation-timing-function: linear; + height: 4px; + width: 4px; + border: 2px solid #fff; + border-right-color: transparent; + border-top-color: transparent; + border-radius: 50%; + display: inline-block; +} diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php index e98d0ce3321..3abf54817fd 100644 --- a/includes/validation/class-amp-validated-url-post-type.php +++ b/includes/validation/class-amp-validated-url-post-type.php @@ -481,11 +481,8 @@ public static function update_validated_url_menu_item() { $menu_name_label = get_post_type_object( self::POST_TYPE_SLUG )->labels->menu_name; $submenu_item[0] = $menu_name_label; - // Display the count of new validation errors next to the label, if there are any. - $new_validation_error_url_count = self::get_validation_error_urls_count(); - if ( 0 < $new_validation_error_url_count ) { - $submenu_item[0] .= ' ' . esc_html( number_format_i18n( $new_validation_error_url_count ) ) . ''; - } + // Append markup to display a loading spinner while the unreviewed count is being fetched. + $submenu_item[0] .= ' '; break; } } @@ -498,7 +495,7 @@ public static function update_validated_url_menu_item() { * * @return int Count of new validation error URLs. */ - protected static function get_validation_error_urls_count() { + public static function get_validation_error_urls_count() { $count = get_transient( static::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT ); if ( false !== $count ) { // Handle case where integer stored in transient gets returned as string when persistent object cache is not diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php index 4c2d9e32dda..347fa284b55 100644 --- a/includes/validation/class-amp-validation-error-taxonomy.php +++ b/includes/validation/class-amp-validation-error-taxonomy.php @@ -1741,14 +1741,8 @@ public static function filter_tag_row_actions( $actions, WP_Term $tag ) { */ public static function add_admin_menu_validation_error_item() { $menu_item_label = esc_html__( 'Error Index', 'amp' ); - $new_error_count = self::get_validation_error_count( - [ - 'group' => [ self::VALIDATION_ERROR_NEW_REJECTED_STATUS, self::VALIDATION_ERROR_NEW_ACCEPTED_STATUS ], - ] - ); - if ( $new_error_count ) { - $menu_item_label .= ' ' . esc_html( number_format_i18n( $new_error_count ) ) . ''; - } + // Append markup to display a loading spinner while the unreviewed count is being fetched. + $menu_item_label .= ' '; $post_menu_slug = 'edit.php?post_type=' . AMP_Validated_URL_Post_Type::POST_TYPE_SLUG; $term_menu_slug = 'edit-tags.php?taxonomy=' . self::TAXONOMY_SLUG . '&post_type=' . AMP_Validated_URL_Post_Type::POST_TYPE_SLUG; diff --git a/src/Admin/ValidationCounts.php b/src/Admin/ValidationCounts.php new file mode 100644 index 00000000000..2353d446e89 --- /dev/null +++ b/src/Admin/ValidationCounts.php @@ -0,0 +1,82 @@ +is_user_enabled(); + } + + /** + * Runs on instantiation. + */ + public function register() { + add_action( 'admin_enqueue_scripts', [ $this, 'enqueue_scripts' ] ); + } + + /** + * Enqueue admin assets. + */ + public function enqueue_scripts() { + $asset_file = AMP__DIR__ . '/assets/js/' . self::ASSETS_HANDLE . '.asset.php'; + $asset = require $asset_file; + $dependencies = $asset['dependencies']; + $version = $asset['version']; + + wp_enqueue_script( + self::ASSETS_HANDLE, + amp_get_asset_url( 'js/' . self::ASSETS_HANDLE . '.js' ), + $dependencies, + $version, + false + ); + + wp_enqueue_style( + self::ASSETS_HANDLE, + amp_get_asset_url( 'css/' . self::ASSETS_HANDLE . '.css' ), + false, + $version + ); + } +} diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php index e3f9d79d2a3..dbe83c93547 100644 --- a/src/AmpWpPlugin.php +++ b/src/AmpWpPlugin.php @@ -66,6 +66,7 @@ final class AmpWpPlugin extends ServiceBasedPlugin { 'admin.options_menu' => Admin\OptionsMenu::class, 'admin.polyfills' => Admin\Polyfills::class, 'admin.paired_browsing' => Admin\PairedBrowsing::class, + 'admin.validation_counts' => Admin\ValidationCounts::class, 'amp_slug_customization_watcher' => AmpSlugCustomizationWatcher::class, 'css_transient_cache.ajax_handler' => Admin\ReenableCssTransientCachingAjaxAction::class, 'css_transient_cache.monitor' => BackgroundTask\MonitorCssTransientCaching::class, @@ -84,6 +85,7 @@ final class AmpWpPlugin extends ServiceBasedPlugin { 'plugin_suppression' => PluginSuppression::class, 'reader_theme_loader' => ReaderThemeLoader::class, 'rest.options_controller' => OptionsRESTController::class, + 'rest.validation_counts_controller' => Validation\ValidationCountsRestController::class, 'server_timing' => Instrumentation\ServerTiming::class, 'site_health_integration' => Admin\SiteHealth::class, 'validated_url_stylesheet_gc' => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class, diff --git a/src/DevTools/UserAccess.php b/src/DevTools/UserAccess.php index 4d63f7bb1d9..28457939083 100644 --- a/src/DevTools/UserAccess.php +++ b/src/DevTools/UserAccess.php @@ -35,8 +35,6 @@ final class UserAccess implements Service, Registerable { /** * Runs on instantiation. - * - * @action rest_api_init */ public function register() { add_action( 'rest_api_init', [ $this, 'register_rest_field' ] ); diff --git a/src/Validation/ValidationCountsRestController.php b/src/Validation/ValidationCountsRestController.php new file mode 100644 index 00000000000..984fc31f3b7 --- /dev/null +++ b/src/Validation/ValidationCountsRestController.php @@ -0,0 +1,130 @@ +namespace = 'amp/v1'; + $this->rest_base = 'unreviewed-validation-counts'; + $this->dev_tools_user_access = $user_access; + } + + /** + * Registers all routes for the controller. + */ + public function register() { + register_rest_route( + $this->namespace, + '/' . $this->rest_base, + [ + [ + 'methods' => WP_REST_Server::READABLE, + 'callback' => [ $this, 'get_items' ], + 'args' => [], + 'permission_callback' => [ $this, 'get_items_permissions_check' ], + ], + 'schema' => [ $this, 'get_public_item_schema' ], + ] + ); + } + + /** + * Checks whether the current user has access to Dev Tools. + * + * @param WP_REST_Request $request Full details about the request. + * @return true|WP_Error True if the request has permission; WP_Error object otherwise. + */ + public function get_items_permissions_check( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable + return $this->dev_tools_user_access->is_user_enabled(); + } + + /** + * Retrieves total unreviewed count for validation URLs and errors. + * + * @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_items( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable + $unreviewed_validated_url_count = AMP_Validated_URL_Post_Type::get_validation_error_urls_count(); + $unreviewed_validation_error_count = AMP_Validation_Error_Taxonomy::get_validation_error_count( + [ + 'group' => [ + AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS, + AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS + ], + ] + ); + + $response = [ + 'validated_urls' => $unreviewed_validated_url_count, + 'errors' => $unreviewed_validation_error_count, + ]; + + return rest_ensure_response( $response ); + } + + /** + * Retrieves the block type' schema, conforming to JSON Schema. + * + * @return array Item schema data. + */ + public function get_item_schema() { + return [ + '$schema' => 'http://json-schema.org/draft-04/schema#', + 'title' => 'amp-wp-validation-status', + 'type' => 'object', + 'properties' => [ + 'validation_urls' => [ + 'type' => 'integer', + 'readonly' => true, + ], + 'errors' => [ + 'type' => 'integer', + 'readonly' => true, + ] + ] + ]; + } +} diff --git a/webpack.config.js b/webpack.config.js index 23c7fa17b2e..e999d93f425 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -79,6 +79,7 @@ const ampValidation = { 'amp-validated-urls-index': './assets/src/amp-validation/amp-validated-urls-index.js', 'amp-validation-detail-toggle': './assets/src/amp-validation/amp-validation-detail-toggle.js', 'amp-validation-single-error-url-details': './assets/src/amp-validation/amp-validation-single-error-url-details.js', + 'amp-validation-counts': './assets/src/amp-validation/counts/index.js', }, plugins: [ ...sharedConfig.plugins, From e7bbd91ff74f56c0661d4f484983f6e544b33251 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Fri, 19 Feb 2021 00:35:07 -0500 Subject: [PATCH 03/14] Add tests --- .../ValidationCountsRestController.php | 28 +++-- tests/php/src/Admin/ValidationCountsTest.php | 85 +++++++++++++++ tests/php/src/OptionsRESTControllerTest.php | 4 +- .../ValidationCountsRestControllerTest.php | 101 ++++++++++++++++++ ...test-class-amp-validated-url-post-type.php | 12 +-- ...st-class-amp-validation-error-taxonomy.php | 4 +- 6 files changed, 209 insertions(+), 25 deletions(-) create mode 100644 tests/php/src/Admin/ValidationCountsTest.php create mode 100644 tests/php/src/Validation/ValidationCountsRestControllerTest.php diff --git a/src/Validation/ValidationCountsRestController.php b/src/Validation/ValidationCountsRestController.php index 984fc31f3b7..010b830c10d 100644 --- a/src/Validation/ValidationCountsRestController.php +++ b/src/Validation/ValidationCountsRestController.php @@ -11,7 +11,6 @@ use AMP_Validated_URL_Post_Type; use AMP_Validation_Error_Taxonomy; use AmpProject\AmpWP\DevTools\UserAccess; -use AmpProject\AmpWP\Services; use WP_Error; use WP_REST_Controller; use AmpProject\AmpWP\Infrastructure\Delayed; @@ -29,6 +28,7 @@ */ final class ValidationCountsRestController extends WP_REST_Controller implements Delayed, Service, Registerable { + /** @var UserAccess DevTools User Access */ private $dev_tools_user_access; /** @@ -46,8 +46,8 @@ public static function get_registration_action() { * @param UserAccess $user_access Instance of UserAccess class. */ public function __construct( UserAccess $user_access ) { - $this->namespace = 'amp/v1'; - $this->rest_base = 'unreviewed-validation-counts'; + $this->namespace = 'amp/v1'; + $this->rest_base = 'unreviewed-validation-counts'; $this->dev_tools_user_access = $user_access; } @@ -77,7 +77,17 @@ public function register() { * @return true|WP_Error True if the request has permission; WP_Error object otherwise. */ public function get_items_permissions_check( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - return $this->dev_tools_user_access->is_user_enabled(); + $dev_tools_access = $this->dev_tools_user_access->is_user_enabled(); + + if ( ! $dev_tools_access ) { + return new WP_Error( + 'amp_rest_no_dev_tools_access', + __( 'Sorry, you are not allowed to view unreviewed counts for validation errors.', 'amp' ), + [ 'status' => rest_authorization_required_code() ] + ); + } + + return true; } /** @@ -87,12 +97,12 @@ public function get_items_permissions_check( $request ) { // phpcs:ignore Variab * @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure. */ public function get_items( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - $unreviewed_validated_url_count = AMP_Validated_URL_Post_Type::get_validation_error_urls_count(); + $unreviewed_validated_url_count = AMP_Validated_URL_Post_Type::get_validation_error_urls_count(); $unreviewed_validation_error_count = AMP_Validation_Error_Taxonomy::get_validation_error_count( [ 'group' => [ AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS, - AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS + AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS, ], ] ); @@ -120,11 +130,11 @@ public function get_item_schema() { 'type' => 'integer', 'readonly' => true, ], - 'errors' => [ + 'errors' => [ 'type' => 'integer', 'readonly' => true, - ] - ] + ], + ], ]; } } diff --git a/tests/php/src/Admin/ValidationCountsTest.php b/tests/php/src/Admin/ValidationCountsTest.php new file mode 100644 index 00000000000..a1fb3b00fcb --- /dev/null +++ b/tests/php/src/Admin/ValidationCountsTest.php @@ -0,0 +1,85 @@ +instance = new ValidationCounts(); + } + + public function test__construct() { + $this->assertInstanceOf( ValidationCounts::class, $this->instance ); + $this->assertInstanceOf( Delayed::class, $this->instance ); + $this->assertInstanceOf( Conditional::class, $this->instance ); + $this->assertInstanceOf( Service::class, $this->instance ); + $this->assertInstanceOf( Registerable::class, $this->instance ); + } + + /** + * Test ::get_registration_action(). + * + * @covers ::get_registration_action() + */ + public function test_get_registration_action() { + self::assertEquals( 'admin_init', ValidationCounts::get_registration_action() ); + } + + /** + * Test ::register(). + * + * @covers ::register() + */ + public function test_register() { + $this->instance->register(); + + $this->assertEquals( 10, has_action( 'admin_enqueue_scripts', [ $this->instance, 'enqueue_scripts' ] ) ); + } + + /** + * Test ::is_needed(). + * + * @covers ::is_needed() + */ + public function test_is_needed() { + $this->assertFalse( ValidationCounts::is_needed() ); + + $admin_user = self::factory()->user->create_and_get( [ 'role' => 'administrator' ] ); + wp_set_current_user( $admin_user->ID ); + update_user_meta( $admin_user->ID, UserAccess::USER_FIELD_DEVELOPER_TOOLS_ENABLED, wp_json_encode( true ) ); + + $this->assertTrue( ValidationCounts::is_needed() ); + } +} diff --git a/tests/php/src/OptionsRESTControllerTest.php b/tests/php/src/OptionsRESTControllerTest.php index 36c58a0729c..d35531f1729 100644 --- a/tests/php/src/OptionsRESTControllerTest.php +++ b/tests/php/src/OptionsRESTControllerTest.php @@ -2,7 +2,7 @@ /** * Tests for OptionsRESTController. * - * @package AMP + * @package AmpProject\AmpWP */ namespace AmpProject\AmpWP\Tests; @@ -16,8 +16,6 @@ /** * Tests for OptionsRESTController. * - * @group amp-options - * * @coversDefaultClass \AmpProject\AmpWP\OptionsRESTController */ class OptionsRESTControllerTest extends DependencyInjectedTestCase { diff --git a/tests/php/src/Validation/ValidationCountsRestControllerTest.php b/tests/php/src/Validation/ValidationCountsRestControllerTest.php new file mode 100644 index 00000000000..a03e08559f2 --- /dev/null +++ b/tests/php/src/Validation/ValidationCountsRestControllerTest.php @@ -0,0 +1,101 @@ +controller = $this->injector->make( ValidationCountsRestController::class ); + } + + /** + * Test ::get_registration_action(). + * + * @covers ::get_registration_action() + */ + public function test_get_registration_action() { + self::assertEquals( 'rest_api_init', ValidationCountsRestController::get_registration_action() ); + } + + /** + * Test ::get_items_permissions_check(). + * + * @covers ::get_items_permissions_check() + */ + public function test_get_items_permissions_check() { + $admin_user = self::factory()->user->create_and_get( [ 'role' => 'administrator' ] ); + + $this->assertWPError( $this->controller->get_items_permissions_check( new WP_REST_Request( 'GET', '/amp/v1/unreviewed-validation-counts' ) ) ); + + wp_set_current_user( $admin_user->ID ); + $this->assertWPError( $this->controller->get_items_permissions_check( new WP_REST_Request( 'GET', '/amp/v1/unreviewed-validation-counts' ) ) ); + + update_user_meta( $admin_user->ID, UserAccess::USER_FIELD_DEVELOPER_TOOLS_ENABLED, wp_json_encode( true ) ); + $this->assertTrue( $this->controller->get_items_permissions_check( new WP_REST_Request( 'GET', '/amp/v1/unreviewed-validation-counts' ) ) ); + } + + /** + * Test ::get_items(). + * + * @covers ::get_items() + */ + public function test_get_items() { + $admin_user = self::factory()->user->create_and_get( [ 'role' => 'administrator' ] ); + wp_set_current_user( $admin_user->ID ); + update_user_meta( $admin_user->ID, UserAccess::USER_FIELD_DEVELOPER_TOOLS_ENABLED, wp_json_encode( true ) ); + + $request = new WP_REST_Request( 'GET', '/amp/v1/unreviewed-validation-counts' ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertEquals( [ 'validated_urls' => 0, 'errors' => 0 ], $response->get_data() ); + + AMP_Validated_URL_Post_Type::store_validation_errors( + [ + [ + 'code' => 'foobar', + ], + ], + get_permalink( self::factory()->post->create() ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertEquals( [ 'validated_urls' => 1, 'errors' => 1 ], $response->get_data() ); + } + + /** + * Test ::get_item_schema(). + * + * @covers ::get_item_schema() + */ + public function test_get_item_schema() { + self::assertEquals( [ 'validation_urls', 'errors' ], array_keys( $this->controller->get_item_schema()['properties'] ) ); + } +} diff --git a/tests/php/validation/test-class-amp-validated-url-post-type.php b/tests/php/validation/test-class-amp-validated-url-post-type.php index b5412398c94..2524ccbbe26 100644 --- a/tests/php/validation/test-class-amp-validated-url-post-type.php +++ b/tests/php/validation/test-class-amp-validated-url-post-type.php @@ -152,19 +152,9 @@ public function test_update_validated_url_menu_item() { ], ]; - $invalid_url_post_id = AMP_Validated_URL_Post_Type::store_validation_errors( - [ - [ - 'code' => 'hello', - ], - ], - get_permalink( self::factory()->post->create() ) - ); - $this->assertNotInstanceOf( 'WP_Error', $invalid_url_post_id ); - AMP_Validated_URL_Post_Type::update_validated_url_menu_item(); - $this->assertSame( 'Validated URLs 1', $submenu[ AMP_Options_Manager::OPTION_NAME ][2][0] ); + $this->assertSame( 'Validated URLs ', $submenu[ AMP_Options_Manager::OPTION_NAME ][2][0] ); $submenu = $original_submenu; } diff --git a/tests/php/validation/test-class-amp-validation-error-taxonomy.php b/tests/php/validation/test-class-amp-validation-error-taxonomy.php index 96764d7ef02..8cb3d018a1e 100644 --- a/tests/php/validation/test-class-amp-validation-error-taxonomy.php +++ b/tests/php/validation/test-class-amp-validation-error-taxonomy.php @@ -1008,10 +1008,10 @@ public function test_add_admin_menu_validation_error_item() { wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); AMP_Validation_Error_Taxonomy::add_admin_menu_validation_error_item(); $expected_submenu = [ - 'Error Index', + 'Error Index ', AMP_Validation_Manager::VALIDATE_CAPABILITY, 'edit-tags.php?taxonomy=amp_validation_error&post_type=amp_validated_url', - 'Error Index', + 'Error Index ', ]; $amp_options = $submenu[ AMP_Options_Manager::OPTION_NAME ]; $this->assertEquals( $expected_submenu, end( $amp_options ) ); From df9c825b00067d80290ea1544da54b73f21ebcdb Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Fri, 19 Feb 2021 00:40:41 -0500 Subject: [PATCH 04/14] Fix lint issue --- .../ValidationCountsRestControllerTest.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/php/src/Validation/ValidationCountsRestControllerTest.php b/tests/php/src/Validation/ValidationCountsRestControllerTest.php index a03e08559f2..65f761da755 100644 --- a/tests/php/src/Validation/ValidationCountsRestControllerTest.php +++ b/tests/php/src/Validation/ValidationCountsRestControllerTest.php @@ -75,7 +75,13 @@ public function test_get_items() { $request = new WP_REST_Request( 'GET', '/amp/v1/unreviewed-validation-counts' ); $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( [ 'validated_urls' => 0, 'errors' => 0 ], $response->get_data() ); + $this->assertEquals( + [ + 'validated_urls' => 0, + 'errors' => 0, + ], + $response->get_data() + ); AMP_Validated_URL_Post_Type::store_validation_errors( [ @@ -87,7 +93,13 @@ public function test_get_items() { ); $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( [ 'validated_urls' => 1, 'errors' => 1 ], $response->get_data() ); + $this->assertEquals( + [ + 'validated_urls' => 1, + 'errors' => 1, + ], + $response->get_data() + ); } /** From 5c83722317af7852bbfd15ece5148f3760783529 Mon Sep 17 00:00:00 2001 From: Pierre Gordon <16200219+pierlon@users.noreply.github.com> Date: Sat, 20 Feb 2021 02:17:23 +0000 Subject: [PATCH 05/14] Register service on `admin_enqueue_scripts` action Co-authored-by: Weston Ruter --- src/Admin/ValidationCounts.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Admin/ValidationCounts.php b/src/Admin/ValidationCounts.php index 2353d446e89..9e5368dd0ba 100644 --- a/src/Admin/ValidationCounts.php +++ b/src/Admin/ValidationCounts.php @@ -35,7 +35,7 @@ final class ValidationCounts implements Service, Registerable, Conditional, Dela * @return string Registration action to use. */ public static function get_registration_action() { - return 'admin_init'; + return 'admin_enqueue_scripts'; } /** @@ -52,7 +52,7 @@ public static function is_needed() { * Runs on instantiation. */ public function register() { - add_action( 'admin_enqueue_scripts', [ $this, 'enqueue_scripts' ] ); + $this->enqueue_scripts(); } /** From 911bbd0e85dd1f1a6d58de4f372ee1264eda57e7 Mon Sep 17 00:00:00 2001 From: Pierre Gordon <16200219+pierlon@users.noreply.github.com> Date: Sat, 20 Feb 2021 02:19:07 +0000 Subject: [PATCH 06/14] Load script in footer Co-authored-by: Weston Ruter --- src/Admin/ValidationCounts.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Admin/ValidationCounts.php b/src/Admin/ValidationCounts.php index 9e5368dd0ba..fe019c6e88a 100644 --- a/src/Admin/ValidationCounts.php +++ b/src/Admin/ValidationCounts.php @@ -69,7 +69,7 @@ public function enqueue_scripts() { amp_get_asset_url( 'js/' . self::ASSETS_HANDLE . '.js' ), $dependencies, $version, - false + true ); wp_enqueue_style( From 14fd4b27794d2b328b52dc72f171e944598920ef Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Fri, 19 Feb 2021 21:20:29 -0500 Subject: [PATCH 07/14] Update jsdoc for `counts` parameter --- assets/src/amp-validation/counts/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/src/amp-validation/counts/index.js b/assets/src/amp-validation/counts/index.js index 00a473aab16..8fe22924363 100644 --- a/assets/src/amp-validation/counts/index.js +++ b/assets/src/amp-validation/counts/index.js @@ -36,7 +36,7 @@ function updateMenuItem( itemEl, count ) { /** * Updates the 'Validated URLs' and 'Error Index' menu items with their respective unreviewed count. * - * @param {Object} counts Counts for menu items. + * @param {any} counts Counts for menu items. * @param {number} counts.validated_urls Unreviewed validated URLs count. * @param {number} counts.errors Unreviewed validation errors count. */ From 9c43ee64566e652dba04515337e893c7bcd48cbf Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Fri, 19 Feb 2021 21:30:09 -0500 Subject: [PATCH 08/14] Update `ValidationCounts` test --- tests/php/src/Admin/ValidationCountsTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/php/src/Admin/ValidationCountsTest.php b/tests/php/src/Admin/ValidationCountsTest.php index a1fb3b00fcb..a1051fe8657 100644 --- a/tests/php/src/Admin/ValidationCountsTest.php +++ b/tests/php/src/Admin/ValidationCountsTest.php @@ -54,7 +54,7 @@ public function test__construct() { * @covers ::get_registration_action() */ public function test_get_registration_action() { - self::assertEquals( 'admin_init', ValidationCounts::get_registration_action() ); + self::assertEquals( 'admin_enqueue_scripts', ValidationCounts::get_registration_action() ); } /** @@ -65,7 +65,8 @@ public function test_get_registration_action() { public function test_register() { $this->instance->register(); - $this->assertEquals( 10, has_action( 'admin_enqueue_scripts', [ $this->instance, 'enqueue_scripts' ] ) ); + $this->assertTrue( wp_script_is( ValidationCounts::ASSETS_HANDLE ) ); + $this->assertTrue( wp_style_is( ValidationCounts::ASSETS_HANDLE ) ); } /** From 70941a5d713af261113b6d4145fe28872145346d Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Mon, 22 Feb 2021 15:32:52 -0500 Subject: [PATCH 09/14] Use intersection observer to determine when to fetch validation counts --- assets/src/amp-validation/counts/index.js | 49 ++++++++++++++++++----- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/assets/src/amp-validation/counts/index.js b/assets/src/amp-validation/counts/index.js index 8fe22924363..68a935f7eca 100644 --- a/assets/src/amp-validation/counts/index.js +++ b/assets/src/amp-validation/counts/index.js @@ -6,6 +6,7 @@ import { isPlainObject } from 'lodash'; /** * WordPress dependencies */ +import { __ } from '@wordpress/i18n'; import apiFetch from '@wordpress/api-fetch'; import domReady from '@wordpress/dom-ready'; @@ -41,12 +42,6 @@ function updateMenuItem( itemEl, count ) { * @param {number} counts.errors Unreviewed validation errors count. */ function updateMenuItemCounts( counts ) { - if ( ! isPlainObject( counts ) ) { - // eslint-disable-next-line no-console - console.error( '[AMP Plugin] An error occurred while retrieving unreviewed validation counts. Received: ' + JSON.stringify( counts ) ); - counts = {}; - } - const { validated_urls: newValidatedUrlCount, errors: newErrorCount } = counts; const errorCountEl = document.getElementById( 'new-error-index-count' ); @@ -56,7 +51,43 @@ function updateMenuItemCounts( counts ) { updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount ); } -domReady( async () => { - const counts = await apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } ); - updateMenuItemCounts( counts ); +/** + * Fetches the validation counts only when the AMP submenu is open for the first time. + * + * @param {HTMLElement} root AMP submenu item. + */ +function createObserver( root ) { + // let hasIntersected = false; + + const observer = new IntersectionObserver( ( entries ) => { + if ( ! entries[ 0 ].isIntersecting ) { + return; + } + + observer.unobserve( entries[ 0 ].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 }` ); + } ); + }, { root } ); + + const target = root.querySelector( 'ul' ); + observer.observe( target ); +} + +domReady( () => { + const ampMenuItem = document.getElementById( 'toplevel_page_amp-options' ); + + // Bail if the AMP submenu is not in the DOM. + if ( ! ampMenuItem ) { + return; + } + + createObserver( ampMenuItem ); } ); From 8d90f2d280b8fc3dced914e4f545e1e46182c909 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Mon, 22 Feb 2021 15:33:50 -0500 Subject: [PATCH 10/14] Harden updates of submenu items --- assets/src/amp-validation/counts/index.js | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/assets/src/amp-validation/counts/index.js b/assets/src/amp-validation/counts/index.js index 68a935f7eca..b38db40d72d 100644 --- a/assets/src/amp-validation/counts/index.js +++ b/assets/src/amp-validation/counts/index.js @@ -1,8 +1,3 @@ -/** - * External dependencies - */ -import { isPlainObject } from 'lodash'; - /** * WordPress dependencies */ @@ -24,8 +19,6 @@ import './style.css'; * @param {number} count Count to set. */ function updateMenuItem( itemEl, count ) { - count = Math.abs( count ); - if ( isNaN( count ) || count === 0 ) { itemEl.parentNode.parentNode.removeChild( itemEl.parentNode ); } else { @@ -37,7 +30,7 @@ function updateMenuItem( itemEl, count ) { /** * Updates the 'Validated URLs' and 'Error Index' menu items with their respective unreviewed count. * - * @param {any} counts Counts for menu items. + * @param {Object} counts Counts for menu items. * @param {number} counts.validated_urls Unreviewed validated URLs count. * @param {number} counts.errors Unreviewed validation errors count. */ @@ -45,10 +38,14 @@ function updateMenuItemCounts( counts ) { const { validated_urls: newValidatedUrlCount, errors: newErrorCount } = counts; const errorCountEl = document.getElementById( 'new-error-index-count' ); - const validatedUrlsCountEl = document.getElementById( 'new-validation-url-count' ); + if ( errorCountEl ) { + updateMenuItem( errorCountEl, newErrorCount ); + } - updateMenuItem( errorCountEl, newErrorCount ); - updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount ); + const validatedUrlsCountEl = document.getElementById( 'new-validation-url-count' ); + if ( validatedUrlsCountEl ) { + updateMenuItem( validatedUrlsCountEl, newValidatedUrlCount ); + } } /** From 2a09ef138b62db756dcd834d1cc4d4a1ea5a2209 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Mon, 22 Feb 2021 16:18:05 -0500 Subject: [PATCH 11/14] Add `browser` env global support to eslint --- .eslintrc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.eslintrc b/.eslintrc index 2ec892c61af..54d6396fbf6 100644 --- a/.eslintrc +++ b/.eslintrc @@ -5,6 +5,9 @@ "plugin:import/recommended", "plugin:eslint-comments/recommended" ], + "env": { + "browser": true + }, "rules": { "block-scoped-var": "error", "complexity": ["error", { "max": 20 } ], From de08cbd6010ec3211970efdf7d6fd4158508bc70 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Mon, 22 Feb 2021 16:25:31 -0500 Subject: [PATCH 12/14] Remove duplicate declaration of global vars --- assets/js/amp-service-worker-runtime-precaching.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/js/amp-service-worker-runtime-precaching.js b/assets/js/amp-service-worker-runtime-precaching.js index 8706b4474b4..ecd606a1ff1 100644 --- a/assets/js/amp-service-worker-runtime-precaching.js +++ b/assets/js/amp-service-worker-runtime-precaching.js @@ -1,4 +1,4 @@ -/* global self, caches, URLS */ +/* global URLS */ // See AMP_Service_Workers::add_amp_runtime_caching() and . { self.addEventListener( 'install', ( event ) => { From ed31fe35c49d645ba3388997f8d3c0ca94c45ec2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 22 Feb 2021 21:00:32 -0800 Subject: [PATCH 13/14] Account for IntersectionObserver not being supported in IE11 by just hiding counts --- assets/src/amp-validation/counts/index.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/assets/src/amp-validation/counts/index.js b/assets/src/amp-validation/counts/index.js index b38db40d72d..41b9413260a 100644 --- a/assets/src/amp-validation/counts/index.js +++ b/assets/src/amp-validation/counts/index.js @@ -54,7 +54,11 @@ function updateMenuItemCounts( counts ) { * @param {HTMLElement} root AMP submenu item. */ function createObserver( root ) { - // let hasIntersected = false; + // For IE11 IntersectionObserver is not available, so just hide the counts entirely. + if ( ! ( 'IntersectionObserver' in window ) ) { + updateMenuItemCounts( { validated_urls: 0, errors: 0 } ); + return; + } const observer = new IntersectionObserver( ( entries ) => { if ( ! entries[ 0 ].isIntersecting ) { From f1005a975c2bb6228829606dd85b78d05b9c3df8 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 23 Feb 2021 10:23:35 -0800 Subject: [PATCH 14/14] Reuse target to unobserve --- assets/src/amp-validation/counts/index.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/assets/src/amp-validation/counts/index.js b/assets/src/amp-validation/counts/index.js index 41b9413260a..eb91e945af5 100644 --- a/assets/src/amp-validation/counts/index.js +++ b/assets/src/amp-validation/counts/index.js @@ -54,18 +54,20 @@ function updateMenuItemCounts( counts ) { * @param {HTMLElement} root AMP submenu item. */ function createObserver( root ) { - // For IE11 IntersectionObserver is not available, so just hide the counts entirely. + // IntersectionObserver is not available in IE11, so just hide the counts entirely for that browser. if ( ! ( 'IntersectionObserver' in window ) ) { updateMenuItemCounts( { validated_urls: 0, errors: 0 } ); return; } - const observer = new IntersectionObserver( ( entries ) => { - if ( ! entries[ 0 ].isIntersecting ) { + const target = root.querySelector( 'ul' ); + + const observer = new IntersectionObserver( ( [ entry ] ) => { + if ( ! entry || ! entry.isIntersecting ) { return; } - observer.unobserve( entries[ 0 ].target ); + observer.unobserve( target ); apiFetch( { path: '/amp/v1/unreviewed-validation-counts' } ).then( ( counts ) => { updateMenuItemCounts( counts ); @@ -78,7 +80,6 @@ function createObserver( root ) { } ); }, { root } ); - const target = root.querySelector( 'ul' ); observer.observe( target ); }