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

Fix possible discrepancy in author search with ignored authors #532

Merged
merged 2 commits into from
Jun 27, 2018
Merged

Fix possible discrepancy in author search with ignored authors #532

merged 2 commits into from
Jun 27, 2018

Conversation

TheCrowned
Copy link
Contributor

This fixes #273.

The issue was that, in some cases, it was possible to add the same author more than once. Even though it was already a co-author of the post, it would still show up in search and be available for another addition - here, the last suggestion is the same user who is already an author of the post:

before

tl;dr - the issue was due to JS referencing existing authors by user_nicename while PHP by user_login.

Getting a peak at what happened when a post is saved, it was clear that there was some mismatch between the newly-added coauthors and the already present ones. It looked like some case issue:

screenshot from 2018-06-05 09-20-20

I expected this to be an issue with search_authors() - in particular, something to do with $ignored_authors. Indeed, diving into the JS and PHP, I found out it was an issue with how ignored authors are handled by the two sides.

JS takes care of making the AJAX request to ajax_suggest(), which in turn calls search_authors(). It sends existing_authors as parameter, populating it with current authors user_nicenames.

function coauthors_create_author_hidden_input ( author ) {
	var input = jQuery( '<input />' )
		.attr({
			'type': 'hidden',
			'id': 'coauthors_hidden_input',
			'name': 'coauthors[]',
			'value': decodeURIComponent( author.nicename )
		});
	return input;
}

jQuery( document ).ajaxSend(function( e, xhr, settings ) {
	var existing_authors = jQuery( 'input[name="coauthors[]"]' ).map(function(){return jQuery( this ).val();}).get();

But on the other side, PHP was deciding whether an author should be excluded from search comparing user_logins with the AJAX-supplied list of user_nicenames. This would work fine in several situations - basically, all the times user_login did not have any special chars or uppercase letters. But when user_nicename was different from user_login, this was clearly an issue.

$found_users[ $found_user->user_login ] = $found_user;

foreach ( $found_users as $key => $found_user ) {
	if ( in_array( $found_user->user_login, $ignored_authors ) ) { 
		unset( $found_users[ $key ] );
	}
}

I thus changed user_login with user_nicename in the PHP bit. After this edit, the issue has gone! :)

after

This edit does not affect anything else, as far as I am been able to check. All PHPUnit tests still succeed as they did before. Also, search_authors() is only called by ajax_suggest(), so it should really only affect its behavior (and that's what my tests have confirmed) :)

@TheCrowned
Copy link
Contributor Author

Also updated tests to search_authors() to work with this edit.

Copy link
Contributor

@sboisvert sboisvert left a comment

Choose a reason for hiding this comment

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

lgtm. Great work on this, Love the explanations and the screenshot. It makes it really easy to follow along and know that this fix is a great one. Thanks!

@mdbitz mdbitz requested a review from philipjohn June 25, 2018 15:57
@mdbitz
Copy link
Contributor

mdbitz commented Jun 25, 2018

Pending @philipjohn for Merge or feedback.

@sboisvert
Copy link
Contributor

@rebeccahum want to give this a glance? If you think we're good with it let's merge it.

cc @philipjohn in case you have thoughts on it.

@rebeccahum rebeccahum merged commit 773a368 into Automattic:master Jun 27, 2018
@garretthyder
Copy link

Thanks for taking the time to address my issue guys. I appreciate the thorough recon and explanation provided by @TheCrowned

rebeccahum added a commit that referenced this pull request Mar 26, 2019
Fix possible discrepancy in author search with ignored authors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discrepancy between submitted coauthors and values returned through get_coauthors
5 participants