feat(onTrack): Added onTrack callback #198

msohailhussain · 2019-11-26T22:29:12Z

When track is called, onTrack should be used to notify user if definition for onTrack is set in the client.

mikeproeng37

Thanks for the PR. There are two main issues I see:

We should be able to attach more than 1 callback for track notifications
I still think we should go through the notification center so that other components in the SDK could subscribe to this notification if they wanted to. This also helps streamline how we do notifications throughout the SDK

msohailhussain · 2019-11-27T18:25:36Z

Sure, let me revise. I will discuss with you first.

mikeproeng37

Please fix test coverage as well

mikeproeng37 · 2019-12-02T17:20:48Z

pkg/client/client.go

+	if o.EventProcessor.ProcessEvent(userEvent) && o.NotificationCenter != nil {
+		trackNotification := notification.TrackNotification{Type: notification.Track, EventKey: eventKey, UserContext: userContext, EventTags: eventTags}
+		var payload []interface{}
+		payload = append(payload, trackNotification, userEvent)


Why make this an array? We can add the UserEvent to the track notification struct

mikeproeng37 · 2019-12-02T17:21:41Z

pkg/client/client.go

+	}
+	handler := func(payload interface{}) {
+		success := false
+		if trackPayload, ok := payload.([]interface{}); ok {


Should be casting to a TrackNotification directly

msohailhussain · 2019-12-02T23:06:27Z

discussed offline, keeping it on hold. TrackNotification will have cyclic import issue if UserEvent is added there.

mikeproeng37

Mostly good, just need to redo the unit tests

mikeproeng37 · 2019-12-03T18:37:15Z

pkg/client/client_test.go

 	return "1.0.0"
 }

+func TestAddandRemoveOnTrack(t *testing.T) {


Can you create a test suite for these new tests to have better code reuse?

We already have some test suites towards the end like this:

func TestClientTestSuite(t *testing.T) {
suite.Run(t, new(ClientTestSuiteAB))
suite.Run(t, new(ClientTestSuiteFM))
}

So you can add 1 test suite for the notifications APIs and another one for event tracking for the tests you added further below. Don't worry about refactoring the current track tests, you can simply add the track notification track tests in the test suite

mikeproeng37 · 2019-12-04T23:50:45Z

pkg/client/client_test.go

+		s.Equal(*s.mockProcessor.Events[0].Conversion, conversionEvent)
+	}
+
+	client := OptimizelyClient{


This client initialization is repeated throughout the tests in the suite. We can initialize in the test setup

mikeproeng37 · 2019-12-04T23:51:47Z

pkg/client/client_test.go

+		DecisionService: s.mockDecisionService,
+		EventProcessor:  s.mockProcessor,
+	}
+	client.OnTrack(onTrack)


This is incorrect, we need to populate the notification center to ensure that there was nothing router through it.

mikeproeng37 · 2019-12-04T23:53:04Z

pkg/client/client_test.go

+	}
+
+	mockNotificationCenter := new(MockNotificationCenter)
+	mockNotificationCenter.On("Send", mock.Anything, mock.Anything).Return(fmt.Errorf(""))


Let's not accept anything. We should correctly type the arguments received in the send method. We don't have to do it for AddHandler below we cannot compare functions. HOwever we should at least ensure that the argument is of type func

mikeproeng37 · 2019-12-04T23:54:38Z

pkg/client/client_test.go

+func (s *ClientTestSuiteTrackNotification) TestOnTrackThrowsErrorWhenAddHandlerFails() {
+
+	mockNotificationCenter := new(MockNotificationCenter)
+	mockNotificationCenter.On("AddHandler", mock.Anything, mock.Anything).Return(-1, fmt.Errorf(""))


Assert the type, don't use mock.Anything

mikeproeng37 · 2019-12-04T23:54:49Z

pkg/client/client_test.go

+func (s *ClientTestSuiteTrackNotification) TestRemoveOnTrackThrowsErrorWhenRemoveHandlerFails() {
+
+	mockNotificationCenter := new(MockNotificationCenter)
+	mockNotificationCenter.On("AddHandler", mock.Anything, mock.Anything).Return(1, nil)


Same as above.

mikeproeng37 · 2019-12-04T23:55:58Z

pkg/client/factory.go

 	}

 	appClient := &OptimizelyClient{executionCtx: executionCtx}
+	appClient.NotificationCenter = registry.GetNotificationCenter(f.SDKKey)


This doesn't need to be set after the object init. You can put it right after the exec context

msohailhussain · 2019-12-05T19:52:29Z

pkg/client/client_test.go

+
+func (s *ClientTestSuiteTrackEvent) TestTrackWithNotification() {
+
+	s.mockProcessor.On("ProcessEvent", mock.Anything).Return(true)


mock.Anything is very general. We are using ConversionEvent, so please use that type.

msohailhussain · 2019-12-05T19:56:29Z

pkg/client/client_test.go

+		s.Equal(*s.mockProcessor.Events[0].Conversion, conversionEvent)
+	}
+
+	s.client.OnTrack(onTrack)


add return ID and assert that.

msohailhussain · 2019-12-05T19:57:14Z

pkg/client/client_test.go

+
+func (s *ClientTestSuiteTrackEvent) TestTrackWithNotificationAndEventTag() {
+
+	s.mockProcessor.On("ProcessEvent", mock.Anything).Return(true)


same as above.

msohailhussain · 2019-12-05T20:03:43Z

pkg/client/client_test.go

+}
+
+func (s *ClientTestSuiteTrackEvent) TestTrackNotificationNotCalledWhenEventProcessorReturnsFalse() {
+	s.mockProcessor.On("ProcessEvent", mock.Anything).Return(false)


same as above and do it in all other places.

msohailhussain · 2019-12-05T20:07:10Z

pkg/client/client_test.go

+	err := s.client.Track("sample_conversion", entities.UserContext{ID: "1212121", Attributes: map[string]interface{}{}}, map[string]interface{}{})
+
+	s.NoError(err)
+	s.Equal(1, len(s.mockProcessor.Events))


How it can be 1. I think this is confusing. The best way would be to mock event.CreateConversionEvent if possible. and return any invalid type and let ProcessEvent do its job. It will return error while invalid type is passed.

msohailhussain · 2019-12-05T20:31:01Z

pkg/client/client_test.go

+	}
+
+	s.client.OnTrack(onTrack)
+	err := s.client.Track("bob", entities.UserContext{ID: "1212121", Attributes: map[string]interface{}{}}, map[string]interface{}{})


this is confusing, what's invalid here. Please add comment.

Updated the test name.

msohailhussain · 2019-12-05T20:35:24Z

pkg/client/client_test.go

+	}
+}
+
+func (s *ClientTestSuiteTrackNotification) TestAddandRemoveOnTrack() {


Please break down this test into 2 or 3 tests.

Add multiple ontrack and assert

Add multiple ontrack, remove by ID and assert then.

Add, remove, add and assert.
Make sure, IDs are always increased(unique).

msohailhussain · 2019-12-05T20:36:17Z

pkg/client/client_test.go

+	s.Equal(0, numberOfCalls)
+}
+
+func (s *ClientTestSuiteTrackNotification) TestOnTrackThrowsErrorWithoutNotificationCenter() {


separate into two tests.

msohailhussain · 2019-12-06T21:51:38Z

LGTM.

mikeproeng37

lgtm

msohailhussain added 2 commits November 25, 2019 16:41

added track listener

470a332

added more unit tests

7b4a20d

msohailhussain changed the title ~~feat(onTrack): Added onTrack callback~~ feat(onTrack): Added onTrack callback - DO NOT REVIEW Nov 26, 2019

msohailhussain and others added 5 commits November 26, 2019 14:38

fixed minor things

00d6893

fixed one more issue

698f518

Merge branch 'master' into sohail/ontrack

fb08a20

Merge branch 'master' into sohail/ontrack

f4aade8

fixes.

1c667a2

msohailhussain changed the title ~~feat(onTrack): Added onTrack callback - DO NOT REVIEW~~ feat(onTrack): Added onTrack callback Nov 27, 2019

msohailhussain marked this pull request as ready for review November 27, 2019 18:19

msohailhussain requested a review from a team as a code owner November 27, 2019 18:19

Merge branch 'master' into sohail/ontrack

432180b

mikeproeng37 suggested changes Nov 27, 2019

View reviewed changes

optimizely-codereview-bot assigned msohailhussain Nov 27, 2019

yasirfolio3 and others added 4 commits November 28, 2019 10:54

Merge branch 'master' into sohail/ontrack

2b771f1

fixes.

74a79a0

nit fixed

0134e14

linter fixes.

62274c4

mikeproeng37 suggested changes Dec 2, 2019

View reviewed changes

yasirfolio3 and others added 2 commits December 3, 2019 13:34

Addressed review comments.

6694d3f

Merge branch 'master' into sohail/ontrack

66f24ac

mikeproeng37 suggested changes Dec 3, 2019

View reviewed changes

yasirfolio3 and others added 4 commits December 4, 2019 14:20

Merge branch 'master' into sohail/ontrack

6231265

Added track suite for track notification and event.

a690ae6

News Tests added to improve coverage.

659cd06

Merge branch 'master' into sohail/ontrack

2f160d8

mikeproeng37 suggested changes Dec 4, 2019

View reviewed changes

Merge branch 'master' into sohail/ontrack

4f93e52

yasirfolio3 added 2 commits December 5, 2019 12:58

implemented suggested changes.

5bcc272

nits fixed.

dde1317

msohailhussain commented Dec 5, 2019

View reviewed changes

yasirfolio3 added 2 commits December 6, 2019 11:01

Updated tests.

839ba78

Made notification center private for client.

b3a34ed

msohailhussain removed their assignment Dec 6, 2019

optimizely-codereview-bot requested review from mikeproeng37 and yasirfolio3 December 6, 2019 18:53

mikeproeng37 approved these changes Dec 6, 2019

View reviewed changes

mikeproeng37 merged commit f5b8ffe into master Dec 6, 2019

mikeproeng37 deleted the sohail/ontrack branch December 6, 2019 21:54


		func (s *ClientTestSuiteTrackEvent) TestTrackWithNotification() {

		s.mockProcessor.On("ProcessEvent", mock.Anything).Return(true)

feat(onTrack): Added onTrack callback #198

feat(onTrack): Added onTrack callback #198

Uh oh!

Conversation

msohailhussain commented Nov 26, 2019

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

msohailhussain commented Nov 27, 2019

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain commented Dec 2, 2019

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain commented Dec 6, 2019

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants