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.

Sign up for GitHub

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

Jump to bottom

Store visitorID related to userID to cookies #6838

Closed
wants to merge 0 commits into from
Closed

Store visitorID related to userID to cookies #6838

wants to merge 0 commits into from

Conversation

jantlwoomy
Copy link
Contributor

When visitor is logging in and we are calling setUserID method, cookie value for uuid should be updated to this userID.

How it was before the fix:
  1. Visitor enters the site, new random is visitorID generated.
  2. Visitor registers on the site. We call setUserID and PiwikTracker.php generates new visitorID related to visitors userID.
  3. Cookie visitorID is still random and differs from php visitorID.
  4. Visitor logs out, all his actions are stored with cookie visitorID value. We cannot tell what visitor does after logging out.
How it is after the fix:
  1. Visitor enters the site, new random is visitorID generated.
  2. Visitor registers on the site. We call setUserID and PiwikTracker.php generates new visitorID related to visitors userID.
  3. Cookie visitorID is the same as php visitorID.
  4. Visitor logs out, all his actions are stored with cookie visitorID value. Now we can tell what visitor does after logging out.

This case scenario works only if trust_visitors_cookies option is true.

@mattab
Copy link
Member

mattab commented Dec 15, 2014

Hi @jantlwoomy thanks for your suggestion!

It looks like a useful change, but it's lacking some tests to make sure it works as expected. Do you think you could try add some tests in this file: https://github.com/piwik/piwik/blob/master/tests/javascript/index.php#L405

these tests are executed in the Continuous integration so you can see whether the tests still pass after each of your commit. Let me know if you have some question...

@mattab mattab added this to the Piwik 2.11.0 milestone Dec 15, 2014
@mattab mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Dec 15, 2014
@jantlwoomy
Copy link
Contributor Author

I can try, but not in next 2 weeks.

@mattab mattab modified the milestones: Short term, Piwik 2.11.0 Dec 15, 2014
@mattab
Copy link
Member

mattab commented Dec 15, 2014

No worries @jantlwoomy - this would be great and we'll wait 👍

@mattab mattab modified the milestones: Short term, Mid term Dec 18, 2014
@asafyish
Copy link

asafyish commented Jan 5, 2015

+1

@jantlwoomy
Copy link
Contributor Author

Please take a look. It's not much but I'm not sure what else we can test in this case.

@asafyish
Copy link

I just tested with this patch and without and that's what I got:
Without:

  1. User visit website.
  2. User login and we setUserId to his email
  3. User visits other page.
    We now have 2 visits with 2 different visitorid - not good for us. we want to record the entire "lifecycle" of the user.

With:

  1. User visit website.
  2. User login and we setUserId to his email
  3. User visits other page.
    We now have 1 visit, with the correct user id and visitor id.
    I think it's a must have.

@mattab mattab modified the milestones: Piwik 2.11.0, Mid term Feb 9, 2015
mattab
mattab reviewed Feb 10, 2015
View reviewed changes
plugins/API/API.php
@@ -160,8 +160,7 @@ public function getSegmentsMetadata($idSites = array(), $_hideImplementationData
'name' => 'General_UserId',
'segment' => 'userId',
'acceptedValues' => 'any non empty unique string identifying the user (such as an email address or a username).',
'sqlSegment' => 'log_visit.idvisitor',
'sqlFilterValue' => array('Piwik\Common', 'convertUserIdToVisitorIdBin'),
'sqlSegment' => 'log_visit.user_id',
Copy link
Member

Choose a reason for hiding this comment

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

could you remove this change from the PR as it was applied today to master?

@mattab
Copy link
Member

mattab commented Feb 10, 2015

I think there is a bug indeed, and this change corrects it. To keep the piwik.js consistent with the PHP client (on which other clients may be based) I would suggest to fix slightly different. In piwik.js getRequest() the uuid used to store in the cookie could use the User ID when set. This would be done here: https://github.com/piwik/piwik/blob/2.11.0-b3/js/piwik.js#L2814-2814

Then whenever you use setUserId and then any track* call, it would update the cookie with the user ID. (this also saves us from duplicating the logic in setUserId). Does it make sense?

@jantlwoomy jantlwoomy closed this Feb 10, 2015
@jantlwoomy
Copy link
Contributor Author

I've tried to remove excess commits and accidentally closed PR. Sorry for that.
Your suggestions looks nice.

@mattab
Copy link
Member

mattab commented Feb 10, 2015

@jantlwoomy feel free to open new pull request and I'll do my best to get it merged quickly!

@jantlwoomy jantlwoomy mentioned this pull request Feb 10, 2015
Merged
@mattab mattab added duplicate For issues that already existed in our issue tracker and were reported previously. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. and removed not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Feb 12, 2015
@mattab mattab mentioned this pull request Apr 17, 2015
Closed
@mattab
Copy link
Member

mattab commented Apr 19, 2018

FYI this will get reverted in #12742

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
duplicate For issues that already existed in our issue tracker and were reported previously. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
Piwik 2.11.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jantlwoomy @mattab @asafyish