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

Avoid storing the full user object in notifications meta #20800

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
4377168
Avoid storing the full WP_User object in db.
pls78 Oct 25, 2023
56eb126
Add upgrade routine.
pls78 Oct 25, 2023
32f466f
Fix cs
pls78 Oct 25, 2023
33e9cf7
Fix cs
pls78 Oct 25, 2023
35ad0b5
Fix unit tests
pls78 Oct 25, 2023
671c37a
Fix more unit tests
pls78 Oct 26, 2023
40cf598
Fix another unit test
pls78 Oct 26, 2023
d7c2c2e
Fix integration tests
pls78 Oct 26, 2023
3d60c25
Make the 21.6 callback public
pls78 Oct 26, 2023
b9ffccc
Enforce the presence of user
pls78 Oct 26, 2023
a322c93
Use proper user factory and fix test
pls78 Oct 27, 2023
426c143
Fix test by setting the current user properly
pls78 Oct 27, 2023
494da26
Revert "Make the 21.6 callback public"
pls78 Oct 27, 2023
d7c62c3
Revert " Add upgrade routine."
pls78 Oct 27, 2023
b098118
Fix cs
pls78 Oct 27, 2023
ae6aec8
Update existing notifications
pls78 Oct 27, 2023
c73a8b3
Fix cs
pls78 Oct 27, 2023
af9d09e
Fix bug preventing notifications to be fetched
pls78 Oct 27, 2023
f06f1ee
Fix cs
pls78 Oct 27, 2023
2fad611
Better implementation of the original idea
pls78 Oct 30, 2023
0015edd
Update admin/class-yoast-notification.php
pls78 Oct 31, 2023
f3b7c7f
Update admin/class-yoast-notification.php
pls78 Oct 31, 2023
b0427ae
Deprecate get_user.
pls78 Oct 31, 2023
8a63a68
Use factory instead of custom method
pls78 Oct 31, 2023
1567063
Fix unit tests
pls78 Oct 31, 2023
2fa9853
Fix test
pls78 Oct 31, 2023
6220e18
Use create_and_get instead of create
pls78 Oct 31, 2023
7115eec
Fix cs
pls78 Oct 31, 2023
2bed3d5
Use create_and_get instead of create
pls78 Oct 31, 2023
0ecd600
Moves user to user_id logic
pls78 Oct 31, 2023
607d8ad
Revert "Moves user to user_id logic"
pls78 Oct 31, 2023
65f3241
Streamlined the logic some more
pls78 Oct 31, 2023
c3d7f1e
Fix cs
pls78 Oct 31, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion admin/class-yoast-notification-center.php
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,6 @@ private static function get_user_input( $key ) {
* @return void
*/
private function retrieve_notifications_from_storage( $user_id ) {

if ( $this->notifications_retrieved ) {
return;
}
Expand All @@ -732,6 +731,7 @@ private function retrieve_notifications_from_storage( $user_id ) {

if ( is_array( $stored_notifications ) ) {
$notifications = array_map( [ $this, 'array_to_notification' ], $stored_notifications );

// Apply array_values to ensure we get a 0-indexed array.
$notifications = array_values( array_filter( $notifications, [ $this, 'filter_notification_current_user' ] ) );

Expand Down Expand Up @@ -841,6 +841,13 @@ private function array_to_notification( $notification_data ) {
$notification_data['message'] = $notification_data['message']->present();
}

if ( isset( $notification_data['options']['user'] ) ) {
$notification_data['options']['user_id'] = $notification_data['options']['user']->ID;
unset( $notification_data['options']['user'] );

$this->notifications_need_storage = true;
}

return new Yoast_Notification(
$notification_data['message'],
$notification_data['options']
Expand Down
31 changes: 20 additions & 11 deletions admin/class-yoast-notification.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class Yoast_Notification {
private $defaults = [
'type' => self::UPDATED,
'id' => '',
'user' => null,
'user_id' => null,
'nonce' => null,
'priority' => 0.5,
'data_json' => [],
Expand Down Expand Up @@ -112,10 +112,14 @@ public function get_id() {
/**
* Retrieve the user to show the notification for.
*
* @deprecated 21.6
* @codeCoverageIgnore
*
* @return WP_User The user to show this notification for.
*/
public function get_user() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return $this->options['user'];
\_deprecated_function( __METHOD__, 'Yoast SEO 21.6' );
return null;
}

/**
Expand All @@ -126,10 +130,7 @@ public function get_user() {
* @return int The user id
*/
public function get_user_id() {
if ( $this->get_user() !== null ) {
return $this->get_user()->ID;
}
return get_current_user_id();
return ( $this->options['user_id'] ?? get_current_user_id() );
}

/**
Expand Down Expand Up @@ -220,7 +221,7 @@ public function display_for_current_user() {
*/
public function match_capabilities() {
// Super Admin can do anything.
if ( is_multisite() && is_super_admin( $this->options['user']->ID ) ) {
if ( is_multisite() && is_super_admin( $this->options['user_id'] ) ) {
return true;
}

Expand Down Expand Up @@ -280,7 +281,15 @@ public function match_capabilities() {
* @return bool
*/
private function has_capability( $capability ) {
$user = $this->options['user'];
$user_id = $this->options['user_id'];
if ( ! is_numeric( $user_id ) ) {
return false;
}
$user = get_user_by( 'id', $user_id );
if ( ! $user ) {
return false;
}

return $user->has_cap( $capability );
}

Expand Down Expand Up @@ -396,9 +405,9 @@ private function normalize_options( $options ) {
$options['capabilities'] = [ 'wpseo_manage_options' ];
}

// Set to the current user if not supplied.
if ( $options['user'] === null ) {
$options['user'] = wp_get_current_user();
// Set to the id of the current user if not supplied.
if ( $options['user_id'] === null ) {
$options['user_id'] = get_current_user_id();
}

return $options;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -800,21 +800,24 @@ public function test_add_notifications_for_multiple_users() {

$instance = $this->get_notification_center();

$user_mock_1 = $this->mock_wp_user( 1, [ 'wpseo_manage_options' => true ] );
$user_mock_2 = $this->mock_wp_user( 2, [ 'wpseo_manage_options' => true ] );
$user_1 = $this->factory->user->create_and_get();
$user_1->add_cap( 'wpseo_manage_options' );

$user_2 = $this->factory->user->create_and_get();
$user_2->add_cap( 'wpseo_manage_options' );

$notification_for_user_1 = new Yoast_Notification(
'Hello, user 1!',
[
'user' => $user_mock_1,
'user_id' => $user_1->ID,
'capabilities' => [ 'wpseo_manage_options' ],
]
);

$notification_for_user_2 = new Yoast_Notification(
'Hello, user 2!',
[
'user' => $user_mock_2,
'user_id' => $user_2->ID,
'capabilities' => [ 'wpseo_manage_options' ],
]
);
Expand All @@ -823,10 +826,10 @@ public function test_add_notifications_for_multiple_users() {
$instance->add_notification( $notification_for_user_2 );

$expected_for_user_1 = [ $notification_for_user_1 ];
$actual_for_user_1 = $instance->get_notifications_for_user( 1 );
$actual_for_user_1 = $instance->get_notifications_for_user( $user_1->ID );

$expected_for_user_2 = [ $notification_for_user_2 ];
$actual_for_user_2 = $instance->get_notifications_for_user( 2 );
$actual_for_user_2 = $instance->get_notifications_for_user( $user_2->ID );

$this->assertEquals( $expected_for_user_1, $actual_for_user_1 );
$this->assertEquals( $expected_for_user_2, $actual_for_user_2 );
Expand All @@ -841,13 +844,14 @@ 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 = $this->factory->user->create_and_get();
$user->add_cap( 'wpseo_manage_options' );

$notification = new Yoast_Notification(
'Hello, user 3!',
[
'id' => 'Yoast_Notification_Test',
'user' => $user_mock,
'user_id' => $user->ID,
'capabilities' => [ 'wpseo_manage_options' ],
]
);
Expand All @@ -856,7 +860,7 @@ public function test_add_notifications_only_once_for_user() {
$instance->add_notification( $notification );

$expected = [ $notification ];
$actual = $instance->get_notifications_for_user( 3 );
$actual = $instance->get_notifications_for_user( $user->ID );

$this->assertEquals( $expected, $actual );
}
Expand Down Expand Up @@ -917,35 +921,4 @@ private function get_notification_center() {

return $notification_center;
}

/**
* Creates a mock WordPress user.
*
* @param int $user_id The ID of the user.
* @param array $caps A map, mapping capabilities to `true` (user has capability) or `false` ( user has not).
*
* @return PHPUnit_Framework_MockObject_Invocation_Object | WP_User
*/
private function mock_wp_user( $user_id, $caps ) {
$user_mock = $this
->getMockBuilder( 'WP_User' )
->setMethods( [ 'has_cap' ] )
->getMock();

$user_mock
->expects( $this->any() )
->method( 'has_cap' )
->with( $this->isType( 'string' ) )
->willReturn(
$this->returnCallback(
static function( $argument ) use ( $caps ) {
return isset( $caps[ $argument ] ) ? $caps[ $argument ] : false;
}
)
);

$user_mock->ID = $user_id;

return $user_mock;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,18 @@ class Yoast_Notification_Test extends WPSEO_UnitTestCase {
*/
private $verify_capability_match_filter_args = [];

/**
* Create user with proper caps.
*/
public function set_up() {
parent::set_up();

$user = $this->factory->user->create_and_get();
$user->add_cap( 'wpseo_manage_options' );

wp_set_current_user( $user->ID );
}

/**
* No ID is not persistent.
*/
Expand All @@ -40,12 +52,13 @@ public function test_not_persistent() {
public function test_set_defaults() {
$subject = new Yoast_Notification( 'message', [] );
$test = $subject->to_array();
$user = wp_get_current_user();

$this->assertEquals(
[
'type' => 'updated',
'id' => '',
'user' => wp_get_current_user(),
'user_id' => $user->ID,
'nonce' => null,
'priority' => 0.5,
'data_json' => [],
Expand Down Expand Up @@ -168,6 +181,7 @@ public function test_match_any_pass() {
]
);


$this->assertTrue( $subject->display_for_current_user() );

$this->remove_cap( 'bla' );
Expand Down
9 changes: 6 additions & 3 deletions tests/unit/admin/admin-features-test.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,12 @@ private function get_admin_with_expectations() {
->with( 'https://example.org', 'dismiss-5star-upsell' )
->andReturn( 'https://example.org?_wpnonce=test-nonce' );

Monkey\Functions\expect( 'wp_get_current_user' )
->once()
->andReturn( Mockery::mock( WP_User::class ) );
$admin_user = Mockery::mock( WP_User::class );
$admin_user->ID = 1;

Monkey\Functions\expect( 'get_current_user_id' )
->once()
->andReturn( $admin_user->ID );

return new WPSEO_Admin();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@
*/
class WPSEO_Admin_Gutenberg_Compatibility_Notification_Test extends TestCase {

/**
* Holds the admin user mock instance.
*
* @var WP_User
*/
private $admin_user;

/**
* Holds the WPSEO_Gutenberg_Compatibility mock instance.
*
Expand Down Expand Up @@ -49,6 +56,11 @@ class WPSEO_Admin_Gutenberg_Compatibility_Notification_Test extends TestCase {
public function set_up() {
parent::set_up();

$this->stubTranslationFunctions();

$this->admin_user = Mockery::mock( WP_User::class );
$this->admin_user->ID = 1;

$this->gutenberg_compatibility_mock = Mockery::mock( WPSEO_Gutenberg_Compatibility::class )->makePartial();
$this->notification_center_mock = Mockery::mock( Yoast_Notification_Center::class );

Expand Down Expand Up @@ -120,22 +132,17 @@ public function data_provider_manage_notification_remove_notification() {
* @covers WPSEO_Gutenberg_Compatibility::get_installed_version
*/
public function test_manage_notification_gutenberg_show_notification() {
Monkey\Functions\stubs(
[
'__' => null,
'wp_get_current_user' => static function() {
return null;
},
]
);

$this->gutenberg_compatibility_mock->allows(
[
'is_installed' => true,
'is_fully_compatible' => false,
]
);

Monkey\Functions\expect( 'get_current_user_id' )
->once()
->andReturn( $this->admin_user->ID );

$this->notification_center_mock->expects( 'add_notification' )->once()->withArgs(
static function ( $arg ) {
// Verify that the added notification is a Yoast_Notification object and has the correct id.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@
*/
class Content_Type_Visibility_Watcher_Actions_Test extends TestCase {

/**
* Holds the admin user mock instance.
*
* @var WP_User
*/
private $admin_user;

/**
* Holds the Options_Helper instance.
*
Expand Down Expand Up @@ -57,6 +64,9 @@ protected function set_up() {

$this->stubTranslationFunctions();

$this->admin_user = Mockery::mock( WP_User::class );
$this->admin_user->ID = 1;

$this->options = Mockery::mock( Options_Helper::class );
$this->notification_center = Mockery::mock( Yoast_Notification_Center::class );
$this->content_type_dismiss_notifications = Mockery::mock( Content_Type_Visibility_Dismiss_Notifications::class );
Expand Down Expand Up @@ -310,10 +320,13 @@ public function test_maybe_add_notification() {
[
'esc_url' => 'https://yoa.st/3.0-content-types',
'admin_url' => 'admin.php?page=wpseo_page_settings',
'wp_get_current_user' => Mockery::mock( WP_User::class ),
]
);

Monkey\Functions\expect( 'get_current_user_id' )
->once()
->andReturn( $this->admin_user->ID );

$this->notification_center
->expects( 'add_notification' )
->once();
Expand Down
8 changes: 6 additions & 2 deletions tests/unit/inc/addon-manager-test.php
Original file line number Diff line number Diff line change
Expand Up @@ -1157,8 +1157,12 @@ public function test_create_notification( $product_name, $short_link ) {
]
);

Monkey\Functions\expect( 'wp_get_current_user' )
->twice();
$admin_user = Mockery::mock( WP_User::class );
$admin_user->ID = 1;

Monkey\Functions\expect( 'get_current_user_id' )
->twice()
->andReturn( $admin_user->ID );

Monkey\Functions\expect( 'YoastSEO' )
->once()
Expand Down
Loading