Audiences, Attributes and Events implementation #438

The-inside-man · 2021-07-19T18:42:39Z

Summary

Audiences Implementation
Audience Support added in OptimizelyConfigService
Modification of Experiments to support serialize functionality from Conditions
Attributes and Events added to OptimizelyConfig
Cleanup of parsing functions to reuse code
Additional test cases added
Add Deprecation for OptimizelyFeature.experimentsMap

Test plan

FSC

Issues

"OASIS-7813"

…hange lists to use OptimizelyAttribute and OptimizelyEvent. Convert Attribute and EventTypes to lists of OptimizleyAttribute and OptimizelyEvent lists before calling OptimizelyConfig constructor.

switch statement, same for conditionUtils. Audience implementation and addition of OptimizelyAudience class. Updated OptimizelyConfig with Audiences and added implementation in OptimizelyConfigService. Updated OptimizelyExperiment to get the serialized audienceConditions list from experiment. Test cases will come in additional Commits.

….equals().

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

keppel2 · 2021-07-19T22:41:10Z

if ( and if( seem to intermix.

keppel2 · 2021-07-19T22:42:05Z

Might be something that can be automatically checked. Don't let it gate a merge.

The-inside-man · 2021-07-20T13:02:28Z

if ( and if( seem to intermix.

good catch! thanks

coveralls · 2021-07-20T17:25:08Z

Pull Request Test Coverage Report for Build 1794

243 of 272 (89.34%) changed or added relevant lines in 16 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.002%) to 90.401%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/Experiment.java	54	55	98.18%
core-api/src/main/java/com/optimizely/ab/config/audience/AudienceIdCondition.java	1	2	50.0%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	9	10	90.0%
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyConfigService.java	102	103	99.03%
core-api/src/main/java/com/optimizely/ab/config/audience/EmptyCondition.java	0	2	0.0%
core-api/src/main/java/com/optimizely/ab/config/audience/NullCondition.java	0	2	0.0%
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyFeature.java	8	13	61.54%
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyConfig.java	6	12	50.0%
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyAudience.java	7	17	41.18%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyFeature.java	1	76.67%

Totals
Change from base Build 1719:	0.002%
Covered Lines:	4709
Relevant Lines:	5209

💛 - Coveralls

The-inside-man · 2021-07-20T20:26:40Z

Testing the serialize portion of the audienceConditions as part of Experiments is a bit different in this one since each condition is actually based off an interface and many of the checks pertaining to edge cases (ie. empty list, or a list of just audienceId's) are covered during the parsing of the datafile and creates either an EmptyCondition or and AudienceIdCondition, both which implement Condition, so when its time to actually serialize we can check if an empty condition or an AudienceIdCondition. If a list of AudienceId's is present in the datafile parsing then it already creates an OrCondition with a list of AudienceIdConditions, meaning we will never have a case in the Experiment that we only have a list of AudienceId's as it will always be an OrCondition object with a list of those ID's.

…imentId.

The-inside-man · 2021-08-10T16:26:19Z

3 failing scenarios shown here: https://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/234971801 pass with custom build on FSC fix branch: https://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/jobs/530428847

The-inside-man · 2021-08-11T12:39:39Z

Passing all test cases for FSC: https://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/235065614 @msohailhussain would you mind a final review/approval on this please

msohailhussain

all looks good but just have few suggestions, want to discuss before approving it.

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

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

msohailhussain · 2021-08-11T20:27:32Z

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

        return "{name='" + name + "\'" +
            ", type='" + type + "\'" +
            ", match='" + match + "\'" +
-            ", value=" + valueStr +
+            ", value=" + ((value instanceof String) ? ("'" + getValueStr() + "'") : getValueStr()) +


why single quote here?

This was part of the original logic that I didn't want to change (Line 127) but don't need for the function above and wanted to reuse the same code minus those quotes. We can chat offline if you like.

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

msohailhussain · 2021-08-11T20:30:07Z

core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyConfig.java

+        this(experimentsMap, featuresMap, revision, sdkKey, environmentKey, attributes, events, audiences, null);
+    }
+
+    public OptimizelyConfig(Map<String, OptimizelyExperiment> experimentsMap,


in case we are supposed to use it internally, can we set access modifier accordingly?

….experimentsMap.

msohailhussain

lgtm

jaeopt

One comment about deprecation notation

jaeopt · 2021-08-12T18:26:40Z

core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyFeature.java

+     * @Deprecated The experimentsMap has been deprecated. </br>
+     *              Use experimentRules and deliveryRules instead.
+     * */
+    @Deprecated()
    private Map<String, OptimizelyExperiment> experimentsMap;


This does not show deprecation note when I open the doc. Can you check? Also it use different deprecation notation. You can make it consistent (for compatibility issue for android support?).

Sure, let me take a look.

@jaeopt Updated to follow suite with Android.

core-api/src/main/java/com/optimizely/ab/internal/ConditionUtils.java

jaeopt

LGTM

The-inside-man added 4 commits June 25, 2021 09:44

[OASIS-7798] - Add support for attributes and events

ed55d43

Add copyright header to OptimizelyEventTest

ebf3a7b

Changes made to remove getAttributesMap and getEventsMap as well as c…

a5eb5c9

…hange lists to use OptimizelyAttribute and OptimizelyEvent. Convert Attribute and EventTypes to lists of OptimizleyAttribute and OptimizelyEvent lists before calling OptimizelyConfig constructor.

The-inside-man added the chore label Jul 19, 2021

The-inside-man requested a review from a team as a code owner July 19, 2021 18:42

optimizely-codereview-bot assigned The-inside-man Jul 19, 2021

The-inside-man removed the request for review from a team July 19, 2021 18:42

keppel2 self-requested a review July 19, 2021 19:32

keppel2 marked this pull request as draft July 19, 2021 19:34

The-inside-man added 3 commits July 19, 2021 15:44

Added spaces before else statement, Changed compare of strings to use…

5b83801

….equals().

Correct logic in retrieving name from audienceId

0aa4ad4

Remove uneeded import and exceptions from jsonConfigParser.

4fa26dd

The-inside-man removed the request for review from keppel2 July 19, 2021 21:03

The-inside-man added 3 commits July 19, 2021 17:03

Fix Format fo brackets.

d064f8d

Formatting

346b458

Simplify method to create audiences list and reduce time complexity.

3c71919

keppel2 reviewed Jul 19, 2021

View reviewed changes

core-api/src/main/java/com/optimizely/ab/config/Experiment.java Outdated Show resolved Hide resolved

The-inside-man added 5 commits July 20, 2021 09:28

Added null chaeck and spaces where required to follow formatting.:

5413845

Updated Null checks as per spotbugsMain

0c84679

Added more null checks to optimizelyConfigService.

6ff39a4

Update experimentsMap with null check on audiences.

83c32e1

Change Logic for empty list in DeliverRules.

725e8a7

The-inside-man added 2 commits July 20, 2021 15:45

Added testcases to check all scenarios of audience conditions.

5f0de11

Update license information.

48a1b42

The-inside-man added 2 commits August 9, 2021 16:01

Change lookup of feature experiments Map to use experimentsMapByExper…

c439a2f

…imentId.

Updated getExperimentsMapForFeature to use new id mapping.

48dda4f

The-inside-man requested a review from msohailhussain August 9, 2021 20:36

The-inside-man marked this pull request as ready for review August 10, 2021 16:43

The-inside-man removed their assignment Aug 11, 2021

optimizely-codereview-bot requested review from jaeopt and keppel2 August 11, 2021 16:34

trigger CI

efd1383

msohailhussain suggested changes Aug 11, 2021

View reviewed changes

optimizely-codereview-bot assigned The-inside-man Aug 11, 2021

The-inside-man added 4 commits August 12, 2021 08:57

Updated change requests. and deprecated warning for OptimizelyFeature…

20683c7

….experimentsMap.

Spelling correction.

d5d5383

Add warning comment about OptimizelyConfig experimentsMap.

41e8e2b

Remove deprecated line saying when will be removed.

a877930

msohailhussain approved these changes Aug 12, 2021

View reviewed changes

keppel2 approved these changes Aug 12, 2021

View reviewed changes

jaeopt requested changes Aug 12, 2021

View reviewed changes

keppel2 self-requested a review August 12, 2021 18:39

keppel2 reviewed Aug 12, 2021

View reviewed changes

core-api/src/main/java/com/optimizely/ab/internal/ConditionUtils.java Show resolved Hide resolved

Update deprecated to follow suite of Android for compatibility.

f79f199

keppel2 self-requested a review August 12, 2021 18:47

Update tests case to use proper order of assert.

04d1fb9

The-inside-man removed their assignment Aug 12, 2021

optimizely-codereview-bot requested a review from jaeopt August 12, 2021 19:32

Update deprecated format.

89722b7

jaeopt approved these changes Aug 12, 2021

View reviewed changes

The-inside-man merged commit 6ce2581 into master Aug 12, 2021

The-inside-man deleted the jbrown/oasis-7813 branch August 12, 2021 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audiences, Attributes and Events implementation #438

Audiences, Attributes and Events implementation #438

The-inside-man commented Jul 19, 2021 •

edited

Loading

keppel2 commented Jul 19, 2021

keppel2 commented Jul 19, 2021

The-inside-man commented Jul 20, 2021

coveralls commented Jul 20, 2021 •

edited

Loading

The-inside-man commented Jul 20, 2021

The-inside-man commented Aug 10, 2021 •

edited

Loading

The-inside-man commented Aug 11, 2021

msohailhussain left a comment

msohailhussain Aug 11, 2021

The-inside-man Aug 12, 2021

msohailhussain Aug 11, 2021

msohailhussain left a comment

jaeopt left a comment

jaeopt Aug 12, 2021

The-inside-man Aug 12, 2021

The-inside-man Aug 12, 2021

jaeopt left a comment

Audiences, Attributes and Events implementation #438

Audiences, Attributes and Events implementation #438

Conversation

The-inside-man commented Jul 19, 2021 • edited Loading

Summary

Test plan

Issues

keppel2 commented Jul 19, 2021

keppel2 commented Jul 19, 2021

The-inside-man commented Jul 20, 2021

coveralls commented Jul 20, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1794

💛 - Coveralls

The-inside-man commented Jul 20, 2021

The-inside-man commented Aug 10, 2021 • edited Loading

The-inside-man commented Aug 11, 2021

msohailhussain left a comment

Choose a reason for hiding this comment

msohailhussain Aug 11, 2021

Choose a reason for hiding this comment

The-inside-man Aug 12, 2021

Choose a reason for hiding this comment

msohailhussain Aug 11, 2021

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt Aug 12, 2021

Choose a reason for hiding this comment

The-inside-man Aug 12, 2021

Choose a reason for hiding this comment

The-inside-man Aug 12, 2021

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

The-inside-man commented Jul 19, 2021 •

edited

Loading

coveralls commented Jul 20, 2021 •

edited

Loading

The-inside-man commented Aug 10, 2021 •

edited

Loading