@MichaelHeerklotz opened this Pull Request on April 18th 2019 Contributor

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 https://github.com/matomo-org/matomo/pull/13620 but without the option to turn the new behavior off, as requested by Matthieu.

He wrote to me in November:

Interesting... we discussed it a while ago and I think the conclusion was that we should unlink these two in core by default. But there were maybe some problems with this which I can't remember (but would be likely noted somewhere on Github). So maybe you could open the PR to do this in core rather than a setting, and we could start to review

@tsteur commented on April 22nd 2019 Member

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

@mattab commented on April 23rd 2019 Member

Looks good to me also, thank you for the PR @MichaelHeerklotz :+1:

Feedback:

  • +1 to update the php matomo tracker in https://github.com/matomo-org/matomo-php-tracker/blob/1.4.1/PiwikTracker.php#L1241-L1245 - would be good 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)
  • the User ID user guide will need to be updated to reflect how User id works. 1) this part: "How requests with a User ID are tracked > Same user from multiple device use case: [....]". After the change, each simultaneous visit on separate devices would each create a new visit (but with the same user id)? 2) maybe there would be other things to add/update in the guide use cases
@MichaelHeerklotz commented on April 24th 2019 Contributor

Thank you.
I think this comment can then also be removed from the queued tracking plugin settings "Number of queue workers" description:

DO NOT USE more than 1 worker if you make use the UserId feature when tracking see https://github.com/piwik/piwik/issues/7691

@mattab commented on May 28th 2019 Member

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)?

@MichaelHeerklotz commented on May 28th 2019 Contributor

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.

@peachp commented on July 30th 2019

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.

@tsteur commented on October 2nd 2019 Member

@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

@MichaelHeerklotz commented on October 3rd 2019 Contributor

@tsteur It should be good to merge now

@tsteur commented on October 4th 2019 Member

@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 3.x-dev green so that all tests pass. From what I see the test changes look fine.

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 👍

@tsteur commented on October 15th 2019 Member

@MichaelHeerklotz let me know what you want to do here. We're planning to release 3.12 soonish.

@diosmosis commented on November 6th 2019 Member

@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?

@tsteur commented on November 6th 2019 Member

@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

@diosmosis commented on November 8th 2019 Member

Seems harder to fix the remaining test failures in a PR so will do so after merging.

This Pull Request was closed on November 8th 2019
Powered by GitHub Issue Mirror