(feat): Feature Management getFeatureVariable Listener #270

mnoman09 · 2019-03-08T13:41:35Z

Summary

Introduction of OnDecision Notification Listener.
Returning default feature variable value if featureEnabled is false.
Implemented onDecisionListener in:
get_feature_variable_string
get_feature_variable_boolean
get_feature_variable_integer
get_feature_variable_double

coveralls · 2019-03-08T14:35:28Z

Pull Request Test Coverage Report for Build 903

61 of 64 (95.31%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 89.219%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/notification/DecisionInfoEnums.java	13	14	92.86%
core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java	17	19	89.47%

Totals
Change from base Build 892:	0.2%
Covered Lines:	2706
Relevant Lines:	3033

💛 - Coveralls

coveralls · 2019-03-08T14:35:28Z

Pull Request Test Coverage Report for Build 976

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

Totals
Change from base Build 975:	0.2%
Covered Lines:	2846
Relevant Lines:	3177

💛 - Coveralls

… info ffeature variable value

aliabbasrizvi · 2019-03-13T17:09:42Z

Returning default feature variable value if featureEnabled is false.

Isn't this already the case?

mnoman09 · 2019-03-14T13:55:18Z

Returning default feature variable value if featureEnabled is false.

Isn't this already the case?

No it was not the case already, we did that change in all sdks

aliabbasrizvi

Where are we asserting that the notification listener is being called with correct params?

aliabbasrizvi · 2019-03-19T19:20:02Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionInfoEnums.java

+
+public class DecisionInfoEnums {
+    public enum GetFeatureVariableDecisionInfo {
+        SOURCE_EXPERIMENT_KEY("source_experiment_key"),


nit. I think alphabetizing will be a good order for this.

…a-sdk into mnoman/getFeatVarListener

mikecdavis · 2019-03-20T17:01:28Z

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

        } else {
            logger.info("User \"{}\" was not bucketed into any variation for feature flag \"{}\". " +
                    "The default value \"{}\" for \"{}\" is being returned.",
                userId, featureKey, variableValue, variableKey
            );
        }

+        Object convertedValue = convertStringToType(variableValue, variableType);


This conversion is now happening twice per api call. Once here and once in the originating method.

mikecdavis · 2019-03-20T17:10:03Z

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

        } else {
            logger.info("User \"{}\" was not bucketed into any variation for feature flag \"{}\". " +
                    "The default value \"{}\" for \"{}\" is being returned.",
                userId, featureKey, variableValue, variableKey
            );
        }

+        Object convertedValue = convertStringToType(variableValue, variableType);
+        Map<String, Object> decisionInfo = new HashMap<>();
+        decisionInfo.put(DecisionInfoEnums.FeatureVariableDecisionInfo.FEATURE_KEY.toString(), featureKey);


Not a valid use of Enums. What you're doing here just requires constants or can even be a builder.
Via constants this would reduce to:

decisionInfo.put(DECISION_FEATURE_KEY, featureKey); decisionInfo.put(DECISION_FEATURE_ENABLED, featureEnabled); ...

Via a builder it would be:

Map<String, Object> decisionInfo = DecisionInfoMap.builder() .withFeatureKey(featureKey) .withFeatureEnabled(featureEnabled) ... .build();

I personally like the builder approach since within the builder you'd abstract some of the conditional logic below.
Actually, in the context of the notification listener the entire thing can benefit from the builder pattern which actually transforms into a fluent pattern if you in corporate the notification itself.

DecisionNotification.builder() .withUserId(userId) .withAttributes(attributes) .withFeatureKey(featureKey) .withFeatureEnabled(featureEnabled) ... .send();

A much cleaner interface :)

mikecdavis · 2019-03-20T17:11:01Z

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

        return variableValue;
    }

+    // Helper method which takes type and variable value and convert it to object to use in Listener DecisionInfo object variable value
+    private Object convertStringToType(String variableValue, FeatureVariable.VariableType type) {


Refactor so we're not doing String to Type conversion multiple times and in multiple places.

mikecdavis · 2019-03-20T17:11:35Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionInfoEnums.java

+
+package com.optimizely.ab.notification;
+
+public class DecisionInfoEnums {


Remove Enum class in favor of something more appropriate. See previous comment about constants vs builder vs fluent approaches.

mikecdavis · 2019-03-20T17:14:52Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListener.java

+    }
+
+    @Override
+    public abstract void onDecision(@Nonnull String type,


Would require a larger refactor, but we can generalize much better if we used actual notification Objects as opposed to positional arguments which as we can see in this example can lead to bugs since one can easily swap type for userId and attributes for decisionInfo. In this example if we had a DecisionNotification object with members type, userId etc, then everything becomes much more explicit.

…t we dont have to parse twice

aliabbasrizvi

Minor feedback. Looks good otherwise.

aliabbasrizvi · 2019-04-09T00:48:52Z

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

+            return variableValue;
+        } catch (Exception exception) {
+            logger.error("BooleanTypeException while trying to parse \"" + variableValue +
+                "\" as Boolean. " + exception);
        }
        return null;


You can just return variableValue here

aliabbasrizvi · 2019-04-09T00:49:16Z

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

+                attributes,
+                FeatureVariable.VariableType.DOUBLE
+            );
+            if (variableValue != null) {


This check seems redundant. You can just change last line to be return variableValue

… per new design (#280) * Changed DecisionSource experiment to feature-test and added source_info * Updated feature_variable to feature-variable * Update core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java Co-Authored-By: mnoman09 <Muhammadnoman@folio3.com>

aliabbasrizvi

Looks good. Lets make the change we talked about in a follow up.

aliabbasrizvi

Going to merge it in its current form. We will have to follow up with changes proposed in the different PR.

Synced offline with Mike about going ahead for now.

loganlinn · 2019-04-23T16:19:04Z

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

+            return variableValue;
+        } catch (Exception exception) {
+            logger.error("NumberFormatException while trying to parse \"" + variableValue +
+                "\" as Double. " + exception);


@mnoman09 this and a other log statements in this PR append the exception as a string on log message. the logger should be passed the exception object as a last argument

mnoman09 added 2 commits March 8, 2019 18:16

Added Listener in getFeatureVariable

e6f6dd2

fixed findBugs Issue

11e1d01

mnoman09 mentioned this pull request Mar 8, 2019

(feat): Feature Management notification listener #269

Closed

aliabbasrizvi requested a review from a team March 11, 2019 17:22

mnoman09 added 3 commits March 12, 2019 12:29

changed OnDecision enum to Decision

708c6fe

Sending converted value to from string to object in listener decision…

c6bf1d8

… info ffeature variable value

sending empty map in case when null is passed in attributes listener

4aa8c44

mnoman09 and others added 2 commits March 14, 2019 19:25

did naming changes

017b0d2

Merge branch 'master' into mnoman/getFeatVarListener

3c20d8e

aliabbasrizvi reviewed Mar 19, 2019

View reviewed changes

mnoman09 added 2 commits March 20, 2019 17:22

Nit fixes and checking if listener is getting called

ff5b878

Merge branch 'mnoman/getFeatVarListener' of github.com:optimizely/jav…

4fa7a37

…a-sdk into mnoman/getFeatVarListener

mikecdavis previously requested changes Mar 20, 2019

View reviewed changes

This was referenced Mar 20, 2019

(feat): Feature Management isFeatureEnabled Listener #271

Merged

(feat): Feature Management Activate and GetVariation Listener #272

Merged

mnoman09 and others added 4 commits March 21, 2019 17:46

Updated getFeatureVariableValueForType to return casted object so tha…

3dde5da

…t we dont have to parse twice

Refact: Added builder pattern in decision listener (#275)

158bee4

Merge branch 'master' into mnoman/getFeatVarListener

9486ab1

Fixed Find bugs errors

52cd72a

mnoman09 requested review from mikecdavis and aliabbasrizvi April 3, 2019 05:59

aliabbasrizvi and others added 3 commits April 4, 2019 17:03

Merge branch 'master' into mnoman/getFeatVarListener

21f24a2

fixed testcase

d728c85

Merge branch 'master' into mnoman/getFeatVarListener

c31522d

aliabbasrizvi reviewed Apr 9, 2019

View reviewed changes

aliabbasrizvi approved these changes Apr 9, 2019

View reviewed changes

mnoman09 and others added 3 commits April 11, 2019 11:04

Merge branch 'master' into mnoman/getFeatVarListener

fc19aff

Conflicts with master resolved

c34c3e7

Nit fixes and remove extra check

8a8b6eb

mnoman09 requested a review from aliabbasrizvi April 18, 2019 14:25

mnoman09 and others added 2 commits April 19, 2019 12:48

Updated naming convension of DecisionNotification keys

2b797fb

aliabbasrizvi approved these changes Apr 22, 2019

View reviewed changes

aliabbasrizvi and others added 2 commits April 22, 2019 16:20

Merge branch 'master' into mnoman/getFeatVarListener

b070cc7

merged master and resolved conflicts

2eee571

mnoman09 requested a review from aliabbasrizvi April 23, 2019 15:28

aliabbasrizvi approved these changes Apr 23, 2019

View reviewed changes

aliabbasrizvi merged commit 1fc9758 into master Apr 23, 2019

aliabbasrizvi deleted the mnoman/getFeatVarListener branch April 23, 2019 16:09

loganlinn reviewed Apr 23, 2019

View reviewed changes


		package com.optimizely.ab.notification;

		public class DecisionInfoEnums {

(feat): Feature Management getFeatureVariable Listener #270

(feat): Feature Management getFeatureVariable Listener #270

Uh oh!

Conversation

mnoman09 commented Mar 8, 2019

Summary

Uh oh!

coveralls commented Mar 8, 2019

Pull Request Test Coverage Report for Build 903

💛 - Coveralls

Uh oh!

coveralls commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 976

💛 - Coveralls

Uh oh!

aliabbasrizvi commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnoman09 commented Mar 14, 2019

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!

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!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

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 Mar 8, 2019 •

edited

Loading

aliabbasrizvi commented Mar 13, 2019 •

edited

Loading