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

Move wpseo-links-table-not-accessible notification to site health #14828

Merged

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Apr 15, 2020

Context

  • We want to move notifications to the WordPress Site Health

Summary

This PR can be summarized in the following changelog entry:

  • Moves the text link counter notification from the SEO Dashboard to the WordPress' Site Health.

Relevant technical choices:

  • added an upgrade routine for 14.1-RC0

Test instructions

This PR can be tested by following these steps:

Make the old notification appear in the Notification Center

  • make sure the text link counter feature is enabled under SEO > General > Features
  • add return false; at the first line of the check_table() method within admin/links/class-link-table-accessible.php
  • alternatively, you can delete the wp_yoast_seo_links table from the database
  • go to SEO dashboard and see the old notification displayed:

Screenshot 2020-04-20 at 11 35 45

Switch to this branch and build

Test not passed

  • go to the WordPress Site Health page
  • see the test result is within the "recommended improvements" section:

Screenshot 2020-04-15 at 14 13 21

  • you may want to repeat the test by doing the same thing for the wp_yoast_seo_meta table and admin/class-meta-table-accessible.php but I'd consider this a redundant check

The copy is:

The text link counter feature is not working as expected
For this feature to work, Yoast SEO needs to create a table in your database. We were unable to create this table automatically.
[link]Find out how to solve this problem on our knowledge base.[/link]

  • trigger the upgrade routine by using the Yoast Test Helper plugin
  • from the root of your Docker environment, delete the related WordPress transient by running ./wp.sh transient delete wpseo_link_table_inaccessible
  • please notice you will need to delete the transient each time you change whether the database tables are accessible or not
  • verify the old notification on SEO > General > Notifications, doesn't appear any longer

Test passed

  • remote the return false added at the first line of the check_table() method
  • if you deleted the database table, restore it by adding this code somewhere in a place where it is executed:
$link_installer = new WPSEO_Link_Installer();
$link_installer->install();
  • remove the above code
  • go to the WordPress Site Health page
  • expand the Passed tests section
  • see the test result within the section

Screenshot 2020-04-15 at 14 14 40

The copy is:

The text link counter is working as expected
The text link counter helps you improve your site structure. [link]Find out how the text link counter can enhance your SEO[/link].

Test with the feature disabled

  • go to SEO > General > Features
  • disable the text link counter feature
  • go to the Site Health page
  • see the test didn't run

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Fixes #14494

@afercia afercia added the changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog label Apr 15, 2020
@afercia
Copy link
Contributor Author

afercia commented Apr 15, 2020

Other teams need to be notified their feedback is needed for this PR to move on:

Todo before merging:

  • check the two shortlinks mentioned above
  • check the copy of the Site Health test results

Todo when merging:

@Annelieke
Copy link

Hi @afercia :)

If I understand correctly, these notifications are moved to WordPress Site Health?

I think it's best to add these two new shortlinks:

  1. https://yoa.st/3zw
  2. https://yoa.st/3zv

@karlijnbok karlijnbok changed the title 14494 move wpseo links table not accessible to site health Move wpseo-links-table-not-accessible notification to site health Apr 17, 2020
@afercia
Copy link
Contributor Author

afercia commented Apr 17, 2020

Thanks @Annelieke 👋

@afercia
Copy link
Contributor Author

afercia commented Apr 17, 2020

Updated the shortlinks.

Question: what should happen when users disable the text link counter feature via the filter we provide? For example:

add_filter( 'wpseo_should_index_links', '__return_false' );

I guess in this case the health check should not run?
Dumb moment: this case is already addressed. Will create a new issue for the UI part (see below).

Also, I noticed that even though the feature is disabled via the filter, the UI still shows stuff related to the links e.g.:

  • the two links columns in the Posts overview
  • the Text link counter button or status on the SEO > Tools page:

Screenshot 2020-04-17 at 12 03 29

Screenshot 2020-04-17 at 12 03 56

@karlijnbok
Copy link
Contributor

Feedback @JessieHenkes regarding the copy is that it looks good, but maybe change "not working as expected" to "not working properly" or "not working correctly".

@karlijnbok
Copy link
Contributor

CR: 👍 Acceptance: 🚧 The notification does not seem to be removed when the upgrade routine is triggered. Have not been able to figure out why.

@afercia
Copy link
Contributor Author

afercia commented Apr 20, 2020

Turns out the notification removal doesn't work because of the boostrap order.

To recap:

  • the upgrade routines run on wpseo_init(), very early on the plugins_loaded hook with priority 14
  • see add_action( 'plugins_loaded', 'wpseo_init', 14 ); in wp-seo-main.php
  • the Yoast_Notification_Center class is initialized on the plugins_loaded hook with normal priority 10 so it is initialized before the upgrade routines
  • however, the actual notifications are retrieved later on the init hook in the Yoast_Notification_Center class, see add_action( 'init', [ $this, 'setup_current_notifications' ], 1 );

To solve this issue, we should either run the upgrade routines later or retrieve the notifications from storage earlier.

The following diff is a proof of concept for the latter option.

diff --git a/admin/class-yoast-notification-center.php b/admin/class-yoast-notification-center.php
index 99eb58ca1..7c51c4e1e 100644
--- a/admin/class-yoast-notification-center.php
+++ b/admin/class-yoast-notification-center.php
@@ -64,7 +64,7 @@ class Yoast_Notification_Center {
 	 */
 	private function __construct() {
 
-		add_action( 'init', [ $this, 'setup_current_notifications' ], 1 );
+		add_action( 'plugins_loaded', [ $this, 'setup_current_notifications' ], 13 );
 
 		add_action( 'all_admin_notices', [ $this, 'display_notifications' ] );
 

Important:
Looking at the history: 5144145 I guess we need to make sure also the current user is initialized.

This problem affects also #14496 / #14840.

@afercia
Copy link
Contributor Author

afercia commented Apr 20, 2020

Updated the test instructions. Removed the parts related to the links used in the copy, as they have been updated.

@afercia
Copy link
Contributor Author

afercia commented Apr 21, 2020

Please re-test the removal of the old notification using the Yoast Test Helper plugin. It should work now.

@karlijnbok karlijnbok added this to the 14.1 milestone Apr 21, 2020
@karlijnbok
Copy link
Contributor

CR & Acceptance: 👍

@karlijnbok karlijnbok merged commit 3643a4c into trunk Apr 21, 2020
@karlijnbok karlijnbok deleted the 14494-move-wpseo-links-table-not-accessible-to-site-health branch April 21, 2020 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move 'wpseo-links-table-not-accessible' to Site Health
3 participants