Fix/intial preferences #184
Conversation
|
@felipe-najson-ntf any idea when this will be merged? |
|
Hello again @borisrorsvort, I'm waiting for the approval by @pooyaj. We hope these days to merge this pr and publish the new version of consent manager. |
There was a problem hiding this comment.
@felipe-najson-ntf Sorry for the delay in reviewing the PR. There are some issues that we need to solve to make sure there is no regression. I pointed to two issues in the review. Also I suggest adding a new StoryBoard page that covers the two issues that this PR is trying fix, so we can test the changes easier.
| defaultDestinationBehavior, | ||
| categoryPreferences: preferences | ||
| }) | ||
| if (!initialPreferences) { |
There was a problem hiding this comment.
@felipe-najson-ntf This line could be problematic. We want to load analytics.js even if the user doesn't provide the initialPreferences. I think adding this if clause will lead to cases where users already consented to everything, and their consent settings is stored in cookies, but then the analytics.js doesn't load.
There was a problem hiding this comment.
This is the work in progress branch:
There was a problem hiding this comment.
Good morning @pooyaj, I do not agree with you on this point. I think that is exactly what users are complaining about, analytics.js initializes automatically without user consent. The idea is to load the initial preferences to the cookie "tracking-preferences" but NOT initialize.
With this we tried not to affect other behaviors of the library except when there are Initial preferences, do not execute it. With this solution when loading the page we will have:
Once the consent is accepted in the modal, tracking starts:
Note: I have changed the if sentence to something more precise in the following commit.
There was a problem hiding this comment.
@pooyaj I just left a pr with the new solution. If you still have doubts of why I included the if sentence we can have a small meeting to conclude with this issue. Greetings
There was a problem hiding this comment.
Sorry for another long delay for the response. I think there is a misunderstanding about the purpose of the initialPreferences. When we set the initialPreferences, it means that we are automatically opt-ing in users to those preferences only if there is no prior consent from the user. It means we assume consent from the user according to the initialPreferences.
Another important point here is that if there is already a consent ( in the cookies, or from the initial preferences ) we want to attempt to call conditionallyLoadAnalytics. If we don't do that, user ends up in a situation where they have to consent everytime they refresh the page ( or every time the consent manager component mounts )
| @@ -246,7 +249,8 @@ export default class ConsentManagerBuilder extends Component<Props, State> { | |||
| preferences, | |||
| isConsentRequired, | |||
| destinationPreferences, | |||
| workspaceAddedNewDestinations: Boolean(workspaceAddedNewDestinations) | |||
| workspaceAddedNewDestinations: Boolean(workspaceAddedNewDestinations), | |||
| havePreferencesChanged: initialPrefencesHaveValue | |||
There was a problem hiding this comment.
We don't need this here. If we set this to true, if will impact the handleSaveConsent and leads to unnecessary refresh.
| if ( | ||
| prevState.havePreferencesChanged || | ||
| newDestinations.length > 0 || | ||
| typeof newPreferences === 'boolean' |
There was a problem hiding this comment.
We have to update the handleSaveConsent function signature to allow for boolean ... so something like this:
handleSaveConsent = (newPreferences: CategoryPreferences | undefined | boolean, shouldReload: boolean)
There was a problem hiding this comment.
Agree with you.
|
@felipe-najson-ntf any news? |
Hello again @borisrorsvort. I just saw the requests from @pooyaj and we will have a meeting next week to close this issue since we are not understanding each other well. |
|
@felipe-najson-ntf @pooyaj Thanks! I really urge you to solve this matter asap as it basically makes all user of that component outlaw in Europe gdpr wise, and that issue has been open for more than a year. |
InitialPreferences working properly. Related to issues #121 #122