UserID no longer overwrites VisitorId #14360
Conversation
|
Looks good to me and makes sense. Not sure if anything else needs updated @mattab . For sure we should also change the behaviour in https://github.com/matomo-org/matomo-php-tracker/blob/1.4.1/PiwikTracker.php#L1241-L1245 |
|
Looks good to me also, thank you for the PR @MichaelHeerklotz 👍 Feedback:
|
|
Thank you.
|
|
HI @MichaelHeerklotz - thanks again for the PR. Would you be able to update the php matomo tracker in https://github.com/matomo-org/matomo-php-tracker/blob/1.4.1/PiwikTracker.php#L1241-L1245 to include the tracker changes in this PR so we can see the overall build results (our tests use the php matomo tracker to track the test data)? |
|
Hello @mattab , I have created PRs for matomo-php-tracker and piwik-dotnet-tracker. I also checked all the other trackers (matomo-sdk-ios, matomo-sdk-android, piwik-java-tracker, etc) and they do not seem to need any changes. |
|
looking forward to this improvement. Currently I "abuse" custom dimension to record the user ID, to keep tracking the visit under same Visitor ID and then will have to access raw SQL data to get what I need. |
|
@MichaelHeerklotz I was just going to give it a test but seeing there's a merge conflict. Can you have a look? I will then see if tests pass and give it also an actual test and look at all the related PRs |
MichaelRoosz
force-pushed
the
userid_update
branch
from
0f39653
to
20a9a6e
Compare
|
@tsteur It should be good to merge now |
|
@MichaelHeerklotz a few tests are now failing see eg https://travis-ci.org/matomo-org/matomo/jobs/593032816#L823-L883. Some unrelated to your change. We're working on getting the We can wait until the 3.x-dev branch passes again or if you want you can already upload some expected files now. We can otherwise also fix the tests after merging. The change itself looks good though 👍 |
|
@MichaelHeerklotz let me know what you want to do here. We're planning to release 3.12 soonish. |
|
@tsteur I made a couple changes to a test/fixture to get them to pass (the UserIdAndVisitorIdTest one). Could you take a look and see if they look ok to you? |
| * Returns true if the last user_id did not change. | ||
| * @return bool | ||
| */ | ||
| protected function lastUserIdWasSetAndDoesMatch(VisitProperties $visitProperties, Request $request) |
There was a problem hiding this comment.
@diosmosis @mattab @MichaelHeerklotz I'm now actually wondering... on a regular website when tracking userId, and then the userId changes, would we create a new visit keeping the same visitorId maybe? Just a random thought maybe.
There was a problem hiding this comment.
Actually, I'm seeing this is what it actually implements. Be just good to have this behaviour confirmed.
There was a problem hiding this comment.
All good, I totally see we want this behaviour. Was not reading the code right initially.
|
@diosmosis looks all good 👍 There are a few more test failures in https://travis-ci.org/matomo-org/matomo/jobs/608100023?utm_medium=notification&utm_source=github_status that look expected and can be updated too. The UI build didn't finish so couldn't look at them. Might need to update some screenshots. We will then need to
|
|
Seems harder to fix the remaining test failures in a PR so will do so after merging. |
With these changes, the visitor ID no longer gets overwritten by the user ID.
This completely logically separates the user ID and the visitor ID feature.
It also solves problems when using the 3rd party cookie (global visitor id across sites) and user ID at the same time.
These are basically the same changes as in #13620 but without the option to turn the new behavior off, as requested by Matthieu.
He wrote to me in November: