Conversation
There was a problem hiding this comment.
I'm not sure if I'm misunderstanding the testing instructions, or what the problem this is trying to fix, but it doesn't seem to be testing well against the instructions in the PR:
-
Navigate to the Jetpack wp-admin with a fresh browser session and check your cookies.
-
You should see only one tk_ai cookie.
I am seeing two here, but they are the same value:
-
Delete your cookies and navigate to the Jetpack wp-admin
I'm getting the warning reported in my above comment -
You should have only one tk_ai cookie with the previous value
I am seeing two again, and they are different values than they were previously.
It seems like something is not being set correctly in the user meta table? Or it's getting deleted somehow?
_inc/lib/tracks/client.php
Outdated
| // User isn't linked at all. Fall back to anonymous ID. | ||
| $anon_id = get_user_meta( $user_id, 'jetpack_tracks_anon_id', true ); | ||
| if ( ! $anon_id ) { | ||
| if ( ! $anon_id || $anon_id !== $_COOKIE['tk_ai'] ) { |
There was a problem hiding this comment.
If this cookie is not set at all, it throws an undefined index warning. Let's add an isset() check here as well.
class.jetpack.php
Outdated
| add_action( 'jetpack_notices', array( $this, 'alert_auto_ssl_fail' ) ); | ||
| } | ||
|
|
||
| jetpack_tracks_get_identity( get_current_user_id() ); |
There was a problem hiding this comment.
This feels like a better home, so we're at least making sure that it's only being set if Jetpack is connected.
jetpack/class.jetpack-tracks.php
Line 15 in 8440afc
09438e3 to
93ba648
Compare
|
Huh, weird ... I'm seeing that behavior now too. |
|
@dereksmart care to give it another swing? I updated the description and steps to test. |
class.jetpack-tracks.php
Outdated
| return; | ||
| } | ||
|
|
||
| jetpack_tracks_get_identity( get_current_user_id() ); |
There was a problem hiding this comment.
Since this only is meant to fix the anonymous ID for unlinked users, I think it's a good idea to make sure it only runs for unlinked users. Something like this?
if ( ! Jetpack::is_user_connected( $user_id = get_current_user_id() ) ) {
jetpack_tracks_get_identity( $user_id );
}
It would prevent many useless calls to this function for already-linked users. This is how many I got on one admin page load:
[24-Aug-2017 19:56:52 UTC] connected user with meta stored
[24-Aug-2017 19:56:54 UTC] connected user with meta stored
[24-Aug-2017 19:56:55 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:56 UTC] connected user with meta stored
[24-Aug-2017 19:56:57 UTC] connected user with meta stored
[24-Aug-2017 19:56:57 UTC] connected user with meta stored
[24-Aug-2017 19:56:57 UTC] connected user with meta stored
|
@dereksmart updated to only check when it's not an ajax request and the user isn't connected. |
|
@withinboredom beautiful! |
|
|
||
| if ( ! headers_sent() ) { | ||
| setcookie( 'tk_ai', $anon_id ); | ||
| setcookie( 'tk_ai', $anon_id, time() + 1000, '/' ); |
There was a problem hiding this comment.
Won't this mean that the cookie expires about 15 minutes after it's set? This seems like it would cause some disruption. Maybe we could check if the value is correct or not and use that to update the cookie, instead of just making the old one expire.
There was a problem hiding this comment.
How would it cause a disruption?
| ); | ||
| } | ||
|
|
||
| if ( 0 === $user_id ) { |
There was a problem hiding this comment.
How is this change related to the task in question? It does seem like it could be a bug, of course... maybe we should write a test to cover this?
There was a problem hiding this comment.
For the same reason we excluded ajax requests (user id 0 for logged out user). We don't want to cookie logged out user.
| return; | ||
| } | ||
|
|
||
| if ( ! defined( 'DOING_AJAX' ) && ! Jetpack::is_user_connected( $user_id = get_current_user_id() ) ) { |
There was a problem hiding this comment.
Can you explain why this change is necessary?
|
Closing because this actually fixes the issue but exposed a worse bug. |
This overwrites the browser's
tk_ai(anonymous id) cookie with the one stored in user meta. This keeps the id synchronized across all browsers and devices.Changes proposed in this Pull Request:
tk_aito the correct parameters to be read from the tracks clientjetpack_tracks_get_identity( get_current_user_id() );asap in plugin initialization in the admin to set the cookie as appropriate, before the js lib can.Testing instructions:
tk_aicookie. If you see more, that's ok. Look for the one with the/path, that's your guy.tk_aicookies to simulate using a different/new device and navigate around wp-admin.tk_aicookie with the previous value.You can also get creative and set the cookie to a bogus value, it should reset back to the value as before.
Proposed changelog entry for your changes: