refac(decision): Return nil variation instead of decision made property. (#79)

Author: Michael Ng

optimizely/client/client.go

@@ -81,7 +81,12 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit
 		return result, e1
 	}
 
-	result = featureDecision.Variation.FeatureEnabled
+	if featureDecision.Variation == nil {
+		result = false
+	} else {
+		result = featureDecision.Variation.FeatureEnabled
+	}
+
 	if result {
 		logger.Info(fmt.Sprintf(`Feature "%s" is enabled for user "%s".`, featureKey, userID))
 	} else {
@@ -90,7 +95,7 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit
 
 	if featureDecision.Source == decision.FeatureTest {
 		// send impression event for feature tests
-		impressionEvent := event.CreateImpressionUserEvent(projectConfig, featureDecision.Experiment, featureDecision.Variation, userContext)
+		impressionEvent := event.CreateImpressionUserEvent(projectConfig, featureDecision.Experiment, *featureDecision.Variation, userContext)
 		o.eventProcessor.ProcessEvent(impressionEvent)
 	}
 	return result, nil
@@ -243,7 +248,7 @@ func (o *OptimizelyClient) getFeatureVariable(featureKey, variableKey string, us
 	}
 
 	featureDecision, e2 := o.decisionService.GetFeatureDecision(featureDecisionContext, userContext)
-	if e2 == nil {
+	if e2 == nil && featureDecision.Variation != nil {
 		if v, ok := featureDecision.Variation.Variables[variable.ID]; ok && featureDecision.Variation.FeatureEnabled {
 			featureValue = v.Value
 		}

optimizely/client/client_test.go

@@ -219,10 +219,7 @@ func TestIsFeatureEnabled(t *testing.T) {
 
 	expectedFeatureDecision := decision.FeatureDecision{
 		Experiment: testExperiment,
-		Variation:  testVariation,
-		Decision: decision.Decision{
-			DecisionMade: true,
-		},
+		Variation:  &testVariation,
 	}
 
 	mockDecisionService := new(MockDecisionService)
@@ -354,17 +351,11 @@ func TestGetEnabledFeatures(t *testing.T) {
 
 	expectedFeatureDecisionEnabled := decision.FeatureDecision{
 		Experiment: testExperimentEnabled,
-		Variation:  testVariationEnabled,
-		Decision: decision.Decision{
-			DecisionMade: true,
-		},
+		Variation:  &testVariationEnabled,
 	}
 	expectedFeatureDecisionDisabled := decision.FeatureDecision{
 		Experiment: testExperimentDisabled,
-		Variation:  testVariationDisabled,
-		Decision: decision.Decision{
-			DecisionMade: true,
-		},
+		Variation:  &testVariationDisabled,
 	}
 
 	mockDecisionService := new(MockDecisionService)
@@ -1434,10 +1425,7 @@ func TestGetFeatureVariableErrorCases(t *testing.T) {
 func getTestFeatureDecision(experiment entities.Experiment, variation entities.Variation, decisionMade bool) decision.FeatureDecision {
 	return decision.FeatureDecision{
 		Experiment: experiment,
-		Variation:  variation,
-		Decision: decision.Decision{
-			DecisionMade: decisionMade,
-		},
+		Variation:  &variation,
 	}
 }
 

optimizely/decision/bucketer/experiment_bucketer.go

@@ -35,7 +35,7 @@ const maxTrafficValue = 10000
 
 // ExperimentBucketer is used to bucket the user into a particular entity in the experiment's traffic alloc range
 type ExperimentBucketer interface {
-	Bucket(bucketingID string, experiment entities.Experiment, group entities.Group) (entities.Variation, reasons.Reason, error)
+	Bucket(bucketingID string, experiment entities.Experiment, group entities.Group) (*entities.Variation, reasons.Reason, error)
 }
 
 // MurmurhashBucketer buckets the user using the mmh3 algorightm
@@ -51,28 +51,28 @@ func NewMurmurhashBucketer(hashSeed uint32) *MurmurhashBucketer {
 }
 
 // Bucket buckets the user into the given experiment
-func (b MurmurhashBucketer) Bucket(bucketingID string, experiment entities.Experiment, group entities.Group) (entities.Variation, reasons.Reason, error) {
+func (b MurmurhashBucketer) Bucket(bucketingID string, experiment entities.Experiment, group entities.Group) (*entities.Variation, reasons.Reason, error) {
 	if experiment.GroupID != "" && group.Policy == "random" {
 		bucketKey := bucketingID + group.ID
 		bucketedExperimentID := b.bucketToEntity(bucketKey, group.TrafficAllocation)
 		if bucketedExperimentID == "" || bucketedExperimentID != experiment.ID {
-			// User is not bucketed into an experiment in the exclusion group, return an empty variation
-			return entities.Variation{}, reasons.NotInGroup, nil
+			// User is not bucketed into an experiment in the exclusion group, return nil variation
+			return nil, reasons.NotInGroup, nil
 		}
 	}
 
 	bucketKey := bucketingID + experiment.ID
 	bucketedVariationID := b.bucketToEntity(bucketKey, experiment.TrafficAllocation)
 	if bucketedVariationID == "" {
-		// User is not bucketed into a variation in the experiment, return an empty variation
-		return entities.Variation{}, reasons.NotBucketedIntoVariation, nil
+		// User is not bucketed into a variation in the experiment, return nil variation
+		return nil, reasons.NotBucketedIntoVariation, nil
 	}
 
 	if variation, ok := experiment.Variations[bucketedVariationID]; ok {
-		return variation, reasons.BucketedIntoVariation, nil
+		return &variation, reasons.BucketedIntoVariation, nil
 	}
 
-	return entities.Variation{ID: bucketedVariationID}, reasons.BucketedVariationNotFound, nil
+	return nil, reasons.BucketedVariationNotFound, nil
 }
 
 func (b MurmurhashBucketer) bucketToEntity(bucketKey string, trafficAllocations []entities.Range) (entityID string) {

optimizely/decision/bucketer/experiment_bucketer_test.go

@@ -109,10 +109,10 @@ func TestBucketExclusionGroups(t *testing.T) {
 	bucketer := NewMurmurhashBucketer(DefaultHashSeed)
 	// ppid2 + 1886780722 (groupId) will generate bucket value of 2434 which maps to experiment 1
 	bucketedVariation, reason, _ := bucketer.Bucket("ppid2", experiment1, exclusionGroup)
-	assert.Equal(t, experiment1.Variations["22222"], bucketedVariation)
+	assert.Equal(t, experiment1.Variations["22222"], *bucketedVariation)
 	assert.Equal(t, reasons.BucketedIntoVariation, reason)
 	// since the bucket value maps to experiment 1, the user will not be bucketed for experiment 2
 	bucketedVariation, reason, _ = bucketer.Bucket("ppid2", experiment2, exclusionGroup)
-	assert.Equal(t, entities.Variation{}, bucketedVariation)
+	assert.Nil(t, bucketedVariation)
 	assert.Equal(t, reasons.NotInGroup, reason)
 }

optimizely/decision/composite_experiment_service.go

@@ -18,38 +18,51 @@
 package decision
 
 import (
+	"github.com/optimizely/go-sdk/optimizely/decision/evaluator"
+	"github.com/optimizely/go-sdk/optimizely/decision/reasons"
 	"github.com/optimizely/go-sdk/optimizely/entities"
 )
 
 // CompositeExperimentService bridges together the various experiment decision services that ship by default with the SDK
 type CompositeExperimentService struct {
-	experimentDecisionServices []ExperimentService
+	audienceTreeEvaluator evaluator.TreeEvaluator
+	experimentServices    []ExperimentService
 }
 
 // NewCompositeExperimentService creates a new instance of the CompositeExperimentService
 func NewCompositeExperimentService() *CompositeExperimentService {
 	// These decision services are applied in order:
-	// 1. Targeting
-	// 2. Bucketing
+	// 1. Bucketing
 	// @TODO(mng): Prepend forced variation and whitelisting services
-	experimentDecisionServices := []ExperimentService{
-		NewExperimentTargetingService(),
-		NewExperimentBucketerService(),
-	}
 	return &CompositeExperimentService{
-		experimentDecisionServices: experimentDecisionServices,
+		audienceTreeEvaluator: evaluator.NewMixedTreeEvaluator(),
+		experimentServices: []ExperimentService{
+			NewExperimentBucketerService(),
+		},
 	}
 }
 
 // GetDecision returns a decision for the given experiment and user context
 func (s CompositeExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) {
-	for _, experimentService := range s.experimentDecisionServices {
-		decision, err := experimentService.GetDecision(decisionContext, userContext)
-		if decision.DecisionMade {
+	experimentDecision := ExperimentDecision{}
+	experiment := decisionContext.Experiment
+
+	// Determine if user can be part of the experiment
+	if experiment.AudienceConditionTree != nil {
+		condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap())
+		evalResult := s.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams)
+		if !evalResult {
+			experimentDecision.Reason = reasons.FailedAudienceTargeting
+			return experimentDecision, nil
+		}
+	}
+
+	// User passed targeting (or the experiment is untargeted), so run through the various decision services
+	for _, experimentService := range s.experimentServices {
+		if decision, err := experimentService.GetDecision(decisionContext, userContext); decision.Variation != nil {
 			return decision, err
 		}
 	}
 
-	// zero-value for DecisionMade is false
-	return ExperimentDecision{}, nil
+	return experimentDecision, nil
 }

optimizely/decision/composite_experiment_service_test.go

@@ -35,53 +35,35 @@ func TestCompositeExperimentServiceGetDecision(t *testing.T) {
 		ID: "test_user_1",
 	}
 
+	expectedVariation := testExp1111.Variations["2222"]
 	expectedExperimentDecision := ExperimentDecision{
-		Variation: testExp1111.Variations["2222"],
-		Decision: Decision{
-			DecisionMade: true,
-		},
+		Variation: &expectedVariation,
 	}
 	// test that we return out of the decision making and the next one doesn't get called
 	mockExperimentDecisionService := new(MockExperimentDecisionService)
 	mockExperimentDecisionService.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
 
 	mockExperimentDecisionService2 := new(MockExperimentDecisionService)
 	compositeExperimentService := &CompositeExperimentService{
-		experimentDecisionServices: []ExperimentService{
-			mockExperimentDecisionService,
-			mockExperimentDecisionService2,
-		},
+		experimentServices: []ExperimentService{mockExperimentDecisionService, mockExperimentDecisionService2},
 	}
 	decision, err := compositeExperimentService.GetDecision(testDecisionContext, testUserContext)
-
-	assert.NoError(t, err)
-	assert.Equal(t, expectedExperimentDecision, decision)
 	mockExperimentDecisionService.AssertExpectations(t)
 	mockExperimentDecisionService2.AssertNotCalled(t, "GetDecision")
 
 	// test that we move on to the next decision service if no decision is made
 	mockExperimentDecisionService = new(MockExperimentDecisionService)
-	expectedExperimentDecision = ExperimentDecision{
-		Decision: Decision{
-			DecisionMade: false,
-		},
-	}
+	expectedExperimentDecision = ExperimentDecision{}
 	mockExperimentDecisionService.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
 
 	mockExperimentDecisionService2 = new(MockExperimentDecisionService)
 	expectedExperimentDecision2 := ExperimentDecision{
-		Variation: testExp1111.Variations["2222"],
-		Decision: Decision{
-			DecisionMade: true,
-		},
+		Variation: &expectedVariation,
 	}
 	mockExperimentDecisionService2.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision2, nil)
 
 	compositeExperimentService = &CompositeExperimentService{
-		experimentDecisionServices: []ExperimentService{
-			mockExperimentDecisionService,
-			mockExperimentDecisionService2,
-		},
+		experimentServices: []ExperimentService{mockExperimentDecisionService, mockExperimentDecisionService2},
 	}
 	decision, err = compositeExperimentService.GetDecision(testDecisionContext, testUserContext)
 
@@ -92,26 +74,15 @@ func TestCompositeExperimentServiceGetDecision(t *testing.T) {
 
 	// test when no decisions are made
 	mockExperimentDecisionService = new(MockExperimentDecisionService)
-	expectedExperimentDecision = ExperimentDecision{
-		Decision: Decision{
-			DecisionMade: false,
-		},
-	}
+	expectedExperimentDecision = ExperimentDecision{}
 	mockExperimentDecisionService.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
 
 	mockExperimentDecisionService2 = new(MockExperimentDecisionService)
-	expectedExperimentDecision2 = ExperimentDecision{
-		Decision: Decision{
-			DecisionMade: false,
-		},
-	}
+	expectedExperimentDecision2 = ExperimentDecision{}
 	mockExperimentDecisionService2.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision2, nil)
 
 	compositeExperimentService = &CompositeExperimentService{
-		experimentDecisionServices: []ExperimentService{
-			mockExperimentDecisionService,
-			mockExperimentDecisionService2,
-		},
+		experimentServices: []ExperimentService{mockExperimentDecisionService, mockExperimentDecisionService2},
 	}
 	decision, err = compositeExperimentService.GetDecision(testDecisionContext, testUserContext)
 
@@ -120,3 +91,50 @@ func TestCompositeExperimentServiceGetDecision(t *testing.T) {
 	mockExperimentDecisionService.AssertExpectations(t)
 	mockExperimentDecisionService2.AssertExpectations(t)
 }
+
+func TestCompositeExperimentServiceGetDecisionTargeting(t *testing.T) {
+	testUserContext := entities.UserContext{
+		ID: "test_user",
+	}
+	testAudienceMap := map[string]entities.Audience{
+		"5555": testAudience5555,
+	}
+	mockProjectConfig := new(mockProjectConfig)
+	mockProjectConfig.On("GetAudienceMap").Return(testAudienceMap)
+
+	testExperimentDecisionContext := ExperimentDecisionContext{
+		Experiment:    &testExp1112,
+		ProjectConfig: mockProjectConfig,
+	}
+	testCondTreeParams := entities.NewTreeParameters(&testUserContext, testAudienceMap)
+
+	// Test user fails targeting
+	mockAudienceTreeEvaluator := new(MockAudienceTreeEvaluator)
+	mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, testCondTreeParams).Return(false)
+	mockExperimentDecisionService := new(MockExperimentDecisionService)
+	testCompositeExperimentService := &CompositeExperimentService{
+		audienceTreeEvaluator: mockAudienceTreeEvaluator,
+		experimentServices:    []ExperimentService{mockExperimentDecisionService},
+	}
+	decision, _ := testCompositeExperimentService.GetDecision(testExperimentDecisionContext, testUserContext)
+	assert.Nil(t, decision.Variation)
+	mockAudienceTreeEvaluator.AssertExpectations(t)
+	mockExperimentDecisionService.AssertNotCalled(t, "GetDecision")
+
+	// Test user passes targeting, moves on to children decision services
+	expectedExperimentDecision := ExperimentDecision{
+		Variation: &testExp1112Var2222,
+	}
+	mockAudienceTreeEvaluator = new(MockAudienceTreeEvaluator)
+	mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, testCondTreeParams).Return(true)
+	mockExperimentDecisionService = new(MockExperimentDecisionService)
+	mockExperimentDecisionService.On("GetDecision", testExperimentDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
+	testCompositeExperimentService = &CompositeExperimentService{
+		audienceTreeEvaluator: mockAudienceTreeEvaluator,
+		experimentServices:    []ExperimentService{mockExperimentDecisionService},
+	}
+	decision, _ = testCompositeExperimentService.GetDecision(testExperimentDecisionContext, testUserContext)
+	assert.Equal(t, decision, expectedExperimentDecision)
+	mockAudienceTreeEvaluator.AssertExpectations(t)
+	mockExperimentDecisionService.AssertExpectations(t)
+}

optimizely/decision/composite_feature_service.go

@@ -54,9 +54,8 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont
 		}
 
 		experimentDecision, err := f.experimentDecisionService.GetDecision(experimentDecisionContext, userContext)
-		// If we get an empty string Variation ID it means that the user is assigned no variation, hence we
-		// move onto Rollout evaluation
-		if experimentDecision.Variation.ID != "" {
+		// Variation not nil means we got a decision and should return it
+		if experimentDecision.Variation != nil {
 			featureDecision := FeatureDecision{
 				Experiment: experiment,
 				Decision:   experimentDecision.Decision,

optimizely/decision/composite_service.go

@@ -55,7 +55,7 @@ func (s CompositeService) GetFeatureDecision(featureDecisionContext FeatureDecis
 			func() {}() // cheat linters
 		}
 
-		if featureDecision.DecisionMade {
+		if featureDecision.Variation != nil {
 			break
 		}
 	}

optimizely/decision/composite_service_test.go

@@ -37,8 +37,7 @@ func TestGetFeatureDecision(t *testing.T) {
 
 	expectedFeatureDecision := FeatureDecision{
 		Experiment: testExp1111,
-		Variation:  testExp1111Var2222,
-		Decision:   Decision{DecisionMade: true},
+		Variation:  &testExp1111Var2222,
 	}
 
 	testFeatureDecisionService := new(MockFeatureDecisionService)

optimizely/decision/entities.go

@@ -47,20 +47,19 @@ const (
 
 // Decision contains base information about a decision
 type Decision struct {
-	DecisionMade bool
-	Reason       reasons.Reason
+	Reason reasons.Reason
 }
 
 // FeatureDecision contains the decision information about a feature
 type FeatureDecision struct {
 	Decision
 	Source     Source
 	Experiment entities.Experiment
-	Variation  entities.Variation
+	Variation  *entities.Variation
 }
 
 // ExperimentDecision contains the decision information about an experiment
 type ExperimentDecision struct {
 	Decision
-	Variation entities.Variation
+	Variation *entities.Variation
 }