feat: Add ODP Segments support in Audience Evaluation #474
Conversation
… methods to accept User Context, updated unit tests to pass User Context.
core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java
Outdated
Show resolved
Hide resolved
|
@jaeopt Can you take a look at this PR whenever you have time |
core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java
Outdated
Show resolved
Hide resolved
| @@ -41,6 +41,7 @@ public class MatchRegistry { | |||
| public static final String SEMVER_LE = "semver_le"; | |||
| public static final String SEMVER_LT = "semver_lt"; | |||
| public static final String SUBSTRING = "substring"; | |||
| public static final String QUALIFIED = "qualified"; | |||
There was a problem hiding this comment.
This is not in "register" list below. Should't we add this to register or remove from here for consistency? It looks like QUALIFIED is handled as an exception case in UserAttribute.
There was a problem hiding this comment.
Looks good! A few small changes suggested.
Also missing tests for validating integrations (no integrations, empty integrations, key-only integration, key-missing integration, etc). See https://github.com/optimizely/swift-sdk/blob/jae/ats/Tests/OptimizelyTests-DataModel/IntegrationTests.swift
| @@ -45,7 +45,7 @@ jobs: | |||
|
|
|||
| test: | |||
| if: startsWith(github.ref, 'refs/tags/') != true | |||
| runs-on: ubuntu-18.04 | |||
| runs-on: macos-latest | |||
There was a problem hiding this comment.
Because some tests were failing for no apparent reason on linux. we have changes it to macox for now. will fix it later as part of a separate ticket
| return key; | ||
| } | ||
|
|
||
| public String getHost() { return host; } |
|
|
||
| public String getHost() { return host; } | ||
|
|
||
| public String getPublicKey() { return publicKey; } |
There was a problem hiding this comment.
I think it is nullable in java unless specified the other way round?
| this.publicKey = publicKey; | ||
| } | ||
|
|
||
| public String getKey() { |
| return "Integration{" + | ||
| "key='" + key + '\'' + | ||
| ", host='" + host + '\'' + | ||
| ", publicKey='" + publicKey + '\'' + |
There was a problem hiding this comment.
Can we make this toString for generic integration types, not only for ODP?
Like "if (host != null) { string += host }..."
I reviewed because Noman was working on this initially. Now i took it up so it does not make sense to approve my own work :)
| GsonConfigParser parser = new GsonConfigParser(); | ||
| String integrationsObject = ", \"integrations\": [" + | ||
| "{ \"key\": \"not-odp\", " + | ||
| "\"host\": \"https://example.com\", " + |
There was a problem hiding this comment.
| "\"host\": \"https://example.com\", " + | |
| "\"any-key\": \"any-value\", " + |
Let's check if it can tolerate future fields. Same for other parsers.
Summary
Test plan
JIRA
OASIS-8382