Skip to content

[FSSDK-10711] Make use of VUID as an opt-in #950

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

Merged
merged 39 commits into from
Oct 16, 2024

Conversation

mikechu-optimizely
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely commented Oct 11, 2024

Summary

  • Client-side use of the visitor user ID (VUID) should be opt-in
  • VuidManager has been hoisted to the top optimizely client level
  • Additional devX and doc improvements

⚠️ NOTE: this is not being merged to the master branch yet, but will be released. We'll need to cherry-pick later.

Test plan

  • Existing tests should pass
  • New tests should also pass
  • FSC end-to-end tests should pass

Issues

FSSDK-10711

Copy link
Contributor

@raju-opti raju-opti left a comment

Choose a reason for hiding this comment

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

We also need to handle the following requirement from the TDD:

  • createUserContext() will be rejected when userId is not provided

@mikechu-optimizely mikechu-optimizely marked this pull request as draft October 15, 2024 15:42
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Added some comments. I believe @raju-opti has better idea about sync/async VuidManager.

@mikechu-optimizely mikechu-optimizely marked this pull request as ready for review October 15, 2024 20:57
Copy link
Contributor

@raju-opti raju-opti left a comment

Choose a reason for hiding this comment

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

A few non critical comments

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

I see most issues fixed. A few more comments.

@coveralls
Copy link

coveralls commented Oct 16, 2024

Coverage Status

coverage: 90.569% (+0.3%) from 90.309%
when pulling f382071 on mike/FSSDK-10711-vuid-opt-in
into 8f572ff on 5.x.x.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@raju-opti raju-opti merged commit 40d1ff4 into 5.x.x Oct 16, 2024
19 checks passed
@raju-opti raju-opti deleted the mike/FSSDK-10711-vuid-opt-in branch October 16, 2024 20:35
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