Match configId even when userId is found #15337
Conversation
fix #15320 fyi @mattab @diosmosis I wonder if maybe the query should be actually and `user_id is null or user_id = $userIdFromRequestIfGiven`?
tsteur
added
not-in-changelog
|
code looks good to me, and the user_id = ? addition makes sense to me after thinking about it. might be good to have a test (if current tests aren't already affected), otherwise looks good to me. |
core/Tracker/Model.php
Outdated
|
|
||
| if (empty($visitRow)) { | ||
| $configIdWhere .= ' AND user_id IS NULL '; | ||
| $configIdWhere .= ' AND ( user_id IS NULL '; |
There was a problem hiding this comment.
@diosmosis @mattab noticing this would create a new visit when userId was previously set, and then user logs out using resetUserId. I suppose this is not what we want as we also don't create a new visit when userId is set initially. I suppose we maybe really only always want to add AND ( user_id IS NULL OR user_id = ? ) when userId is not empty?
There was a problem hiding this comment.
Also when user is found by idVisitor, then we would also not create a new visit after resetUserId so we need to make sure to have consistent behavior no matter if cookies are enabled or disabled... I will later change the query I reckon to only compare userId when a userId is set
There was a problem hiding this comment.
If userId is null, we'd want to look for ( user_id IS NULL ) though, correct? Otherwise it would match the logged out user. Then again, ( user_id IS NULL OR user_id = ? ) would also match the user after they login (ie, logged out => login), so maybe both behaviors are desired?
There was a problem hiding this comment.
That's current desired/ implemented behaviour AFAIK and that's how it also works when it matches the visit by visitorId. See thread in matomo-org/developer-documentation#320
We just need to make sure to always have consistent behaviour on log in and log out. Either we always create a new visit on log in and log out, or never.
There was a problem hiding this comment.
I thought the point of separating user ID from visitor ID was so you could tell when a user logged in/out. Ie, it would count as the same visitor (along with same user different idvisitor being different visitors but one user), but count as different users? Maybe this isn't clear anymore w/ the userid change...
There was a problem hiding this comment.
I'm not sure? If that's the case, then I think this wasn't implemented.
There was a problem hiding this comment.
I think you're right it was never implemented. It does seem more useful to me though. I guess it could go in another issue.
|
@diosmosis I'll merge for now. We'll see what comes out of matomo-org/developer-documentation#320 re forcing new visits |
fix #15320
refs #15337
fyi @mattab @diosmosis
I wonder if maybe the query should be actually and
user_id is null or user_id = $userIdFromRequestIfGiven?Edit: Added the
or user_id = ?