Update Cookie Storage #998
Conversation
| export const enum ConsentSource { | ||
| Implicit = 0, | ||
| API = 1, | ||
| GCM = 2 |
There was a problem hiding this comment.
Could we add APIV1 vs. APIV2 at the same time or in a separate PR (if you prefer that).
| analytics_Storage: config.track ? Constant.Granted : Constant.Denied, | ||
| source: u.consent || u.ad_consent ? ConsentSource.Cookie : ConsentSource.Implicit, | ||
| ad_Storage: u.ad_consent || (config.track && consentStatus.source != ConsentSource.Cookie) ? Constant.Granted : Constant.Denied, | ||
| analytics_Storage: u.consent || config.track ? Constant.Granted : Constant.Denied, |
| analytics_Storage: config.track ? Constant.Granted : Constant.Denied, | ||
| source: u.consent || u.ad_consent ? ConsentSource.Cookie : ConsentSource.Implicit, | ||
| ad_Storage: u.ad_consent || (config.track && consentStatus.source != ConsentSource.Cookie) ? Constant.Granted : Constant.Denied, | ||
| analytics_Storage: u.consent || config.track ? Constant.Granted : Constant.Denied, |
| ad_Storage: config.track ? Constant.Granted : Constant.Denied, | ||
| analytics_Storage: config.track ? Constant.Granted : Constant.Denied, | ||
| source: u.consent || u.ad_consent ? ConsentSource.Cookie : ConsentSource.Implicit, | ||
| ad_Storage: u.ad_consent || (config.track && consentStatus.source != ConsentSource.Cookie) ? Constant.Granted : Constant.Denied, |
There was a problem hiding this comment.
Can you explain this logic?
Style nit: This name is a bit confusing. Better would be something like "currentCurreent' consent or similar. Unify everything to state not status as well in naming things. Refers to: packages/clarity-js/src/data/metadata.ts:18 in 46911be. [](commit_id = 46911be, deletion_comment = False) |
| if (core.active() && consentData.analytics_Storage) { | ||
| config.track = true; | ||
| track(user(), BooleanFlag.True); | ||
| track(user(), consentData.analytics_Storage, consentData.ad_Storage); |
There was a problem hiding this comment.
Minor: Might be clean to just pass the cseontnData as a single argument not splitting it up here into separate arg (enables us to add more permissions later). And lot of bool parameters are easy to get wrong (order, etc.).
| } | ||
| } | ||
|
|
||
| function track(u: User, consent: BooleanFlag = null): void { |
There was a problem hiding this comment.
Rename to analytics to make it unambiguous. Consent own its own is too ambiguous.
| if (u.expiry === null || Math.abs(end - u.expiry) >= Setting.CookieInterval || u.consent !== consent || u.dob !== dob) { | ||
| let cookieParts = [data.userId, Setting.CookieVersion, end.toString(36), consent, dob]; | ||
| if (u.expiry === null || Math.abs(end - u.expiry) >= Setting.CookieInterval || u.consent !== consent || u.ad_consent != ad_consent || u.dob !== dob) { | ||
| let cookieParts = [data.userId, Setting.CookieVersion, end.toString(36), consent, ad_consent, dob]; |
There was a problem hiding this comment.
This is not backwards/forwards compatible. Better to append to end. Think about how this work in current client (or older) and newer ones in all combination (older cookie, new client) and (new cookies, old client).
Update cookie consent to store ad storage as well