Fix/missing wp user type

[eddiesshop]

Description

I came across this bug when attempting to do the following:

$author_data = [
    'wp_user_nicename', // WP User who also happens to be a Guest Author
    'guest_author_only_nicename'
];
$coauthors_plus->add_coauthors( $post_id, $author_data, $append = false );

I placed my WP_User first in the array because of this block of code. It will update the wp_posts.post_author column for the first WP_User it finds.

However, I noticed that the wp_posts.post_author column was not being updated as I was expecting.

I started digging through the code and found that this block of code was essentially being skipped, due to the fact that this function always returns a Guest Author object.

This is actually causing the add_coauthors function to always respond with false for instances where wp_posts.post_author = 0 or where the user no longer exists on the system. Even if you're passing in a valid WP_User who is also a Guest Author, it would respond with false, as was my case.

With this fix, we retain the fact that the original user_nicename we passed is also a WP_User. In this way, we don't waste compute cycles in that foreach loop where it would never find a valid WP_User it's trying to look for.

Deploy Notes

Are there any new dependencies added that should be taken into account when deploying to WordPress.org?
No

Steps to Test

Outline the steps to test and verify the PR here.

You will need 5 authors:

1. WP_User who is not linked to Guest Author ( user_nicename: wp_user_1 )
2. WP_User who is not linked to Guest Author ( user_nicename: wp_user_2 )
3. Guest Author who is not linked ( user_nicename: ga_1 )
4. Guest Author who is not linked ( user_nicename: ga_2 )
5. WP_User who is linked to Guest Author ( user_nicename: wp_user_and_ga_1 )

Example:

1. Check out PR.
2. Instantiate CoAuthor_Plus: $cap = new CoAuthor_plus();
3. Execute $cap->add_coauthors( $post_id, [ 'wp_user_1' ] ); result should be true. wp_posts.post_author should be equal to wp_users.ID
4. Execute $cap->add_coauthors( $post_id, [ 'ga_1', 'ga_2' ] ); result should be false. This is because $append = false and there was no wp_user passed as a CoAuthor. Notice also that wp_posts.post_author remains untouched.
5. Execute Step 1 again, then execute Step 4 again, this time passing $append = true, like so: $cap->add_coauthors( $post_id, [ 'ga_1', 'ga_2' ], true ); result should be true. Notice, that all 3 authors are correctly displayed.
6. Execute $cap->add_coauthors( $post_id, [ 'ga_1', 'wp_user_2' ], true ); result should be true. Notice that wp_posts.post_author is not overwritten. This is because we are appending authors to the post.
7. Execute $cap->add_coauthors( $post_id, [ 'ga_2', 'wp_user_and_ga_1' ] ); result should be true. Notice here that the WP_User is placed second in the array, but it is correctly picked up now, and the wp_posts.post_author column is correctly updated to match the user's wp_users.ID column.

[eddiesshop]

The previous 2 commits address another issue I found during some internal testing.

What was happening was that using get_coauthor_by and searching with a Guest Author, if that Guest Author has a valid link to a WP_User, it is summarily ignored. Functions like add_coauthors expect at least one coauthor to be a valid WP_User so that the wp_posts.post_author column can be appropriately updated. The only case where this function is returning an expected value is when you search by the WP_User first. When it arrives at $guest_author = $this->guest_authors->get_guest_author_by( $key, $value, $force );, $guest_author === false. It is then forced to move to the switch statement to find a user via their WP_User data.

With this refactor, when you arrive at $guest_author = $this->guest_authors->get_guest_author_by( $key, $value, $force ); and a valid $guest_author is obtained, it will now check if the linked_account attribute is set. If so, it will attempt to find the corresponding user for the Guest Account. Crucially, this change still gives priority to returning a Guest Author first and foremost. Only when a Guest Author is not found, will it search for a WP_User. If found, it will also search to see if a linked Guest Author account exists. If it does, it will return that Guest Author object instead, without losing the fact that this account also has a WP_User associated with it. If the plugin is disabled, then it will attempt to find a WP_User.

So using the examples outlined in the description, the below scenario should be tested as well:
8. Execute $cap->add_coauthors( $post_id, [ 'wp_user_2', 'ga_1', 'wp_user_and_ga_1' ] ); result should be true. Notice that wp_posts.post_author is equal to the ID for wp_user_2. Also, all 3 authors can be found when looking at the terms associated to $post_id.
9. Execute $cap->add_coauthors( $post_id, [ 'wp_user_and_ga_1', 'ga_1', 'wp_user_2' ] ); result should be same as above, except now wp_posts.post_author is linked to wp_user_and_ga_1.
10. Execute $cap->add_coauthors( $post_id, [ 'ga_1' ] ); result should be false. Since this is a pure GA account, wp_post.post_author is not updated, however we can see a term-taxonomy associated with this $post_id. Follow this up by executing $cap->add_coauthors( $post_id, [ 'wp_user_2', 'wp_user_and_ga_1' ], true ); the result should be true. Notice that wp_posts.post_author is equal to wp_user_2's user ID.

[GaryJones]

Thanks @eddiesshop. A couple of quick meta questions:

1. Should/Can the fix for the extra issue go into a new PR?
2. Since most of the steps are to execute code, could you please add suitable tests (happy and sad paths) for each fix?

[GaryJones]

Hi @eddiesshop - thank you for your patience as I reviewed it.

I appreciate the effort gone into creating all the tests, and seeing them pass really helps me to be confident that the expected behaviour for add_coauthors() would work as intended.

The beauty of tests lies in keeping things granular, and not making repeated assertions as they add to the noise and make it much harder to see what the real focus of a test is. Keeping the "Arrange, Act, Assert" structure in mind helps to ensure that the assertions are only happening after the key act (in this case, usually the call to add_coauthors()) has happened. Earlier tests may also assert before the act as well if we need to confirm the WP core default behaviour around the factory generations.

[GaryJones]

Suggested change

	$this->assertEquals( 1, $query->found_posts );
	$this->assertEquals( $this->author1->ID, $query->posts[0]->post_author );

[GaryJones]

It strongly feels like these assertions should go into a test specifically for get_coauthor_by() - so that this test method can just call the happy path of get_coauthor_by() and focus on what happens when the guest author is added with add_coauthors(). i.e. the whole test should be something like:

// Arrange:
// Create linked guest author from User 1.
// Create linked guest author from User 2.
// Create post with Editor 1 as the author.

// Act:
// Add both guest authors to the post.

// Assert:
// Assert the post author is User 1.

That should be it. All the other assertions should be handled (happy and sad paths) in other tests. The only thing this test should be doing is adding multiple ordered linked tests and see if the post_author field was updated.

[GaryJones]

Data provider please.

[GaryJones]

If the test is to check that a user_login field is correctly converted, and applied, then you only need two users at most, and it can be simplified down.

[GaryJones]

I greatly appreciate the extra documentation on this test method. But, other than behaviour around when a post starts off with post_author = 0, what else is being tested here that hasn't already been tested above?