refact(decision-reasons): Refactoring reasons for decide api (#303)

Author: yasirfolio3

pkg/client/client.go

@@ -96,7 +96,9 @@ func (o *OptimizelyClient) decide(userContext OptimizelyUserContext, key string,
 	decisionReasons := decide.NewDecisionReasons(&allOptions)
 	decisionContext.Variable = entities.Variable{}
 
-	featureDecision, err := o.DecisionService.GetFeatureDecision(decisionContext, usrContext, &allOptions, decisionReasons)
+	featureDecision, reasons, err := o.DecisionService.GetFeatureDecision(decisionContext, usrContext, &allOptions)
+	decisionReasons.Append(reasons)
+
 	if err != nil {
 		o.logger.Warning(fmt.Sprintf(`Received error while making a decision for feature "%s": %s`, key, err))
 	}
@@ -116,7 +118,8 @@ func (o *OptimizelyClient) decide(userContext OptimizelyUserContext, key string,
 
 	variableMap := map[string]interface{}{}
 	if !allOptions.ExcludeVariables {
-		variableMap = o.getDecisionVariableMap(feature, featureDecision.Variation, flagEnabled, decisionReasons)
+		variableMap, reasons = o.getDecisionVariableMap(feature, featureDecision.Variation, flagEnabled)
+		decisionReasons.Append(reasons)
 	}
 	optimizelyJSON := optimizelyjson.NewOptimizelyJSONfromMap(variableMap)
 	reasonsToReport := decisionReasons.ToReport()
@@ -808,7 +811,7 @@ func (o *OptimizelyClient) getFeatureDecision(featureKey, variableKey string, us
 
 	decisionContext.Variable = variable
 	options := &decide.Options{}
-	featureDecision, err = o.DecisionService.GetFeatureDecision(decisionContext, userContext, options, decide.NewDecisionReasons(options))
+	featureDecision, _, err = o.DecisionService.GetFeatureDecision(decisionContext, userContext, options)
 	if err != nil {
 		o.logger.Warning(fmt.Sprintf(`Received error while making a decision for feature "%s": %s`, featureKey, err))
 		return decisionContext, featureDecision, nil
@@ -839,7 +842,7 @@ func (o *OptimizelyClient) getExperimentDecision(experimentKey string, userConte
 	}
 
 	options := &decide.Options{}
-	experimentDecision, err = o.DecisionService.GetExperimentDecision(decisionContext, userContext, options, decide.NewDecisionReasons(options))
+	experimentDecision, _, err = o.DecisionService.GetExperimentDecision(decisionContext, userContext, options)
 	if err != nil {
 		o.logger.Warning(fmt.Sprintf(`Received error while making a decision for experiment "%s": %s`, experimentKey, err))
 		return decisionContext, experimentDecision, nil
@@ -947,7 +950,8 @@ func (o *OptimizelyClient) Close() {
 	o.execGroup.TerminateAndWait()
 }
 
-func (o *OptimizelyClient) getDecisionVariableMap(feature entities.Feature, variation *entities.Variation, featureEnabled bool, decisionReasons decide.DecisionReasons) map[string]interface{} {
+func (o *OptimizelyClient) getDecisionVariableMap(feature entities.Feature, variation *entities.Variation, featureEnabled bool) (map[string]interface{}, decide.DecisionReasons) {
+	reasons := decide.NewDecisionReasons(nil)
 	valuesMap := map[string]interface{}{}
 
 	for _, v := range feature.VariableMap {
@@ -961,12 +965,12 @@ func (o *OptimizelyClient) getDecisionVariableMap(feature entities.Feature, vari
 
 		typedValue, typedError := o.getTypedValue(val, v.Type)
 		if typedError != nil {
-			decisionReasons.AddError(decide.GetDecideMessage(decide.VariableValueInvalid, v.Key))
+			reasons.AddError(decide.GetDecideMessage(decide.VariableValueInvalid, v.Key))
 		}
 		valuesMap[v.Key] = typedValue
 	}
 
-	return valuesMap
+	return valuesMap, reasons
 }
 
 func isNil(v interface{}) bool {

pkg/client/client_test.go

@@ -117,14 +117,14 @@ type MockDecisionService struct {
 	mock.Mock
 }
 
-func (m *MockDecisionService) GetFeatureDecision(decisionContext decision.FeatureDecisionContext, userContext entities.UserContext, options *decide.Options, reasons decide.DecisionReasons) (decision.FeatureDecision, error) {
-	args := m.Called(decisionContext, userContext, options, reasons)
-	return args.Get(0).(decision.FeatureDecision), args.Error(1)
+func (m *MockDecisionService) GetFeatureDecision(decisionContext decision.FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (decision.FeatureDecision, decide.DecisionReasons, error) {
+	args := m.Called(decisionContext, userContext, options)
+	return args.Get(0).(decision.FeatureDecision), args.Get(1).(decide.DecisionReasons), args.Error(2)
 }
 
-func (m *MockDecisionService) GetExperimentDecision(decisionContext decision.ExperimentDecisionContext, userContext entities.UserContext, options *decide.Options, reasons decide.DecisionReasons) (decision.ExperimentDecision, error) {
-	args := m.Called(decisionContext, userContext, options, reasons)
-	return args.Get(0).(decision.ExperimentDecision), args.Error(1)
+func (m *MockDecisionService) GetExperimentDecision(decisionContext decision.ExperimentDecisionContext, userContext entities.UserContext, options *decide.Options) (decision.ExperimentDecision, decide.DecisionReasons, error) {
+	args := m.Called(decisionContext, userContext, options)
+	return args.Get(0).(decision.ExperimentDecision), args.Get(1).(decide.DecisionReasons), args.Error(2)
 }
 
 func (m *MockDecisionService) OnDecision(callback func(note notification.DecisionNotification)) (int, error) {
@@ -160,11 +160,11 @@ func (m *PanickingConfigManager) GetConfig() (config.ProjectConfig, error) {
 type PanickingDecisionService struct {
 }
 
-func (m *PanickingDecisionService) GetFeatureDecision(decisionContext decision.FeatureDecisionContext, userContext entities.UserContext, options *decide.Options, reasons decide.DecisionReasons) (decision.FeatureDecision, error) {
+func (m *PanickingDecisionService) GetFeatureDecision(decisionContext decision.FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (decision.FeatureDecision, decide.DecisionReasons, error) {
 	panic("I'm panicking")
 }
 
-func (m *PanickingDecisionService) GetExperimentDecision(decisionContext decision.ExperimentDecisionContext, userContext entities.UserContext, options *decide.Options, reasons decide.DecisionReasons) (decision.ExperimentDecision, error) {
+func (m *PanickingDecisionService) GetExperimentDecision(decisionContext decision.ExperimentDecisionContext, userContext entities.UserContext, options *decide.Options) (decision.ExperimentDecision, decide.DecisionReasons, error) {
 	panic("I'm panicking")
 }
 

pkg/client/fixtures_test.go

@@ -21,5 +21,6 @@ package decide
 type DecisionReasons interface {
 	AddError(format string, arguments ...interface{})
 	AddInfo(format string, arguments ...interface{}) string
+	Append(reasons DecisionReasons)
 	ToReport() []string
 }

pkg/decide/decision_reasons.go

@@ -75,3 +75,37 @@ func TestInfoLogsAreOnlyReportedWhenIncludeReasonsOptionIsSet(t *testing.T) {
 	assert.Equal(t, "info message", reportedReasons[2])
 	assert.Equal(t, "info message: unexpected string", reportedReasons[3])
 }
+
+func TestAppend(t *testing.T) {
+	options := &Options{
+		DisableDecisionEvent:     true,
+		EnabledFlagsOnly:         true,
+		IgnoreUserProfileService: true,
+		ExcludeVariables:         true,
+		IncludeReasons:           true,
+	}
+	reasons1 := NewDecisionReasons(options)
+	reasons1.AddError("error message")
+	reasons1.AddError("error message: code %d", 121)
+	reasons1.AddInfo("info message")
+	reasons1.AddInfo("info message: %s", "unexpected string")
+
+	// Shouldn't append info logs if include reasons is set to false
+	reasons2 := NewDecisionReasons(nil)
+	reasons2.Append(reasons1)
+	reportedReasons := reasons2.ToReport()
+	assert.Equal(t, 2, len(reportedReasons))
+	assert.Equal(t, "error message", reportedReasons[0])
+	assert.Equal(t, "error message: code 121", reportedReasons[1])
+
+	// Should append info logs if include reasons is set to true
+	options.IncludeReasons = true
+	reasons2 = NewDecisionReasons(options)
+	reasons2.Append(reasons1)
+	reportedReasons = reasons2.ToReport()
+	assert.Equal(t, 4, len(reportedReasons))
+	assert.Equal(t, "error message", reportedReasons[0])
+	assert.Equal(t, "error message: code 121", reportedReasons[1])
+	assert.Equal(t, "info message", reportedReasons[2])
+	assert.Equal(t, "info message: unexpected string", reportedReasons[3])
+}

pkg/decide/decision_reasons_test.go

@@ -29,10 +29,14 @@ type DefaultDecisionReasons struct {
 
 // NewDecisionReasons returns a new instance of DecisionReasons.
 func NewDecisionReasons(options *Options) *DefaultDecisionReasons {
+	includeReasons := false
+	if options != nil {
+		includeReasons = options.IncludeReasons
+	}
 	return &DefaultDecisionReasons{
 		errors:         []string{},
 		logs:           []string{},
-		includeReasons: options.IncludeReasons,
+		includeReasons: includeReasons,
 	}
 }
 
@@ -51,6 +55,16 @@ func (o *DefaultDecisionReasons) AddInfo(format string, arguments ...interface{}
 	return message
 }
 
+// Append appends given reasons.
+func (o *DefaultDecisionReasons) Append(reasons DecisionReasons) {
+	if decisionReasons, ok := reasons.(*DefaultDecisionReasons); ok {
+		o.errors = append(o.errors, decisionReasons.errors...)
+		if o.includeReasons {
+			o.logs = append(o.logs, decisionReasons.logs...)
+		}
+	}
+}
+
 // ToReport returns reasons to be reported.
 func (o *DefaultDecisionReasons) ToReport() []string {
 	reasons := o.errors

pkg/decide/default_decision_reasons.go

@@ -83,17 +83,20 @@ func NewCompositeExperimentService(sdkKey string, options ...CESOptionFunc) *Com
 }
 
 // GetDecision returns a decision for the given experiment and user context
-func (s CompositeExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext, options *decide.Options, reasons decide.DecisionReasons) (decision ExperimentDecision, err error) {
+func (s CompositeExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext, options *decide.Options) (decision ExperimentDecision, reasons decide.DecisionReasons, err error) {
 	// Run through the various decision services until we get a decision
+	reasons = decide.NewDecisionReasons(options)
 	for _, experimentService := range s.experimentServices {
-		decision, err = experimentService.GetDecision(decisionContext, userContext, options, reasons)
+		var decisionReasons decide.DecisionReasons
+		decision, decisionReasons, err = experimentService.GetDecision(decisionContext, userContext, options)
+		reasons.Append(decisionReasons)
 		if err != nil {
 			s.logger.Debug(fmt.Sprintf("%v", err))
 		}
 		if decision.Variation != nil && err == nil {
-			return decision, err
+			return decision, reasons, err
 		}
 	}
 
-	return decision, err
+	return decision, reasons, err
 }

pkg/decision/composite_experiment_service.go

@@ -61,13 +61,13 @@ func (s *CompositeExperimentTestSuite) TestGetDecision() {
 	expectedExperimentDecision := ExperimentDecision{
 		Variation: &expectedVariation,
 	}
-	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext, s.options, s.reasons).Return(expectedExperimentDecision, nil)
+	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext, s.options).Return(expectedExperimentDecision, s.reasons, nil)
 
 	compositeExperimentService := &CompositeExperimentService{
 		experimentServices: []ExperimentService{s.mockExperimentService, s.mockExperimentService2},
 		logger:             logging.GetLogger("sdkKey", "ExperimentService"),
 	}
-	decision, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext, s.options, s.reasons)
+	decision, _, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext, s.options)
 	s.Equal(expectedExperimentDecision, decision)
 	s.NoError(err)
 	s.mockExperimentService.AssertExpectations(s.T())
@@ -83,18 +83,18 @@ func (s *CompositeExperimentTestSuite) TestGetDecisionFallthrough() {
 
 	expectedVariation := testExp1111.Variations["2222"]
 	expectedExperimentDecision := ExperimentDecision{}
-	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext, s.options, s.reasons).Return(expectedExperimentDecision, nil)
+	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext, s.options).Return(expectedExperimentDecision, s.reasons, nil)
 
 	expectedExperimentDecision2 := ExperimentDecision{
 		Variation: &expectedVariation,
 	}
-	s.mockExperimentService2.On("GetDecision", s.testDecisionContext, testUserContext, s.options, s.reasons).Return(expectedExperimentDecision2, nil)
+	s.mockExperimentService2.On("GetDecision", s.testDecisionContext, testUserContext, s.options).Return(expectedExperimentDecision2, s.reasons, nil)
 
 	compositeExperimentService := &CompositeExperimentService{
 		experimentServices: []ExperimentService{s.mockExperimentService, s.mockExperimentService2},
 		logger:             logging.GetLogger("sdkKey", "CompositeExperimentService"),
 	}
-	decision, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext, s.options, s.reasons)
+	decision, _, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext, s.options)
 
 	s.NoError(err)
 	s.Equal(expectedExperimentDecision2, decision)
@@ -108,16 +108,16 @@ func (s *CompositeExperimentTestSuite) TestGetDecisionNoDecisionsMade() {
 		ID: "test_user_1",
 	}
 	expectedExperimentDecision := ExperimentDecision{}
-	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext, s.options, s.reasons).Return(expectedExperimentDecision, nil)
+	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext, s.options).Return(expectedExperimentDecision, s.reasons, nil)
 
 	expectedExperimentDecision2 := ExperimentDecision{}
-	s.mockExperimentService2.On("GetDecision", s.testDecisionContext, testUserContext, s.options, s.reasons).Return(expectedExperimentDecision2, nil)
+	s.mockExperimentService2.On("GetDecision", s.testDecisionContext, testUserContext, s.options).Return(expectedExperimentDecision2, s.reasons, nil)
 
 	compositeExperimentService := &CompositeExperimentService{
 		experimentServices: []ExperimentService{s.mockExperimentService, s.mockExperimentService2},
 		logger:             logging.GetLogger("sdkKey", "CompositeExperimentService"),
 	}
-	decision, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext, s.options, s.reasons)
+	decision, _, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext, s.options)
 
 	s.NoError(err)
 	s.Equal(expectedExperimentDecision2, decision)
@@ -139,12 +139,12 @@ func (s *CompositeExperimentTestSuite) TestGetDecisionReturnsError() {
 	shouldBeIgnoredDecision := ExperimentDecision{
 		Variation: &testExp1114Var2225,
 	}
-	s.mockExperimentService.On("GetDecision", testDecisionContext, testUserContext, s.options, s.reasons).Return(shouldBeIgnoredDecision, errors.New("Error making decision"))
+	s.mockExperimentService.On("GetDecision", testDecisionContext, testUserContext, s.options).Return(shouldBeIgnoredDecision, s.reasons, errors.New("Error making decision"))
 
 	expectedDecision := ExperimentDecision{
 		Variation: &testExp1114Var2226,
 	}
-	s.mockExperimentService2.On("GetDecision", testDecisionContext, testUserContext, s.options, s.reasons).Return(expectedDecision, nil)
+	s.mockExperimentService2.On("GetDecision", testDecisionContext, testUserContext, s.options).Return(expectedDecision, s.reasons, nil)
 
 	compositeExperimentService := &CompositeExperimentService{
 		experimentServices: []ExperimentService{
@@ -153,7 +153,7 @@ func (s *CompositeExperimentTestSuite) TestGetDecisionReturnsError() {
 		},
 		logger: logging.GetLogger("sdkKey", "CompositeExperimentService"),
 	}
-	decision, err := compositeExperimentService.GetDecision(testDecisionContext, testUserContext, s.options, s.reasons)
+	decision, _, err := compositeExperimentService.GetDecision(testDecisionContext, testUserContext, s.options)
 	s.Equal(expectedDecision, decision)
 	s.NoError(err)
 	s.mockExperimentService.AssertExpectations(s.T())

pkg/decision/composite_experiment_service_test.go

@@ -43,18 +43,21 @@ func NewCompositeFeatureService(sdkKey string, compositeExperimentService Experi
 }
 
 // GetDecision returns a decision for the given feature and user context
-func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options, reasons decide.DecisionReasons) (FeatureDecision, error) {
+func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) {
 	var featureDecision = FeatureDecision{}
+	reasons := decide.NewDecisionReasons(options)
 	var err error
 	for _, featureDecisionService := range f.featureServices {
-		featureDecision, err = featureDecisionService.GetDecision(decisionContext, userContext, options, reasons)
+		var decisionReasons decide.DecisionReasons
+		featureDecision, decisionReasons, err = featureDecisionService.GetDecision(decisionContext, userContext, options)
+		reasons.Append(decisionReasons)
 		if err != nil {
 			f.logger.Debug(fmt.Sprintf("%v", err))
 		}
 
 		if featureDecision.Variation != nil && err == nil {
-			return featureDecision, err
+			return featureDecision, reasons, err
 		}
 	}
-	return featureDecision, err
+	return featureDecision, reasons, err
 }