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

fix: do not store anonymous token in cookie when user has opted out #233

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Oct 28, 2020

Summary

This PR follows userHasOptedOut a step further.
Even with userHasOptedOut: true, search-insights created anonymous user token and stored it in the cookie, which is totally not used at all. (I'm not talking about the possibility of that customers could've read that cookie directly for some reason)

In my opinion, this shouldn't be treated as a breaking change. What do you think?


ps. I put the people I recently talked about this with as reviewers.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b13c0ac:

Sandbox Source
Vanilla Configuration

@eunjae-lee eunjae-lee requested a review from a team October 28, 2020 17:09
@ghost ghost requested review from francoischalifour and shortcuts and removed request for a team October 28, 2020 17:10
@eunjae-lee eunjae-lee requested review from tkrugg and Haroenv and removed request for shortcuts October 28, 2020 17:10
@Haroenv
Copy link
Contributor

Haroenv commented Oct 28, 2020

I think it makes sense, although we might need to change opt out to opt in in the next major

@tkrugg
Copy link
Contributor

tkrugg commented Oct 29, 2020

From my perspective, this does not break any feature because the library does nothing with the content of the cookie when passed userHasOptedOut: true. Is the cookie still used by InstantSearch with Insights ?

@eunjae-lee
Copy link
Contributor Author

From my perspective, this does not break any feature because the library does nothing with the content of the cookie when passed userHasOptedOut: true. Is the cookie still used by InstantSearch with Insights ?

Hey @tkrugg , this cookie is still read by InstantSearch with Insights (but not written).

By the way, we need to think a little more about InstantSearch side, because so far we already assumed we always have a token because of anonymous user token. However now there could be really nothing, meaning

hasUserOptedOut: true
+
no explicit token setting

will make a hole. We should prevent events being sent with no token and alert the devs via warning messages and docs. Does it make sense? @Haroenv @francoischalifour @tkrugg

@Haroenv
Copy link
Contributor

Haroenv commented Oct 29, 2020

yes, we should not send when there's no user token, nor anonymous one and error in development for that case

@francoischalifour
Copy link
Member

@eunjae-lee Makes sense to me.

@eunjae-lee eunjae-lee merged commit 8669b67 into master Oct 29, 2020
@eunjae-lee eunjae-lee deleted the fix/no-cookie-when-opted-out branch October 29, 2020 16:58
@tkrugg
Copy link
Contributor

tkrugg commented Oct 30, 2020

By the way, we need to think a little more about InstantSearch side, because so far we already assumed we always have a token because of anonymous user token. However now there could be really nothing, meaning

hasUserOptedOut: true
+
no explicit token setting

will make a hole. We should prevent events being sent with no token and alert the devs via warning messages and docs. Does it make sense? @Haroenv @francoischalifour @tkrugg

Hey @eunjae-lee this isn't quite clear to me yet.
When hasUserOptedOut: true, we already are in a situation where events cannot be sent.
Also, an error is thrown when userToken was not supplied. So I wouldn't worry about the case you're highlighting.

FYI documentation is being updated to reflect this change here: algolia/doc#5053.

@eunjae-lee
Copy link
Contributor Author

@tkrugg oh you're right 🤦🏻‍♂️ Thank you!

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.

4 participants