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

Improve URL Listing view #1397

Merged
merged 63 commits into from Sep 16, 2018

Conversation

Projects
None yet
4 participants
@miina
Collaborator

miina commented Sep 4, 2018

Fixes #1362.

Updates the Error by URLs view according to this sketch.

if ( isset( $source['type'], $source['name'] ) ) {
$invalid_sources[ $source['type'] ][] = $source['name'];
foreach ( $sources['sources'] as $source ) {

This comment has been minimized.

@miina

miina Sep 4, 2018

Collaborator

^ Changed this since the sources were not properly displaying in the table -- the type and name were never set since the actual structure of $validation_error seems to be like this:
screen shot 2018-09-04 at 5 21 41 pm

Meaning that the type and name would be set in $validation_error['sources']['sources'][0], not in $validation_error['sources'].

However, this change broke the tests since the input data of test is assuming that a validation error looks like this instead:
screen shot 2018-09-04 at 5 43 21 pm

Is the structure of validation error incorrect for some reason and it should be as assumed in the tests? Maybe the fix should be removing the second nested sources instead, or is it like this for some reason? Thoughts?

cc @kienstra @westonruter

This comment has been minimized.

@kienstra

kienstra Sep 6, 2018

Collaborator

Hi @miina,
That's interesting. I couldn't reproduce that issue.

It looks like there aren't double-nested 'sources' arrays, like in your first screenshot when I run the develop branch:

name-type-local

Maybe the validation error data model changed.

It might help to delete all validation error posts and terms, and regenerate them (thanks to Weston, though I might not have copied it right):

wp post delete $(wp post list --post_type='amp_invalid_url' --format=ids)
wp term list amp_validation_error --field=term_id | xargs wp term delete amp_validation_error

You might already have some logic to output validation errors, but here's a simple plugin I've used (also partially copied from Weston)

And the WP-CLI script can then regenerate errors:

wp amp validate-site --limit=5
@miina

This comment has been minimized.

Collaborator

miina commented Sep 4, 2018

@kienstra Created this WIP PR as a start for resolving #1362, however, I'm going to offline from Wed-Fri (5-7th of September) so if you'd like to pick this up meanwhile or implement together with #1361 (there are a few matching tasks) then feel free to do so.

kienstra added some commits Sep 6, 2018

Conform to validation error data model.
It look like there aren't double-nested 'source' arrays.
So remove the nested foreach loop.
Merge in develop, resolve conflict.
In the develop branch,
filter_views_edit() is removed entirely,
while it was simply edited in this branch:
1362-improved_url_listing_view
So favor the develop branch,
where it's removed.
Update status names to latest design
For example, change 'To Fix Later' to 'Rejected'.
Also, update the labels.
I don't think all of them need to have 'Errors by URL',
like 'No errors by URL found'.

@jacobschweitzer jacobschweitzer requested a review from westonruter Sep 11, 2018

jacobschweitzer and others added some commits Sep 11, 2018

Address unit test errors
Updates the unit tests to the latest changes
They now pass locally.
Addres PHPCS warning by aligning =
The build failed, with:
Equals sign not aligned with surrounding assignments.
So address this by aligning the =
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Sep 13, 2018

Request For Review

Hi @westonruter,
Could you please review this, now that the Travis build passes?

Thanks, Weston!

}
if ( isset( $sources['theme'] ) ) {
$output[] = '<div class="source">';
$output[] = sprintf( '<span class="dashicons dashicons-admin-appearance"></span><strong>%s</strong>', esc_html( $sources['theme']['name'] ) );

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

I'm getting a PHP notice here:

image

The name isn't stored in the validation error (no should it be). The $sources['theme'] variable contains an array like [ 'twentyseventeen' ]. So this should be looping over each $theme and get wp_get_theme( $theme )->get('Name'). If the theme does not exist, then the slug should be shown instead (and obviously the result is stale in this case).

}
$output[] = '<div class="' . $sources_container_classes . '">';
$output[] = implode( '<br/>', array_unique( $sources['plugin'] ) );

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

This should obtain the actual names of the plugins using get_plugin_data() and getting the Name returned. If the plugin doesn't exist then show the slug (and this result is obviously stale).

/**
* Enqueue style.
*/
public static function enqueue_admin_assets() {

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

Can this name be made more specific? There is enqueue_edit_post_screen_scripts. So should it be enqueue_post_list_screen_scripts?

},

addViewErrorsByTypeLinkButton: function() {
$( '.wp-heading-inline' ).after( '<a href="' + ampAdminTables.errorsByTypeLink + '" class="page-title-action">View errors by <strong>Type</strong></a>' );

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

This shouldn't be using string concatenation for constructing HTML. It would be preferable to do var link = $( '<a>...</a>' ); And then do link.prop('href', ampAdminTables.errorsByTypeLink).

},

addViewErrorsByTypeLinkButton: function() {
$( '.wp-heading-inline' ).after( '<a href="' + ampAdminTables.errorsByTypeLink + '" class="page-title-action">View errors by <strong>Type</strong></a>' );

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

The text here is not translatable. I suggest doing this: at the admin footer action whenever you are printing scripts, output right to the page the entire link tag with esc_url() and esc_html_e() and such. Give that link an ID and include a hidden attribute. Then in this addViewErrorsByTypeLinkButton function, just move the element in the DOM and remove the hidden attribute.

$( '.wp-heading-inline' ).after( '<a href="' + ampAdminTables.errorsByTypeLink + '" class="page-title-action">View errors by <strong>Type</strong></a>' );
},

boldURLInPageTitle: function() {

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

We can remove this if we go with “Invalid URL” per Slack convo.

( function( $ ) {
'use strict';

var adminTables = {

This comment has been minimized.

@westonruter

This comment has been minimized.

@jacobschweitzer

jacobschweitzer Sep 14, 2018

Collaborator

@westonruter I started working on this but realized a dependency of the script you mentioned is wp-dom-ready which is a part of Gutenberg. So this script doesn't load for me since on my test site Gutenberg is not activated. Are we assuming that everyone will have Gutenberg as well or why is this a dependency? Or did I miss something in the build process?

This comment has been minimized.

@kienstra

kienstra Sep 14, 2018

Collaborator

Hi @westonruter and @jacobschweitzer,
Do you think we should add dom-ready to package.json, like how i18n is included there:

  "@wordpress/dom-ready": "^2.0.0",
  "@wordpress/i18n": "^1.1.0",

Still, npm run dev seemed to build amp-validation-error-detail-toggle.js fine in my local, and that script ran without error on the 'AMP Validation Error Index' page.

This comment has been minimized.

@jacobschweitzer

jacobschweitzer Sep 14, 2018

Collaborator

@kienstra I've added that line to package.json and ran npm run build / npm run dev but the script still won't load on the Error Index page. Am I missing something here? I did have to install webpack and wp-cli but I have those now so I'm not sure if there is another dependency I'm missing since there aren't any errors in the build process now.

@@ -286,25 +318,34 @@ public static function display_invalid_url_validation_error_counts_summary( $pos
$result = array();
if ( $counts['new'] ) {
$result[] = esc_html( sprintf(
if ( current_theme_supports( 'amp' ) && AMP_Validation_Manager::is_sanitization_forcibly_accepted() ) {

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

We can probably remove current_theme_supports( 'amp' ) &&

AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS => esc_html__( 'Removed Elements', 'amp' ),
AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES => esc_html__( 'Removed Attributes', 'amp' ),
AMP_Validation_Error_Taxonomy::SOURCES_INVALID_OUTPUT => esc_html__( 'Incompatible Sources', 'amp' ),
'error_status' => sprintf( '%s<span class="dashicons dashicons-editor-help"></span>', esc_html__( 'Status', 'amp' ) ), // @todo Create actual tooltip.

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

The 'error_status' here can be replaced with the AMP_Validation_Error_Taxonomy::ERROR_STATUS constant I believe.

/* translators: %s is count */
__( '&#x274C; Rejected: %s', 'amp' ),
__( '<span class="dashicons dashicons-warning rejected"></span><span class="error-status rejected">%1$s: %2$s</span>', 'amp' ),

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

I don't think the span shouldn't be included in the translation string.

__( '&#x2705; Accepted: %s', 'amp' ),
$result[] = sprintf(
/* translators: 1. Title, 2. %s is count */
__( '<span class="amp-logo-icon"></span><span class="error-status accepted">%1$s: %2$s</span>', 'amp' ),

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

I don't think the span shouldn't be included in the translation string.

/* translators: %s is count */
__( '&#x2753; New: %s', 'amp' ),
__( '<span class="dashicons dashicons-%1$s identified"></span><span class="error-status identified">%2$s: %3$s</span>', 'amp' ),

This comment has been minimized.

@westonruter

westonruter Sep 13, 2018

Member

I don't think the span shouldn't be included in the translation string.

@jacobschweitzer

This comment has been minimized.

Collaborator

jacobschweitzer commented Sep 14, 2018

@westonruter , @kienstra found that if you remove the domReady from the webpack config, remove it as a dependency of the script loading, and do npm run dev again we can get the script to run successfully. It is included here: 56915da but it is a workaround, so if you can come up with a fix or have another idea that'd be awesome.

Otherwise these changes are ready for review. Thanks @westonruter !

jacobschweitzer and others added some commits Sep 15, 2018

Apply remaining changes from code review
* Output theme name if available instead of theme slug.
* Fix logic for obtaining plugin name when plugin is not in directory or plugin file doesn't use slug.
* Use innerText instead of innerHTML for security hardening.
* Better encapsulate logic in \AMP_Validation_Error_Taxonomy::render_link_to_errors_by_url().
* Add missing @wordpress/dom-ready dependency to fix workaround 56915da described in #1397 (comment)
* Update button from "View errors by URL" to "View Invalid URLs".
@westonruter

@westonruter , @kienstra found that if you remove the domReady from the webpack config, remove it as a dependency of the script loading, and do npm run dev again we can get the script to run successfully. It is included here: 56915da but it is a workaround, so if you can come up with a fix or have another idea that'd be awesome.

I think the problem is just that @wordpress/dom-ready hasn't been saved as a dependency in package.json. If I add it as a dependency then it builds and works. Your change in 56915da is mostly right anyway as wp-dom-ready is not a registered script, but rather it is an internal dependency that is added to the built script. Fix applied in 716a869.

}
const heading = document.querySelector( '.wp-heading-inline' );
const link = document.createElement( 'a' );
link.innerHTML = errorIndexAnchor;

This comment has been minimized.

@westonruter

westonruter Sep 16, 2018

Member

innerText should be used instead of innerHTML because the latter is susceptible to an injection attack, if errorIndexAnchor is coming from an untrusted source.

link.setAttribute( 'href', errorIndexLink );
link.setAttribute( 'class', 'page-title-action' );
heading.after( link );
}

This comment has been minimized.

@westonruter

westonruter Sep 16, 2018

Member

Either this function or AMP_Validation_Error_Taxonomy::render_link_to_errors_by_url() should be refactored to be taking the same approach.

$plugin_names = array();
$plugin_slugs = array_unique( $sources['plugin'] );
foreach ( $plugin_slugs as $plugin_slug ) {
$plugin_data = get_plugin_data( WP_PLUGIN_DIR . '/' . $plugin_slug . '/' . $plugin_slug . '.php' );

This comment has been minimized.

@westonruter

westonruter Sep 16, 2018

Member

This won't work for plugins that don't have directories or which don't use the slug as the plugin's main PHP file.

image

$output[] = '<span class="dashicons dashicons-admin-appearance"></span>';
$themes = array_unique( $sources['theme'] );
foreach ( $themes as $theme_slug ) {
$output[] = sprintf( '<strong>%s</strong><br/>', esc_html( $theme_slug ) );

This comment has been minimized.

@westonruter

westonruter Sep 16, 2018

Member

Still needing to get wp_get_theme( $theme_slug )->get( 'Name' ) here.

@westonruter westonruter changed the title from [WIP] Improve URL Listing view to Improve URL Listing view Sep 16, 2018

@westonruter westonruter added this to the v1.0 milestone Sep 16, 2018

@westonruter westonruter merged commit ec4642d into develop Sep 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the 1362-improved_url_listing_view branch Sep 16, 2018

This was referenced Sep 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment