refac(decision): Use entities object reference and added helpers file (#42)

Michael Ng · web-flow · commit f5acd50ba8d2 · 2019-07-19T10:50:02.000-07:00
diff --git a/.gitignore b/.gitignore
@@ -0,0 +1,2 @@
+.cover
+coverage.html
diff --git a/optimizely/client/client.go b/optimizely/client/client.go
@@ -44,8 +44,13 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit
 	}
 
 	projectConfig := o.configManager.GetConfig()
+	feature, err := projectConfig.GetFeatureByKey(featureKey)
+	if err != nil {
+		logger.Error("Error retrieving feature", err)
+		return false, err
+	}
 	featureDecisionContext := decision.FeatureDecisionContext{
-		FeatureKey:    featureKey,
+		Feature:       &feature,
 		ProjectConfig: projectConfig,
 	}
 
diff --git a/optimizely/client/client_test.go b/optimizely/client/client_test.go
@@ -68,15 +68,19 @@ func TestIsFeatureEnabled(t *testing.T) {
 		Variations: map[string]entities.Variation{"22222": testVariation},
 	}
 	testFeatureKey := "test_feature_key"
-
+	testFeature := entities.Feature{
+		ID:                 "22222",
+		Key:                testFeatureKey,
+		FeatureExperiments: []entities.Experiment{testExperiment},
+	}
 	// Test happy path
 	mockConfig := new(MockProjectConfig)
+	mockConfig.On("GetFeatureByKey", testFeatureKey).Return(testFeature, nil)
 	mockConfigManager := new(MockProjectConfigManager)
 	mockConfigManager.On("GetConfig").Return(mockConfig)
-
 	// Set up the mock decision service and its return value
 	testDecisionContext := decision.FeatureDecisionContext{
-		FeatureKey:    testFeatureKey,
+		Feature:       &testFeature,
 		ProjectConfig: mockConfig,
 	}
 
@@ -121,19 +125,14 @@ func TestIsFeatureEnabledErrorCases(t *testing.T) {
 	mockConfigManager.AssertNotCalled(t, "GetFeatureByKey")
 	mockDecisionService.AssertNotCalled(t, "GetFeatureDecision")
 
-	// Test decision serviceinvalid feature key
+	// Test invalid feature key
 	expectedError := errors.New("Invalid feature key")
 	mockConfig := new(MockProjectConfig)
+	mockConfig.On("GetFeatureByKey", testFeatureKey).Return(entities.Feature{}, expectedError)
 
 	mockConfigManager = new(MockProjectConfigManager)
 	mockConfigManager.On("GetConfig").Return(mockConfig)
-
-	testFeatureDecisionContext := decision.FeatureDecisionContext{
-		FeatureKey:    testFeatureKey,
-		ProjectConfig: mockConfig,
-	}
 	mockDecisionService = new(MockDecisionService)
-	mockDecisionService.On("GetFeatureDecision", testFeatureDecisionContext, testUserContext).Return(decision.FeatureDecision{}, expectedError)
 	client = OptimizelyClient{
 		configManager:   mockConfigManager,
 		decisionService: mockDecisionService,
@@ -145,5 +144,5 @@ func TestIsFeatureEnabledErrorCases(t *testing.T) {
 	}
 	assert.False(t, result)
 	mockConfigManager.AssertExpectations(t)
-	mockDecisionService.AssertExpectations(t)
+	mockDecisionService.AssertNotCalled(t, "GetDecision")
 }
diff --git a/optimizely/config/datafileProjectConfig/entities/entities.go b/optimizely/config/datafileProjectConfig/entities/entities.go
@@ -59,6 +59,7 @@ type Variation struct {
 	FeatureEnabled bool   `json:"featureEnabled"`
 }
 
+// Event represents an event from the Optimizely datafile
 type Event struct {
 	ID            string   `json:"id"`
 	Key           string   `json:"key"`
diff --git a/optimizely/decision/composite_experiment_service.go b/optimizely/decision/composite_experiment_service.go
@@ -42,11 +42,6 @@ func NewCompositeExperimentService() *CompositeExperimentService {
 
 // GetDecision returns a decision for the given experiment and user context
 func (s CompositeExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) {
-	_, err := decisionContext.ProjectConfig.GetExperimentByKey(decisionContext.ExperimentKey)
-	if err != nil {
-		return ExperimentDecision{}, err
-	}
-
 	for _, experimentService := range s.experimentDecisionServices {
 		decision, err := experimentService.GetDecision(decisionContext, userContext)
 		if decision.DecisionMade == true {
diff --git a/optimizely/decision/composite_experiment_service_test.go b/optimizely/decision/composite_experiment_service_test.go
@@ -22,34 +22,12 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"github.com/optimizely/go-sdk/optimizely/entities"
-	"github.com/stretchr/testify/mock"
 )
 
-type MockExperimentDecisionService struct {
-	mock.Mock
-}
-
-func (m *MockExperimentDecisionService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) {
-	args := m.Called(decisionContext, userContext)
-	return args.Get(0).(ExperimentDecision), args.Error(1)
-}
-
 func TestCompositeExperimentServiceGetDecision(t *testing.T) {
-	testExperimentKey := "test_experiment"
-	testExperiment := entities.Experiment{
-		ID:  "111111",
-		Key: testExperimentKey,
-		Variations: map[string]entities.Variation{
-			"22222": entities.Variation{
-				ID:  "22222",
-				Key: "22222",
-			},
-		},
-	}
-	mockProjectConfig := new(MockProjectConfig)
-	mockProjectConfig.On("GetExperimentByKey", testExperimentKey).Return(testExperiment, nil)
+	mockProjectConfig := new(mockProjectConfig)
 	testDecisionContext := ExperimentDecisionContext{
-		ExperimentKey: testExperimentKey,
+		Experiment:    &testExp1111,
 		ProjectConfig: mockProjectConfig,
 	}
 
@@ -58,7 +36,7 @@ func TestCompositeExperimentServiceGetDecision(t *testing.T) {
 	}
 
 	expectedExperimentDecision := ExperimentDecision{
-		Variation: testExperiment.Variations["22222"],
+		Variation: testExp1111.Variations["2222"],
 		Decision: Decision{
 			DecisionMade: true,
 		},
@@ -92,7 +70,7 @@ func TestCompositeExperimentServiceGetDecision(t *testing.T) {
 
 	mockExperimentDecisionService2 = new(MockExperimentDecisionService)
 	expectedExperimentDecision2 := ExperimentDecision{
-		Variation: testExperiment.Variations["22222"],
+		Variation: testExp1111.Variations["2222"],
 		Decision: Decision{
 			DecisionMade: true,
 		},
diff --git a/optimizely/decision/composite_feature_service.go b/optimizely/decision/composite_feature_service.go
@@ -39,16 +39,13 @@ func NewCompositeFeatureService(experimentDecisionService ExperimentDecisionServ
 // GetDecision returns a decision for the given feature and user context
 func (featureService CompositeFeatureService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext) (FeatureDecision, error) {
 	featureDecision := FeatureDecision{}
-	feature, err := decisionContext.ProjectConfig.GetFeatureByKey(decisionContext.FeatureKey)
-	if err != nil {
-		return featureDecision, err
-	}
+	feature := decisionContext.Feature
 
 	// Check if user is bucketed in feature experiment
 	// @TODO: add in a feature decision service that takes into account multiple experiments (via group mutex)
 	experiment := feature.FeatureExperiments[0]
 	experimentDecisionContext := ExperimentDecisionContext{
-		ExperimentKey: experiment.Key,
+		Experiment:    &experiment,
 		ProjectConfig: decisionContext.ProjectConfig,
 	}
 
diff --git a/optimizely/decision/composite_service_test.go b/optimizely/decision/composite_service_test.go
@@ -21,58 +21,14 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
-	"github.com/stretchr/testify/mock"
-
-	"github.com/optimizely/go-sdk/optimizely"
 	"github.com/optimizely/go-sdk/optimizely/entities"
 )
 
-type MockProjectConfig struct {
-	optimizely.ProjectConfig
-	mock.Mock
-}
-
-func (c *MockProjectConfig) GetFeatureByKey(featureKey string) (entities.Feature, error) {
-	args := c.Called(featureKey)
-	return args.Get(0).(entities.Feature), args.Error(1)
-}
-
-func (c *MockProjectConfig) GetExperimentByKey(experimentKey string) (entities.Experiment, error) {
-	args := c.Called(experimentKey)
-	return args.Get(0).(entities.Experiment), args.Error(1)
-}
-
-func (c *MockProjectConfig) GetAudienceByID(audienceID string) (entities.Audience, error) {
-	args := c.Called(audienceID)
-	return args.Get(0).(entities.Audience), args.Error(1)
-}
-
-type MockFeatureDecisionService struct {
-	mock.Mock
-}
-
-func (m *MockFeatureDecisionService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext) (FeatureDecision, error) {
-	args := m.Called(decisionContext, userContext)
-	return args.Get(0).(FeatureDecision), args.Error(1)
-}
-
 func TestGetFeatureDecision(t *testing.T) {
-	testFeatureKey := "my_test_feature"
-	testVariation := entities.Variation{
-		ID:             "11111",
-		FeatureEnabled: true,
-	}
-	testExperiment := entities.Experiment{
-		Variations: map[string]entities.Variation{"11111": testVariation},
-	}
-	testFeature := entities.Feature{
-		Key:                testFeatureKey,
-		FeatureExperiments: []entities.Experiment{testExperiment},
-	}
-	mockProjectConfig := new(MockProjectConfig)
-	mockProjectConfig.On("GetFeatureByKey", testFeatureKey).Return(testFeature, nil)
+	mockProjectConfig := new(mockProjectConfig)
+	mockProjectConfig.On("GetFeatureByKey", testFeat3333Key).Return(testFeat3333, nil)
 	decisionContext := FeatureDecisionContext{
-		FeatureKey:    testFeatureKey,
+		Feature:       &testFeat3333,
 		ProjectConfig: mockProjectConfig,
 	}
 
@@ -81,8 +37,8 @@ func TestGetFeatureDecision(t *testing.T) {
 	}
 
 	expectedFeatureDecision := FeatureDecision{
-		Experiment: testExperiment,
-		Variation:  testVariation,
+		Experiment: testExp1111,
+		Variation:  testExp1111Var2222,
 		Decision:   Decision{DecisionMade: true},
 	}
 
diff --git a/optimizely/decision/entities.go b/optimizely/decision/entities.go
@@ -23,13 +23,13 @@ import (
 
 // ExperimentDecisionContext contains the information needed to be able to make a decision for a given experiment
 type ExperimentDecisionContext struct {
-	ExperimentKey string
+	Experiment    *entities.Experiment
 	ProjectConfig optimizely.ProjectConfig
 }
 
 // FeatureDecisionContext contains the information needed to be able to make a decision for a given feature
 type FeatureDecisionContext struct {
-	FeatureKey    string
+	Feature       *entities.Feature
 	ProjectConfig optimizely.ProjectConfig
 }
 
diff --git a/optimizely/decision/experiment_bucketer_service.go b/optimizely/decision/experiment_bucketer_service.go
@@ -43,7 +43,7 @@ func NewExperimentBucketerService() *ExperimentBucketerService {
 // GetDecision returns the decision with the variation the user is bucketed into
 func (s ExperimentBucketerService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) {
 	experimentDecision := ExperimentDecision{}
-	experiment, _ := decisionContext.ProjectConfig.GetExperimentByKey(decisionContext.ExperimentKey)
+	experiment := decisionContext.Experiment
 	var group entities.Group
 	if experiment.GroupID != "" {
 		// @TODO: figure out what to do if group is not found
@@ -56,7 +56,7 @@ func (s ExperimentBucketerService) GetDecision(decisionContext ExperimentDecisio
 	} else {
 		bLogger.Debug(fmt.Sprintf(`Using bucketing ID: "%s"`, bucketingID))
 	}
-	variation := s.bucketer.Bucket(bucketingID, experiment, group)
+	variation := s.bucketer.Bucket(bucketingID, *experiment, group)
 	experimentDecision.DecisionMade = true
 	experimentDecision.Variation = variation
 	return experimentDecision, nil
diff --git a/optimizely/decision/experiment_bucketer_service_test.go b/optimizely/decision/experiment_bucketer_service_test.go
@@ -34,29 +34,9 @@ func (m *MockBucketer) Bucket(bucketingID string, experiment entities.Experiment
 }
 
 func TestExperimentBucketerGetDecision(t *testing.T) {
-	testVariation := entities.Variation{
-		ID:  "22222",
-		Key: "22222",
-	}
-	testExperimentKey := "test_experiment"
-	testExperiment := entities.Experiment{
-		ID:  "111111",
-		Key: testExperimentKey,
-		Variations: map[string]entities.Variation{
-			"22222": testVariation,
-		},
-		TrafficAllocation: []entities.Range{
-			entities.Range{
-				EntityID:   "22222",
-				EndOfRange: 10000,
-			},
-		},
-	}
-
-	mockProjectConfig := new(MockProjectConfig)
-	mockProjectConfig.On("GetExperimentByKey", testExperimentKey).Return(testExperiment, nil)
+	mockProjectConfig := new(mockProjectConfig)
 	testDecisionContext := ExperimentDecisionContext{
-		ExperimentKey: testExperimentKey,
+		Experiment:    &testExp1111,
 		ProjectConfig: mockProjectConfig,
 	}
 
@@ -65,13 +45,13 @@ func TestExperimentBucketerGetDecision(t *testing.T) {
 	}
 
 	expectedDecision := ExperimentDecision{
-		Variation: testVariation,
+		Variation: testExp1111Var2222,
 		Decision: Decision{
 			DecisionMade: true,
 		},
 	}
 	mockBucketer := new(MockBucketer)
-	mockBucketer.On("Bucket", testUserContext.ID, testExperiment, entities.Group{}).Return(testVariation, nil)
+	mockBucketer.On("Bucket", testUserContext.ID, testExp1111, entities.Group{}).Return(testExp1111Var2222, nil)
 
 	experimentBucketerService := ExperimentBucketerService{
 		bucketer: mockBucketer,
diff --git a/optimizely/decision/experiment_targeting_service.go b/optimizely/decision/experiment_targeting_service.go
@@ -36,7 +36,7 @@ func NewExperimentTargetingService() *ExperimentTargetingService {
 // GetDecision applies audience targeting from the given experiment to the given user. The only decision it makes is whether to exclude the user from the experiment.
 func (s ExperimentTargetingService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) {
 	experimentDecision := ExperimentDecision{}
-	experiment, _ := decisionContext.ProjectConfig.GetExperimentByKey(decisionContext.ExperimentKey)
+	experiment := decisionContext.Experiment
 
 	if experiment.AudienceConditionTree != nil {
 
diff --git a/optimizely/decision/experiment_targeting_service_test.go b/optimizely/decision/experiment_targeting_service_test.go
@@ -38,6 +38,7 @@ func (m *MockAudienceEvaluator) Evaluate(audience entities.Audience, condTreePar
 // test with mocking
 func TestExperimentTargetingGetDecisionNoAudienceCondTree(t *testing.T) {
 	testAudience := entities.Audience{
+		ID: "33333",
 		ConditionTree: &entities.TreeNode{
 			Operator: "or",
 			Nodes: []*entities.TreeNode{
@@ -50,17 +51,14 @@ func TestExperimentTargetingGetDecisionNoAudienceCondTree(t *testing.T) {
 			},
 		},
 	}
-	mockProjectConfig := new(MockProjectConfig)
 
-	testExperimentKey := "test_experiment"
-	testExperiment := entities.Experiment{
-		ID:          "111111",
-		AudienceIds: []string{"33333"},
-	}
+	// make a copy of the testExp1111 so we do not mutate the original one
+	var testExperiment = testExp1111
+	testExperiment.AudienceIds = []string{"33333"}
+	mockProjectConfig := new(mockProjectConfig)
 	mockProjectConfig.On("GetAudienceByID", "33333").Return(testAudience, nil)
-	mockProjectConfig.On("GetExperimentByKey", testExperimentKey).Return(testExperiment, nil)
 	testDecisionContext := ExperimentDecisionContext{
-		ExperimentKey: testExperimentKey,
+		Experiment:    &testExperiment,
 		ProjectConfig: mockProjectConfig,
 	}
 
@@ -135,14 +133,14 @@ func TestExperimentTargetingGetDecisionWithAudienceCondTree(t *testing.T) {
 	testExperimentKey := "test_experiment"
 	testExperiment := entities.Experiment{
 		ID:          "111111",
+		Key:         testExperimentKey,
 		AudienceIds: []string{"33333"},
 	}
 
-	mockProjectConfig := new(MockProjectConfig)
+	mockProjectConfig := new(mockProjectConfig)
 	mockProjectConfig.On("GetAudienceByID", "33333").Return(testAudience, nil)
-	mockProjectConfig.On("GetExperimentByKey", testExperimentKey).Return(testExperiment, nil)
 	testDecisionContext := ExperimentDecisionContext{
-		ExperimentKey: testExperimentKey,
+		Experiment:    &testExperiment,
 		ProjectConfig: mockProjectConfig,
 	}
 	// test does not pass audience evaluation
diff --git a/optimizely/decision/helpers_test.go b/optimizely/decision/helpers_test.go
diff --git a/optimizely/entities/user_context.go b/optimizely/entities/user_context.go
diff --git a/scripts/coverage.sh b/scripts/coverage.sh

Original file line number	Diff line number	Diff line change
`@@ -44,8 +44,13 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit`
`44`	`44`	`}`
`45`	`45`
`46`	`46`	`projectConfig := o.configManager.GetConfig()`
	`47`	`+ feature, err := projectConfig.GetFeatureByKey(featureKey)`
	`48`	`+ if err != nil {`
	`49`	`+ logger.Error("Error retrieving feature", err)`
	`50`	`+ return false, err`
	`51`	`+ }`
`47`	`52`	`featureDecisionContext := decision.FeatureDecisionContext{`
`48`		`- FeatureKey: featureKey,`
	`53`	`+ Feature: &feature,`
`49`	`54`	`ProjectConfig: projectConfig,`
`50`	`55`	`}`
`51`	`56`
Original file line number	Diff line number	Diff line change
`@@ -59,6 +59,7 @@ type Variation struct {`
`59`	`59`	FeatureEnabled bool `json:"featureEnabled"`
`60`	`60`	`}`
`61`	`61`
	`62`	`+// Event represents an event from the Optimizely datafile`
`62`	`63`	`type Event struct {`
`63`	`64`	ID string `json:"id"`
`64`	`65`	Key string `json:"key"`
Original file line number	Diff line number	Diff line change
`@@ -23,13 +23,13 @@ import (`
`23`	`23`
`24`	`24`	`// ExperimentDecisionContext contains the information needed to be able to make a decision for a given experiment`
`25`	`25`	`type ExperimentDecisionContext struct {`
`26`		`- ExperimentKey string`
	`26`	`+ Experiment *entities.Experiment`
`27`	`27`	`ProjectConfig optimizely.ProjectConfig`
`28`	`28`	`}`
`29`	`29`
`30`	`30`	`// FeatureDecisionContext contains the information needed to be able to make a decision for a given feature`
`31`	`31`	`type FeatureDecisionContext struct {`
`32`		`- FeatureKey string`
	`32`	`+ Feature *entities.Feature`
`33`	`33`	`ProjectConfig optimizely.ProjectConfig`
`34`	`34`	`}`
`35`	`35`