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

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

Merged
merged 10 commits into from Feb 1, 2018

Conversation

Projects
None yet
3 participants
@znerol
Contributor

znerol commented Oct 3, 2017

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

@znerol

This comment has been minimized.

Show comment
Hide comment
@znerol

znerol Oct 3, 2017

Contributor

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

Contributor

znerol commented Oct 3, 2017

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

@1u

This comment has been minimized.

Show comment
Hide comment
@1u

1u Oct 9, 2017

nice!

1u commented Oct 9, 2017

nice!

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 16, 2017

Member

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

it's noise

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Jan 24, 2018

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@znerol

znerol Jan 25, 2018

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@znerol

znerol Jan 25, 2018

Contributor

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

Contributor

znerol commented Jan 25, 2018

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

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();

This comment has been minimized.

@mattab

mattab Jan 26, 2018

Member

Here could you also test that getUserId is empty string ?

@mattab

mattab Jan 26, 2018

Member

Here could you also test that getUserId is empty string ?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Jan 26, 2018

Member

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

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

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

mattab added a commit to matomo-org/matomo-php-tracker that referenced this pull request Feb 1, 2018

Implements resetUserId()
Implements the new resetUserId() method from JS tracker in matomo-org/matomo#12141 matomo-org/matomo#7556

@mattab mattab referenced this pull request Feb 1, 2018

Open

Implements resetUserId() #29

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Mar 27, 2018

Member

-> 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 🎉

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 changed the title from Add a method to de-assign a user id to 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