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

Call setVisitorIdCookie later so disableCookie config can be set first #18135

Closed
wants to merge 6 commits into from

Conversation

justinvelluppillai
Copy link
Contributor

Description:

Fixes #17892

Review

…t and avoid rogue cookie creation when creating tracker instances
@justinvelluppillai
Copy link
Contributor Author

build js

js/piwik.js Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Oct 28, 2021
@sgiehl sgiehl removed the Stale The label used by the Close Stale Issues action label Oct 28, 2021
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Nov 12, 2021
@tsteur
Copy link
Member

tsteur commented Nov 12, 2021

ping @justinvelluppillai there's still one of my comments left

@tsteur tsteur removed the Stale The label used by the Close Stale Issues action label Nov 12, 2021
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Nov 27, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2022

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale for long The label used by the Close Stale Issues action label Jan 8, 2022
@tsteur
Copy link
Member

tsteur commented Jan 9, 2022

ping @justinvelluppillai

@github-actions github-actions bot removed the Stale for long The label used by the Close Stale Issues action label Jan 10, 2022
@justinvelluppillai
Copy link
Contributor Author

build js

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Feb 14, 2022
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

This works for me, did quite some testing around it ( but didn't check the tests). @sgiehl can you think of any issue with this?

@justinvelluppillai
Copy link
Contributor Author

justinvelluppillai commented Feb 17, 2022

If I do it this way I will need to update quite a few tests which rely on the visitorId cookie being set when tracker is created.

eg see failing tests here #18789

@tsteur
Copy link
Member

tsteur commented Feb 17, 2022

Indeed, seeing https://app.travis-ci.com/github/matomo-org/matomo/jobs/559513056 there are few tests failing. Not sure if they highlight a regression or if they just need to be tweaked. Maybe var visitorIdStart = tracker.getVisitorId(); would need to be executed after the first tracking request when the ID will be set?

@justinvelluppillai
Copy link
Contributor Author

Indeed, seeing https://app.travis-ci.com/github/matomo-org/matomo/jobs/559513056 there are few tests failing. Not sure if they highlight a regression or if they just need to be tweaked. Maybe var visitorIdStart = tracker.getVisitorId(); would need to be executed after the first tracking request when the ID will be set?

Yes I'd need to add a tracking request to all those calls for those expected outcomes to be right. Would need to understand the rationale for the tests to know if I can just change them or it would be a regression also.

@sgiehl
Copy link
Member

sgiehl commented Feb 23, 2022

@justinvelluppillai does #18789 replace this PR? If so be good to close this one then.

@justinvelluppillai
Copy link
Contributor Author

This PR has most of the conversation history so worth keeping. The other is just a sandbox for running test experiments. I don't mind which one is closed eventually once I have ironed out the issues but was expecting the other one to be closed and this one merged. Neither has needs review label so shouldn't be a problem meantime, right?

@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Mar 11, 2022
@justinvelluppillai justinvelluppillai deleted the issue/17892-disable-cookies-fix branch November 16, 2022 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't create test cookies when cookies are disabled
3 participants