From 5643101ad1b67ce80061c456e23fc00b0408d70f Mon Sep 17 00:00:00 2001 From: Gabe Mulley Date: Wed, 8 Oct 2014 15:51:49 -0400 Subject: [PATCH] Small changes to the segment.io event handler --- common/djangoapps/track/views/segmentio.py | 20 ++++--- .../track/views/tests/test_segmentio.py | 55 +++++++++++++------ 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/common/djangoapps/track/views/segmentio.py b/common/djangoapps/track/views/segmentio.py index 01951c9496..d89d32ff88 100644 --- a/common/djangoapps/track/views/segmentio.py +++ b/common/djangoapps/track/views/segmentio.py @@ -93,14 +93,19 @@ def track_segmentio_event(request): # The POST body will contain the JSON encoded event full_segment_event = request.json + # We mostly care about the properties + segment_event = full_segment_event.get('properties', {}) + def logged_failure_response(*args, **kwargs): """Indicate a failure and log information about the event that will aide debugging efforts""" failed_response = failure_response(*args, **kwargs) log.warning('Unable to process event received from segment.io: %s', json.dumps(full_segment_event)) return failed_response - # Selectively listen to particular channels - channel = full_segment_event.get('channel') + # Selectively listen to particular channels, note that the client can set the "event_source" field in the + # "properties" dict to override the channel provided by segment.io. This is necessary because there is a bug in some + # segment.io client libraries that prevented them from sending correct channel fields. + channel = segment_event.get('event_source') allowed_channels = [c.lower() for c in getattr(settings, 'TRACKING_SEGMENTIO_ALLOWED_CHANNELS', [])] if not channel or channel.lower() not in allowed_channels: return response(WARNING_IGNORED_CHANNEL, committed=False) @@ -111,15 +116,15 @@ def track_segmentio_event(request): if not action or action.lower() not in allowed_actions: return response(WARNING_IGNORED_ACTION, committed=False) - # We mostly care about the properties - segment_event = full_segment_event.get('properties', {}) - context = {} # Start with the context provided by segment.io in the "client" field if it exists segment_context = full_segment_event.get('context') if segment_context: context['client'] = segment_context + user_agent = segment_context.get('userAgent', '') + else: + user_agent = '' # Overlay any context provided in the properties context.update(segment_event.get('context', {})) @@ -136,7 +141,7 @@ def track_segmentio_event(request): except ValueError: return logged_failure_response(ERROR_INVALID_USER_ID) else: - context['user_id'] = user_id + context['user_id'] = user.id # course_id is expected to be provided in the context when applicable course_id = context.get('course_id') @@ -173,6 +178,7 @@ def track_segmentio_event(request): event = { "username": user.username, "event_type": event_type, + "name": segment_event.get('name', ''), # Will be either "mobile", "browser" or "server". These names happen to be identical to the names we already # use so no mapping is necessary. "event_source": channel, @@ -181,7 +187,7 @@ def track_segmentio_event(request): "context": complete_context, "page": segment_event.get('page'), "host": complete_context.get('host', ''), - "agent": '', + "agent": user_agent, "ip": segment_event.get('ip', ''), "event": segment_event.get('event', {}), } diff --git a/common/djangoapps/track/views/tests/test_segmentio.py b/common/djangoapps/track/views/tests/test_segmentio.py index 54f9763882..d3746067a8 100644 --- a/common/djangoapps/track/views/tests/test_segmentio.py +++ b/common/djangoapps/track/views/tests/test_segmentio.py @@ -40,7 +40,7 @@ class SegmentIOTrackingTestCase(TestCase): self.mock_tracker = patcher.start() self.addCleanup(patcher.stop) - def test_segmentio_tracking_get_request(self): + def test_get_request(self): request = self.request_factory.get(ENDPOINT) response = segmentio.track_segmentio_event(request) self.assertEquals(response.status_code, 405) @@ -49,7 +49,7 @@ class SegmentIOTrackingTestCase(TestCase): @override_settings( TRACKING_SEGMENTIO_WEBHOOK_SECRET=None ) - def test_segmentio_tracking_no_secret_config(self): + def test_no_secret_config(self): request = self.request_factory.post(ENDPOINT) response = segmentio.track_segmentio_event(request) self.assert_segmentio_uncommitted_response(response, segmentio.ERROR_UNAUTHORIZED, 401) @@ -61,12 +61,12 @@ class SegmentIOTrackingTestCase(TestCase): self.assertEquals(parsed_content, {'committed': False, 'message': expected_message}) self.assertFalse(self.mock_tracker.send.called) # pylint: disable=maybe-no-member - def test_segmentio_tracking_no_secret_provided(self): + def test_no_secret_provided(self): request = self.request_factory.post(ENDPOINT) response = segmentio.track_segmentio_event(request) self.assert_segmentio_uncommitted_response(response, segmentio.ERROR_UNAUTHORIZED, 401) - def test_segmentio_tracking_secret_mismatch(self): + def test_secret_mismatch(self): request = self.create_request(key='y') response = segmentio.track_segmentio_event(request) self.assert_segmentio_uncommitted_response(response, segmentio.ERROR_UNAUTHORIZED, 401) @@ -93,7 +93,7 @@ class SegmentIOTrackingTestCase(TestCase): @data('server', 'browser', 'Browser') def test_segmentio_ignore_channels(self, channel): - response = self.post_segmentio_event(channel=channel) + response = self.post_segmentio_event(event_source=channel) self.assert_segmentio_uncommitted_response(response, segmentio.WARNING_IGNORED_CHANNEL, 200) def create_segmentio_event(self, **kwargs): @@ -104,17 +104,20 @@ class SegmentIOTrackingTestCase(TestCase): "event": "Did something", "properties": { 'event_type': kwargs.get('event_type', ''), + 'event_source': kwargs.get('event_source', 'mobile'), 'event': kwargs.get('event', {}), 'context': { 'course_id': kwargs.get('course_id') or '', - } + }, + 'name': str(sentinel.name), }, "channel": kwargs.get('channel', 'mobile'), "context": { "library": { "name": "unknown", "version": "unknown" - } + }, + 'userAgent': str(sentinel.user_agent), }, "receivedAt": "2014-08-27T16:33:39.100Z", "timestamp": "2014-08-27T16:33:39.215Z", @@ -129,22 +132,23 @@ class SegmentIOTrackingTestCase(TestCase): }, "action": action } + return sample_event def create_segmentio_event_json(self, **kwargs): """Return a json string containing a fake segment.io event""" return json.dumps(self.create_segmentio_event(**kwargs)) - def test_segmentio_tracking_no_user_for_user_id(self): + def test_no_user_for_user_id(self): response = self.post_segmentio_event(user_id=40) self.assert_segmentio_uncommitted_response(response, segmentio.ERROR_USER_NOT_EXIST, 400) - def test_segmentio_tracking_invalid_user_id(self): + def test_invalid_user_id(self): response = self.post_segmentio_event(user_id='foobar') self.assert_segmentio_uncommitted_response(response, segmentio.ERROR_INVALID_USER_ID, 400) @data('foo/bar/baz', 'course-v1:foo+bar+baz') - def test_segmentio_tracking(self, course_id): + def test_success(self, course_id): middleware = TrackMiddleware() request = self.create_request( @@ -165,8 +169,9 @@ class SegmentIOTrackingTestCase(TestCase): 'ip': '', 'event_source': 'mobile', 'event_type': str(sentinel.event_type), + 'name': str(sentinel.name), 'event': {'foo': 'bar'}, - 'agent': '', + 'agent': str(sentinel.user_agent), 'page': None, 'time': datetime.strptime("2014-08-27T16:33:39.215Z", "%Y-%m-%dT%H:%M:%S.%fZ"), 'host': 'testserver', @@ -179,7 +184,8 @@ class SegmentIOTrackingTestCase(TestCase): 'library': { 'name': 'unknown', 'version': 'unknown' - } + }, + 'userAgent': str(sentinel.user_agent) }, 'received_at': datetime.strptime("2014-08-27T16:33:39.100Z", "%Y-%m-%dT%H:%M:%S.%fZ"), }, @@ -189,7 +195,7 @@ class SegmentIOTrackingTestCase(TestCase): self.mock_tracker.send.assert_called_once_with(expected_event) # pylint: disable=maybe-no-member - def test_segmentio_tracking_invalid_course_id(self): + def test_invalid_course_id(self): request = self.create_request( data=self.create_segmentio_event_json(course_id='invalid'), content_type='application/json' @@ -199,9 +205,9 @@ class SegmentIOTrackingTestCase(TestCase): self.assertEquals(response.status_code, 200) self.assertTrue(self.mock_tracker.send.called) # pylint: disable=maybe-no-member - def test_segmentio_tracking_missing_event_type(self): + def test_missing_event_type(self): sample_event_raw = self.create_segmentio_event() - sample_event_raw['properties'] = {} + del sample_event_raw['properties']['event_type'] request = self.create_request( data=json.dumps(sample_event_raw), content_type='application/json' @@ -211,7 +217,7 @@ class SegmentIOTrackingTestCase(TestCase): response = segmentio.track_segmentio_event(request) self.assert_segmentio_uncommitted_response(response, segmentio.ERROR_MISSING_EVENT_TYPE, 400) - def test_segmentio_tracking_missing_timestamp(self): + def test_missing_timestamp(self): sample_event_raw = self.create_event_without_fields('timestamp') request = self.create_request( data=json.dumps(sample_event_raw), @@ -232,7 +238,7 @@ class SegmentIOTrackingTestCase(TestCase): return event - def test_segmentio_tracking_missing_received_at(self): + def test_missing_received_at(self): sample_event_raw = self.create_event_without_fields('receivedAt') request = self.create_request( data=json.dumps(sample_event_raw), @@ -242,3 +248,18 @@ class SegmentIOTrackingTestCase(TestCase): response = segmentio.track_segmentio_event(request) self.assert_segmentio_uncommitted_response(response, segmentio.ERROR_MISSING_RECEIVED_AT, 400) + + def test_string_user_id(self): + User.objects.create(pk=USER_ID, username=str(sentinel.username)) + response = self.post_segmentio_event(user_id=str(USER_ID)) + result = self.assert_segmentio_committed_response(response) + self.assertEquals(result['context']['user_id'], USER_ID) + + def assert_segmentio_committed_response(self, response): + """Assert that an event was emitted""" + self.assertEquals(response.status_code, 200) + parsed_content = json.loads(response.content) + self.assertEquals(parsed_content, {'committed': True}) + self.assertTrue(self.mock_tracker.send.called) # pylint: disable=maybe-no-member + return self.mock_tracker.send.mock_calls[0][1][0] +