diff --git a/pkg/api/handlers/interface.go b/pkg/api/handlers/interface.go index e85d8cc7..ebbef1bc 100644 --- a/pkg/api/handlers/interface.go +++ b/pkg/api/handlers/interface.go @@ -35,9 +35,10 @@ type UserEventAPI interface { // UserAPI defines the supported user scoped APIs. type UserAPI interface { // TODO - // GetFeature(w http.ResponseWriter, r *http.Request) // ListFeatures(w http.ResponseWriter, r *http.Request) - ActivateFeature(w http.ResponseWriter, r *http.Request) + GetFeature(w http.ResponseWriter, r *http.Request) + TrackFeature(w http.ResponseWriter, r *http.Request) + TrackEvent(w http.ResponseWriter, r *http.Request) } diff --git a/pkg/api/handlers/user.go b/pkg/api/handlers/user.go index 19ae84ac..6e2d527a 100644 --- a/pkg/api/handlers/user.go +++ b/pkg/api/handlers/user.go @@ -26,6 +26,7 @@ import ( "github.com/optimizely/sidedoor/pkg/api/middleware" "github.com/optimizely/sidedoor/pkg/api/models" + "github.com/optimizely/sidedoor/pkg/optimizely" ) type eventTags map[string]interface{} @@ -35,13 +36,7 @@ type UserHandler struct{} // TrackEvent - track a given event for the current user func (h *UserHandler) TrackEvent(w http.ResponseWriter, r *http.Request) { - optlyClient, err := middleware.GetOptlyClient(r) - if err != nil { - RenderError(err, http.StatusUnprocessableEntity, w, r) - return - } - - optlyContext, err := middleware.GetOptlyContext(r) + optlyClient, optlyContext, err := parseContext(r) if err != nil { RenderError(err, http.StatusUnprocessableEntity, w, r) return @@ -71,25 +66,60 @@ func (h *UserHandler) TrackEvent(w http.ResponseWriter, r *http.Request) { render.NoContent(w, r) } -// ActivateFeature - Return the feature and record impression -func (h *UserHandler) ActivateFeature(w http.ResponseWriter, r *http.Request) { - optlyClient, err := middleware.GetOptlyClient(r) +// GetFeature - Return the feature and record impression if applicable. +func (h *UserHandler) GetFeature(w http.ResponseWriter, r *http.Request) { + optlyClient, optlyContext, err := parseContext(r) if err != nil { RenderError(err, http.StatusUnprocessableEntity, w, r) return } - optlyContext, err := middleware.GetOptlyContext(r) + featureKey := chi.URLParam(r, "featureKey") + renderFeature(w, r, featureKey, optlyClient, optlyContext) +} + +// TrackFeature - Return the feature and record impression if applicable. +// Tracking impressions is only supported for "Feature Tests" as part of the SDK contract. +func (h *UserHandler) TrackFeature(w http.ResponseWriter, r *http.Request) { + optlyClient, optlyContext, err := parseContext(r) if err != nil { RenderError(err, http.StatusUnprocessableEntity, w, r) return } featureKey := chi.URLParam(r, "featureKey") - enabled, variables, err := optlyClient.GetAndTrackFeatureWithContext(featureKey, optlyContext) + + // HACK - Triggers an impression event when applicable. This is not + // ideal since we're making TWO decisions now. OASIS-5549 + if _, softErr := optlyClient.IsFeatureEnabled(featureKey, *optlyContext.UserContext); softErr != nil { + // Swallowing the error to allow the response to be made and not break downstream consumers. + middleware.GetLogger(r).Error().Err(softErr).Str("featureKey", featureKey).Msg("Calling IsFeatureEnabled") + } + + renderFeature(w, r, featureKey, optlyClient, optlyContext) +} + +// parseContext extract the common references from the request context +func parseContext(r *http.Request) (*optimizely.OptlyClient, *optimizely.OptlyContext, error) { + optlyClient, err := middleware.GetOptlyClient(r) + if err != nil { + return nil, nil, err + } + + optlyContext, err := middleware.GetOptlyContext(r) + if err != nil { + return nil, nil, err + } + + return optlyClient, optlyContext, nil +} + +// renderFeature excapsulates extracting a Feature from the Optimizely SDK and rendering a feature response. +func renderFeature(w http.ResponseWriter, r *http.Request, featureKey string, optlyClient *optimizely.OptlyClient, optlyContext *optimizely.OptlyContext) { + enabled, variables, err := optlyClient.GetFeatureWithContext(featureKey, optlyContext) if err != nil { - middleware.GetLogger(r).Error().Str("featureKey", featureKey).Str("userID", optlyContext.GetUserID()).Msg("Calling ActivateFeature") + middleware.GetLogger(r).Error().Str("featureKey", featureKey).Msg("Calling GetFeature") RenderError(err, http.StatusInternalServerError, w, r) return } diff --git a/pkg/api/handlers/user_test.go b/pkg/api/handlers/user_test.go index 306b1664..f80e99fa 100644 --- a/pkg/api/handlers/user_test.go +++ b/pkg/api/handlers/user_test.go @@ -74,13 +74,39 @@ func (suite *UserTestSuite) SetupTest() { mux.Use(userMW.ClientCtx, userMW.UserCtx) mux.Post("/events/{eventKey}", userAPI.TrackEvent) mux.Post("/events/{eventKey}/", userAPI.TrackEvent) // Needed to assert non-empty eventKey - mux.Post("/features/{featureKey}", userAPI.ActivateFeature) + + mux.Get("/features/{featureKey}", userAPI.GetFeature) + mux.Post("/features/{featureKey}", userAPI.TrackFeature) suite.mux = mux suite.tc = testClient } -func (suite *UserTestSuite) TestActivateFeature() { +func (suite *UserTestSuite) TestGetFeatureWithFeatureTest() { + feature := entities.Feature{Key: "one"} + suite.tc.AddFeatureTest(feature) + + req := httptest.NewRequest("GET", "/features/one", nil) + rec := httptest.NewRecorder() + suite.mux.ServeHTTP(rec, req) + + suite.Equal(http.StatusOK, rec.Code) + + // Unmarshal response + var actual models.Feature + err := json.Unmarshal(rec.Body.Bytes(), &actual) + suite.NoError(err) + + expected := models.Feature{ + Key: "one", + Enabled: true, + } + + suite.Equal(0, len(suite.tc.GetProcessedEvents())) + suite.Equal(expected, actual) +} + +func (suite *UserTestSuite) TestTrackFeatureWithFeatureRollout() { feature := entities.Feature{Key: "one"} suite.tc.AddFeatureRollout(feature) @@ -100,9 +126,39 @@ func (suite *UserTestSuite) TestActivateFeature() { Enabled: true, } + suite.Equal(0, len(suite.tc.GetProcessedEvents())) suite.Equal(expected, actual) } +func (suite *UserTestSuite) TestTrackFeatureWithFeatureTest() { + feature := entities.Feature{Key: "one"} + suite.tc.AddFeatureTest(feature) + + req := httptest.NewRequest("POST", "/features/one", nil) + rec := httptest.NewRecorder() + suite.mux.ServeHTTP(rec, req) + + suite.Equal(http.StatusOK, rec.Code) + + // Unmarshal response + var actual models.Feature + err := json.Unmarshal(rec.Body.Bytes(), &actual) + suite.NoError(err) + + expected := models.Feature{ + Key: "one", + Enabled: true, + } + suite.Equal(expected, actual) + + events := suite.tc.GetProcessedEvents() + suite.Equal(1, len(events)) + + impression := events[0] + suite.Equal("campaign_activated", impression.Impression.Key) + suite.Equal("testUser", impression.VisitorID) +} + func (suite *UserTestSuite) TestGetFeaturesMissingFeature() { // Create a request to pass to our handler. We don't have any query parameters for now, so we'll // pass 'nil' as the third parameter. @@ -204,7 +260,8 @@ func TestUserMissingClientCtx(t *testing.T) { userHandler := new(UserHandler) handlers := []func(w http.ResponseWriter, r *http.Request){ - userHandler.ActivateFeature, + userHandler.GetFeature, + userHandler.TrackFeature, userHandler.TrackEvent, } @@ -223,7 +280,8 @@ func TestUserMissingOptlyCtx(t *testing.T) { userHandler := new(UserHandler) handlers := []func(w http.ResponseWriter, r *http.Request){ - userHandler.ActivateFeature, + userHandler.GetFeature, + userHandler.TrackFeature, userHandler.TrackEvent, } diff --git a/pkg/api/router.go b/pkg/api/router.go index 2284e4be..cd57172c 100644 --- a/pkg/api/router.go +++ b/pkg/api/router.go @@ -63,9 +63,11 @@ func NewRouter(opt *RouterOptions) *chi.Mux { r.Route("/users/{userID}", func(r chi.Router) { r.Use(opt.middleware.ClientCtx, opt.middleware.UserCtx) - // TODO r.Get("/features/{featureKey}", opt.userAPI.ActivateFeature) - r.Post("/features/{featureKey}", opt.userAPI.ActivateFeature) + r.Post("/events/{eventKey}", opt.userAPI.TrackEvent) + + r.Get("/features/{featureKey}", opt.userAPI.GetFeature) + r.Post("/features/{featureKey}", opt.userAPI.GetFeature) }) return r diff --git a/pkg/api/router_test.go b/pkg/api/router_test.go index bd3154c4..6ff42985 100644 --- a/pkg/api/router_test.go +++ b/pkg/api/router_test.go @@ -71,7 +71,11 @@ func (m *MockUserAPI) TrackEvent(w http.ResponseWriter, r *http.Request) { renderPathParams(w, r) } -func (m *MockUserAPI) ActivateFeature(w http.ResponseWriter, r *http.Request) { +func (m *MockUserAPI) GetFeature(w http.ResponseWriter, r *http.Request) { + renderPathParams(w, r) +} + +func (m *MockUserAPI) TrackFeature(w http.ResponseWriter, r *http.Request) { renderPathParams(w, r) } @@ -134,19 +138,23 @@ func (suite *RouterTestSuite) TestGetFeature() { suite.assertValid(rec, expected) } -func (suite *RouterTestSuite) TestActivateFeatures() { - req := httptest.NewRequest("POST", "/users/me/features/one", nil) - rec := httptest.NewRecorder() - suite.mux.ServeHTTP(rec, req) +func (suite *RouterTestSuite) TestGetFeatures() { + methods := []string{"GET", "POST"} - suite.Equal("expected", rec.Header().Get(clientHeaderKey)) - suite.Equal("expected", rec.Header().Get(userHeaderKey)) + for _, m := range methods { + req := httptest.NewRequest(m, "/users/me/features/one", nil) + rec := httptest.NewRecorder() + suite.mux.ServeHTTP(rec, req) - expected := map[string]string{ - "userID": "me", - "featureKey": "one", + suite.Equal("expected", rec.Header().Get(clientHeaderKey)) + suite.Equal("expected", rec.Header().Get(userHeaderKey)) + + expected := map[string]string{ + "userID": "me", + "featureKey": "one", + } + suite.assertValid(rec, expected) } - suite.assertValid(rec, expected) } func (suite *RouterTestSuite) TestTrackEvent() { diff --git a/pkg/optimizely/client.go b/pkg/optimizely/client.go index ef59db0f..2aaef786 100644 --- a/pkg/optimizely/client.go +++ b/pkg/optimizely/client.go @@ -62,12 +62,6 @@ func (c *OptlyClient) TrackEventWithContext(eventKey string, ctx *OptlyContext, return c.Track(eventKey, *ctx.UserContext, eventTags) } -// GetAndTrackFeatureWithContext calls the OptimizelyClient with the current OptlyContext this does NOT track experiment conversions -func (c *OptlyClient) GetAndTrackFeatureWithContext(featureKey string, ctx *OptlyContext) (enabled bool, variableMap map[string]string, err error) { - // TODO add tracking - return c.GetFeatureWithContext(featureKey, ctx) -} - // GetFeatureWithContext calls the OptimizelyClient with the current OptlyContext func (c *OptlyClient) GetFeatureWithContext(featureKey string, ctx *OptlyContext) (enabled bool, variableMap map[string]string, err error) { return c.GetAllFeatureVariables(featureKey, *ctx.UserContext) diff --git a/pkg/optimizely/client_test.go b/pkg/optimizely/client_test.go index acc4ea3d..f2d2a67a 100644 --- a/pkg/optimizely/client_test.go +++ b/pkg/optimizely/client_test.go @@ -63,18 +63,6 @@ func (suite *ClientTestSuite) TestGetNonExistentFeature() { } } -func (suite *ClientTestSuite) TestGetAndTrackFeatureWithContext() { - basicFeature := entities.Feature{Key: "basic"} - suite.testClient.AddFeatureRollout(basicFeature) - enabled, variableMap, err := suite.optlyClient.GetAndTrackFeatureWithContext("basic", suite.optlyContext) - - suite.NoError(err) - suite.True(enabled) - suite.Equal(0, len(variableMap)) - - // TODO add assertion that a tracking call was sent for FeatureTest -} - func (suite *ClientTestSuite) TestGetBasicFeature() { basicFeature := entities.Feature{Key: "basic"} suite.testClient.AddFeatureRollout(basicFeature) diff --git a/pkg/optimizelytest/client.go b/pkg/optimizelytest/client.go index ed0c0564..84aa18b0 100644 --- a/pkg/optimizelytest/client.go +++ b/pkg/optimizelytest/client.go @@ -64,6 +64,11 @@ func (t *TestClient) AddFeatureRollout(f entities.Feature) { t.ProjectConfig.AddFeatureRollout(f) } +// AddFeatureTest is a helper method for adding feature rollouts to the ProjectConfig to fascilitate testing. +func (t *TestClient) AddFeatureTest(f entities.Feature) { + t.ProjectConfig.AddFeatureTest(f) +} + // GetProcessedEvents returns the UserEvent objects sent to the event processor. func (t *TestClient) GetProcessedEvents() []event.UserEvent { return t.EventProcessor.GetEvents() diff --git a/pkg/optimizelytest/config.go b/pkg/optimizelytest/config.go index 403fb4f5..1fa4587c 100644 --- a/pkg/optimizelytest/config.go +++ b/pkg/optimizelytest/config.go @@ -173,6 +173,33 @@ func (c TestProjectConfig) AddFeature(f entities.Feature) *TestProjectConfig { return &c } +// AddFeatureTest adds the feature and supporting entities to complete the feature test modeling +func (c TestProjectConfig) AddFeatureTest(f entities.Feature) *TestProjectConfig { + experimentID := c.getNextID() + variationID := c.getNextID() + layerID := c.getNextID() + + variation := entities.Variation{ + Key: variationID, + ID: variationID, + FeatureEnabled: true, + } + + experiment := entities.Experiment{ + Key: experimentID, + LayerID: layerID, + ID: experimentID, + Variations: map[string]entities.Variation{variationID: variation}, + TrafficAllocation: []entities.Range{ + entities.Range{EntityID: variationID, EndOfRange: 10000}, + }, + } + + f.FeatureExperiments = []entities.Experiment{experiment} + c.FeatureMap[f.Key] = f + return &c +} + // AddFeatureRollout adds the feature and supporting entities to complete the rollout modeling func (c TestProjectConfig) AddFeatureRollout(f entities.Feature) *TestProjectConfig { experimentID := c.getNextID() @@ -181,13 +208,13 @@ func (c TestProjectConfig) AddFeatureRollout(f entities.Feature) *TestProjectCon layerID := c.getNextID() variation := entities.Variation{ - Key: "rollout_var", + Key: variationID, ID: variationID, FeatureEnabled: true, } experiment := entities.Experiment{ - Key: "background_experiment", + Key: experimentID, LayerID: layerID, ID: experimentID, Variations: map[string]entities.Variation{variationID: variation},