From fade4a10af906cb71df7d8c5ef459a7080cc0748 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Fri, 2 Jun 2017 13:25:51 -0400 Subject: [PATCH] Implement event for forum thread views EDUCATOR-341 --- common/djangoapps/track/views/segmentio.py | 48 ++- common/djangoapps/track/views/tests/base.py | 112 ++++++ .../track/views/tests/test_segmentio.py | 149 ++------ common/djangoapps/util/testing.py | 17 + lms/djangoapps/discussion/tests/test_views.py | 159 ++++++-- lms/djangoapps/discussion/views.py | 75 +++- .../django_comment_client/base/__init__.py | 2 + .../base/event_transformers.py | 158 ++++++++ .../django_comment_client/base/tests.py | 348 +++++++++++++++++- .../django_comment_client/base/views.py | 19 +- lms/djangoapps/django_comment_client/utils.py | 37 +- lms/envs/common.py | 2 +- lms/lib/comment_client/thread.py | 34 +- lms/lib/comment_client/user.py | 36 +- 14 files changed, 985 insertions(+), 211 deletions(-) create mode 100644 common/djangoapps/track/views/tests/base.py create mode 100644 lms/djangoapps/django_comment_client/base/event_transformers.py diff --git a/common/djangoapps/track/views/segmentio.py b/common/djangoapps/track/views/segmentio.py index 3d6cb92500..7cdc35daf5 100644 --- a/common/djangoapps/track/views/segmentio.py +++ b/common/djangoapps/track/views/segmentio.py @@ -28,6 +28,10 @@ ERROR_MISSING_TIMESTAMP = 'Required timestamp field not found' ERROR_MISSING_RECEIVED_AT = 'Required receivedAt field not found' +FORUM_THREAD_VIEWED_EVENT_LABEL = 'Forum: View Thread' +BI_SCREEN_VIEWED_EVENT_NAME = u'edx.bi.app.navigation.screen' + + @require_POST @expect_json @csrf_exempt @@ -141,11 +145,8 @@ def track_segmentio_event(request): # pylint: disable=too-many-statements if not segment_event_type or (segment_event_type.lower() not in allowed_types): return - if 'name' not in segment_properties: - raise EventValidationError(ERROR_MISSING_NAME) - # Ignore event names that are unsupported - segment_event_name = segment_properties['name'] + segment_event_name = _get_segmentio_event_name(segment_properties) disallowed_substring_names = [ a.lower() for a in getattr(settings, 'TRACKING_SEGMENTIO_DISALLOWED_SUBSTRING_NAMES', []) ] @@ -220,10 +221,49 @@ def track_segmentio_event(request): # pylint: disable=too-many-statements context['ip'] = segment_properties.get('context', {}).get('ip', '') + # For Business Intelligence events: Add label to context + if 'label' in segment_properties: + context['label'] = segment_properties['label'] + + # For Android-sourced Business Intelligence events: add course ID to context + if 'course_id' in segment_properties: + context['course_id'] = segment_properties['course_id'] + with tracker.get_tracker().context('edx.segmentio', context): tracker.emit(segment_event_name, segment_properties.get('data', {})) +def _get_segmentio_event_name(event_properties): + """ + Get the name of a SegmentIO event. + + Args: + event_properties: dict + The properties of the event, which should contain the event's + name or, in the case of an old Android screen event, its screen + label. + + Returns: str + The name (or effective name) of the event. + + Note: + In older versions of the Android app, screen-view tracking events + did not have a name. So, in order to capture forum-thread-viewed events + from those old-versioned apps, we have to accept the event based on + its screen label. We return an event name that matches screen-view + events in the iOS app and newer versions of the Android app. + + Raises: + EventValidationError if name is missing + """ + if 'name' in event_properties: + return event_properties['name'] + elif event_properties.get('label') == FORUM_THREAD_VIEWED_EVENT_LABEL: + return BI_SCREEN_VIEWED_EVENT_NAME + else: + raise EventValidationError(ERROR_MISSING_NAME) + + def parse_iso8601_timestamp(timestamp): """Parse a particular type of ISO8601 formatted timestamp""" return datetime.datetime.strptime(timestamp, "%Y-%m-%dT%H:%M:%S.%fZ") diff --git a/common/djangoapps/track/views/tests/base.py b/common/djangoapps/track/views/tests/base.py new file mode 100644 index 0000000000..6c63359027 --- /dev/null +++ b/common/djangoapps/track/views/tests/base.py @@ -0,0 +1,112 @@ +""" +Base class for tests related to emitted events to one of the tracking 'views' +(e.g. SegmentIO). +""" +import json +from mock import sentinel + +from django.test.client import RequestFactory +from django.test.utils import override_settings + +from track.views import segmentio +from track.tests import EventTrackingTestCase + + +SEGMENTIO_TEST_SECRET = 'anything' +SEGMENTIO_TEST_ENDPOINT = '/segmentio/test/event' +SEGMENTIO_TEST_USER_ID = 10 + +_MOBILE_SHIM_PROCESSOR = [ + {'ENGINE': 'track.shim.LegacyFieldMappingProcessor'}, + {'ENGINE': 'track.shim.PrefixedEventProcessor'}, +] + + +@override_settings( + TRACKING_SEGMENTIO_WEBHOOK_SECRET=SEGMENTIO_TEST_SECRET, + TRACKING_IGNORE_URL_PATTERNS=[SEGMENTIO_TEST_ENDPOINT], + TRACKING_SEGMENTIO_ALLOWED_TYPES=['track'], + TRACKING_SEGMENTIO_DISALLOWED_SUBSTRING_NAMES=[], + TRACKING_SEGMENTIO_SOURCE_MAP={'test-app': 'mobile'}, + EVENT_TRACKING_PROCESSORS=_MOBILE_SHIM_PROCESSOR, +) +class SegmentIOTrackingTestCaseBase(EventTrackingTestCase): + """ + Base class for tests that test the processing of Segment events. + """ + + def setUp(self): + super(SegmentIOTrackingTestCaseBase, self).setUp() + self.maxDiff = None # pylint: disable=invalid-name + self.request_factory = RequestFactory() + + def create_request(self, key=None, **kwargs): + """Create a fake request that emulates a request from the Segment servers to ours""" + if key is None: + key = SEGMENTIO_TEST_SECRET + + request = self.request_factory.post(SEGMENTIO_TEST_ENDPOINT + "?key=" + key, **kwargs) + if 'data' in kwargs: + request.json = json.loads(kwargs['data']) + + return request + + def post_segmentio_event(self, **kwargs): + """Post a fake Segment event to the view that processes it""" + request = self.create_request( + data=self.create_segmentio_event_json(**kwargs), + content_type='application/json' + ) + segmentio.track_segmentio_event(request) + + def create_segmentio_event(self, **kwargs): + """Populate a fake Segment event with data of interest""" + action = kwargs.get('action', 'Track') + sample_event = { + "userId": kwargs.get('user_id', SEGMENTIO_TEST_USER_ID), + "event": "Did something", + "properties": { + 'name': kwargs.get('name', str(sentinel.name)), + 'data': kwargs.get('data', {}), + 'context': { + 'course_id': kwargs.get('course_id') or '', + 'app_name': 'edx.mobile.android', + } + }, + "channel": 'server', + "context": { + "library": { + "name": kwargs.get('library_name', 'test-app'), + "version": "unknown" + }, + "app": { + "version": "1.0.1", + }, + 'userAgent': str(sentinel.user_agent), + }, + "receivedAt": "2014-08-27T16:33:39.100Z", + "timestamp": "2014-08-27T16:33:39.215Z", + "type": action.lower(), + "projectId": "u0j33yjkr8", + "messageId": "qy52hwp4", + "version": 2, + "integrations": {}, + "options": { + "library": "unknown", + "providers": {} + }, + "action": action + } + + if 'context' in kwargs: + sample_event['properties']['context'].update(kwargs['context']) + if 'label' in kwargs: + sample_event['properties']['label'] = kwargs['label'] + if kwargs.get('exclude_name') is True: + del sample_event['properties']['name'] + + return sample_event + + def create_segmentio_event_json(self, **kwargs): + """Return a json string containing a fake Segment event""" + return json.dumps(self.create_segmentio_event(**kwargs)) diff --git a/common/djangoapps/track/views/tests/test_segmentio.py b/common/djangoapps/track/views/tests/test_segmentio.py index 8b0bf980f3..e23fb489c6 100644 --- a/common/djangoapps/track/views/tests/test_segmentio.py +++ b/common/djangoapps/track/views/tests/test_segmentio.py @@ -8,23 +8,17 @@ from mock import sentinel from nose.plugins.attrib import attr from django.contrib.auth.models import User -from django.test.client import RequestFactory from django.test.utils import override_settings from openedx.core.lib.tests.assertions.events import assert_event_matches from track.middleware import TrackMiddleware -from track.tests import EventTrackingTestCase from track.views import segmentio - - -SECRET = 'anything' -ENDPOINT = '/segmentio/test/event' -USER_ID = 10 - -MOBILE_SHIM_PROCESSOR = [ - {'ENGINE': 'track.shim.LegacyFieldMappingProcessor'}, - {'ENGINE': 'track.shim.PrefixedEventProcessor'}, -] +from track.views.tests.base import ( + SegmentIOTrackingTestCaseBase, + SEGMENTIO_TEST_SECRET, + SEGMENTIO_TEST_ENDPOINT, + SEGMENTIO_TEST_USER_ID +) def expect_failure_with_message(message): @@ -39,24 +33,13 @@ def expect_failure_with_message(message): @attr(shard=3) @ddt -@override_settings( - TRACKING_SEGMENTIO_WEBHOOK_SECRET=SECRET, - TRACKING_IGNORE_URL_PATTERNS=[ENDPOINT], - TRACKING_SEGMENTIO_ALLOWED_TYPES=['track'], - TRACKING_SEGMENTIO_DISALLOWED_SUBSTRING_NAMES=['.bi.'], - TRACKING_SEGMENTIO_SOURCE_MAP={'test-app': 'mobile'}, - EVENT_TRACKING_PROCESSORS=MOBILE_SHIM_PROCESSOR, -) -class SegmentIOTrackingTestCase(EventTrackingTestCase): - """Test processing of Segment events""" - - def setUp(self): - super(SegmentIOTrackingTestCase, self).setUp() - self.maxDiff = None # pylint: disable=invalid-name - self.request_factory = RequestFactory() +class SegmentIOTrackingTestCase(SegmentIOTrackingTestCaseBase): + """ + Test processing of Segment events. + """ def test_get_request(self): - request = self.request_factory.get(ENDPOINT) + request = self.request_factory.get(SEGMENTIO_TEST_ENDPOINT) response = segmentio.segmentio_event(request) self.assertEquals(response.status_code, 405) self.assert_no_events_emitted() @@ -65,13 +48,13 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): TRACKING_SEGMENTIO_WEBHOOK_SECRET=None ) def test_no_secret_config(self): - request = self.request_factory.post(ENDPOINT) + request = self.request_factory.post(SEGMENTIO_TEST_ENDPOINT) response = segmentio.segmentio_event(request) self.assertEquals(response.status_code, 401) self.assert_no_events_emitted() def test_no_secret_provided(self): - request = self.request_factory.post(ENDPOINT) + request = self.request_factory.post(SEGMENTIO_TEST_ENDPOINT) response = segmentio.segmentio_event(request) self.assertEquals(response.status_code, 401) self.assert_no_events_emitted() @@ -82,83 +65,11 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): self.assertEquals(response.status_code, 401) self.assert_no_events_emitted() - def create_request(self, key=None, **kwargs): - """Create a fake request that emulates a request from the Segment servers to ours""" - if key is None: - key = SECRET - - request = self.request_factory.post(ENDPOINT + "?key=" + key, **kwargs) - if 'data' in kwargs: - request.json = json.loads(kwargs['data']) - - return request - @data('identify', 'Group', 'Alias', 'Page', 'identify', 'screen') def test_segmentio_ignore_actions(self, action): self.post_segmentio_event(action=action) self.assert_no_events_emitted() - @data('edx.bi.some_name', 'EDX.BI.CAPITAL_NAME') - def test_segmentio_ignore_names(self, name): - self.post_segmentio_event(name=name) - self.assert_no_events_emitted() - - def post_segmentio_event(self, **kwargs): - """Post a fake Segment event to the view that processes it""" - request = self.create_request( - data=self.create_segmentio_event_json(**kwargs), - content_type='application/json' - ) - segmentio.track_segmentio_event(request) - - def create_segmentio_event(self, **kwargs): - """Populate a fake Segment event with data of interest""" - action = kwargs.get('action', 'Track') - sample_event = { - "userId": kwargs.get('user_id', USER_ID), - "event": "Did something", - "properties": { - 'name': kwargs.get('name', str(sentinel.name)), - 'data': kwargs.get('data', {}), - 'context': { - 'course_id': kwargs.get('course_id') or '', - 'app_name': 'edx.mobile.android', - } - }, - "channel": 'server', - "context": { - "library": { - "name": kwargs.get('library_name', 'test-app'), - "version": "unknown" - }, - "app": { - "version": "1.0.1", - }, - 'userAgent': str(sentinel.user_agent), - }, - "receivedAt": "2014-08-27T16:33:39.100Z", - "timestamp": "2014-08-27T16:33:39.215Z", - "type": action.lower(), - "projectId": "u0j33yjkr8", - "messageId": "qy52hwp4", - "version": 2, - "integrations": {}, - "options": { - "library": "unknown", - "providers": {} - }, - "action": action - } - - if 'context' in kwargs: - sample_event['properties']['context'].update(kwargs['context']) - - return sample_event - - def create_segmentio_event_json(self, **kwargs): - """Return a json string containing a fake Segment event""" - return json.dumps(self.create_segmentio_event(**kwargs)) - def test_segmentio_ignore_unknown_libraries(self): self.post_segmentio_event(library_name='foo') self.assert_no_events_emitted() @@ -179,7 +90,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): data=self.create_segmentio_event_json(data={'foo': 'bar'}, course_id=course_id), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) middleware.process_request(request) # The middleware normally emits an event, make sure it doesn't in this case. @@ -207,10 +118,10 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): 'name': 'edx.mobile.android', 'version': '1.0.1', }, - 'user_id': USER_ID, + 'user_id': SEGMENTIO_TEST_USER_ID, 'course_id': course_id, 'org_id': u'foo', - 'path': ENDPOINT, + 'path': SEGMENTIO_TEST_ENDPOINT, 'client': { 'library': { 'name': 'test-app', @@ -233,7 +144,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): data=self.create_segmentio_event_json(course_id='invalid'), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) segmentio.track_segmentio_event(request) self.assert_events_emitted() @@ -245,7 +156,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): data=json.dumps(sample_event_raw), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) segmentio.track_segmentio_event(request) @@ -257,7 +168,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): data=json.dumps(sample_event_raw), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) segmentio.track_segmentio_event(request) @@ -268,7 +179,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): data=json.dumps(sample_event_raw), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) segmentio.track_segmentio_event(request) @@ -279,7 +190,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): data=json.dumps(sample_event_raw), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) segmentio.track_segmentio_event(request) @@ -294,8 +205,8 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): return event def test_string_user_id(self): - User.objects.create(pk=USER_ID, username=str(sentinel.username)) - self.post_segmentio_event(user_id=str(USER_ID)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) + self.post_segmentio_event(user_id=str(SEGMENTIO_TEST_USER_ID)) self.assert_events_emitted() def test_hiding_failure(self): @@ -304,7 +215,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): data=json.dumps(sample_event_raw), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) response = segmentio.segmentio_event(request) self.assertEquals(response.status_code, 200) @@ -350,7 +261,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): }), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) middleware.process_request(request) try: @@ -371,10 +282,10 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): 'time': datetime.strptime("2014-08-27T16:33:39.215Z", "%Y-%m-%dT%H:%M:%S.%fZ"), 'host': 'testserver', 'context': { - 'user_id': USER_ID, + 'user_id': SEGMENTIO_TEST_USER_ID, 'course_id': course_id, 'org_id': 'foo', - 'path': ENDPOINT, + 'path': SEGMENTIO_TEST_ENDPOINT, 'client': { 'library': { 'name': 'test-app', @@ -484,7 +395,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): ), content_type='application/json' ) - User.objects.create(pk=USER_ID, username=str(sentinel.username)) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(sentinel.username)) middleware.process_request(request) try: @@ -505,10 +416,10 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase): 'time': datetime.strptime("2014-08-27T16:33:39.215Z", "%Y-%m-%dT%H:%M:%S.%fZ"), 'host': 'testserver', 'context': { - 'user_id': USER_ID, + 'user_id': SEGMENTIO_TEST_USER_ID, 'course_id': course_id, 'org_id': 'foo', - 'path': ENDPOINT, + 'path': SEGMENTIO_TEST_ENDPOINT, 'client': { 'library': { 'name': 'test-app', diff --git a/common/djangoapps/util/testing.py b/common/djangoapps/util/testing.py index 4e242805c2..e179497e50 100644 --- a/common/djangoapps/util/testing.py +++ b/common/djangoapps/util/testing.py @@ -96,12 +96,29 @@ class EventTestMixin(object): kwargs ) + def assert_event_emission_count(self, event_name, expected_count): + """ + Verify that the event with the given name was emitted + a specific number of times. + """ + actual_count = 0 + for call_args in self.mock_tracker.emit.call_args_list: + if call_args[0][0] == event_name: + actual_count += 1 + self.assertEqual(actual_count, expected_count) + def reset_tracker(self): """ Reset the mock tracker in order to forget about old events. """ self.mock_tracker.reset_mock() + def get_latest_call_args(self): + """ + Return the arguments of the latest call to emit. + """ + return self.mock_tracker.emit.call_args[0] + class PatchMediaTypeMixin(object): """ diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 2277e18fdf..78c997a1dd 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -4,15 +4,19 @@ from datetime import datetime import ddt from django.core.urlresolvers import reverse -from django.http import Http404 +from django.http import HttpResponse, Http404 from django.test.client import Client, RequestFactory from django.test.utils import override_settings from django.utils import translation from mock import ANY, Mock, call, patch from nose.tools import assert_true +from rest_framework.test import APIRequestFactory +from common.test.utils import MockSignalHandlerMixin, disable_signal from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory +from discussion_api import api +from discussion_api.tests.utils import CommentsServiceMockMixin, make_minimal_cs_thread from django_comment_client.constants import TYPE_ENTRY, TYPE_SUBCATEGORY from django_comment_client.permissions import get_team from django_comment_client.tests.group_id import ( @@ -20,6 +24,7 @@ from django_comment_client.tests.group_id import ( GroupIdAssertionMixin, NonCohortedTopicGroupIdTestMixin ) +from django_comment_client.base.views import create_thread from django_comment_client.tests.unicode import UnicodeTestMixin from django_comment_client.tests.utils import ( CohortedTestCase, @@ -28,21 +33,27 @@ from django_comment_client.tests.utils import ( topic_name_to_id ) from django_comment_client.utils import strip_none -from django_comment_common.models import CourseDiscussionSettings, ForumsConfig -from django_comment_common.utils import ThreadContext +from django_comment_common.models import ( + CourseDiscussionSettings, + ForumsConfig, + FORUM_ROLE_STUDENT, + Role +) +from django_comment_common.utils import ThreadContext, seed_permissions_roles from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from lms.djangoapps.discussion import views from lms.djangoapps.discussion.views import _get_discussion_default_topic_id from lms.djangoapps.discussion.views import course_discussions_settings_handler -from lms.djangoapps.teams.tests.factories import CourseTeamFactory +from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory from lms.lib.comment_client.utils import CommentClientPaginatedResult from openedx.core.djangoapps.course_groups.models import CourseUserGroup from openedx.core.djangoapps.course_groups.tests.helpers import config_course_cohorts from openedx.core.djangoapps.course_groups.tests.test_views import CohortViewsTestCase from openedx.core.djangoapps.util.testing import ContentGroupTestCase from openedx.features.enterprise_support.tests.mixins.enterprise import EnterpriseTestConsentRequired -from student.tests.factories import CourseEnrollmentFactory, UserFactory -from util.testing import UrlResetMixin +from student.roles import CourseStaffRole, UserBasedRole +from student.tests.factories import CourseAccessRoleFactory, CourseEnrollmentFactory, UserFactory +from util.testing import EventTestMixin, UrlResetMixin from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ( @@ -167,7 +178,7 @@ def make_mock_thread_data( return thread_data -def make_mock_request_impl( +def make_mock_perform_request_impl( course, text, thread_id="dummy_thread_id", @@ -175,11 +186,10 @@ def make_mock_request_impl( commentable_id=None, num_thread_responses=1, ): - def mock_request_impl(*args, **kwargs): + def mock_perform_request_impl(*args, **kwargs): url = args[1] - data = None if url.endswith("threads") or url.endswith("user_profile"): - data = { + return { "collection": [ make_mock_thread_data( course=course, @@ -192,7 +202,7 @@ def make_mock_request_impl( ] } elif thread_id and url.endswith(thread_id): - data = make_mock_thread_data( + return make_mock_thread_data( course=course, text=text, thread_id=thread_id, @@ -201,7 +211,7 @@ def make_mock_request_impl( commentable_id=commentable_id ) elif "/users/" in url: - data = { + res = { "default_sort_key": "date", "upvoted_ids": [], "downvoted_ids": [], @@ -209,13 +219,39 @@ def make_mock_request_impl( } # comments service adds these attributes when course_id param is present if kwargs.get('params', {}).get('course_id'): - data.update({ + res.update({ "threads_count": 1, "comments_count": 2 }) + return res + else: + return None + return mock_perform_request_impl + + +def make_mock_request_impl( + course, + text, + thread_id="dummy_thread_id", + group_id=None, + commentable_id=None, + num_thread_responses=1, +): + impl = make_mock_perform_request_impl( + course, + text, + thread_id=thread_id, + group_id=group_id, + commentable_id=commentable_id, + num_thread_responses=num_thread_responses + ) + + def mock_request_impl(*args, **kwargs): + data = impl(*args, **kwargs) if data: return Mock(status_code=200, text=json.dumps(data), json=Mock(return_value=data)) - return Mock(status_code=404) + else: + return Mock(status_code=404) return mock_request_impl @@ -370,18 +406,18 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): # course is outside the context manager that is verifying the number of queries, # and with split mongo, that method ends up querying disabled_xblocks (which is then # cached and hence not queried as part of call_single_thread). - (ModuleStoreEnum.Type.mongo, False, 1, 5, 3, 14, 1), - (ModuleStoreEnum.Type.mongo, False, 50, 5, 3, 14, 1), + (ModuleStoreEnum.Type.mongo, False, 1, 6, 4, 17, 4), + (ModuleStoreEnum.Type.mongo, False, 50, 6, 4, 17, 4), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 13, 1), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 13, 1), + (ModuleStoreEnum.Type.split, False, 1, 3, 3, 16, 4), + (ModuleStoreEnum.Type.split, False, 50, 3, 3, 16, 4), # Enabling Enterprise integration should have no effect on the number of mongo queries made. - (ModuleStoreEnum.Type.mongo, True, 1, 5, 3, 14, 1), - (ModuleStoreEnum.Type.mongo, True, 50, 5, 3, 14, 1), + (ModuleStoreEnum.Type.mongo, True, 1, 6, 4, 17, 4), + (ModuleStoreEnum.Type.mongo, True, 50, 6, 4, 17, 4), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 13, 1), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 13, 1), + (ModuleStoreEnum.Type.split, True, 1, 3, 3, 16, 4), + (ModuleStoreEnum.Type.split, True, 50, 3, 3, 16, 4), ) @ddt.unpack def test_number_of_mongo_queries( @@ -1917,3 +1953,82 @@ class DefaultTopicIdGetterTestCase(ModuleStoreTestCase): expected_id = 'another_discussion_id' result = _get_discussion_default_topic_id(course) self.assertEqual(expected_id, result) + + +class ThreadViewedEventTestCase(EventTestMixin, ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): + """ + Forum thread views are expected to launch analytics events. Test these here. + """ + + CATEGORY_ID = 'i4x-edx-discussion-id' + CATEGORY_NAME = 'Discussion 1' + PARENT_CATEGORY_NAME = 'Chapter 1' + + DUMMY_THREAD_ID = 'dummythreadids' + DUMMY_TITLE = 'Dummy title' + DUMMY_URL = 'https://example.com/dummy/url/' + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(ThreadViewedEventTestCase, self).setUp('eventtracking.tracker') + + self.course = CourseFactory.create() + seed_permissions_roles(self.course.id) + + PASSWORD = 'test' + self.student = UserFactory.create(password=PASSWORD) + CourseEnrollmentFactory(user=self.student, course_id=self.course.id) + + self.staff = UserFactory.create(is_staff=True) + UserBasedRole(user=self.staff, role=CourseStaffRole.ROLE).add_course(self.course.id) + + self.category = ItemFactory.create( + parent_location=self.course.location, + category='discussion', + discussion_id=self.CATEGORY_ID, + discussion_category=self.PARENT_CATEGORY_NAME, + discussion_target=self.CATEGORY_NAME, + ) + self.team = CourseTeamFactory.create( + name='Team 1', + course_id=self.course.id, + topic_id='arbitrary-topic-id', + discussion_topic_id=self.category.discussion_id, + ) + CourseTeamMembershipFactory.create(team=self.team, user=self.student) + self.client.login(username=self.student.username, password=PASSWORD) + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + @patch('lms.lib.comment_client.utils.perform_request') + def test_thread_viewed_event(self, mock_perform_request): + mock_perform_request.side_effect = make_mock_perform_request_impl( + course=self.course, + text=self.DUMMY_TITLE, + thread_id=self.DUMMY_THREAD_ID, + commentable_id=self.category.discussion_id, + ) + url = '/courses/{0}/discussion/forum/{1}/threads/{2}'.format( + unicode(self.course.id), + self.category.discussion_id, + self.DUMMY_THREAD_ID + ) + self.client.get(url, HTTP_X_REQUESTED_WITH='XMLHttpRequest') + + expected_event = { + 'id': self.DUMMY_THREAD_ID, + 'title': self.DUMMY_TITLE, + 'commentable_id': self.category.discussion_id, + 'category_id': self.category.discussion_id, + 'category_name': self.category.discussion_target, + 'user_forums_roles': [FORUM_ROLE_STUDENT], + 'user_course_roles': [], + 'target_username': self.student.username, + 'team_id': self.team.id, + 'url': self.DUMMY_URL, + } + expected_event_items = expected_event.items() + + self.assert_event_emission_count('edx.forum.thread.viewed', 1) + _, event = self.get_latest_call_args() + event_items = event.items() + self.assertTrue(kv_pair in event_items for kv_pair in expected_event_items) diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index ba44f365e3..4a0f126e18 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -28,6 +28,7 @@ import lms.lib.comment_client as cc from courseware.access import has_access from courseware.courses import get_course_with_access from courseware.views.views import CourseTabView +from django_comment_client.base.views import track_thread_viewed_event from django_comment_client.constants import TYPE_ENTRY from django_comment_client.permissions import get_team, has_permission from django_comment_client.utils import ( @@ -291,10 +292,13 @@ def single_thread(request, course_key, discussion_id, thread_id): cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() is_staff = has_permission(request.user, 'openclose_thread', course.id) - - thread = _find_thread(request, course, discussion_id=discussion_id, thread_id=thread_id) - if not thread: - raise Http404 + thread = _load_thread_for_viewing( + request, + course, + discussion_id=discussion_id, + thread_id=thread_id, + raise_event=True, + ) with newrelic_function_trace("get_annotated_content_infos"): annotated_content_info = utils.get_annotated_content_infos( @@ -358,6 +362,34 @@ def _find_thread(request, course, discussion_id, thread_id): return thread +def _load_thread_for_viewing(request, course, discussion_id, thread_id, raise_event): + """ + Loads the discussion thread with the specified ID and fires an + edx.forum.thread.viewed event. + + Args: + request: The Django request. + course_id: The ID of the owning course. + discussion_id: The ID of the owning discussion. + thread_id: The ID of the thread. + raise_event: Whether an edx.forum.thread.viewed tracking event should + be raised + + Returns: + The thread in question if the user can see it. + + Raises: + Http404 if the thread does not exist or the user cannot + see it. + """ + thread = _find_thread(request, course, discussion_id=discussion_id, thread_id=thread_id) + if not thread: + raise Http404 + if raise_event: + track_thread_viewed_event(request, course, thread) + return thread + + def _create_base_discussion_view_context(request, course_key): """ Returns the default template context for rendering any discussion view. @@ -393,20 +425,20 @@ def _get_discussion_default_topic_id(course): return entry['id'] -def _create_discussion_board_context(request, course_key, discussion_id=None, thread_id=None): +def _create_discussion_board_context(request, base_context, thread=None): """ Returns the template context for rendering the discussion board. """ - context = _create_base_discussion_view_context(request, course_key) + context = base_context.copy() course = context['course'] + course_key = course.id + thread_id = thread.id if thread else None + discussion_id = thread.commentable_id if thread else None course_settings = context['course_settings'] user = context['user'] cc_user = cc.User.from_django_user(user) user_info = context['user_info'] - if thread_id: - thread = _find_thread(request, course, discussion_id=discussion_id, thread_id=thread_id) - if not thread: - raise Http404 + if thread: # Since we're in page render mode, and the discussions UI will request the thread list itself, # we need only return the thread information for this one. @@ -637,12 +669,25 @@ class DiscussionBoardFragmentView(EdxFragmentView): """ course_key = CourseKey.from_string(course_id) try: - context = _create_discussion_board_context( - request, - course_key, - discussion_id=discussion_id, - thread_id=thread_id, + base_context = _create_base_discussion_view_context(request, course_key) + # Note: + # After the thread is rendered in this fragment, an AJAX + # request is made and the thread is completely loaded again + # (yes, this is something to fix). Because of this, we pass in + # raise_event=False to _load_thread_for_viewing avoid duplicate + # tracking events. + thread = ( + _load_thread_for_viewing( + request, + base_context['course'], + discussion_id=discussion_id, + thread_id=thread_id, + raise_event=False, + ) + if thread_id + else None ) + context = _create_discussion_board_context(request, base_context, thread=thread) html = render_to_string('discussion/discussion_board_fragment.html', context) inline_js = render_to_string('discussion/discussion_board_js.template', context) diff --git a/lms/djangoapps/django_comment_client/base/__init__.py b/lms/djangoapps/django_comment_client/base/__init__.py index e69de29bb2..4d367612a4 100644 --- a/lms/djangoapps/django_comment_client/base/__init__.py +++ b/lms/djangoapps/django_comment_client/base/__init__.py @@ -0,0 +1,2 @@ +# This import registers the ForumThreadViewedEventTransformer +import event_transformers # pylint: disable=unused-import diff --git a/lms/djangoapps/django_comment_client/base/event_transformers.py b/lms/djangoapps/django_comment_client/base/event_transformers.py new file mode 100644 index 0000000000..736f344f46 --- /dev/null +++ b/lms/djangoapps/django_comment_client/base/event_transformers.py @@ -0,0 +1,158 @@ +""" +Transformers for Discussion-related events. +""" +from django.contrib.auth.models import User +from django.core.urlresolvers import reverse, NoReverseMatch + +from eventtracking.processors.exceptions import EventEmissionExit +from opaque_keys import InvalidKeyError +from opaque_keys.edx.locator import CourseLocator + +from django_comment_client.base.views import add_truncated_title_to_event_data +from django_comment_client.permissions import get_team +from django_comment_client.utils import get_cached_discussion_id_map_by_course_id +from track.transformers import EventTransformer, EventTransformerRegistry +from track.views.segmentio import ( + BI_SCREEN_VIEWED_EVENT_NAME, + FORUM_THREAD_VIEWED_EVENT_LABEL +) + + +def _get_string(dictionary, key, del_if_bad=True): + """ + Get a string from a dictionary by key. + + If the key is not in the dictionary or does not refer to a string: + - Return None + - Optionally delete the key (del_if_bad) + """ + if key in dictionary: + value = dictionary[key] + if isinstance(value, basestring): + return value + else: + if del_if_bad: + del dictionary[key] + return None + else: + return None + + +@EventTransformerRegistry.register +class ForumThreadViewedEventTransformer(EventTransformer): + """ + Transformer for forum-thread-viewed mobile navigation events. + """ + + match_key = BI_SCREEN_VIEWED_EVENT_NAME + + def process_event(self): + """ + Process incoming mobile navigation events. + + For forum-thread-viewed events, change their names to + edx.forum.thread.viewed and manipulate their data to conform with + edx.forum.thread.viewed event design. + + Throw out other events. + """ + + # Get event context dict + # Throw out event if context nonexistent or wrong type + context = self.get('context') + if not isinstance(context, dict): + raise EventEmissionExit() + + # Throw out event if it's not a forum thread view + if _get_string(context, 'label', del_if_bad=False) != FORUM_THREAD_VIEWED_EVENT_LABEL: + raise EventEmissionExit() + + # Change name and event type + self['name'] = 'edx.forum.thread.viewed' + self['event_type'] = self['name'] + + # If no event data, set it to an empty dict + if 'event' not in self: + self['event'] = {} + self.event = {} + + # Throw out the context dict within the event data + # (different from the context dict extracted above) + if 'context' in self.event: + del self.event['context'] + + # Parse out course key + course_id_string = _get_string(context, 'course_id') if context else None + course_id = None + if course_id_string: + try: + course_id = CourseLocator.from_string(course_id_string) + except InvalidKeyError: + pass + + # Change 'thread_id' field to 'id' + thread_id = _get_string(self.event, 'thread_id') + if thread_id: + del self.event['thread_id'] + self.event['id'] = thread_id + + # Change 'topic_id' to 'commentable_id' + commentable_id = _get_string(self.event, 'topic_id') + if commentable_id: + del self.event['topic_id'] + self.event['commentable_id'] = commentable_id + + # Change 'action' to 'title' and truncate + title = _get_string(self.event, 'action') + if title is not None: + del self.event['action'] + add_truncated_title_to_event_data(self.event, title) + + # Change 'author' to 'target_username' + author = _get_string(self.event, 'author') + if author is not None: + del self.event['author'] + self.event['target_username'] = author + + # Load user + username = _get_string(self, 'username') + user = None + if username: + try: + user = User.objects.get(username=username) + except User.DoesNotExist: + pass + + # If in a category, add category name and ID + if course_id and commentable_id and user: + id_map = get_cached_discussion_id_map_by_course_id(course_id, [commentable_id], user) + if commentable_id in id_map: + self.event['category_name'] = id_map[commentable_id]['title'] + self.event['category_id'] = commentable_id + + # Add thread URL + if course_id and commentable_id and thread_id: + url_kwargs = { + 'course_id': course_id_string, + 'discussion_id': commentable_id, + 'thread_id': thread_id + } + try: + self.event['url'] = reverse('single_thread', kwargs=url_kwargs) + except NoReverseMatch: + pass + + # Add user's forum and course roles + if course_id and user: + self.event['user_forums_roles'] = [ + role.name for role in user.roles.filter(course_id=course_id) + ] + self.event['user_course_roles'] = [ + role.role for role in user.courseaccessrole_set.filter(course_id=course_id) + ] + + # Add team ID + if commentable_id: + team = get_team(commentable_id) + if team: + self.event['team_id'] = team.team_id diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 2a6df7f338..75c0323e0b 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -2,6 +2,7 @@ """Tests for django comment client views.""" import json import logging +import mock from contextlib import contextmanager import ddt @@ -9,6 +10,7 @@ from django.contrib.auth.models import User from django.core.management import call_command from django.core.urlresolvers import reverse from django.test.client import RequestFactory +from eventtracking.processors.exceptions import EventEmissionExit from mock import ANY, Mock, patch from nose.plugins.attrib import attr from nose.tools import assert_equal, assert_true @@ -25,18 +27,34 @@ from django_comment_client.tests.group_id import ( ) from django_comment_client.tests.unicode import UnicodeTestMixin from django_comment_client.tests.utils import CohortedTestCase, ForumsEnableMixin -from django_comment_common.models import CourseDiscussionSettings, Role, assign_role +from django_comment_common.models import ( + assign_role, + CourseDiscussionSettings, + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_STUDENT, + Role +) from django_comment_common.utils import ThreadContext, seed_permissions_roles, set_course_discussion_settings from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory from lms.lib.comment_client import Thread from openedx.core.djangoapps.course_groups.cohorts import set_course_cohorted from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from student.roles import CourseStaffRole, UserBasedRole from student.tests.factories import CourseAccessRoleFactory, CourseEnrollmentFactory, UserFactory from util.testing import UrlResetMixin from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls +from track.middleware import TrackMiddleware +from track.views import segmentio +from track.views.tests.base import ( + SegmentIOTrackingTestCaseBase, + SEGMENTIO_TEST_USER_ID +) + +from event_transformers import ForumThreadViewedEventTransformer + log = logging.getLogger(__name__) @@ -1734,7 +1752,7 @@ class ForumEventTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, MockReque @patch('eventtracking.tracker.emit') @patch('lms.lib.comment_client.utils.requests.request', autospec=True) - def test_thread_event(self, __, mock_emit): + def test_thread_created_event(self, __, mock_emit): request = RequestFactory().post( "dummy_url", { "thread_type": "discussion", @@ -1983,3 +2001,329 @@ class UsersEndpointTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, MockRe response = self.make_request(username="other") self.assertEqual(response.status_code, 200) self.assertEqual(json.loads(response.content)["users"], []) + + +@ddt.ddt +class SegmentIOForumThreadViewedEventTestCase(SegmentIOTrackingTestCaseBase): + + def _raise_navigation_event(self, label, include_name): + middleware = TrackMiddleware() + kwargs = {'label': label} + if include_name: + kwargs['name'] = 'edx.bi.app.navigation.screen' + else: + kwargs['exclude_name'] = True + request = self.create_request( + data=self.create_segmentio_event_json(**kwargs), + content_type='application/json', + ) + User.objects.create(pk=SEGMENTIO_TEST_USER_ID, username=str(mock.sentinel.username)) + middleware.process_request(request) + try: + response = segmentio.segmentio_event(request) + self.assertEquals(response.status_code, 200) + finally: + middleware.process_response(request, None) + + @ddt.data(True, False) + def test_thread_viewed(self, include_name): + """ + Tests that a SegmentIO thread viewed event is accepted and transformed. + + Only tests that the transformation happens at all; does not + comprehensively test that it happens correctly. + ForumThreadViewedEventTransformerTestCase tests for correctness. + """ + self._raise_navigation_event('Forum: View Thread', include_name) + event = self.get_event() + self.assertEqual(event['name'], 'edx.forum.thread.viewed') + self.assertEqual(event['event_type'], event['name']) + + @ddt.data(True, False) + def test_non_thread_viewed(self, include_name): + """ + Tests that other BI events are thrown out. + """ + self._raise_navigation_event('Forum: Create Thread', include_name) + self.assert_no_events_emitted() + + +def _get_transformed_event(input_event): + transformer = ForumThreadViewedEventTransformer(**input_event) + transformer.transform() + return transformer + + +def _create_event( + label='Forum: View Thread', + include_context=True, + inner_context=None, + username=None, + course_id=None, + **event_data +): + result = {'name': 'edx.bi.app.navigation.screen'} + if include_context: + result['context'] = {'label': label} + if course_id: + result['context']['course_id'] = str(course_id) + if username: + result['username'] = username + if event_data: + result['event'] = event_data + if inner_context: + if not event_data: + result['event'] = {} + result['event']['context'] = inner_context + return result + + +def _create_and_transform_event(**kwargs): + event = _create_event(**kwargs) + return event, _get_transformed_event(event) + + +@ddt.ddt +class ForumThreadViewedEventTransformerTestCase(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): + """ + Test that the ForumThreadViewedEventTransformer transforms events correctly + and without raising exceptions. + + Because the events passed through the transformer can come from external + sources (e.g., a mobile app), we carefully test a myriad of cases, including + those with incomplete and malformed events. + """ + + CATEGORY_ID = 'i4x-edx-discussion-id' + CATEGORY_NAME = 'Discussion 1' + PARENT_CATEGORY_NAME = 'Chapter 1' + + TEAM_CATEGORY_ID = 'i4x-edx-team-discussion-id' + TEAM_CATEGORY_NAME = 'Team Chat' + TEAM_PARENT_CATEGORY_NAME = PARENT_CATEGORY_NAME + + DUMMY_CATEGORY_ID = 'i4x-edx-dummy-commentable-id' + DUMMY_THREAD_ID = 'dummy_thread_id' + + @mock.patch.dict("student.models.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(ForumThreadViewedEventTransformerTestCase, self).setUp() + self.courses_by_store = { + ModuleStoreEnum.Type.mongo: CourseFactory.create( + org='TestX', + course='TR-101', + run='Event_Transform_Test', + default_store=ModuleStoreEnum.Type.mongo, + ), + ModuleStoreEnum.Type.split: CourseFactory.create( + org='TestX', + course='TR-101S', + run='Event_Transform_Test_Split', + default_store=ModuleStoreEnum.Type.split, + ), + } + self.course = self.courses_by_store['mongo'] + self.student = UserFactory.create() + self.staff = UserFactory.create(is_staff=True) + UserBasedRole(user=self.staff, role=CourseStaffRole.ROLE).add_course(self.course.id) + CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) + self.category = ItemFactory.create( + parent_location=self.course.location, + category='discussion', + discussion_id=self.CATEGORY_ID, + discussion_category=self.PARENT_CATEGORY_NAME, + discussion_target=self.CATEGORY_NAME, + ) + self.team_category = ItemFactory.create( + parent_location=self.course.location, + category='discussion', + discussion_id=self.TEAM_CATEGORY_ID, + discussion_category=self.TEAM_PARENT_CATEGORY_NAME, + discussion_target=self.TEAM_CATEGORY_NAME, + ) + self.team = CourseTeamFactory.create( + name='Team 1', + course_id=self.course.id, + topic_id='arbitrary-topic-id', + discussion_topic_id=self.team_category.discussion_id, + ) + + def test_missing_context(self): + event = _create_event(include_context=False) + with self.assertRaises(EventEmissionExit): + _get_transformed_event(event) + + def test_no_data(self): + event, event_trans = _create_and_transform_event() + event['name'] = 'edx.forum.thread.viewed' + event['event_type'] = event['name'] + event['event'] = {} + self.assertDictEqual(event_trans, event) + + def test_inner_context(self): + _, event_trans = _create_and_transform_event(inner_context={}) + self.assertNotIn('context', event_trans['event']) + + def test_non_thread_view(self): + event = _create_event( + label='Forum: Create Thread', + course_id=self.course.id, + topic_id=self.DUMMY_CATEGORY_ID, + thread_id=self.DUMMY_THREAD_ID, + ) + with self.assertRaises(EventEmissionExit): + _get_transformed_event(event) + + def test_bad_field_types(self): + event, event_trans = _create_and_transform_event( + course_id={}, + topic_id=3, + thread_id=object(), + action=3.14, + ) + event['name'] = 'edx.forum.thread.viewed' + event['event_type'] = event['name'] + self.assertDictEqual(event_trans, event) + + def test_bad_course_id(self): + event, event_trans = _create_and_transform_event(course_id='non-existent-course-id') + event_data = event_trans['event'] + self.assertNotIn('category_id', event_data) + self.assertNotIn('category_name', event_data) + self.assertNotIn('url', event_data) + self.assertNotIn('user_forums_roles', event_data) + self.assertNotIn('user_course_roles', event_data) + + def test_bad_username(self): + event, event_trans = _create_and_transform_event(username='non-existent-username') + event_data = event_trans['event'] + self.assertNotIn('category_id', event_data) + self.assertNotIn('category_name', event_data) + self.assertNotIn('user_forums_roles', event_data) + self.assertNotIn('user_course_roles', event_data) + + def test_bad_url(self): + event, event_trans = _create_and_transform_event( + course_id=self.course.id, + topic_id='malformed/commentable/id', + thread_id='malformed/thread/id', + ) + self.assertNotIn('url', event_trans['event']) + + def test_renamed_fields(self): + AUTHOR = 'joe-the-plumber' + event, event_trans = _create_and_transform_event( + course_id=self.course.id, + topic_id=self.DUMMY_CATEGORY_ID, + thread_id=self.DUMMY_THREAD_ID, + author=AUTHOR, + ) + self.assertEqual(event_trans['event']['commentable_id'], self.DUMMY_CATEGORY_ID) + self.assertEqual(event_trans['event']['id'], self.DUMMY_THREAD_ID) + self.assertEqual(event_trans['event']['target_username'], AUTHOR) + + def test_titles(self): + + # No title + _, event_1_trans = _create_and_transform_event() + self.assertNotIn('title', event_1_trans['event']) + self.assertNotIn('title_truncated', event_1_trans['event']) + + # Short title + _, event_2_trans = _create_and_transform_event( + action='!', + ) + self.assertIn('title', event_2_trans['event']) + self.assertIn('title_truncated', event_2_trans['event']) + self.assertFalse(event_2_trans['event']['title_truncated']) + + # Long title + _, event_3_trans = _create_and_transform_event( + action=('covfefe' * 200), + ) + self.assertIn('title', event_3_trans['event']) + self.assertIn('title_truncated', event_3_trans['event']) + self.assertTrue(event_3_trans['event']['title_truncated']) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_urls(self, store): + course = self.courses_by_store[store] + commentable_id = self.DUMMY_CATEGORY_ID + thread_id = self.DUMMY_THREAD_ID + _, event_trans = _create_and_transform_event( + course_id=course.id, + topic_id=commentable_id, + thread_id=thread_id, + ) + expected_path = '/courses/{0}/discussion/forum/{1}/threads/{2}'.format( + course.id, commentable_id, thread_id + ) + self.assertTrue(event_trans['event'].get('url').endswith(expected_path)) + + def test_categories(self): + + # Bad category + _, event_trans_1 = _create_and_transform_event( + username=self.student.username, + course_id=self.course.id, + topic_id='non-existent-category-id', + ) + self.assertNotIn('category_id', event_trans_1['event']) + self.assertNotIn('category_name', event_trans_1['event']) + + # Good category + _, event_trans_2 = _create_and_transform_event( + username=self.student.username, + course_id=self.course.id, + topic_id=self.category.discussion_id, + ) + self.assertEqual(event_trans_2['event'].get('category_id'), self.category.discussion_id) + full_category_name = '{0} / {1}'.format(self.category.discussion_category, self.category.discussion_target) + self.assertEqual(event_trans_2['event'].get('category_name'), full_category_name) + + def test_roles(self): + + # No user + _, event_trans_1 = _create_and_transform_event( + course_id=self.course.id, + ) + self.assertNotIn('user_forums_roles', event_trans_1['event']) + self.assertNotIn('user_course_roles', event_trans_1['event']) + + # Student user + _, event_trans_2 = _create_and_transform_event( + course_id=self.course.id, + username=self.student.username, + ) + self.assertEqual(event_trans_2['event'].get('user_forums_roles'), [FORUM_ROLE_STUDENT]) + self.assertEqual(event_trans_2['event'].get('user_course_roles'), []) + + # Course staff user + _, event_trans_3 = _create_and_transform_event( + course_id=self.course.id, + username=self.staff.username, + ) + self.assertEqual(event_trans_3['event'].get('user_forums_roles'), []) + self.assertEqual(event_trans_3['event'].get('user_course_roles'), [CourseStaffRole.ROLE]) + + def test_teams(self): + + # No category + _, event_trans_1 = _create_and_transform_event( + course_id=self.course.id, + ) + self.assertNotIn('team_id', event_trans_1) + + # Non-team category + _, event_trans_2 = _create_and_transform_event( + course_id=self.course.id, + topic_id=self.CATEGORY_ID, + ) + self.assertNotIn('team_id', event_trans_2) + + # Team category + _, event_trans_3 = _create_and_transform_event( + course_id=self.course.id, + topic_id=self.TEAM_CATEGORY_ID, + ) + self.assertEqual(event_trans_3['event'].get('team_id'), self.team.team_id) diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index a7ff804d95..6fdf23f81e 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -45,7 +45,7 @@ from django_comment_common.signals import ( thread_voted ) from django_comment_common.utils import ThreadContext -from eventtracking import tracker +import eventtracking from lms.djangoapps.courseware.exceptions import CourseAccessRedirect from util.file import store_uploaded_file @@ -82,7 +82,7 @@ def track_forum_event(request, event_name, course, obj, data, id_map=None): role.role for role in user.courseaccessrole_set.filter(course_id=course.id) ] - tracker.emit(event_name, data) + eventtracking.tracker.emit(event_name, data) def track_created_event(request, event_name, course, obj, data): @@ -97,7 +97,7 @@ def track_created_event(request, event_name, course, obj, data): track_forum_event(request, event_name, course, obj, data) -def add_truncated_title_to_event_data(event_data, full_title): +def add_truncated_title_to_event_data(event_data, full_title): # pylint: disable=invalid-name event_data['title_truncated'] = (len(full_title) > TRACKING_MAX_FORUM_TITLE) event_data['title'] = full_title[:TRACKING_MAX_FORUM_TITLE] @@ -158,6 +158,19 @@ def track_voted_event(request, course, obj, vote_value, undo_vote=False): track_forum_event(request, event_name, course, obj, event_data) +def track_thread_viewed_event(request, course, thread): + """ + Send analytics event for a viewed thread. + """ + event_name = _EVENT_NAME_TEMPLATE.format(obj_type='thread', action_name='viewed') + event_data = {} + event_data['commentable_id'] = thread.commentable_id + if hasattr(thread, 'username'): + event_data['target_username'] = thread.username + add_truncated_title_to_event_data(event_data, thread.title) + track_forum_event(request, event_name, course, thread, event_data) + + def permitted(func): """ View decorator to verify the user is authorized to access this endpoint. diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index a9eee26f58..a439ac8fc2 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -130,11 +130,19 @@ def get_accessible_discussion_xblocks(course, user, include_all=False): # pylin Return a list of all valid discussion xblocks in this course that are accessible to the given user. """ - all_xblocks = modulestore().get_items(course.id, qualifiers={'category': 'discussion'}, include_orphans=False) + return get_accessible_discussion_xblocks_by_course_id(course.id, user, include_all=include_all) + + +def get_accessible_discussion_xblocks_by_course_id(course_id, user, include_all=False): # pylint: disable=invalid-name + """ + Return a list of all valid discussion xblocks in this course that + are accessible to the given user. + """ + all_xblocks = modulestore().get_items(course_id, qualifiers={'category': 'discussion'}, include_orphans=False) return [ xblock for xblock in all_xblocks - if has_required_keys(xblock) and (include_all or has_access(user, 'load', xblock, course.id)) + if has_required_keys(xblock) and (include_all or has_access(user, 'load', xblock, course_id)) ] @@ -174,6 +182,14 @@ def get_cached_discussion_key(course_id, discussion_id): def get_cached_discussion_id_map(course, discussion_ids, user): + """ + Returns a dict mapping discussion_ids to respective discussion xblock metadata if it is cached and visible to the + user. If not, returns the result of get_discussion_id_map + """ + return get_cached_discussion_id_map_by_course_id(course.id, discussion_ids, user) + + +def get_cached_discussion_id_map_by_course_id(course_id, discussion_ids, user): # pylint: disable=invalid-name """ Returns a dict mapping discussion_ids to respective discussion xblock metadata if it is cached and visible to the user. If not, returns the result of get_discussion_id_map @@ -181,16 +197,16 @@ def get_cached_discussion_id_map(course, discussion_ids, user): try: entries = [] for discussion_id in discussion_ids: - key = get_cached_discussion_key(course.id, discussion_id) + key = get_cached_discussion_key(course_id, discussion_id) if not key: continue xblock = modulestore().get_item(key) - if not (has_required_keys(xblock) and has_access(user, 'load', xblock, course.id)): + if not (has_required_keys(xblock) and has_access(user, 'load', xblock, course_id)): continue entries.append(get_discussion_id_map_entry(xblock)) return dict(entries) except DiscussionIdMapIsNotCached: - return get_discussion_id_map(course, user) + return get_discussion_id_map_by_course_id(course_id, user) def get_discussion_id_map(course, user): @@ -198,7 +214,16 @@ def get_discussion_id_map(course, user): Transform the list of this course's discussion xblocks (visible to a given user) into a dictionary of metadata keyed by discussion_id. """ - return dict(map(get_discussion_id_map_entry, get_accessible_discussion_xblocks(course, user))) + return get_discussion_id_map_by_course_id(course.id, user) + + +def get_discussion_id_map_by_course_id(course_id, user): # pylint: disable=invalid-name + """ + Transform the list of this course's discussion xblocks (visible to a given user) into a dictionary of metadata keyed + by discussion_id. + """ + xblocks = get_accessible_discussion_xblocks_by_course_id(course_id, user) + return dict(map(get_discussion_id_map_entry, xblocks)) def _filter_unstarted_categories(category_map, course): diff --git a/lms/envs/common.py b/lms/envs/common.py index 984277fa06..3be2b78cb7 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -739,7 +739,7 @@ if FEATURES.get('ENABLE_SQL_TRACKING_LOGS'): TRACKING_SEGMENTIO_WEBHOOK_SECRET = None TRACKING_SEGMENTIO_ALLOWED_TYPES = ['track'] -TRACKING_SEGMENTIO_DISALLOWED_SUBSTRING_NAMES = ['.bi.'] +TRACKING_SEGMENTIO_DISALLOWED_SUBSTRING_NAMES = [] TRACKING_SEGMENTIO_SOURCE_MAP = { 'analytics-android': 'mobile', 'analytics-ios': 'mobile', diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index 07ab6d3c39..2e9aa717ee 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -5,15 +5,7 @@ import settings import models from eventtracking import tracker -from .utils import ( - CommentClientPaginatedResult, - CommentClientRequestError, - extract, - merge_dict, - perform_request, - strip_blank, - strip_none -) +import utils log = logging.getLogger(__name__) @@ -61,15 +53,15 @@ class Thread(models.Model): default_params = {'page': 1, 'per_page': 20, 'course_id': query_params['course_id']} - params = merge_dict(default_params, strip_blank(strip_none(query_params))) + params = utils.merge_dict(default_params, utils.strip_blank(utils.strip_none(query_params))) if query_params.get('text'): url = cls.url(action='search') else: - url = cls.url(action='get_all', params=extract(params, 'commentable_id')) + url = cls.url(action='get_all', params=utils.extract(params, 'commentable_id')) if params.get('commentable_id'): del params['commentable_id'] - response = perform_request( + response = utils.perform_request( 'get', url, params, @@ -107,7 +99,7 @@ class Thread(models.Model): ) ) - return CommentClientPaginatedResult( + return utils.CommentClientPaginatedResult( collection=response.get('collection', []), page=response.get('page', 1), num_pages=response.get('num_pages', 1), @@ -149,9 +141,9 @@ class Thread(models.Model): 'resp_skip': kwargs.get('response_skip'), 'resp_limit': kwargs.get('response_limit'), } - request_params = strip_none(request_params) + request_params = utils.strip_none(request_params) - response = perform_request( + response = utils.perform_request( 'get', url, request_params, @@ -166,9 +158,9 @@ class Thread(models.Model): elif voteable.type == 'comment': url = _url_for_flag_comment(voteable.id) else: - raise CommentClientRequestError("Can only flag/unflag threads or comments") + raise utils.CommentClientRequestError("Can only flag/unflag threads or comments") params = {'user_id': user.id} - response = perform_request( + response = utils.perform_request( 'put', url, params, @@ -183,13 +175,13 @@ class Thread(models.Model): elif voteable.type == 'comment': url = _url_for_unflag_comment(voteable.id) else: - raise CommentClientRequestError("Can only flag/unflag for threads or comments") + raise utils.CommentClientRequestError("Can only flag/unflag for threads or comments") params = {'user_id': user.id} #if you're an admin, when you unflag, remove ALL flags if removeAll: params['all'] = True - response = perform_request( + response = utils.perform_request( 'put', url, params, @@ -201,7 +193,7 @@ class Thread(models.Model): def pin(self, user, thread_id): url = _url_for_pin_thread(thread_id) params = {'user_id': user.id} - response = perform_request( + response = utils.perform_request( 'put', url, params, @@ -213,7 +205,7 @@ class Thread(models.Model): def un_pin(self, user, thread_id): url = _url_for_un_pin_thread(thread_id) params = {'user_id': user.id} - response = perform_request( + response = utils.perform_request( 'put', url, params, diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 22106504c2..56787c0c53 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -3,7 +3,7 @@ import settings import models -from .utils import CommentClientPaginatedResult, CommentClientRequestError, merge_dict, perform_request +import utils class User(models.Model): @@ -36,7 +36,7 @@ class User(models.Model): Calls cs_comments_service to mark thread as read for the user """ params = {'source_type': source.type, 'source_id': source.id} - perform_request( + utils.perform_request( 'post', _url_for_read(self.id), params, @@ -46,7 +46,7 @@ class User(models.Model): def follow(self, source): params = {'source_type': source.type, 'source_id': source.id} - response = perform_request( + response = utils.perform_request( 'post', _url_for_subscription(self.id), params, @@ -56,7 +56,7 @@ class User(models.Model): def unfollow(self, source): params = {'source_type': source.type, 'source_id': source.id} - response = perform_request( + response = utils.perform_request( 'delete', _url_for_subscription(self.id), params, @@ -70,9 +70,9 @@ class User(models.Model): elif voteable.type == 'comment': url = _url_for_vote_comment(voteable.id) else: - raise CommentClientRequestError("Can only vote / unvote for threads or comments") + raise utils.CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id, 'value': value} - response = perform_request( + response = utils.perform_request( 'put', url, params, @@ -87,9 +87,9 @@ class User(models.Model): elif voteable.type == 'comment': url = _url_for_vote_comment(voteable.id) else: - raise CommentClientRequestError("Can only vote / unvote for threads or comments") + raise utils.CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id} - response = perform_request( + response = utils.perform_request( 'delete', url, params, @@ -100,11 +100,11 @@ class User(models.Model): def active_threads(self, query_params={}): if not self.course_id: - raise CommentClientRequestError("Must provide course_id when retrieving active threads for the user") + raise utils.CommentClientRequestError("Must provide course_id when retrieving active threads for the user") url = _url_for_user_active_threads(self.id) params = {'course_id': self.course_id.to_deprecated_string()} - params = merge_dict(params, query_params) - response = perform_request( + params = utils.merge_dict(params, query_params) + response = utils.perform_request( 'get', url, params, @@ -116,11 +116,11 @@ class User(models.Model): def subscribed_threads(self, query_params={}): if not self.course_id: - raise CommentClientRequestError("Must provide course_id when retrieving subscribed threads for the user") + raise utils.CommentClientRequestError("Must provide course_id when retrieving subscribed threads for the user") url = _url_for_user_subscribed_threads(self.id) params = {'course_id': self.course_id.to_deprecated_string()} - params = merge_dict(params, query_params) - response = perform_request( + params = utils.merge_dict(params, query_params) + response = utils.perform_request( 'get', url, params, @@ -128,7 +128,7 @@ class User(models.Model): metric_tags=self._metric_tags, paged_results=True ) - return CommentClientPaginatedResult( + return utils.CommentClientPaginatedResult( collection=response.get('collection', []), page=response.get('page', 1), num_pages=response.get('num_pages', 1), @@ -144,19 +144,19 @@ class User(models.Model): if self.attributes.get('group_id'): retrieve_params['group_id'] = self.group_id try: - response = perform_request( + response = utils.perform_request( 'get', url, retrieve_params, metric_action='model.retrieve', metric_tags=self._metric_tags, ) - except CommentClientRequestError as e: + except utils.CommentClientRequestError as e: if e.status_code == 404: # attempt to gracefully recover from a previous failure # to sync this user to the comments service. self.save() - response = perform_request( + response = utils.perform_request( 'get', url, retrieve_params,