Added BotFiltering #184

mnoman09 · 2018-06-05T08:00:38Z

Added BotFiltering
Added unit tests

* Updated and added botfiltering * Added BotFilteringAttribute attribute to fix existing unit tests * Added enum of reserved attributes

using startsWith instead of indexof in project config

coveralls · 2018-06-05T12:35:03Z

Pull Request Test Coverage Report for Build 528

53 of 53 (100.0%) changed or added relevant lines in 10 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 89.045%

Totals
Change from base Build 523:	0.2%
Covered Lines:	2219
Relevant Lines:	2492

💛 - Coveralls

aliabbasrizvi · 2018-06-07T05:33:28Z

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

@@ -140,6 +145,7 @@ public ProjectConfig(String accountId, String projectId, String version, String
    // v4 constructor
    public ProjectConfig(String accountId,
                         boolean anonymizeIP,
+                         Boolean botFiltering,


Why is this Boolean and not boolean like anonymizeIP or vice versa?

In Java, "Boolean" is nullable and "boolean" is not. we need botfiltering variable to be nullable so that if botfiltering is not in datafile than we assign null value to it and we won't add botfiltering attribute in attributesList.

The interesting thing about what you are saying here is that anonymizeIP is also optional based on datafile type (for instance v2 does not contain anonymizeIP).

Yeah precisely. Shouldn't anonymizeIP be Boolean then?

anonymizeIp default value is false and we are not checking in our sdk if its null it's just getting passed false if not set, but for botfiltering we have three values which are True, False and Null, if botFiltering value is null than we don't add its attribute in list.

The same holds true for anonymizeIP as it can be null in an older datafile.

aliabbasrizvi · 2018-06-07T05:34:13Z

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

@@ -285,6 +292,35 @@ public Experiment getExperimentForKey(@Nonnull String experimentKey,
        return groupExperiments;
    }

+    /**
+     * Checks is attributeKey is reserved or not and is it exist in attributeKeyMapping


nit. and ...if it exists.. and not ...is it exist...

aliabbasrizvi · 2018-06-07T05:37:35Z

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

+ */
+package com.optimizely.ab.internal;
+
+public enum ReservedAttributeKey {


Perhaps call it ControlAttribute like other SDKs.

* Nit changes

thomaszurkan-optimizely

Can we also make sure it passes compatibility tests?

thomaszurkan-optimizely · 2018-06-11T19:33:19Z

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

@@ -140,6 +145,7 @@ public ProjectConfig(String accountId, String projectId, String version, String
    // v4 constructor
    public ProjectConfig(String accountId,
                         boolean anonymizeIP,
+                         Boolean botFiltering,


The interesting thing about what you are saying here is that anonymizeIP is also optional based on datafile type (for instance v2 does not contain anonymizeIP).

thomaszurkan-optimizely · 2018-06-11T19:35:25Z

build

aliabbasrizvi · 2018-06-12T00:14:39Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

@@ -765,7 +766,8 @@ public UserProfileService getUserProfileService() {
        Map<String, Attribute> attributeKeyMapping = projectConfig.getAttributeKeyMapping();
        for (Map.Entry<String, String> attribute : attributes.entrySet()) {
            if (!attributeKeyMapping.containsKey(attribute.getKey()) &&
-                    attribute.getKey() != com.optimizely.ab.bucketing.DecisionService.BUCKETING_ATTRIBUTE) {
+                    !attribute.getKey().equals(ControlAttribute.BUCKETING_ATTRIBUTE.toString()) &&


I think the idea is to allow all attributes starting with $opt_ to go through. This is being very restrictive.

Please note this will currently filter $opt_bot_filtering. As a result the functionality is broken.

aliabbasrizvi · 2018-06-12T00:17:27Z

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

@@ -140,6 +145,7 @@ public ProjectConfig(String accountId, String projectId, String version, String
    // v4 constructor
    public ProjectConfig(String accountId,
                         boolean anonymizeIP,
+                         Boolean botFiltering,


Yeah precisely. Shouldn't anonymizeIP be Boolean then?

…ibute which starts with $opt reserved key

mnoman09 · 2018-06-12T12:47:42Z

build

aliabbasrizvi

I am still seeing issues when I run the end-to-end tests on this @msohailhussain . Details in JIRA. Can you try some manual testing at your end?

aliabbasrizvi · 2018-06-12T20:58:00Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

@@ -25,6 +25,7 @@
 import com.optimizely.ab.error.ErrorHandler;
 import com.optimizely.ab.internal.ExperimentUtils;

+import com.optimizely.ab.internal.ControlAttribute;


nit. This should be on line 27 closer to other Optimizely packages

And then a new line after that.

aliabbasrizvi · 2018-06-12T20:58:53Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

@@ -749,7 +750,7 @@ public UserProfileService getUserProfileService() {
     *
     * @param projectConfig the current project config
     * @param attributes the attributes map to validate and potentially filter. The reserved key for bucketing id
-     * {@link DecisionService#BUCKETING_ATTRIBUTE} is kept.
+     * {@link DecisionService#{ProjectConfig.RESERVED_ATTRIBUTE_PREFIX} is kept.


DecisionService should not be mentioned here any more right?

aliabbasrizvi · 2018-06-12T20:59:47Z

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

@@ -73,6 +74,7 @@ public String toString() {
    private final String revision;
    private final String version;
    private final boolean anonymizeIP;


I think the point @thomaszurkan-optimizely and I were trying to make is that perhaps anonymizeIPalso needs to be Boolean and not boolean

aliabbasrizvi · 2018-06-12T21:00:20Z

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

@@ -140,6 +145,7 @@ public ProjectConfig(String accountId, String projectId, String version, String
    // v4 constructor
    public ProjectConfig(String accountId,
                         boolean anonymizeIP,
+                         Boolean botFiltering,


The same holds true for anonymizeIP as it can be null in an older datafile.

aliabbasrizvi · 2018-06-12T21:01:07Z

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

+     */
+    public String getAttributeId(ProjectConfig projectConfig, String attributeKey) {
+        String attributeIdOrKey = null;
+        if (!attributeKey.equals(ControlAttribute.BOT_FILTERING_ATTRIBUTE.toString())) {


This check is unnecessary. I know why you put it here, but it is an overkill.

aliabbasrizvi · 2018-06-12T21:04:33Z

core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderTest.java

+                com.optimizely.ab.event.internal.payload.Attribute.CUSTOM_ATTRIBUTE_TYPE,
+                "Chrome");
+
+        com.optimizely.ab.event.internal.payload.Attribute BotFilteringFeature = new com.optimizely.ab.event.internal.payload.Attribute(


I am confused by the styling here. Shouldn't this be botFilteringFeature?

aliabbasrizvi · 2018-06-12T21:15:46Z

I think I know the problem now. You need to pass in bot filtering in attributes as boolean and not as string. That is a requirement of the API.

aliabbasrizvi

LGTM.

aliabbasrizvi · 2018-06-13T21:10:42Z

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

+     */
+    public String getAttributeId(ProjectConfig projectConfig, String attributeKey) {
+        String attributeIdOrKey = null;
+        com.optimizely.ab.config.Attribute attribute = projectConfig.getAttributeKeyMapping().get(attributeKey);


Why the inline import? Why not at the head of the file?

mnoman09 and others added 2 commits June 4, 2018 17:38

Bot filtering Update

b7b4c14

* Updated and added botfiltering * Added BotFilteringAttribute attribute to fix existing unit tests * Added enum of reserved attributes

Reverts spacing changes.

9595522

using startsWith instead of indexof in project config

mnoman09 closed this Jun 5, 2018

Added Catch block for runtime exception

92f8eaa

mnoman09 reopened this Jun 5, 2018

aliabbasrizvi suggested changes Jun 7, 2018

View reviewed changes

mnoman09 and others added 2 commits June 7, 2018 14:45

* Renames ReservedAttirbuteKey class to ControlAttribute class

798af3f

* Nit changes

Merge branch 'master' into mnoman/BotFiltering

1b54a50

thomaszurkan-optimizely reviewed Jun 11, 2018

View reviewed changes

aliabbasrizvi suggested changes Jun 12, 2018

View reviewed changes

updated check in filterAttributes to generic to not remove every attr…

101bd77

…ibute which starts with $opt reserved key

aliabbasrizvi suggested changes Jun 12, 2018

View reviewed changes

Changed Attribute value to object type so that it can work with booleans

a99faa8

aliabbasrizvi approved these changes Jun 13, 2018

View reviewed changes

aliabbasrizvi merged commit 3485072 into master Jun 13, 2018

aliabbasrizvi deleted the mnoman/BotFiltering branch June 13, 2018 21:15

Added BotFiltering #184

Added BotFiltering #184

Uh oh!

Conversation

mnoman09 commented Jun 5, 2018

Uh oh!

coveralls commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 528

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnoman09 Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaszurkan-optimizely commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnoman09 commented Jun 12, 2018

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi commented Jun 12, 2018

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jun 5, 2018 •

edited

Loading

mnoman09 Jun 7, 2018 •

edited

Loading