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.

Sign up for GitHub

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

Jump to bottom

feat: Add ODP Segments support in Audience Evaluation #474

Merged
merged 28 commits into from
Jul 21, 2022
Merged

feat: Add ODP Segments support in Audience Evaluation #474

merged 28 commits into from
Jul 21, 2022

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Jun 16, 2022 •
edited by zashraf1985
Loading

Summary

  1. Added ODP Segments support in Audience evaluation
  2. Adding logic to parse Integrations from datafile and make odp host and key available in projectConfig.

Test plan

  • Manually tested thoroughly.
  • Added New unit tests.
  • Existing unit tests pass.
  • Existing Full stack compatibility tests pass.

JIRA

OASIS-8382

@mnoman09 mnoman09 mentioned this pull request Jun 16, 2022
Closed
@mnoman09 mnoman09 marked this pull request as ready for review June 27, 2022 06:12
@mnoman09 mnoman09 requested a review from a team as a code owner June 27, 2022 06:12
@mnoman09 mnoman09 requested a review from jaeopt June 30, 2022 01:30
@mnoman09 mnoman09 removed their assignment Jun 30, 2022
@zashraf1985
Copy link
Contributor

@jaeopt Can you take a look at this PR whenever you have time

jaeopt
jaeopt requested changes Jul 14, 2022
View reviewed changes
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. A couple of small fix suggestions.

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/match/MatchRegistry.java Outdated
@@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix copyright year.

jaeopt
jaeopt requested changes Jul 19, 2022
View reviewed changes
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! 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

.github/workflows/java.yml
@@ -45,7 +45,7 @@ jobs:

test:
if: startsWith(github.ref, 'refs/tags/') != true
runs-on: ubuntu-18.04
runs-on: macos-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

why macos?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

core-api/src/main/java/com/optimizely/ab/config/Integration.java
return key;
}

public String getHost() { return host; }
Copy link
Contributor

Choose a reason for hiding this comment

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

@nullable

core-api/src/main/java/com/optimizely/ab/config/Integration.java

public String getHost() { return host; }

public String getPublicKey() { return publicKey; }
Copy link
Contributor

Choose a reason for hiding this comment

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

@nullable

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is nullable in java unless specified the other way round?

core-api/src/main/java/com/optimizely/ab/config/Integration.java
this.publicKey = publicKey;
}

public String getKey() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@nonnull

core-api/src/main/java/com/optimizely/ab/config/Integration.java Outdated
return "Integration{" +
"key='" + key + '\'' +
", host='" + host + '\'' +
", publicKey='" + publicKey + '\'' +
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 make this toString for generic integration types, not only for ODP?
Like "if (host != null) { string += host }..."

jaeopt
jaeopt requested changes Jul 20, 2022
View reviewed changes
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.

All changes look good except that I do not see integration tests. Missing?

@zashraf1985 zashraf1985 dismissed their stale review July 21, 2022 03:27

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 :)

jaeopt
jaeopt requested changes Jul 21, 2022
View reviewed changes
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. One small change suggested.

core-api/src/test/java/com/optimizely/ab/config/parser/GsonConfigParserTest.java
GsonConfigParser parser = new GsonConfigParser();
String integrationsObject = ", \"integrations\": [" +
"{ \"key\": \"not-odp\", " +
"\"host\": \"https://example.com\", " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"\"host\": \"https://example.com\", " +
"\"any-key\": \"any-value\", " +

Let's check if it can tolerate future fields. Same for other parsers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

jaeopt
jaeopt approved these changes Jul 21, 2022
View reviewed changes
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

@zashraf1985 zashraf1985 merged commit 6b9b965 into master Jul 21, 2022
@zashraf1985 zashraf1985 deleted the gene/ats branch July 21, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaeopt jaeopt jaeopt approved these changes

@zashraf1985 zashraf1985 Awaiting requested review from zashraf1985

@mikechu-optimizely mikechu-optimizely Awaiting requested review from mikechu-optimizely

Assignees

@mnoman09 mnoman09

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mnoman09 @zashraf1985 @jaeopt @mikechu-optimizely @optimizely-codereview-bot @genemitelmanopti @NomanShoaib @msohailhussain