jazzband / django-analytical

Analytics services for Django projects
MIT License
1.2k stars 168 forks source link

allow to set identity_func by ANALYTICAL_IDENTITY_FUNC settings #189

Closed PetrDlouhy closed 2 years ago

PetrDlouhy commented 3 years ago

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[bot] commented 3 years ago

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%) :arrow_up:

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.

bittner commented 2 years ago

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

PetrDlouhy commented 2 years ago

@bittner I will look at it.

PetrDlouhy commented 2 years ago

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

PetrDlouhy commented 2 years ago

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

PetrDlouhy commented 2 years ago

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 commented 2 years ago

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! :cry:

PetrDlouhy commented 2 years ago

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