(feat): Feature Management isFeatureEnabled Listener #271

mnoman09 · 2019-03-08T13:44:58Z

Summary

Introduction of OnDecision Notification Listener.
Returning default feature variable value if featureEnabled is false.
Implemented onDecisionListener and added unit tests in:
is_feature_enabled
get_enabled_features

coveralls · 2019-03-08T13:57:42Z

Pull Request Test Coverage Report for Build 901

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

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/notification/DecisionInfoEnums.java	10	11	90.91%
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:	2701
Relevant Lines:	3028

💛 - Coveralls

coveralls · 2019-03-08T13:57:42Z

Pull Request Test Coverage Report for Build 974

47 of 47 (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.388%

Totals
Change from base Build 972:	0.2%
Covered Lines:	2788
Relevant Lines:	3119

💛 - Coveralls

…sion listener

aliabbasrizvi

Please point me to where confirmation is happening that notification listener is being called with correct parameters

aliabbasrizvi · 2019-03-19T18:02:51Z

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

@@ -24,7 +24,6 @@
 import javax.annotation.Nonnull;
 import java.util.Map;

-


Seems like an unnecessary change.

aliabbasrizvi · 2019-03-19T18:03:24Z

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

+            return key;
+        }
+    }
+


nit. remove extraneous line

aliabbasrizvi · 2019-03-19T18:04:37Z

...-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListenerInterface.java

+     * onDecision called when an activate was triggered
+     *
+     * @param type         - The notification type.
+     * @param userId       - The userId passed into activate.


Please update the docstrings here as mentioned in the other PR.

aliabbasrizvi · 2019-03-19T18:08:44Z

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java

+
+        verify(mockEventHandler, times(2)).dispatchEvent(any(LogEvent.class));
+
+        assertTrue(optimizely.notificationCenter.removeNotificationListener(notificationId));


Where is the check for notification listener being called with correct params happening?

mnoman09 · 2019-03-20T06:51:44Z

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

+     */
+    @Override
+    public final void notify(Object... args) {
+        assert (args[0] instanceof String);


@aliabbasrizvi here we added multiple assert checks which will throw exception if invalid arguments are passed and it will make sure that correct arguments are passed. so I think we dont need to add this in unit tests

We should have type specific checks i.e. for type: experiment we should check decisionInfo has correct experiment and variation key. Similarly for type: feature we should check decisionInfo has correct data.

Can you point me to where such assertions are happening? If not, then we have incomplete tests.

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

...-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListenerInterface.java

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java

…fo (#281)

…Listnr # Conflicts: # core-api/src/main/java/com/optimizely/ab/Optimizely.java # core-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListener.java # core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java # core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java # core-api/src/test/java/com/optimizely/ab/notification/NotificationCenterTest.java

aliabbasrizvi · 2019-04-18T16:06:07Z

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java

+        testDecisionInfoMap.put(FEATURE_KEY, validFeatureKey);
+        testDecisionInfoMap.put(FEATURE_ENABLED, true);
+        testDecisionInfoMap.put(SOURCE, FeatureDecision.DecisionSource.ROLLOUT);
+            testDecisionInfoMap.put(SOURCE_INFO, new HashMap<>());


nit. fix indentation

aliabbasrizvi · 2019-04-18T16:13:05Z

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

+            .withFeatureEnabled(featureEnabled)
+            .withSource(decisionSource)
+            .withExperimentKey(sourceExperimentKey)
+            .withVariationKey(sourceVariationKey)


May be remove these 2 build options and replace with withSourceInfo option which takes SourceInfo object.

You can then define 2 implementations SourceInfo. 1 for rollout and 1 for feature test and then use a get method on them to get the source info that you build in the builder here. That may be a better way to structure this.

Are you suggesting to make methods of get source info in optimizely or in DecisionNotification.java?
By the way I think current implementation is much simpler and fully utilizing builder pattern.

I am suggesting something like:

public interface SourceInfo { HashMap<String, Object> get() } public class FeatureTestSourceInfo implements SourceInfo { } public class RolloutSourceInfo implements SourceInfo { }

This may allow us to hopefully add more SourceInfo options in the future.

Discussed offline. This will happen in a follow up.

Synced with Mike offline. Going to go ahead with this.

mnoman09 added 3 commits March 8, 2019 17:25

Added Listener In isFeatureEnabled

d5de529

Added getEnabledFeatures unitTests

bbb38ba

bug fix

d9b42d4

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 and others added 4 commits March 12, 2019 12:31

Changed OnDecision enum to Decision

f634e9f

Updated testcases and passed attributes empty instead of null in deci…

8250e95

…sion listener

Did naming changes

76f4e62

Merge branch 'master' into mnoman/isFeatEnabledNotif

c461797

aliabbasrizvi suggested changes Mar 19, 2019

View reviewed changes

mnoman09 commented Mar 20, 2019

View reviewed changes

mnoman09 added 2 commits March 20, 2019 12:01

nit changes

0c8e099

Nit fix and added check of is listener Called

0d841b6

mikecdavis previously requested changes Mar 20, 2019

View reviewed changes

mikecdavis mentioned this pull request Mar 20, 2019

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

Merged

mnoman09 and others added 2 commits April 2, 2019 18:13

Merge branch 'master' into mnoman/isFeatEnabledNotif

d3099aa

Added builder pattern and refactored code

a790882

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

Merge branch 'master' into mnoman/isFeatEnabledNotif

221cb90

aliabbasrizvi approved these changes Apr 5, 2019

View reviewed changes

mnoman09 and others added 4 commits April 8, 2019 10:35

Merge branch 'master' into mnoman/isFeatEnabledNotif

0593274

Merge branch 'master' into mnoman/isFeatEnabledNotif

8f07155

Changed DecisionSource experiment to feature-test and added source_in…

e8b4500

…fo (#281)

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

aliabbasrizvi suggested changes Apr 18, 2019

View reviewed changes

Nit fix

6f08403

Changed naming convention of decision Notification keys

887ba63

mnoman09 requested a review from aliabbasrizvi April 22, 2019 05:36

aliabbasrizvi approved these changes Apr 22, 2019

View reviewed changes

Merge branch 'master' into mnoman/isFeatEnabledNotif

d95af12

aliabbasrizvi merged commit 6198fd6 into master Apr 22, 2019

aliabbasrizvi deleted the mnoman/isFeatEnabledNotif branch April 22, 2019 23:37

		@@ -24,7 +24,6 @@
		import javax.annotation.Nonnull;
		import java.util.Map;


		verify(mockEventHandler, times(2)).dispatchEvent(any(LogEvent.class));

		assertTrue(optimizely.notificationCenter.removeNotificationListener(notificationId));

(feat): Feature Management isFeatureEnabled Listener #271

(feat): Feature Management isFeatureEnabled Listener #271

Uh oh!

Conversation

mnoman09 commented Mar 8, 2019

Summary

Uh oh!

coveralls commented Mar 8, 2019

Pull Request Test Coverage Report for Build 901

💛 - 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 974

💛 - Coveralls

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!

Uh oh!

Uh oh!

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!

Uh oh!

coveralls commented Mar 8, 2019 •

edited

Loading