-
Notifications
You must be signed in to change notification settings - Fork 37
feat: odp datafile parsing and audience evaluation #393
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few small changes suggested.
More tests for datafile "integrations" processing including invalid format, missing, and empty will be good.
https://github.com/optimizely/swift-sdk/blob/jae/ats/Tests/OptimizelyTests-DataModel/IntegrationTests.swift
@@ -54,6 +54,7 @@ def __init__( | |||
self.client = optimizely_client | |||
self.logger = logger | |||
self.user_id = user_id | |||
self.qualified_segments: list[str] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this "qualified_segments" publicly get and set under lock-protected.
You can add this support in the other PR (OptimizelyUserContext) later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added leading underscore to indicate a private variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most changes look good. I suggested a few more changes.
Not sure about the plan for atomic (public) getter/setter in optimizely_user_context. We can chat.
evaluator = condition_helper.CustomAttributeConditionEvaluator( | ||
qualified_condition_list, self.user_context, self.mock_client_logger, | ||
) | ||
|
||
self.assertStrictTrue(evaluator.evaluate(0)) | ||
|
||
def test_qualified__returns_false__when_user_is_not_qualified(self, ): | ||
self.user_context.qualified_segments = ['odp-segment-1'] | ||
self.user_context._qualified_segments = ['odp-segment-1'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need support for atomic getter for "qualified_segments"? like "get_user_attributes()"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I thought we were going to implement those during a subsequent PR. I've added a getter and setter for qualified segments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
A new field
integrations
and new audience typeodp.audiences
will be added to the datafile schema for ODP integration.parse the odp values (
publicKey
andhost
)accept datafile with and without the
integrations
section or empty section as well.support a new audience with the
odp.audiences
andqualified
match.support any (allowed) combination of audiences, including ODP and other existing audiences.
Test plan
Added/extended tests in: