From f7c5a109d4e0ea6db1a0665dc32c1ad64e409f40 Mon Sep 17 00:00:00 2001 From: Manjinder Singh <49171515+jinder1s@users.noreply.github.com> Date: Thu, 5 Aug 2021 18:05:20 -0400 Subject: [PATCH] Revert "feat: add explicit courserun_key parameter to /event endpoint" (#28410) --- common/djangoapps/track/contexts.py | 25 +++++---------- .../djangoapps/track/tests/test_contexts.py | 17 ---------- common/djangoapps/track/views/__init__.py | 9 ++---- .../track/views/tests/test_views.py | 26 --------------- common/static/js/spec/logger_spec.js | 32 +++++++++---------- common/static/js/src/logger.js | 6 ---- .../js/spec/edxnotes/views/note_item_spec.js | 3 +- 7 files changed, 28 insertions(+), 90 deletions(-) diff --git a/common/djangoapps/track/contexts.py b/common/djangoapps/track/contexts.py index 8bbd40ed39..531b62b385 100644 --- a/common/djangoapps/track/contexts.py +++ b/common/djangoapps/track/contexts.py @@ -12,32 +12,23 @@ from openedx.core.lib.request_utils import COURSE_REGEX log = logging.getLogger(__name__) -def course_context_from_url(url, course_id_string=None): +def course_context_from_url(url): """ - If course_id_string string is not present, extracts it from the given `url`. Either way, then passes - it on to `course_context_from_course_id()`. + Extracts the course_context from the given `url` and passes it on to + `course_context_from_course_id()`. """ url = url or '' - course_id = None - if not course_id_string: - match = COURSE_REGEX.match(url) - if match: - course_id_string = match.group('course_id') - if not course_id_string: - log.debug( - 'no course_id found in "{url}"'.format( - url=str(url)[0:256] - ), - exc_info=True - ) - else: + match = COURSE_REGEX.match(url) + course_id = None + 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=str(course_id_string)[0:256] + course_id=course_id_string ), exc_info=True ) diff --git a/common/djangoapps/track/tests/test_contexts.py b/common/djangoapps/track/tests/test_contexts.py index fc3a7d97d0..e216c60fc0 100644 --- a/common/djangoapps/track/tests/test_contexts.py +++ b/common/djangoapps/track/tests/test_contexts.py @@ -13,23 +13,6 @@ 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 b111623e2a..2d3deb3bde 100644 --- a/common/djangoapps/track/views/__init__.py +++ b/common/djangoapps/track/views/__init__.py @@ -69,8 +69,7 @@ def user_track(request): """ Log when POST call to "event" URL is made by a user. - GET or POST call should provide "event_type", "event", and "page" arguments. It may optionally provide - a "courserun_key" argument (otherwise may be extracted from the page). + GET or POST call should provide "event_type", "event", and "page" arguments. """ try: username = request.user.username @@ -84,13 +83,11 @@ def user_track(request): 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 = contexts.course_context_from_url(page) context_override['username'] = username context_override['event_source'] = 'browser' context_override['page'] = page diff --git a/common/djangoapps/track/views/tests/test_views.py b/common/djangoapps/track/views/tests/test_views.py index b12b37efc6..2ffc754908 100644 --- a/common/djangoapps/track/views/tests/test_views.py +++ b/common/djangoapps/track/views/tests/test_views.py @@ -42,32 +42,6 @@ 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 3c92005200..2939420465 100644 --- a/common/static/js/spec/logger_spec.js +++ b/common/static/js/spec/logger_spec.js @@ -10,13 +10,13 @@ // proper deprecation and notification for external authors. it('can send a request to log event', function() { spyOn(jQuery, 'ajaxWithPrefix'); - Logger.log('example'); + Logger.log('example', 'data'); expect(jQuery.ajaxWithPrefix).toHaveBeenCalledWith({ url: '/event', type: 'POST', data: { event_type: 'example', - event: '{"courserun_key":"edX/999/test"}', + event: '"data"', page: window.location.href }, async: true @@ -25,13 +25,13 @@ it('can send a request with custom options to log event', function() { spyOn(jQuery, 'ajaxWithPrefix'); - Logger.log('example', null, null, {type: 'GET', async: false}); + Logger.log('example', 'data', null, {type: 'GET', async: false}); expect(jQuery.ajaxWithPrefix).toHaveBeenCalledWith({ url: '/event', type: 'GET', data: { event_type: 'example', - event: '{"courserun_key":"edX/999/test"}', + event: '"data"', page: window.location.href }, async: false @@ -59,13 +59,13 @@ $meta_tag = $(''); $meta_tag.appendTo('body'); spyOn(jQuery, 'ajax'); - Logger.log('example'); + Logger.log('example', 'data'); expect(jQuery.ajax).toHaveBeenCalledWith({ url: 'undefined/event', type: 'POST', data: { event_type: 'example', - event: '{"courserun_key":"edX/999/test"}', + event: '"data"', page: window.location.href }, async: true @@ -76,13 +76,13 @@ $meta_tag = $(''); $meta_tag.appendTo('body'); spyOn(jQuery, 'ajax'); - Logger.log('example'); + Logger.log('example', 'data'); expect(jQuery.ajax).toHaveBeenCalledWith({ url: '/event', type: 'POST', data: { event_type: 'example', - event: '{"courserun_key":"edX/999/test"}', + event: '"data"', page: window.location.href }, async: true @@ -93,13 +93,13 @@ $meta_tag = $(''); $meta_tag.appendTo('body'); spyOn(jQuery, 'ajax'); - Logger.log('example'); + Logger.log('example', 'data'); expect(jQuery.ajax).toHaveBeenCalledWith({ url: 'testpath/event', type: 'POST', data: { event_type: 'example', - event: '{"courserun_key":"edX/999/test"}', + event: '"data"', page: window.location.href }, async: true @@ -122,24 +122,24 @@ }); it('can listen to events when the element name is unknown', function() { - Logger.log('example'); - expect(this.callbacks[0]).toHaveBeenCalledWith('example', undefined, null); - expect(this.callbacks[1]).toHaveBeenCalledWith('example', undefined, null); + Logger.log('example', 'data'); + expect(this.callbacks[0]).toHaveBeenCalledWith('example', 'data', null); + expect(this.callbacks[1]).toHaveBeenCalledWith('example', 'data', 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', null, 'element'); + Logger.log('example', 'data', 'element'); expect(this.callbacks[0]).not.toHaveBeenCalled(); expect(this.callbacks[1]).not.toHaveBeenCalled(); - expect(this.callbacks[2]).toHaveBeenCalledWith('example', null, 'element'); + expect(this.callbacks[2]).toHaveBeenCalledWith('example', 'data', 'element'); expect(this.callbacks[3]).not.toHaveBeenCalled(); }); it('can catch exceptions', function() { var callback = function() { - Logger.log('exception'); + Logger.log('exception', 'data'); }; Logger.listen('exception', null, function() { throw new Error(); diff --git a/common/static/js/src/logger.js b/common/static/js/src/logger.js index 7fb1d7f5ae..2eb0710148 100644 --- a/common/static/js/src/logger.js +++ b/common/static/js/src/logger.js @@ -56,12 +56,6 @@ } } // 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), 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 a0a84b8a09..624d97f5dc 100644 --- a/lms/static/js/spec/edxnotes/views/note_item_spec.js +++ b/lms/static/js/spec/edxnotes/views/note_item_spec.js @@ -119,8 +119,7 @@ define([ { note_id: 'id-123', component_usage_id: 'usage_id-123', - view: 'Test View', - courserun_key: 'edX/999/test' + view: 'Test View' }, null, {