From dc08d0ea7b653de9eec0b423da4381e2467601bb Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Thu, 31 Oct 2019 14:55:40 -0700 Subject: [PATCH 1/4] feat(decision): User profile service integration. --- pkg/decision/composite_experiment_service.go | 6 +- pkg/decision/entities.go | 11 ++ pkg/decision/interface.go | 6 + pkg/decision/persisting_experiment_service.go | 119 ++++++++++++++++++ 4 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 pkg/decision/persisting_experiment_service.go diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index 37eaf95aa..a5af8e385 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -34,14 +34,16 @@ type CompositeExperimentService struct { // NewCompositeExperimentService creates a new instance of the CompositeExperimentService func NewCompositeExperimentService() *CompositeExperimentService { + persistingExperimentService := NewPersistingExperimentService(NewExperimentBucketerService()) + // These decision services are applied in order: // 1. Whitelist - // 2. Bucketing + // 2. Bucketing (wrapped with user profile service) // @TODO(mng): Prepend forced variation return &CompositeExperimentService{ experimentServices: []ExperimentService{ NewExperimentWhitelistService(), - NewExperimentBucketerService(), + persistingExperimentService, }, } } diff --git a/pkg/decision/entities.go b/pkg/decision/entities.go index 4bfdf7aa2..86ba7ff2a 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.go @@ -63,3 +63,14 @@ type ExperimentDecision struct { Decision Variation *entities.Variation } + +// UserDecisionKey is used to access the saved decisions in a user profile +type UserDecisionKey struct { + ExperimentID string +} + +// UserProfile represents a saved user profile +type UserProfile struct { + ID string + ExperimentBucketMap map[UserDecisionKey]string +} diff --git a/pkg/decision/interface.go b/pkg/decision/interface.go index c81c1181b..1bfff601f 100644 --- a/pkg/decision/interface.go +++ b/pkg/decision/interface.go @@ -39,3 +39,9 @@ type ExperimentService interface { type FeatureService interface { GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext) (FeatureDecision, error) } + +// UserProfileService is used to save and retrieve past bucketing decisions for users +type UserProfileService interface { + Lookup(string) (UserProfile, error) + Save(UserProfile) error +} diff --git a/pkg/decision/persisting_experiment_service.go b/pkg/decision/persisting_experiment_service.go new file mode 100644 index 000000000..45e7cae47 --- /dev/null +++ b/pkg/decision/persisting_experiment_service.go @@ -0,0 +1,119 @@ +/**************************************************************************** + * Copyright 2019, Optimizely, Inc. and contributors * + * * + * Licensed under the Apache License, Version 2.0 (the "License"); * + * you may not use this file except in compliance with the License. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +// Package decision // +package decision + +import ( + "fmt" + + "github.com/optimizely/go-sdk/pkg/entities" + "github.com/optimizely/go-sdk/pkg/logging" +) + +var pesLogger = logging.GetLogger("pkg/decision/persisting_experiment_service") + +// PersistingExperimentService attempts to retrieve a saved decision from the user profile service +// for the user before having the ExperimentBucketerService compute it. +// If computed, the decision is saved back to the user profile service if provided. +type PersistingExperimentService struct { + experimentBucketedService ExperimentService + userProfileService UserProfileService +} + +// PESOptionFunc is used to extend the PersistingExperimentService with additional options +type PESOptionFunc func(*PersistingExperimentService) + +// WithUserProfileService sets the user profile service on the persistingExperimentService instance +func WithUserProfileService(userProfileService UserProfileService) PESOptionFunc { + return func(f *PersistingExperimentService) { + f.userProfileService = userProfileService + } +} + +// NewPersistingExperimentService returns a new instance of the PersistingExperimentService +func NewPersistingExperimentService(experimentBucketerService ExperimentService, options ...PESOptionFunc) *PersistingExperimentService { + persistingExperimentService := &PersistingExperimentService{ + experimentBucketedService: experimentBucketerService, + } + + for _, opt := range options { + opt(persistingExperimentService) + } + + return persistingExperimentService +} + +// GetDecision returns the decision with the variation the user is bucketed into +func (p PersistingExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { + var experimentDecision ExperimentDecision + var err error + var userProfile UserProfile + // check to see if there is a saved decision for the user + if p.userProfileService != nil { + experimentDecision, userProfile = p.getSavedDecision(decisionContext, userContext) + if experimentDecision.Variation != nil { + return experimentDecision, nil + } + } + + experimentDecision, err = p.experimentBucketedService.GetDecision(decisionContext, userContext) + if experimentDecision.Variation != nil && p.userProfileService != nil { + // save decision if a user profile service is provided + p.saveDecision(userProfile, decisionContext.Experiment, experimentDecision) + } + + return experimentDecision, err +} + +func (p PersistingExperimentService) getSavedDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, UserProfile) { + experimentDecision := ExperimentDecision{} + + if p.userProfileService == nil { + return experimentDecision, UserProfile{} + } + + userProfile, err := p.userProfileService.Lookup(userContext.ID) + if err != nil { + errMessage := fmt.Sprintf(`Error looking up user from user profile service: %s`, err) + bLogger.Warning(errMessage) + return experimentDecision, UserProfile{} + } + + // look up experiment decision from user profile + decisionKey := UserDecisionKey{ExperimentID: decisionContext.Experiment.ID} + if savedVariationID, ok := userProfile.ExperimentBucketMap[decisionKey]; ok { + if variation, ok := decisionContext.Experiment.Variations[savedVariationID]; ok { + experimentDecision.Variation = &variation + bLogger.Debug(fmt.Sprintf(`User "%s" was previously bucketed into variation "%s" of experiment "%s".`, userContext.ID, variation.Key, decisionContext.Experiment.Key)) + } else { + bLogger.Warning(fmt.Sprintf(`User "%s" was previously bucketed into variation with ID "%s" for experiment "%s", but no matching variation was found.`, userContext.ID, savedVariationID, decisionContext.Experiment.Key)) + } + } + + return experimentDecision, userProfile +} + +func (p PersistingExperimentService) saveDecision(userProfile UserProfile, experiment *entities.Experiment, decision ExperimentDecision) { + if p.userProfileService != nil { + decisionKey := UserDecisionKey{ExperimentID: experiment.ID} + userProfile.ExperimentBucketMap[decisionKey] = decision.Variation.ID + err := p.userProfileService.Save(userProfile) + if err != nil { + bLogger.Error(`Unable to save decision to user profile service`, err) + } + } +} From 9a542138862acabefef8af16a395021fa30d5e91 Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Mon, 4 Nov 2019 11:55:56 -0800 Subject: [PATCH 2/4] address PR feedback. --- pkg/decision/composite_experiment_service.go | 6 +- pkg/decision/persisting_experiment_service.go | 45 ++--- .../persisting_experiment_service_test.go | 177 ++++++++++++++++++ 3 files changed, 194 insertions(+), 34 deletions(-) create mode 100644 pkg/decision/persisting_experiment_service_test.go diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index a5af8e385..37eaf95aa 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -34,16 +34,14 @@ type CompositeExperimentService struct { // NewCompositeExperimentService creates a new instance of the CompositeExperimentService func NewCompositeExperimentService() *CompositeExperimentService { - persistingExperimentService := NewPersistingExperimentService(NewExperimentBucketerService()) - // These decision services are applied in order: // 1. Whitelist - // 2. Bucketing (wrapped with user profile service) + // 2. Bucketing // @TODO(mng): Prepend forced variation return &CompositeExperimentService{ experimentServices: []ExperimentService{ NewExperimentWhitelistService(), - persistingExperimentService, + NewExperimentBucketerService(), }, } } diff --git a/pkg/decision/persisting_experiment_service.go b/pkg/decision/persisting_experiment_service.go index 45e7cae47..f817fa092 100644 --- a/pkg/decision/persisting_experiment_service.go +++ b/pkg/decision/persisting_experiment_service.go @@ -34,44 +34,31 @@ type PersistingExperimentService struct { userProfileService UserProfileService } -// PESOptionFunc is used to extend the PersistingExperimentService with additional options -type PESOptionFunc func(*PersistingExperimentService) - -// WithUserProfileService sets the user profile service on the persistingExperimentService instance -func WithUserProfileService(userProfileService UserProfileService) PESOptionFunc { - return func(f *PersistingExperimentService) { - f.userProfileService = userProfileService - } -} - // NewPersistingExperimentService returns a new instance of the PersistingExperimentService -func NewPersistingExperimentService(experimentBucketerService ExperimentService, options ...PESOptionFunc) *PersistingExperimentService { +func NewPersistingExperimentService(experimentBucketerService ExperimentService, userProfileService UserProfileService) *PersistingExperimentService { persistingExperimentService := &PersistingExperimentService{ experimentBucketedService: experimentBucketerService, - } - - for _, opt := range options { - opt(persistingExperimentService) + userProfileService: userProfileService, } return persistingExperimentService } // GetDecision returns the decision with the variation the user is bucketed into -func (p PersistingExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { - var experimentDecision ExperimentDecision - var err error +func (p PersistingExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (experimentDecision ExperimentDecision, err error) { + if p.userProfileService == nil { + return p.experimentBucketedService.GetDecision(decisionContext, userContext) + } + var userProfile UserProfile // check to see if there is a saved decision for the user - if p.userProfileService != nil { - experimentDecision, userProfile = p.getSavedDecision(decisionContext, userContext) - if experimentDecision.Variation != nil { - return experimentDecision, nil - } + experimentDecision, userProfile = p.getSavedDecision(decisionContext, userContext) + if experimentDecision.Variation != nil { + return experimentDecision, nil } experimentDecision, err = p.experimentBucketedService.GetDecision(decisionContext, userContext) - if experimentDecision.Variation != nil && p.userProfileService != nil { + if experimentDecision.Variation != nil { // save decision if a user profile service is provided p.saveDecision(userProfile, decisionContext.Experiment, experimentDecision) } @@ -81,16 +68,11 @@ func (p PersistingExperimentService) GetDecision(decisionContext ExperimentDecis func (p PersistingExperimentService) getSavedDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, UserProfile) { experimentDecision := ExperimentDecision{} - - if p.userProfileService == nil { - return experimentDecision, UserProfile{} - } - userProfile, err := p.userProfileService.Lookup(userContext.ID) if err != nil { errMessage := fmt.Sprintf(`Error looking up user from user profile service: %s`, err) bLogger.Warning(errMessage) - return experimentDecision, UserProfile{} + return experimentDecision, UserProfile{ID: userContext.ID} } // look up experiment decision from user profile @@ -110,6 +92,9 @@ func (p PersistingExperimentService) getSavedDecision(decisionContext Experiment func (p PersistingExperimentService) saveDecision(userProfile UserProfile, experiment *entities.Experiment, decision ExperimentDecision) { if p.userProfileService != nil { decisionKey := UserDecisionKey{ExperimentID: experiment.ID} + if userProfile.ExperimentBucketMap == nil { + userProfile.ExperimentBucketMap = map[UserDecisionKey]string{} + } userProfile.ExperimentBucketMap[decisionKey] = decision.Variation.ID err := p.userProfileService.Save(userProfile) if err != nil { diff --git a/pkg/decision/persisting_experiment_service_test.go b/pkg/decision/persisting_experiment_service_test.go new file mode 100644 index 000000000..41680dc3c --- /dev/null +++ b/pkg/decision/persisting_experiment_service_test.go @@ -0,0 +1,177 @@ +/**************************************************************************** + * Copyright 2019, Optimizely, Inc. and contributors * + * * + * Licensed under the Apache License, Version 2.0 (the "License"); * + * you may not use this file except in compliance with the License. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +// Package decision // +package decision + +import ( + "errors" + "testing" + + "github.com/optimizely/go-sdk/pkg/entities" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/suite" +) + +type MockUserProfileService struct { + UserProfileService + mock.Mock +} + +func (m *MockUserProfileService) Lookup(userID string) (UserProfile, error) { + args := m.Called(userID) + return args.Get(0).(UserProfile), args.Error(1) +} + +func (m *MockUserProfileService) Save(userProfile UserProfile) error { + args := m.Called(userProfile) + return args.Error(0) +} + +var testUserContext entities.UserContext = entities.UserContext{ + ID: "test_user_1", +} + +type PersistingExperimentServiceTestSuite struct { + suite.Suite + mockProjectConfig *mockProjectConfig + mockExperimentService *MockExperimentDecisionService + mockUserProfileService *MockUserProfileService + testComputedDecision ExperimentDecision + testDecisionContext ExperimentDecisionContext +} + +func (s *PersistingExperimentServiceTestSuite) SetupTest() { + s.mockProjectConfig = new(mockProjectConfig) + s.mockExperimentService = new(MockExperimentDecisionService) + s.mockUserProfileService = new(MockUserProfileService) + s.testDecisionContext = ExperimentDecisionContext{ + Experiment: &testExp1113, + ProjectConfig: s.mockProjectConfig, + } + + computedVariation := testExp1113.Variations["2223"] + s.testComputedDecision = ExperimentDecision{ + Variation: &computedVariation, + } + s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext).Return(s.testComputedDecision, nil) +} + +func (s *PersistingExperimentServiceTestSuite) TestNilUserProfileService() { + persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, nil) + decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) + s.Equal(s.testComputedDecision, decision) + s.NoError(err) + s.mockExperimentService.AssertExpectations(s.T()) +} + +func (s *PersistingExperimentServiceTestSuite) TestSavedVariationFound() { + decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + savedUserProfile := UserProfile{ + ID: testUserContext.ID, + ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: testExp1113Var2224.ID}, + } + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(savedUserProfile, nil) + s.mockUserProfileService.On("Save", mock.Anything) + + persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) + decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) + savedDecision := ExperimentDecision{ + Variation: &testExp1113Var2224, + } + s.Equal(savedDecision, decision) + s.NoError(err) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision", s.testDecisionContext, testUserContext) + s.mockUserProfileService.AssertNotCalled(s.T(), "Save", mock.Anything) +} + +func (s *PersistingExperimentServiceTestSuite) TestNoSavedVariation() { + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(UserProfile{ID: testUserContext.ID}, nil) // empty user profile + decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + updatedUserProfile := UserProfile{ + ID: testUserContext.ID, + ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, + } + + s.mockUserProfileService.On("Save", updatedUserProfile).Return(nil) + persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) + decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) + s.Equal(s.testComputedDecision, decision) + s.NoError(err) + s.mockExperimentService.AssertExpectations(s.T()) + s.mockUserProfileService.AssertExpectations(s.T()) +} + +func (s *PersistingExperimentServiceTestSuite) TestSavedVariationNoLongerValid() { + decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + savedUserProfile := UserProfile{ + ID: testUserContext.ID, + ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: "forgotten_variation"}, + } + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(savedUserProfile, nil) // empty user profile + + updatedUserProfile := UserProfile{ + ID: testUserContext.ID, + ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, + } + s.mockUserProfileService.On("Save", updatedUserProfile).Return(nil) + persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) + decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) + s.Equal(s.testComputedDecision, decision) + s.NoError(err) + s.mockExperimentService.AssertExpectations(s.T()) + s.mockUserProfileService.AssertExpectations(s.T()) +} + +func (s *PersistingExperimentServiceTestSuite) TestErrorGettingSavedVariation() { + userProfileErr := errors.New("could not retrieve user profile") + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(UserProfile{ID: testUserContext.ID}, userProfileErr) // empty user profile + + decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + updatedUserProfile := UserProfile{ + ID: testUserContext.ID, + ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, + } + + s.mockUserProfileService.On("Save", updatedUserProfile).Return(nil) + persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) + decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) + s.Equal(s.testComputedDecision, decision) + s.NoError(err) + s.mockExperimentService.AssertExpectations(s.T()) + s.mockUserProfileService.AssertExpectations(s.T()) +} + +func (s *PersistingExperimentServiceTestSuite) TestErrorSavingVariation() { + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(UserProfile{ID: testUserContext.ID}, nil) // empty user profile + decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + updatedUserProfile := UserProfile{ + ID: testUserContext.ID, + ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, + } + + s.mockUserProfileService.On("Save", updatedUserProfile).Return(errors.New("could not save")) + persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) + decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) + s.Equal(s.testComputedDecision, decision) + s.NoError(err) + s.mockExperimentService.AssertExpectations(s.T()) + s.mockUserProfileService.AssertExpectations(s.T()) +} + +func TestPersistingExperimentServiceTestSuite(t *testing.T) { + suite.Run(t, new(PersistingExperimentServiceTestSuite)) +} From ff7c204458888366329ff675bf0880f933c44ac6 Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Mon, 4 Nov 2019 14:49:20 -0800 Subject: [PATCH 3/4] fix: use the correct logger. --- pkg/decision/persisting_experiment_service.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/decision/persisting_experiment_service.go b/pkg/decision/persisting_experiment_service.go index f817fa092..5efe6452c 100644 --- a/pkg/decision/persisting_experiment_service.go +++ b/pkg/decision/persisting_experiment_service.go @@ -71,7 +71,7 @@ func (p PersistingExperimentService) getSavedDecision(decisionContext Experiment userProfile, err := p.userProfileService.Lookup(userContext.ID) if err != nil { errMessage := fmt.Sprintf(`Error looking up user from user profile service: %s`, err) - bLogger.Warning(errMessage) + pesLogger.Warning(errMessage) return experimentDecision, UserProfile{ID: userContext.ID} } @@ -80,9 +80,9 @@ func (p PersistingExperimentService) getSavedDecision(decisionContext Experiment if savedVariationID, ok := userProfile.ExperimentBucketMap[decisionKey]; ok { if variation, ok := decisionContext.Experiment.Variations[savedVariationID]; ok { experimentDecision.Variation = &variation - bLogger.Debug(fmt.Sprintf(`User "%s" was previously bucketed into variation "%s" of experiment "%s".`, userContext.ID, variation.Key, decisionContext.Experiment.Key)) + pesLogger.Debug(fmt.Sprintf(`User "%s" was previously bucketed into variation "%s" of experiment "%s".`, userContext.ID, variation.Key, decisionContext.Experiment.Key)) } else { - bLogger.Warning(fmt.Sprintf(`User "%s" was previously bucketed into variation with ID "%s" for experiment "%s", but no matching variation was found.`, userContext.ID, savedVariationID, decisionContext.Experiment.Key)) + pesLogger.Warning(fmt.Sprintf(`User "%s" was previously bucketed into variation with ID "%s" for experiment "%s", but no matching variation was found.`, userContext.ID, savedVariationID, decisionContext.Experiment.Key)) } } @@ -98,7 +98,7 @@ func (p PersistingExperimentService) saveDecision(userProfile UserProfile, exper userProfile.ExperimentBucketMap[decisionKey] = decision.Variation.ID err := p.userProfileService.Save(userProfile) if err != nil { - bLogger.Error(`Unable to save decision to user profile service`, err) + pesLogger.Error(`Unable to save decision to user profile service`, err) } } } From 2e2caae43698fd271844cb338c7e2511a3cbf5af Mon Sep 17 00:00:00 2001 From: Mike Ng Date: Mon, 4 Nov 2019 21:55:28 -0800 Subject: [PATCH 4/4] fix: Address PR feedback. --- pkg/decision/entities.go | 9 +++ pkg/decision/interface.go | 4 +- pkg/decision/persisting_experiment_service.go | 20 +++--- .../persisting_experiment_service_test.go | 62 ++++--------------- 4 files changed, 31 insertions(+), 64 deletions(-) diff --git a/pkg/decision/entities.go b/pkg/decision/entities.go index 86ba7ff2a..38933a45a 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.go @@ -67,6 +67,15 @@ type ExperimentDecision struct { // UserDecisionKey is used to access the saved decisions in a user profile type UserDecisionKey struct { ExperimentID string + Field string +} + +// NewUserDecisionKey returns a new UserDecisionKey with the given experiment ID +func NewUserDecisionKey(experimentID string) UserDecisionKey { + return UserDecisionKey{ + ExperimentID: experimentID, + Field: "variation_id", + } } // UserProfile represents a saved user profile diff --git a/pkg/decision/interface.go b/pkg/decision/interface.go index 1bfff601f..de4af1a62 100644 --- a/pkg/decision/interface.go +++ b/pkg/decision/interface.go @@ -42,6 +42,6 @@ type FeatureService interface { // UserProfileService is used to save and retrieve past bucketing decisions for users type UserProfileService interface { - Lookup(string) (UserProfile, error) - Save(UserProfile) error + Lookup(string) UserProfile + Save(UserProfile) } diff --git a/pkg/decision/persisting_experiment_service.go b/pkg/decision/persisting_experiment_service.go index 5efe6452c..30a20d84d 100644 --- a/pkg/decision/persisting_experiment_service.go +++ b/pkg/decision/persisting_experiment_service.go @@ -68,15 +68,14 @@ func (p PersistingExperimentService) GetDecision(decisionContext ExperimentDecis func (p PersistingExperimentService) getSavedDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, UserProfile) { experimentDecision := ExperimentDecision{} - userProfile, err := p.userProfileService.Lookup(userContext.ID) - if err != nil { - errMessage := fmt.Sprintf(`Error looking up user from user profile service: %s`, err) - pesLogger.Warning(errMessage) - return experimentDecision, UserProfile{ID: userContext.ID} - } + userProfile := p.userProfileService.Lookup(userContext.ID) // look up experiment decision from user profile - decisionKey := UserDecisionKey{ExperimentID: decisionContext.Experiment.ID} + decisionKey := NewUserDecisionKey(decisionContext.Experiment.ID) + if userProfile.ExperimentBucketMap == nil { + return experimentDecision, userProfile + } + if savedVariationID, ok := userProfile.ExperimentBucketMap[decisionKey]; ok { if variation, ok := decisionContext.Experiment.Variations[savedVariationID]; ok { experimentDecision.Variation = &variation @@ -91,14 +90,11 @@ func (p PersistingExperimentService) getSavedDecision(decisionContext Experiment func (p PersistingExperimentService) saveDecision(userProfile UserProfile, experiment *entities.Experiment, decision ExperimentDecision) { if p.userProfileService != nil { - decisionKey := UserDecisionKey{ExperimentID: experiment.ID} + decisionKey := NewUserDecisionKey(experiment.ID) if userProfile.ExperimentBucketMap == nil { userProfile.ExperimentBucketMap = map[UserDecisionKey]string{} } userProfile.ExperimentBucketMap[decisionKey] = decision.Variation.ID - err := p.userProfileService.Save(userProfile) - if err != nil { - pesLogger.Error(`Unable to save decision to user profile service`, err) - } + p.userProfileService.Save(userProfile) } } diff --git a/pkg/decision/persisting_experiment_service_test.go b/pkg/decision/persisting_experiment_service_test.go index 41680dc3c..0dad4620f 100644 --- a/pkg/decision/persisting_experiment_service_test.go +++ b/pkg/decision/persisting_experiment_service_test.go @@ -18,7 +18,6 @@ package decision import ( - "errors" "testing" "github.com/optimizely/go-sdk/pkg/entities" @@ -31,14 +30,13 @@ type MockUserProfileService struct { mock.Mock } -func (m *MockUserProfileService) Lookup(userID string) (UserProfile, error) { +func (m *MockUserProfileService) Lookup(userID string) UserProfile { args := m.Called(userID) - return args.Get(0).(UserProfile), args.Error(1) + return args.Get(0).(UserProfile) } -func (m *MockUserProfileService) Save(userProfile UserProfile) error { - args := m.Called(userProfile) - return args.Error(0) +func (m *MockUserProfileService) Save(userProfile UserProfile) { + m.Called(userProfile) } var testUserContext entities.UserContext = entities.UserContext{ @@ -79,12 +77,12 @@ func (s *PersistingExperimentServiceTestSuite) TestNilUserProfileService() { } func (s *PersistingExperimentServiceTestSuite) TestSavedVariationFound() { - decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + decisionKey := NewUserDecisionKey(s.testDecisionContext.Experiment.ID) savedUserProfile := UserProfile{ ID: testUserContext.ID, ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: testExp1113Var2224.ID}, } - s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(savedUserProfile, nil) + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(savedUserProfile) s.mockUserProfileService.On("Save", mock.Anything) persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) @@ -99,14 +97,14 @@ func (s *PersistingExperimentServiceTestSuite) TestSavedVariationFound() { } func (s *PersistingExperimentServiceTestSuite) TestNoSavedVariation() { - s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(UserProfile{ID: testUserContext.ID}, nil) // empty user profile - decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(UserProfile{ID: testUserContext.ID}) // empty user profile + decisionKey := NewUserDecisionKey(s.testDecisionContext.Experiment.ID) updatedUserProfile := UserProfile{ ID: testUserContext.ID, ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, } - s.mockUserProfileService.On("Save", updatedUserProfile).Return(nil) + s.mockUserProfileService.On("Save", updatedUserProfile) persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) s.Equal(s.testComputedDecision, decision) @@ -116,54 +114,18 @@ func (s *PersistingExperimentServiceTestSuite) TestNoSavedVariation() { } func (s *PersistingExperimentServiceTestSuite) TestSavedVariationNoLongerValid() { - decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} + decisionKey := NewUserDecisionKey(s.testDecisionContext.Experiment.ID) savedUserProfile := UserProfile{ ID: testUserContext.ID, ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: "forgotten_variation"}, } - s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(savedUserProfile, nil) // empty user profile + s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(savedUserProfile) // empty user profile updatedUserProfile := UserProfile{ ID: testUserContext.ID, ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, } - s.mockUserProfileService.On("Save", updatedUserProfile).Return(nil) - persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) - decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) - s.Equal(s.testComputedDecision, decision) - s.NoError(err) - s.mockExperimentService.AssertExpectations(s.T()) - s.mockUserProfileService.AssertExpectations(s.T()) -} - -func (s *PersistingExperimentServiceTestSuite) TestErrorGettingSavedVariation() { - userProfileErr := errors.New("could not retrieve user profile") - s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(UserProfile{ID: testUserContext.ID}, userProfileErr) // empty user profile - - decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} - updatedUserProfile := UserProfile{ - ID: testUserContext.ID, - ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, - } - - s.mockUserProfileService.On("Save", updatedUserProfile).Return(nil) - persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) - decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) - s.Equal(s.testComputedDecision, decision) - s.NoError(err) - s.mockExperimentService.AssertExpectations(s.T()) - s.mockUserProfileService.AssertExpectations(s.T()) -} - -func (s *PersistingExperimentServiceTestSuite) TestErrorSavingVariation() { - s.mockUserProfileService.On("Lookup", testUserContext.ID).Return(UserProfile{ID: testUserContext.ID}, nil) // empty user profile - decisionKey := UserDecisionKey{ExperimentID: s.testDecisionContext.Experiment.ID} - updatedUserProfile := UserProfile{ - ID: testUserContext.ID, - ExperimentBucketMap: map[UserDecisionKey]string{decisionKey: s.testComputedDecision.Variation.ID}, - } - - s.mockUserProfileService.On("Save", updatedUserProfile).Return(errors.New("could not save")) + s.mockUserProfileService.On("Save", updatedUserProfile) persistingExperimentService := NewPersistingExperimentService(s.mockExperimentService, s.mockUserProfileService) decision, err := persistingExperimentService.GetDecision(s.testDecisionContext, testUserContext) s.Equal(s.testComputedDecision, decision)