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

Add button into Site Health to reenable CSS transient caching #4522

Merged
merged 30 commits into from
Apr 10, 2020

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented Apr 3, 2020

Add CSS monitoring time series to Site Health debugging info

Summary

This PR adds a button to the Site Health AMP panel to reenable CSS transient caching when it was disabled.

Button:
Image 2020-04-04 at 9 27 00 AM

Button when the AJAX request failed:
Image 2020-04-04 at 9 26 14 AM

Button when the AJAX request succeeded:
Image 2020-04-04 at 9 29 01 AM

Fixes #4523

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@schlessera
Copy link
Collaborator Author

schlessera commented Apr 3, 2020

The colors still need to be adapted to fit the WordPress color palette.

However, there's currently a problem with the AMP_Options_Manager: Within the AJAX request, the update_option() call just fails and returns false.

@westonruter Are there any requirements to using the AMP_Options_Manager ? Do you have an idea of why this would fail within an AJAX request?

@pierlon
Copy link
Contributor

pierlon commented Apr 3, 2020

Do you have an idea of why this would fail within an AJAX request?

Perhaps the WP nonce is not being sent along with the request?

@schlessera
Copy link
Collaborator Author

The code does execute the call to the AMP_Options_Manager, I could verify that.

src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
@westonruter westonruter added this to the v1.5.3 milestone Apr 3, 2020
@schlessera
Copy link
Collaborator Author

Figured it out. The AMP_Options_Manager has an unexpected validation behavior. If an unknown option is passed through, it doesn't allow it to make a change, but also doesn't unset it either, so the option just remains as it was.

@schlessera
Copy link
Collaborator Author

Tests failed for unrelated timeout in E2E tests:
https://travis-ci.org/github/ampproject/amp-wp/jobs/671649112#L861-L871

 ● Featured Image Notice › should not display a notice, nor suggest cropping, when the image is the expected size
    TimeoutError: Text found "Crop Image"
      waiting for function failed: timeout 500ms exceeded
  ● Featured Image Notice › should display a notice when the image is too small, but not suggest cropping
    TimeoutError: Text found "Crop Image"
      waiting for function failed: timeout 500ms exceeded

src/Admin/AjaxAction.php Outdated Show resolved Hide resolved
@pierlon
Copy link
Contributor

pierlon commented Apr 7, 2020

Tests failed for unrelated timeout in E2E tests

@schlessera these tests are failing because of an error being thrown (see #4522 (comment)).

@pierlon
Copy link
Contributor

pierlon commented Apr 7, 2020

I think this button could be highlighted a little better, given how deep it's hidden in Site Health. I don't see how the user would know about it unless they've been told where it is.

@schlessera
Copy link
Collaborator Author

I don't see how the user would know about it unless they've been told where it is.

I assumed for now that this is mainly for the support team to be able to point them to some place to reset the cache instead of handing them a snippet filter or a custom filter to do so.

I don't think this should be a standard user operation.

But I'm happy to change this if my assumption is incorrect.

@westonruter @amedina Thoughts?

@schlessera schlessera requested a review from pierlon April 7, 2020 04:38
@schlessera
Copy link
Collaborator Author

As there's no code in place for replacing the nonce once it's been used, the button only works once per page load. However, there's no valid reason to push it multiple times, so I disabled it after the first use, and any further tries will need a reload.

Image 2020-04-09 at 5 56 34 PM

schlessera and others added 2 commits April 9, 2020 18:17
…aching-reenable-button

* 'develop' of github.com:ampproject/amp-wp:
  Restore unification of multi-page post content in Reader mode (#4547)
  Prevent styles from being removed when in Customizer preview with Standard mode (#4553)
  Omit Jetpack from being activated during PHPUnit test runs
  Use title case for Paired Browsing link in edit post screen (#4540)
  Ensure that validation query vars persist through redirects (#4544)
  Update dependency babel-jest to v25.2.6 (#4510)
  Update dependency css-loader to v3.5.0 (#4537)
  Update dependency autoprefixer to v9.7.6 (#4539)
  Add requirements to plugin file header (#4543)
  Force status code of validation responses to be 200 (#4533)
  Update optimizer test specs (#4527)
  Bump stable tag to 1.5.2
  Cache response status and headers when fetching external stylesheets (#4509)
  Fix securing multi-line mustache templates (#4521)
  Add CSS monitoring time series to Site Health debugging info (#4519)
  Update hostname used for WordPress TV embeds to fix external HTTP requests (#4524)
  Mock Imgur embed tests
  Mock Facebook embed tests
  Standardize file and class names for embed handlers
@westonruter
Copy link
Member

westonruter commented Apr 10, 2020

I fleshed out the guidance a bit more, and added link to learn more about object caching:

image

When enabled:

image

@westonruter
Copy link
Member

Confirmed error condition is shown properly:

image

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
@westonruter westonruter merged commit d1ea42f into develop Apr 10, 2020
@westonruter westonruter deleted the add-css-caching-reenable-button branch April 10, 2020 06:07
westonruter added a commit that referenced this pull request Apr 10, 2020
* Add button into Site Health to reenable CSS transient caching

* Use wp_jsonencode() instead of addslashes()

Co-Authored-By: Weston Ruter <westonruter@google.com>

* Remove unneeded PHPCS exception

* Use wp.ajax and heredoc for JS script

* Update styling

* Add capability check

* Use current_user_can()

* Add CSS transient caching option to options manager validation

* Fix code style issues

* Only read Option::DISABLE_CSS_TRANSIENT_CACHING if exists

* Remove <script> tags from JS code passed to wp_add_inline_script()

* Remove pesky 2nd blank line

Co-Authored-By: Weston Ruter <westonruter@google.com>

* Rename AjaxAction to ReenableCssTransientCachingAjaxAction

* Remove $access and default to authenticated users only

* Remove $scope and default to admin backend only

* Remove $action and hardcode as const

* Remove $callback and move method into AjaxAction

* Remove $selector and hardcode as const

* Only register AJAX action on site-health.php screen

* Use tabs instead of spaces to indent CSS

* Escape button label translations

* Add nonce verification

* Disable button after click

* Use DOMContentLoaded instead of a timeout

* Indent JS code with tabs instead of spaces

* Document $hook_suffix argument

* Flesh out guidance for CSS transient caching

* Avoid string interpolation for JS injection to better ensure late-escaping

* Use nowdoc instead of heredoc

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Co-authored-by: Weston Ruter <westonruter@google.com>
@kienstra
Copy link
Contributor

kienstra commented Apr 14, 2020

Looks good

Hi @amedina,
This worked as expected in my local.

After forcing the option to be true:

AMP_Options_Manager::update_option( 'amp_css_transient_monitor_disable_caching', true );

...the test appeared in Site Health:

looks-good-here

tests-passed

westonruter added a commit that referenced this pull request Apr 15, 2020
* tag '1.5.3':
  Bump 1.5.3
  Bump version to 1.5.3-RC1
  Fix handling of Mustache templates (#4583)
  Stub request based on test scenario (#4588)
  Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579)
  Restrict doing plugin upgrade routine when not in admin (#4538)
  Add new accessibility sanitizer (#4535)
  Fix unit tests (#4564)
  Add button into Site Health to reenable CSS transient caching (#4522)
  Restore unification of multi-page post content in Reader mode (#4547)
  Prevent styles from being removed when in Customizer preview with Standard mode (#4553)
  Omit Jetpack from being activated during PHPUnit test runs (#4474)
  Mock Facebook embed tests (#4474)
  Mock Imgur embed tests (#4474)
  Use title case for Paired Browsing link in edit post screen (#4540)
  Ensure that validation query vars persist through redirects (#4544)
  Add requirements to plugin file header (#4543)
  Force status code of validation responses to be 200 (#4533)
  Update optimizer test specs (#4527)
  Bump 1.5.3-alpha
@schlessera schlessera added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Caching cla: yes Signed the Google CLA CSS Integration: WP Core WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add button to reenable CSS transient cache
5 participants