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

Correct the trackContentImpression command argument with the appropri… #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

OphelieMicoine
Copy link

…ate field names for Content Impressions

Like mentionned, the data.fieldName for the argument of the command trackContentImpression was the ones for Content Interaction. That pull request solved the issue.

- enableCookies if data.useCookies is true
- change order of setUserId option to get it before user anonimization
- deanonymizeUser if data.ipCollectionMode is true
- deanonymizeUser if data.setSessionIdStrictPrivacyMode is true
- if resetUserId and setUserIsAnonymous to true if data.setSessionIdStrictPrivacyMode is false
@OphelieMicoine
Copy link
Author

Hello @harmasz @abartczakpiwik @mbaersch and happy new year !!!

When you get time, I will be very honored if you can review this pull request please ?

@abartczakpiwik
Copy link
Contributor

Hi @OphelieMicoine,

It looks like there are few additional new things related to data anonymization, cookies and user ID. Are those changes intentional?

If so, please make sure to only address content impression related issues in this PR as 1. the PR name implies this, 2. it would be best to discuss potential breaking changes separately, so they don't end up in the master branch without proper approval.

@OphelieMicoine
Copy link
Author

Yes It was intentionnal because this options were missing to apply hybrid data collection. Ok I will make a new commit for this feature.

@abartczakpiwik
Copy link
Contributor

Yes It was intentionnal because this options were missing to apply hybrid data collection. Ok I will make a new commit for this feature.

As a side note - I will be working on a separate anonymization/deanonymization template in the upcoming weeks. After initial inspection it looks like the base template (this repository) might not require changes as most of the fields already support variables (anonymization, cookies, device data and so on).

@OphelieMicoine
Copy link
Author

OphelieMicoine commented Jan 8, 2025 via email

@abartczakpiwik
Copy link
Contributor

@OphelieMicoine could you push another commit with only the content impression changes? Don't want to close the whole PR :).

…te field names for Content Impressions

Like mentionned, the data.fieldName for the argument of the command trackContentImpression was the ones for Content Interaction. That pull request solved the issue.
@OphelieMicoine
Copy link
Author

Hello @abartczakpiwik sorry for the delay. It is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants