From bbb14a2c6a213f7de2a740c9f14326b9c028c83d Mon Sep 17 00:00:00 2001 From: Ken Clary Date: Fri, 30 Jul 2021 10:23:23 -0400 Subject: [PATCH] feat: add explicit courserun_key parameter to /event endpoint We add 'courserun_key' (aka "course_id" though that's technically a misnomer) as an optional parameter to the /event endpoint url. If it is not present, it will still be parsed out of the url, if the url is of the right format. Additionally, Logger.log() in js adds this parameter to its /event call, pulling it from the $$course_id global. This provides opportunity for MFEs to (separately) provide the key without concern about url parsing. TNL-7752 --- common/djangoapps/track/contexts.py | 22 +++++++---- .../djangoapps/track/tests/test_contexts.py | 17 +++++++++ common/djangoapps/track/views/__init__.py | 5 ++- .../track/views/tests/test_views.py | 26 +++++++++++++ common/static/js/spec/logger_spec.js | 38 ++++++++----------- common/static/js/src/logger.js | 7 +++- .../js/spec/edxnotes/views/note_item_spec.js | 3 +- 7 files changed, 85 insertions(+), 33 deletions(-) diff --git a/common/djangoapps/track/contexts.py b/common/djangoapps/track/contexts.py index db7923b1b8..8bbd40ed39 100644 --- a/common/djangoapps/track/contexts.py +++ b/common/djangoapps/track/contexts.py @@ -20,19 +20,27 @@ def course_context_from_url(url, course_id_string=None): url = url or '' course_id = None - if course_id_string is None: + if not course_id_string: match = COURSE_REGEX.match(url) if match: course_id_string = match.group('course_id') - try: - course_id = CourseKey.from_string(course_id_string) - except InvalidKeyError: - log.warning( - 'unable to parse course_id "{course_id}"'.format( - course_id=course_id_string + if not course_id_string: + log.debug( + 'no course_id found in "{url}"'.format( + url=str(url)[0:256] ), exc_info=True ) + else: + try: + course_id = CourseKey.from_string(course_id_string) + except InvalidKeyError: + log.warning( + 'unable to parse course_id "{course_id}"'.format( + course_id=str(course_id_string)[0:256] + ), + exc_info=True + ) return course_context_from_course_id(course_id) diff --git a/common/djangoapps/track/tests/test_contexts.py b/common/djangoapps/track/tests/test_contexts.py index e216c60fc0..fc3a7d97d0 100644 --- a/common/djangoapps/track/tests/test_contexts.py +++ b/common/djangoapps/track/tests/test_contexts.py @@ -13,6 +13,23 @@ class TestContexts(TestCase): # lint-amnesty, pylint: disable=missing-class-doc SPLIT_COURSE_ID = 'course-v1:test+course_name+course_run' ORG_ID = 'test' + @ddt.data( + (COURSE_ID, ''), + (COURSE_ID, '/more/stuff'), + (COURSE_ID, '?format=json'), + (SPLIT_COURSE_ID, ''), + (SPLIT_COURSE_ID, '/more/stuff'), + (SPLIT_COURSE_ID, '?format=json') + ) + @ddt.unpack + def test_explicit_course_id(self, course_id, postfix): + url = f'http://foo.bar.com/courses/not-a-course-id{postfix}' + self.assert_parses_explicit_course_id(url, course_id) + + def assert_parses_explicit_course_id(self, url, course_id): + assert contexts.course_context_from_url(url, course_id_string=course_id) ==\ + {'course_id': course_id, 'org_id': self.ORG_ID, 'enterprise_uuid': ''} + @ddt.data( (COURSE_ID, ''), (COURSE_ID, '/more/stuff'), diff --git a/common/djangoapps/track/views/__init__.py b/common/djangoapps/track/views/__init__.py index 9021183224..b111623e2a 100644 --- a/common/djangoapps/track/views/__init__.py +++ b/common/djangoapps/track/views/__init__.py @@ -79,16 +79,17 @@ def user_track(request): name = _get_request_value(request, 'event_type') data = _get_request_value(request, 'event', {}) - course_id_string = _get_request_value(request, 'courserun_key', None) page = _get_request_value(request, 'page') if isinstance(data, str) and len(data) > 0: try: data = json.loads(data) - _add_user_id_for_username(data) except ValueError: pass + _add_user_id_for_username(data) + course_id_string = data.get('courserun_key') if (data and 'courserun_key' in data) else None + context_override = contexts.course_context_from_url(page, course_id_string) context_override['username'] = username context_override['event_source'] = 'browser' diff --git a/common/djangoapps/track/views/tests/test_views.py b/common/djangoapps/track/views/tests/test_views.py index 2ffc754908..b12b37efc6 100644 --- a/common/djangoapps/track/views/tests/test_views.py +++ b/common/djangoapps/track/views/tests/test_views.py @@ -42,6 +42,32 @@ class TestTrackViews(EventTrackingTestCase): # lint-amnesty, pylint: disable=mi } def test_user_track(self): + request = self.request_factory.get('/event', { + 'page': self.url_with_course, + 'event_type': sentinel.event_type, + 'event': '{"courserun_key": "explicit/course/id"}' + }) + + views.user_track(request) + + actual_event = self.get_event() + expected_event = { + 'context': { + 'course_id': 'explicit/course/id', + 'org_id': 'explicit', + 'event_source': 'browser', + 'page': self.url_with_course, + 'username': 'anonymous' + }, + 'data': { + 'courserun_key': 'explicit/course/id' + }, + 'timestamp': FROZEN_TIME, + 'name': str(sentinel.event_type) + } + assert_event_matches(expected_event, actual_event) + + def test_user_track_with_implicit_course_id(self): request = self.request_factory.get('/event', { 'page': self.url_with_course, 'event_type': sentinel.event_type, diff --git a/common/static/js/spec/logger_spec.js b/common/static/js/spec/logger_spec.js index 03c0ed18e5..3c92005200 100644 --- a/common/static/js/spec/logger_spec.js +++ b/common/static/js/spec/logger_spec.js @@ -10,14 +10,13 @@ // proper deprecation and notification for external authors. it('can send a request to log event', function() { spyOn(jQuery, 'ajaxWithPrefix'); - Logger.log('example', 'data'); + Logger.log('example'); expect(jQuery.ajaxWithPrefix).toHaveBeenCalledWith({ url: '/event', type: 'POST', data: { event_type: 'example', - event: '"data"', - courserun_key: 'edx/999/test', + event: '{"courserun_key":"edX/999/test"}', page: window.location.href }, async: true @@ -26,14 +25,13 @@ it('can send a request with custom options to log event', function() { spyOn(jQuery, 'ajaxWithPrefix'); - Logger.log('example', 'data', null, {type: 'GET', async: false}); + Logger.log('example', null, null, {type: 'GET', async: false}); expect(jQuery.ajaxWithPrefix).toHaveBeenCalledWith({ url: '/event', type: 'GET', data: { event_type: 'example', - event: '"data"', - courserun_key: 'edx/999/test', + event: '{"courserun_key":"edX/999/test"}', page: window.location.href }, async: false @@ -61,14 +59,13 @@ $meta_tag = $(''); $meta_tag.appendTo('body'); spyOn(jQuery, 'ajax'); - Logger.log('example', 'data'); + Logger.log('example'); expect(jQuery.ajax).toHaveBeenCalledWith({ url: 'undefined/event', type: 'POST', data: { event_type: 'example', - event: '"data"', - courserun_key: 'edx/999/test', + event: '{"courserun_key":"edX/999/test"}', page: window.location.href }, async: true @@ -79,14 +76,13 @@ $meta_tag = $(''); $meta_tag.appendTo('body'); spyOn(jQuery, 'ajax'); - Logger.log('example', 'data'); + Logger.log('example'); expect(jQuery.ajax).toHaveBeenCalledWith({ url: '/event', type: 'POST', data: { event_type: 'example', - event: '"data"', - courserun_key: 'edx/999/test', + event: '{"courserun_key":"edX/999/test"}', page: window.location.href }, async: true @@ -97,14 +93,13 @@ $meta_tag = $(''); $meta_tag.appendTo('body'); spyOn(jQuery, 'ajax'); - Logger.log('example', 'data'); + Logger.log('example'); expect(jQuery.ajax).toHaveBeenCalledWith({ url: 'testpath/event', type: 'POST', data: { event_type: 'example', - event: '"data"', - courserun_key: 'edx/999/test', + event: '{"courserun_key":"edX/999/test"}', page: window.location.href }, async: true @@ -127,24 +122,24 @@ }); it('can listen to events when the element name is unknown', function() { - Logger.log('example', 'data'); - expect(this.callbacks[0]).toHaveBeenCalledWith('example', 'data', null); - expect(this.callbacks[1]).toHaveBeenCalledWith('example', 'data', null); + Logger.log('example'); + expect(this.callbacks[0]).toHaveBeenCalledWith('example', undefined, null); + expect(this.callbacks[1]).toHaveBeenCalledWith('example', undefined, null); expect(this.callbacks[2]).not.toHaveBeenCalled(); expect(this.callbacks[3]).not.toHaveBeenCalled(); }); it('can listen to events when the element name is known', function() { - Logger.log('example', 'data', 'element'); + Logger.log('example', null, 'element'); expect(this.callbacks[0]).not.toHaveBeenCalled(); expect(this.callbacks[1]).not.toHaveBeenCalled(); - expect(this.callbacks[2]).toHaveBeenCalledWith('example', 'data', 'element'); + expect(this.callbacks[2]).toHaveBeenCalledWith('example', null, 'element'); expect(this.callbacks[3]).not.toHaveBeenCalled(); }); it('can catch exceptions', function() { var callback = function() { - Logger.log('exception', 'data'); + Logger.log('exception'); }; Logger.listen('exception', null, function() { throw new Error(); @@ -188,7 +183,6 @@ data: { event_type: 'page_close', event: '', - courserun_key: 'edx/999/test', page: window.location.href }, async: false diff --git a/common/static/js/src/logger.js b/common/static/js/src/logger.js index c031b55e0e..7fb1d7f5ae 100644 --- a/common/static/js/src/logger.js +++ b/common/static/js/src/logger.js @@ -56,10 +56,15 @@ } } // Regardless of whether any callbacks were made, log this event. + if (typeof $$course_id !== 'undefined') { + if (!data) { + data = {} + } + data.courserun_key = $$course_id; + } return sendRequest({ event_type: eventType, event: JSON.stringify(data), - courserun_key: $$course_id, page: window.location.href }, requestOptions); }, diff --git a/lms/static/js/spec/edxnotes/views/note_item_spec.js b/lms/static/js/spec/edxnotes/views/note_item_spec.js index 624d97f5dc..a0a84b8a09 100644 --- a/lms/static/js/spec/edxnotes/views/note_item_spec.js +++ b/lms/static/js/spec/edxnotes/views/note_item_spec.js @@ -119,7 +119,8 @@ define([ { note_id: 'id-123', component_usage_id: 'usage_id-123', - view: 'Test View' + view: 'Test View', + courserun_key: 'edX/999/test' }, null, {