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

allow to set identity_func by ANALYTICAL_IDENTITY_FUNC settings #189

Closed
wants to merge 2 commits into from

Conversation

PetrDlouhy
Copy link
Contributor

Google has in it's conditions for enabling UserID requirement, that prohibits sending personal data (such as e-mail address) to analytics.
I am using e-mail address as username, so using GTag would break the requirements for me.

This enables me to set user UUID for the UserID parameter.

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #189 (5ef70a6) into main (f487cb8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #189   +/-   ##
=======================================
  Coverage   94.49%   94.49%           
=======================================
  Files          31       31           
  Lines        1344     1345    +1     
=======================================
+ Hits         1270     1271    +1     
  Misses         74       74           
Impacted Files Coverage Δ
analytical/utils.py 97.75% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f487cb8...5ef70a6. Read the comment docs.

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

Can you extend the tests to match your changes, please? And also mention ANALYTICAL_IDENTIFY_FUNC in the docs.

Thank you for your understanding.

analytical/utils.py Outdated Show resolved Hide resolved
@bittner
Copy link
Member

bittner commented Jul 10, 2022

@PetrDlouhy Are you still interested to take this PR ahead?

@PetrDlouhy
Copy link
Contributor Author

@bittner I will look at it.

@PetrDlouhy PetrDlouhy changed the title allow to set identity_func by ANALYTICAL_IDENTIFY_FUNC settings allow to set identity_func by ANALYTICAL_IDENTITY_FUNC settings Jul 12, 2022
@PetrDlouhy
Copy link
Contributor Author

@bittner I added simple test, some docs and also renamed the setting from ANALITICAL_IDENTIFY_FUNC to ANALYTICAL_IDENTITY_FUNC.
Please approve the workflows.

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

IIUC, the problem you're trying to solve relates to hard requirements of some of Analytical's analytics providers. Shouldn't this be solved in the respective modules instead of requiring our users to code an identity function?


.. data:: ANALYTICAL_IDENTITY_FUNC

Default: Identity function dependend on provider
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Default: Identity function dependend on provider
Default: Identity function dependent on provider

docs/settings.rst Outdated Show resolved Hide resolved
docs/settings.rst Outdated Show resolved Hide resolved
default settings of diferent providers.

E.g. Google has in it's conditions for enabling UserID requirement, that prohibits
sending personal data (such as e-mail address) to analytics.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sending personal data (such as e-mail address) to analytics.
sending personal data (such as an e-mail address) to analytics.

@PetrDlouhy
Copy link
Contributor Author

@bittner I have fixed the typos.

I think, that possibility to manually override the identity function allows flexibility in corner cases. E.g. I want to use my UUID field to keep consistency of my data even if the default behavior of gtag provider is fixed.

I don't know other providers, but the django-analytical might use hash from the username as identity function for google_analytics_gtag provider. But that would interrupt consistency of the collected data for users that are updating this app. So I am not sure how to update this correctly (Make a new provider and mark the current obsolete?).

It is also truth, that one identity function might not fit for all providers, if more of them are used. So maybe we might better use settings like:

ANALYTICAL_IDENTITY_FUNC_OVERRIDES = {
    'google_analytics_gtag': lambda user: user.uuid,
}

and

ANALYTICAL_IDENTITY_FUNC_DEFAULT = lambda user: user.uuid

which would override only the default user.get_username() function.

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

I agree that making the identity func dependent on the provider would make sense. I would keep it simple, though.

ANALYTICAL_IDENTITY_FUNC = {
    'google_analytics_gtag': lambda user: user.uuid,
}

or maybe even

ANALYTICAL_AUTO_IDENTIFY = {
    'google_analytics_gtag': lambda user: user.uuid,
    'matomo': True,
}

Though I just noticed something else: The feature you attempt to contribute already exists (get_identity, lines 75+). Instead of a (lamda) function, though, you simply add an analytical_identity value (or the service specific value) to the template context.

Isn't that good enough?

analytical/utils.py Outdated Show resolved Hide resolved
Comment on lines +40 to +41
In such case add uuid field to the user and set ```ANALYTICAL_IDENTITY_FUNC``` to
```lambda user: user.uuid```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In such case add uuid field to the user and set ```ANALYTICAL_IDENTITY_FUNC``` to
```lambda user: user.uuid```
In such a case add the ``uuid`` field to the user and set ``ANALYTICAL_IDENTITY_FUNC`` to
``lambda user: user.uuid`` or a proper function that takes ``user`` as an argument, e.g.
.. code-block:: python
def uuid_identity(user):
return user.uuid
ANALYTICAL_IDENTITY_FUNC = uuid_identity

Comment on lines +36 to +38
E.g. Google has in its conditions for enabling UserID the requirement, that prohibits
sending personal data (such as an e-mail address) to analytics.
If e-mail address is used as username, using GTag would break the requirements.
Copy link
Member

@bittner bittner Jul 13, 2022

Choose a reason for hiding this comment

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

Suggested change
E.g. Google has in its conditions for enabling UserID the requirement, that prohibits
sending personal data (such as an e-mail address) to analytics.
If e-mail address is used as username, using GTag would break the requirements.
For example, Google Analytics requires not sending personal data (such as an e-mail address) for
enabling UserID. If an e-mail address is used as username using GTag would break that requirement.

I have to admit, I don't understand "for enabling UserID".

Also, do you have any references for your claim of the GA requirement? Is it a hard requirement or is it "just" in their terms of use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant transition from google_analytics to google_analytics_gtag with user identity which I underwent in my project.

The requirements are their terms of use, which they write very suggestively: https://developers.google.com/analytics/solutions/crm-integration#user_id
And also I think I had to agree to that when switching on the User ID feature in Google Analytics.

@PetrDlouhy
Copy link
Contributor Author

You are right. The analytical_identity in context works. Maybe we should document it better (it is described under few providers, but not in general docs nor for google_analytics_gtag provider.

Also some providers (like google_analytics_gtag) doesn't pass the prefix parameter, so it is not possible to use provider specific value.

@bittner
Copy link
Member

bittner commented Jul 13, 2022

Maybe we should document it better (it is described under few providers, but not in general docs nor for google_analytics_gtag provider.

Great! It would be awesome if you could provide the missing pieces to the documentation. A working code sample would be fantastic.

Also some providers (like google_analytics_gtag) doesn't pass the prefix parameter, so it is not possible to use provider specific value.

That was already reported (#188, #197). Can you spot how to fix this problem?

P.S.: Too sad for the nice test you added just recently! 😢

@PetrDlouhy
Copy link
Contributor Author

PetrDlouhy commented Jul 14, 2022

I am closing this as it is deprecated by #217, #218 and #219.

I have added at least some variant of the test in #218

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.

None yet

2 participants