Skip to content

feat: add user context #305

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

Closed
wants to merge 4 commits into from
Closed

Conversation

thomaszurkan-optimizely
Copy link
Contributor

Summary

  • Added user context and create off of optimizely. Added unit tests as well.

Test plan

Unit tests added

@coveralls
Copy link

coveralls commented Nov 9, 2020

Coverage Status

Coverage decreased (-0.2%) to 96.136% when pulling 61fe356 on decideApiUserContext into edf5528 on master.

@@ -0,0 +1,34 @@
# Copyright 2019, Optimizely
Copy link
Contributor

Choose a reason for hiding this comment

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

2020

@@ -0,0 +1,84 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Omit this line.

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.

Looks good.
Should add mutex control for attributes. More tests suggested.

Returns:
None
"""
self.user_attributes[attribute_key] = attribute_value
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a mutex control for attributes read/write?
"setAttributes" and attribute -reading for decide can happen in other threads. See C#-sdk as a good reference (optimizely/csharp-sdk#253)

uc.set_attribute("key", "value")
self.assertEqual(uc.user_attributes["key"], "value", "should have added attribute")
uc.set_attribute("key", "value2")
self.assertEqual(uc.user_attributes["key"], "value2", "should have new attribute")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have more tests:

  • construct with non-empty attributes
  • setAttribute can override the init attributes
  • attributes not changed once constructed when the caller copy is updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 314

@jaeopt jaeopt closed this Feb 1, 2021
@jaeopt
Copy link
Contributor

jaeopt commented Feb 1, 2021

Merged with #309

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.

5 participants