Skip to content

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

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
Merged
merged 13 commits into from
Apr 18, 2019
Merged

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

merged 13 commits into from
Apr 18, 2019

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Mar 8, 2019

Summary

  • Introduction of OnDecision Notification Listener.
  • Marked Activate Listener as deprecated.
  • Implemented onDecisionListener in:
  • get_variation
  • activate

##Ticket:
OASIS-4228

@mnoman09
Added New Listener of activate in OnDecision and depreciated old one
1f8838d 
added unit tests for getVariation listener and activate listeners
@coveralls
Copy link

Pull Request Test Coverage Report for Build 902

  • 47 of 50 (94.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 89.224%
Changes Missing Coverage Covered Lines Changed/Added Lines %
core-api/src/main/java/com/optimizely/ab/notification/DecisionInfoEnums.java 7 8 87.5%
core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java 17 19 89.47%
Totals Coverage Status
Change from base Build 892: 0.2%
Covered Lines: 2691
Relevant Lines: 3016
💛 - Coveralls

@coveralls
Copy link

coveralls commented Mar 8, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 961

  • 105 of 115 (91.3%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 89.236%
Changes Missing Coverage Covered Lines Changed/Added Lines %
core-api/src/main/java/com/optimizely/ab/bucketing/FeatureDecision.java 5 6 83.33%
core-api/src/main/java/com/optimizely/ab/config/ProjectConfigUtils.java 10 11 90.91%
core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java 28 30 93.33%
core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java 43 49 87.76%
Totals Coverage Status
Change from base Build 955: 0.2%
Covered Lines: 2744
Relevant Lines: 3075
💛 - Coveralls

@mnoman09 mnoman09 mentioned this pull request Mar 8, 2019
Closed
@aliabbasrizvi aliabbasrizvi requested a review from a team March 11, 2019 17:22
aliabbasrizvi
aliabbasrizvi suggested changes Mar 13, 2019
View reviewed changes
Copy link
Contributor

@aliabbasrizvi aliabbasrizvi 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 for most part. Name changes needed.

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

public class DecisionInfoEnums {
public enum ActivateVariationDecisionInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be called ExperimentDecisionInfo.

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java Outdated
@@ -33,13 +33,32 @@
* more flexible.
*/
public class NotificationCenter {
public enum OnDecisionNotificationType {
IS_FEATURE_ENABLED("feature"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be DecisionNotificationType and suggest changing:
IS_FEATURE_ENABLED --> FEATURE
GET_FEATURE_VARIABLE --> FEATURE_VARIABLE

...-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListenerInterface.java Outdated
* @param type - The notification type.
* @param userId - The userId passed into activate.
* @param attributes - The filtered attribute list passed into activate
* @param decisionInfo - The decision Information containing all parameters passed in api.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit information and API.

@mnoman09 mnoman09 requested a review from aliabbasrizvi March 15, 2019 07:23
aliabbasrizvi
aliabbasrizvi suggested changes Mar 19, 2019
View reviewed changes
Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Minor feedback. Please take a look.

...-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListenerInterface.java Outdated
* onDecision called when an activate was triggered
*
* @param type - The notification type.
* @param userId - The userId passed into activate.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. userId passed to the API.

...-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListenerInterface.java Outdated
*
* @param type - The notification type.
* @param userId - The userId passed into activate.
* @param attributes - The filtered attribute list passed into activate
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. attribute map passed to the API

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java
@@ -110,7 +150,7 @@ public void onActivate(@Nonnull Experiment experiment, @Nonnull String userId, @
*/
public int addTrackNotificationListener(final TrackNotificationListenerInterface trackNotificationListenerInterface) {
if (trackNotificationListenerInterface instanceof TrackNotificationListener) {
return addNotificationListener(NotificationType.Activate, (NotificationListener) trackNotificationListenerInterface);
return addNotificationListener(NotificationType.Track, (NotificationListener) trackNotificationListenerInterface);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. We will have to fix this in a patch version in the meanwhile I think.

Thoughts @thomaszurkan-optimizely ?

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java Outdated
@@ -42,6 +42,8 @@
import com.optimizely.ab.notification.ActivateNotificationListener;
import com.optimizely.ab.notification.NotificationCenter;
import com.optimizely.ab.notification.TrackNotificationListener;
import com.optimizely.ab.notification.DecisionNotificationListener;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. Please alphabetize.

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java Outdated
// activate the experiment
Variation actualVariation = optimizely.activate(activatedExperiment.getKey(), userId, null);

// verify that the bucketing algorithm was called correctly
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems irrelevant / incorrect.

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

assertNull(actualVariation);

assertTrue(optimizely.notificationCenter.removeNotificationListener(notificationId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check that notification listener was called?

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

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

assertTrue(optimizely.notificationCenter.removeNotificationListener(notificationId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check notification listener was called?

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java
Variation actualVariation = optimizely.getVariation(experiment.getKey(), userId, testUserAttributes);

verify(mockBucketer).bucket(experiment, bucketingId);

assertThat(actualVariation, is(bucketedVariation));
assertTrue(optimizely.notificationCenter.removeNotificationListener(notificationId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the check for listener being called?

mikecdavis
mikecdavis previously requested changes Mar 20, 2019
View reviewed changes
core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java
@@ -2636,6 +2638,153 @@ public void getVariationWithInvalidDatafile() throws Exception {

//======== Notification listeners ========//

Boolean isListenerCalled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment in #271 as to how to properly assert something like this.

core-api/src/main/java/com/optimizely/ab/Optimizely.java Outdated
Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes);

Map<String, Object> decisionInfo = new HashMap<>();
decisionInfo.put(DecisionInfoEnums.ExperimentDecisionInfo.EXPERIMENT_KEY.toString(), experiment.getKey());
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment in #270 on how to make this cleaner.

aliabbasrizvi
aliabbasrizvi suggested changes Apr 4, 2019
View reviewed changes
core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java
return this;
}

public ExperimentDecisionNotificationBuilder withVariation(Variation variation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this should be withVariationKey. Just seems inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I am passing variation here so that on build we can verify null check before getting key from its object

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java
if (decisionNotificationListener != null) {
for (NotificationHolder holder : decisionListenerHolder) {
if (holder.decisionNotificationListener == decisionNotificationListener) {
// TODO: 3/27/2019 change log level from warn to info and to return existing listener ID
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this relevant still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we need to fix it in separate PR as Mike davis suggested

@aliabbasrizvi aliabbasrizvi merged commit ff89bcf into master Apr 18, 2019
@aliabbasrizvi aliabbasrizvi deleted the mnoman/ActivateVarNewListener branch April 18, 2019 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaszurkan-optimizely thomaszurkan-optimizely Awaiting requested review from thomaszurkan-optimizely

@loganlinn loganlinn Awaiting requested review from loganlinn

@mikecdavis mikecdavis Awaiting requested review from mikecdavis

1 more reviewer

@aliabbasrizvi aliabbasrizvi aliabbasrizvi approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mnoman09 @coveralls @mikecdavis @aliabbasrizvi @mfahadahmed