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

Display when validation results are stale due to active theme/plugin changes #1375

Merged
merged 2 commits into from Aug 29, 2018

Conversation

@westonruter
Copy link
Member

@westonruter westonruter commented Aug 29, 2018

Given validation errors are caused by an active theme or plugin, once the offending theme or plugin is deactivated any validation results obtained while active are no longer fresh. If another theme or plugin has been activated, then different validation errors could be present. In both cases, the user needs to be informed that the validation results need to be refreshed by re-checking the URL.

This pull request adds the underlying building blocks needed for displaying staleness, however the UI should not be considered finalized:

image

image

@westonruter westonruter added this to the v1.0 milestone Aug 29, 2018
@westonruter westonruter requested a review from hellofromtonya Aug 29, 2018
@westonruter westonruter added this to In progress in v1.0 Aug 29, 2018
@westonruter westonruter moved this from In progress to Ready for review in v1.0 Aug 29, 2018
echo '<br>';
if ( ! empty( $staleness['theme'] ) && ! empty( $staleness['plugins'] ) ) {
esc_html_e( 'Different theme and plugins were active when these results were obtained. Please re-check.', 'amp' );
} elseif ( ! empty( $staleness['theme'] ) ) {
Copy link
Contributor

@hellofromtonya hellofromtonya Aug 29, 2018

@westonruter The 'theme' key may not exist in the returned $staleness array. We either need to initialize $staleness with this key back here on line 479 or check isset().

Copy link
Member Author

@westonruter westonruter Aug 29, 2018

Actually, empty() and isset() are both PHP language constructs which will not complain about undefined array indexes. So using isset( $staleness['theme'] ) && (bool) $staleness['theme'] is functionally equivalent to ! empty( $staleness['theme'] ).

The absence of the theme key is important for the return value to satisfy an empty condition, as array( 'theme' => array() ) is not empty for the overall check above in:

https://github.com/Automattic/amp-wp/blob/54b1272d05060d70d3ab2ace696fd9f7b3e540ed/includes/validation/class-amp-invalid-url-post-type.php#L1149-L1150

Copy link
Contributor

@hellofromtonya hellofromtonya Aug 29, 2018

@westonruter True. Valid point. Thank you.

esc_html_e( 'Different theme and plugins were active when these results were obtained. Please re-check.', 'amp' );
} elseif ( ! empty( $staleness['theme'] ) ) {
esc_html_e( 'A different theme was active when these results were obtained. Please re-check.', 'amp' );
} elseif ( ! empty( $staleness['plugin'] ) ) {
Copy link
Contributor

@hellofromtonya hellofromtonya Aug 29, 2018

@westonruter Shouldn't this key be 'plugins'? And this conditional has the same concern of isset() as above.

Copy link
Member Author

@westonruter westonruter Aug 29, 2018

Good catch. Fixed in 6b5cb8b.

@westonruter westonruter merged commit 2b51ab1 into develop Aug 29, 2018
2 checks passed
@westonruter westonruter deleted the add/staleness branch Aug 29, 2018
@westonruter westonruter moved this from Ready for review to Ready for QA in v1.0 Aug 29, 2018
@kienstra
Copy link
Contributor

@kienstra kienstra commented Sep 18, 2018

Request To Test

Hi @csossi,
Could you please test this?

  1. On the staging site, activate a plugin like 'AMP Invalid'
  2. Expected: The Invalid URLs page has 'Stale results' text:

invalid-urls

  1. Expected: On clicking a single URL as shown above, there should be a notice in the 'Status' box like:

stale-results-single

@csossi
Copy link

@csossi csossi commented Sep 18, 2018

Verified in QA

@csossi csossi moved this from Ready for QA to Ready for Merging in v1.0 Sep 18, 2018
@csossi csossi removed their assignment Sep 18, 2018
@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v1.0
In Production
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants