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

Generate an error taxonomy for AMP error types #1360

Closed
2 of 4 tasks
postphotos opened this issue Aug 27, 2018 · 12 comments
Closed
2 of 4 tasks

Generate an error taxonomy for AMP error types #1360

postphotos opened this issue Aug 27, 2018 · 12 comments
Assignees
Labels
Milestone

Comments

@postphotos
Copy link
Contributor

postphotos commented Aug 27, 2018

As a user/developer, I should have a list of all the possible errors the AMP for WordPress plugin may generate and I should have them organized in a logical taxonomy (by CSS, by HTML attribute, by HTML element, or by Javascript). As a developer using the plugin, I should be able to use these taxonomies to sort and filter in various given views.

  • AC1: Determine all current errors the sanitizer outputs.
  • AC2: Organize this based on the logical taxonomy (by CSS, by HTML attribute, by HTML element, or by Javascript) discussed.
  • AC3: Share this with the documentation for the new product site.
  • AC4: Prepare this for the other views (index by error and single) where this will be used.
@kienstra
Copy link
Contributor

kienstra commented Aug 27, 2018

Question About Taxonomy

Hi @postphotos,
Thanks for opening this, and the clear ACs.

I should be able to use these taxonomies to sort and filter in various given views.

It sounds like this means a taxonomy in the WordPress sense.

Is there flexibility in implementing this? The amp_validation_error is a taxonomy itself.

@kienstra
Copy link
Contributor

kienstra commented Aug 27, 2018

Update: it looks like the answer to this question is "yes," based on this design.

Hi @postphotos,
Building on the question above, would there be a way to somehow filter by error type on the "AMP Validation Errors" page?

validation-error-page

@postphotos
Copy link
Contributor Author

Hi @kienstra, that's the idea here. This ticket explores and implements the data model to allow the other views to have the features we think they need to help a user work with errors.

As a user, I should be able to get to just the errors of a given type (e.g. CSS or Javascript) to be able to see or action on just that subset of errors on both the URL and Error type screens.

On the single page, the new view also groups these errors by this typology, also linking to given documentation about how to best resolve it. For example, some markup attributes and CSS properties are just fine to be omitted in AMP, but I'd be wary of blocking some Javascript tags. In short, we want to use this data elsewhere in the tool and first need to make sure we can parse through (or collect it) as the sanitizer and plugin works.

@postphotos
Copy link
Contributor Author

postphotos commented Aug 27, 2018

@kienstra The current default post faceting (All | New | Rejected | Accepted) would be replaced with the new interface's dropdown filters.

Notes about the current version of the sketch:

  • Bulk Action does not appear there, but we intend to keep that feature.
  • At this point, we intend to have two dropdowns (Error Types and Error Status) and have backlogged template (Template Filtering Discovery #1367) and source (Source Filtering Discovery #1368). Search, of course, would still work in filtering the layout views as it does now.

cc: @jwold @jillchu

@kienstra
Copy link
Contributor

kienstra commented Aug 28, 2018

Non-CSS Error Codes (that I could find so far)

'invalid_element'
(This code also looks to apply when enqueuing scripts or invalid stylesheets. In those cases, it would have a node_name of script or link, respectively.)
'invalid_attribute'

CSS Parser Error Codes
'css_parse_error'
'excessive_css'
'illegal_css_at_rule'
'illegal_css_important'
'illegal_css_property'
'removed_unused_css_rules'
'unrecognized_css'
'disallowed_file_extension'
'file_path_not_found'

The error codes, like invalid_attribute, are stored in the term description. That description is a JSON blob, like:

code-invalid-attribute

On the 'AMP Validation Errors' page, invalid_attribute and invalid_element codes also display the node_name after:

invalid-attribute

That node_name also looks to be in the JSON blob in the term description.

@westonruter
Copy link
Member

westonruter commented Aug 28, 2018

In regards to data model for a taxonomy of taxonomies, see #1361 (comment) for a discussion of a possible solution.

Nevertheless, it seems that doing a full-text search of the term description field (a JSON blob) is going to be the most practical solution here. This would mean that the error type (category) should be stored inside the JSON blob itself with the error code, and that to filter by this field it would require doing a query like the following, assuming there is a "css_error" type that is common category for all CSS-related validation errors:

WHERE description LIKE '%"type":"css_error"%';

This would be in addition to the term status condition achieved via term_group.

This isn't ideal by any means, but I think it will be OK given that the type queries would only be done in the admin for these screens.

@westonruter
Copy link
Member

An important consequence of this implementation is this: we would not be able to readily limit the dropdown of type options to just those which actually exist in the database. Nor would we readily be able to provide a count of the given error types that exist with each option.

@kienstra
Copy link
Contributor

kienstra commented Aug 28, 2018

@westonruter, thanks a lot for your detailed suggestions for the data model.

@postphotos postphotos changed the title Generate a plugin taxonomy for error types Generate a error taxonomy for AMP error types Sep 4, 2018
@postphotos
Copy link
Contributor Author

Updated title to more clearly explain the intent of the ticket, props @amedina 👍

@kienstra
Copy link
Contributor

kienstra commented Sep 6, 2018

Change 'Trash' Text

On the 'Errors by URL' page, please change the 'Trash' text:

update-1360

@kienstra
Copy link
Contributor

kienstra commented Sep 6, 2018

Request To Change Text

Hi @johnwatkins0,
Could you please change the text here on the 'Invalid AMP Pages (URLs)' page? The screenshot below has the same changes as the one above. But it's taken from my local environment, using the develop branch:

change-text-develop-branch

@kienstra
Copy link
Contributor

Moving To 'Read For Merging'

If it's alright, I'm moving this to 'Ready For Merging,' as this will be tested in #1361 and #1362.

@amedina amedina changed the title Generate a error taxonomy for AMP error types Generate an error taxonomy for AMP error types Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants