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.

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

JS Tracker, New method resetUserId to de-assign a user id after a logout #12141

Merged
merged 10 commits into from Feb 1, 2018
Merged

JS Tracker, New method resetUserId to de-assign a user id after a logout #12141

merged 10 commits into from Feb 1, 2018

Conversation

znerol
Copy link
Contributor

@znerol znerol commented Oct 3, 2017

Replicates the user-signout mechanism from the PHP tracker in order to fix #7556

@znerol znerol mentioned this pull request Oct 3, 2017
Closed
@znerol
Copy link
Contributor Author

znerol commented Oct 3, 2017

Tests pass except for the screenshot thing. Is that a signal or noise?

@1u
Copy link

1u commented Oct 9, 2017

nice!

@mattab
Copy link
Member

mattab commented Oct 16, 2017

Tests pass except for the screenshot thing. Is that a signal or noise?

it's noise

@mattab mattab added the Needs Review PRs that need a code review label Nov 23, 2017
@mattab mattab added this to the Priority Backlog (Help wanted) milestone Jan 10, 2018
@mattab mattab modified the milestones: Priority Backlog (Help wanted), 3.3.1 Jan 24, 2018
@mattab
Copy link
Member

mattab commented Jan 24, 2018

Thanks for creating this Pull request @znerol 👍

Feedback:

  • This PR is solving a very important use case for which we currently don't have a good solution: logging out users in SPA (currently the User Id stays in subsequent pageviews in SPA, even after logout..)
  • rename setNewVisitorId: since User ID logout use case in Single Page Apps is the main use case for this method, maybe we call it resetUserId (or unsetUserId, or even logoutUserId) if anyone has some thoughts?
  • in the setUserId method, let's not call the new method when user id is false. It's best to force users to call resetUserId separately when needed.

@znerol feel free to make the changes, otherwise we will likely work on this next month.

@mattab mattab modified the milestones: 3.3.1, 3.4.0 Jan 24, 2018
@znerol
Copy link
Contributor Author

znerol commented Jan 25, 2018

Update PR, I've opted for resetUserId. It not only clears the user id but also generates a new visitor id. unsetUserId does not really capture that bit. Also logoutUserId could be misleading, we are not talking about privilege levels here.

@znerol
Copy link
Contributor Author

znerol commented Jan 25, 2018

Should I squash the commits? (Except for the minify piwik.js one.)

mattab
mattab reviewed Jan 26, 2018
View reviewed changes
tests/javascript/index.php
tracker.setUserId(false);
ok(getVisitorIdFromCookie(tracker).length == 16, "after setting empty user id, visitor ID from cookie should still be 16 chars, got: " + getVisitorIdFromCookie(tracker));
equal(getVisitorIdFromCookie(tracker), visitorId, "after setting empty user id, visitor ID from cookie should be the same as previously ("+ visitorId +")");
tracker.resetUserId();
Copy link
Member

Choose a reason for hiding this comment

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

Here could you also test that getUserId is empty string ?

@mattab
Copy link
Member

mattab commented Jan 26, 2018

Thanks for updating the PR! fyi: It's not needed to squash commits as when we merge it will squash them.

Code review

  • left a comment.
  • feel free to add the new feature to CHANGELOG.md but we'll do it otherwise
  • Looks good to me otherwise! Maybe @tsteur you can take a quick look?

after merge will need to update the documentation in: https://developer.matomo.org/api-reference/tracking-javascript

@mattab mattab modified the milestones: 3.4.0, 3.3.1 Jan 26, 2018
@mattab mattab merged commit a99e0ef into matomo-org:3.x-dev Feb 1, 2018
@mattab mattab mentioned this pull request Feb 1, 2018
Merged
mattab added a commit to matomo-org/matomo-php-tracker that referenced this pull request Feb 1, 2018
@mattab
Implements resetUserId()
9cd2b64 
Implements the new resetUserId() method from JS tracker in matomo-org/matomo#12141 matomo-org/matomo#7556
@mattab mattab mentioned this pull request Feb 1, 2018
Closed
@mattab
Copy link
Member

mattab commented Mar 27, 2018

-> Check out the documentation for this new feature here: https://developer.matomo.org/guides/tracking-javascript-guide#when-user-logs-out-reset-user-id

it is available in Matomo 3.4.0 due to release today 🎉

@mattab mattab added Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. labels Mar 27, 2018
@mattab mattab changed the title Add a method to de-assign a user id JS Tracker, New method resetUserId to de-assign a user id after a logout Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattab mattab mattab left review comments

Assignees
No one assigned
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Needs Review PRs that need a code review
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

UserID "Signing out use-case" - actions still attributed to the same Visitor
3 participants
@znerol @1u @mattab