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 avatar collision on profile, post and edit screens #996

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

katag9k
Copy link
Contributor

@katag9k katag9k commented Sep 6, 2023

Description

Fixes filtering issues when a user has the same id as a guest-author post id on the post list screen, profile screen and the post edit takeover pop-up.

Related: #960 #973 #974

Steps to Test

Create a guest user that has the same post_id as another WP user_id.

In one browser, log in as the WordPress user and edit a post. In another browser login as the guest user. While logged in as a guest user, go to the post list at /wp-admin/edit.php the post will be locked as it's being edited by the WordPress user, however, the incorrect guest user's gravatar will be displayed.

Screenshot 2023-09-06 at 2 35 42 PM

Correct avatar should be:
Screenshot 2023-09-06 at 2 58 47 PM

Click edit on the post that is being edited, the warning popup will also have the incorrect gravatar from the guest user.
Screenshot 2023-09-06 at 2 37 52 PM

Correct avatar should be:

Screenshot 2023-09-06 at 1 22 37 PM

Take over the post editing. Go back to the browser where the Guest User was editing the post and wait for the "Somone else has taken over this post" pop-up. The incorrect avatar will be displayed:

Screenshot on 2023-09-15 at 14-10-16

The correct avatar should be:

Screenshot on 2023-09-15 at 14-05-54

Now log in as the WordPress user and go to the profile page at /wp-admin/profile.php the incorrect gravatar is displayed.

Screenshot 2023-09-06 at 2 40 37 PM

The correct avatar for the WordPress user:
Screenshot 2023-09-06 at 2 59 38 PM

@BrookeDot
Copy link

@katag9k, @GaryJones after some more testing it appears that this PR may be missing the condition when the post lock is removed via JS/Ajax. Adding the following appears to resolve it, but the two conditionals could likely be combined.

Thoughts?

// Do not filter the avatar if this is doing a heartbeat request on WP refresh lock
		if ( wp_doing_ajax() && isset( $_POST['action'] ) && $_POST['action'] === 'heartbeat' ) {
			return $args;
		}

@GaryJones
Copy link
Contributor

@katag9k The merge conflict will need addressing please, as well as considering Brooke's comment.

@katag9k
Copy link
Contributor Author

katag9k commented Sep 15, 2023

@BrookeDot Thank you, PR updated with the suggested code.

@GaryJones We are conflict free.

Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

With this PR, we've now got a whole lot more exceptions to the rule than whatever circumstance the rule was meant to be.

Can you work out where the RIGHT places are that should be filtered, and just target those?

}

// Do not filter the avatar if this is doing a heartbeat request on WP refresh lock
if ( wp_doing_ajax() && isset( $_POST['action'] ) && 'heartbeat' === $_POST['action'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a nonce that can be verified here for the heartbeat?
(PHPCS failure)

php/class-coauthors-plus.php Outdated Show resolved Hide resolved
php/class-coauthors-plus.php Outdated Show resolved Hide resolved
katag9k and others added 2 commits October 18, 2023 11:51
Co-authored-by: Gary Jones <gary.jones@automattic.com>
Co-authored-by: Gary Jones <gary.jones@automattic.com>
@GaryJones GaryJones modified the milestones: 3.6.1, 3.6.2 Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants