Avoid storing the full user object in notifications meta #20800

pls78 · 2023-10-25T14:45:43Z

Context

We want to avoid storing the full WP_User object in each user's notification.

Summary

This PR can be summarized in the following changelog entry:

Do not store in the wp_yoast_notifications usermeta the full WP_User object.

Relevant technical choices:

Notifications already saved in the database are managed on a user-by-user basis by the Notification_Center::maybe_unset_notification_user_field method. This is going to be less impactful for sites with a large number of users with respect to using an upgrade routine.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Without this PR checked out

Go to Yoast SEO -> General -> Dashboard tab
- take note of the notifications (Problems and Notifications section) you have there and their state (visible or hidden)
- if you have no notifications, use the Yoast Test helper to Reset indexable tables&migrations: this would trigger a notification about the need to run the SEO data optimization
Go in your database, wp_usermeta table, and look for the row where User Id = your user id, meta_key = wp_yoast_notifications
- search the Meta value for WP_User and verify you have one instance per notification
Create another user with SEO Manager role and repeat the points above for this user

With this PR checked out

Login with one of the two users
Go to Yoast SEO -> General -> Dashboard tab
- verify you still see your previous notifications with the correct status
Go in your database, wp_usermeta table, and look for the row where User Id = your user id, meta_key = wp_yoast_notifications
- search the Meta value for WP_User and verify you have no instances in any of the notifications
- now look for the row where User Id = your other user id, meta_key = wp_yoast_notifications
- search the Meta value for WP_User and verify you still have one instance per notification

Relevant test scenarios

Changes should be tested with the browser console open
Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
Changes should be tested on different editors (Block/Classic/Elementor/other)
Changes should be tested on different browsers
Changes should be tested on multisite

Test instructions for QA when the code is in the RC

QA should use the same steps as above.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

This PR changes one of the base mechanisms used by the Notification center to associate each notification to its user: please perform a regression test for the Notification center.

UI changes

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

Other environments

This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify 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.
During testing, I had activated all plugins that Yoast SEO provides integrations for.
I have added unit tests to verify the code works as intended.
If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
I have written this PR in accordance with my team's definition of done.
I have checked that the base branch is correctly set.

Innovation

No innovation project is applicable for this PR.
This PR falls under an innovation project. I have attached the innovation label.
I have added my hours to the WBSO document.

Fixes #20796

* Store only the user ID in the db. * Fix unit tests to take into account accessing the ID attribute of the WP_User object.

The routine will unset the 'user' field in yoast_notifications user meta and add a new 'user_id' field with the user ID.

This reverts commit 3d60c25.

This reverts commit 56eb126.

igorschoester

CR 🏗️

igorschoester · 2023-10-30T14:00:35Z

admin/class-yoast-notification-center.php

+			foreach ( $notifications as $notification ) {
+				$notification_has_been_updated = $notification->user_to_user_id();
+				// Just one notification changed is enough to update the storage.
+				if ( $notification_has_been_updated ) {
+					$this->notifications_need_storage = true;
+				}
+			}
+


I detected an order problem. In filter_notification_current_user the user_id is already expected to be there.
But here you convert it only after the filter.

I was thinking of a solution:

Move the whole loop above the filter

Move the convert code into the array_to_notification method

But then I thought: why not check in the constructor of the notification? Specifically the normalize_options already has some logic that looks like a spot at first sight?

I think I reached a good middle point:

Moved the convert code in the array_to_notification method

No need to loop anymore becuse of the array_map calling array_to notification

No need to add the ugly public method in Yoast_Notification 🙂

By doing so we can still set the notifications_need_storage flag and save to db the new user_id asap

igorschoester · 2023-10-30T14:07:14Z

admin/class-yoast-notification.php

 	 *
-	 * @return WP_User The user to show this notification for.
+	 * @return int The user id.
 	 */
 	public function get_user() {


Changing the return value and type is a BC break. I think this is actually just a helper for the method below.

Probably we can just deprecate it?
But perhaps it is wise to not alter it and retrieve the user after getting the ID instead (similar to your has_capability change)? For BC sake.

I deprecated it, it wouldn't make sense anymore after this PR gets merged 🙂

admin/class-yoast-notification.php

igorschoester · 2023-10-30T14:31:32Z

tests/integration/notifications/test-class-yoast-notification-center.php

@@ -841,13 +841,15 @@ public function test_add_notifications_only_once_for_user() {

 		$instance = $this->get_notification_center();

-		$user_mock = $this->mock_wp_user( 3, [ 'wpseo_manage_options' => true ] );
+		$user_id = $this->factory->user->create();


I think there is a create_and_get you can use. If you chose to apply, please also do so in the other tests 😉

Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>

Instead of introducing a breaking change by changing the return type of get_user, we deprecate it as it was only used here.

This reverts commit 0ecd600.

thijsoo

CR + ACC 👍 Looks good after the changes :)

pls78 added 2 commits October 25, 2023 16:43

Avoid storing the full WP_User object in db.

4377168

* Store only the user ID in the db. * Fix unit tests to take into account accessing the ID attribute of the WP_User object.

Add upgrade routine.

56eb126

The routine will unset the 'user' field in yoast_notifications user meta and add a new 'user_id' field with the user ID.

pls78 added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Oct 25, 2023

pls78 added 2 commits October 25, 2023 16:47

Fix cs

32f466f

Fix cs

33e9cf7

pls78 linked an issue Oct 25, 2023 that may be closed by this pull request

Refactor Yoast notifications so it does not store the full WP_User #20796

Closed

pls78 added 16 commits October 25, 2023 17:57

Fix unit tests

35ad0b5

Fix more unit tests

671c37a

Fix another unit test

40cf598

Fix integration tests

d7c2c2e

Make the 21.6 callback public

3d60c25

Enforce the presence of user

b9ffccc

Use proper user factory and fix test

a322c93

Fix test by setting the current user properly

426c143

Revert "Make the 21.6 callback public"

494da26

This reverts commit 3d60c25.

Revert " Add upgrade routine."

d7c62c3

This reverts commit 56eb126.

Fix cs

b098118

Update existing notifications

ae6aec8

Fix cs

c73a8b3

Fix bug preventing notifications to be fetched

af9d09e

Fix cs

f06f1ee

Better implementation of the original idea

2fad611

pls78 marked this pull request as ready for review October 30, 2023 14:33

igorschoester requested changes Oct 30, 2023

View reviewed changes

pls78 and others added 6 commits October 31, 2023 09:37

Update admin/class-yoast-notification.php

0015edd

Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>

Update admin/class-yoast-notification.php

f3b7c7f

Co-authored-by: Igor <35524806+igorschoester@users.noreply.github.com>

Deprecate get_user.

b0427ae

Instead of introducing a breaking change by changing the return type of get_user, we deprecate it as it was only used here.

Use factory instead of custom method

8a63a68

Fix unit tests

1567063

Fix test

2fa9853

pls78 added 7 commits October 31, 2023 12:44

Use create_and_get instead of create

6220e18

Fix cs

7115eec

Use create_and_get instead of create

2bed3d5

Moves user to user_id logic

0ecd600

Revert "Moves user to user_id logic"

607d8ad

This reverts commit 0ecd600.

Streamlined the logic some more

65f3241

Fix cs

c3d7f1e

pls78 requested a review from igorschoester October 31, 2023 14:50

thijsoo approved these changes Nov 7, 2023

View reviewed changes

thijsoo added this to the 21.7 milestone Nov 7, 2023

thijsoo merged commit 9db8b9f into trunk Nov 7, 2023
21 checks passed

thijsoo deleted the avoid-storing-the-full-user-object-in-notifications-meta branch November 7, 2023 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid storing the full user object in notifications meta #20800

Avoid storing the full user object in notifications meta #20800

pls78 commented Oct 25, 2023 •

edited

Loading

igorschoester left a comment

igorschoester Oct 30, 2023 •

edited

Loading

pls78 Oct 31, 2023

igorschoester Oct 30, 2023

pls78 Oct 31, 2023

igorschoester Oct 30, 2023

pls78 Oct 31, 2023

thijsoo left a comment

Avoid storing the full user object in notifications meta #20800

Avoid storing the full user object in notifications meta #20800

Conversation

pls78 commented Oct 25, 2023 • edited Loading

Context

Summary

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

Without this PR checked out

With this PR checked out

Relevant test scenarios

Test instructions for QA when the code is in the RC

Impact check

UI changes

Other environments

Documentation

Quality assurance

Innovation

igorschoester left a comment

Choose a reason for hiding this comment

igorschoester Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

pls78 Oct 31, 2023

Choose a reason for hiding this comment

igorschoester Oct 30, 2023

Choose a reason for hiding this comment

pls78 Oct 31, 2023

Choose a reason for hiding this comment

igorschoester Oct 30, 2023

Choose a reason for hiding this comment

pls78 Oct 31, 2023

Choose a reason for hiding this comment

thijsoo left a comment

Choose a reason for hiding this comment

pls78 commented Oct 25, 2023 •

edited

Loading

igorschoester Oct 30, 2023 •

edited

Loading