From 2117d168a4f611d185307e1d82c4e9d35eb8296f Mon Sep 17 00:00:00 2001 From: Dino Cikatic Date: Fri, 27 Feb 2015 13:24:33 +0100 Subject: [PATCH 1/4] SOL-217 Adding basic analytics events logging for courseware search and index --- .../views/tests/test_course_index.py | 6 +- .../xmodule/modulestore/courseware_index.py | 69 ++++++++++++++++++- .../xmodule/modulestore/mongo/draft.py | 2 +- .../modulestore/split_mongo/split_draft.py | 2 +- common/lib/xmodule/xmodule/tests/__init__.py | 5 +- .../search/collections/search_collection.js | 5 +- .../js/search/views/search_item_view.js | 34 ++++++++- .../js/search/views/search_list_view.js | 2 +- lms/static/js/spec/search/search_spec.js | 1 + 9 files changed, 114 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index f44cfe49f8..6c11083966 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -571,15 +571,13 @@ class TestCourseReIndex(CourseTestCase): self.assertEqual(response['results'], []) # Start manual reindex - errors = CoursewareSearchIndexer.do_course_reindex(modulestore(), self.course.id) - self.assertEqual(errors, None) + CoursewareSearchIndexer.do_course_reindex(modulestore(), self.course.id) self.html.display_name = "My expanded HTML" modulestore().update_item(self.html, ModuleStoreEnum.UserID.test) # Start manual reindex - errors = CoursewareSearchIndexer.do_course_reindex(modulestore(), self.course.id) - self.assertEqual(errors, None) + CoursewareSearchIndexer.do_course_reindex(modulestore(), self.course.id) # Check results indexed now response = perform_search( diff --git a/common/lib/xmodule/xmodule/modulestore/courseware_index.py b/common/lib/xmodule/xmodule/modulestore/courseware_index.py index 1aebdd3bab..182117687c 100644 --- a/common/lib/xmodule/xmodule/modulestore/courseware_index.py +++ b/common/lib/xmodule/xmodule/modulestore/courseware_index.py @@ -3,6 +3,7 @@ from __future__ import absolute_import import logging +from django.conf import settings from django.utils.translation import ugettext as _ from opaque_keys.edx.locator import CourseLocator from search.search_engine_base import SearchEngine @@ -10,6 +11,7 @@ from search.search_engine_base import SearchEngine from . import ModuleStoreEnum from .exceptions import ItemNotFoundError + # Use default index and document names for now INDEX_NAME = "courseware_index" DOCUMENT_TYPE = "courseware_content" @@ -36,6 +38,7 @@ class CoursewareSearchIndexer(object): Add to courseware search index from given location and its children """ error_list = [] + indexed_count = 0 # TODO - inline for now, need to move this out to a celery task searcher = SearchEngine.get_search_engine(INDEX_NAME) if not searcher: @@ -115,6 +118,7 @@ class CoursewareSearchIndexer(object): remove_index_item_location(location) else: index_item_location(location, None) + indexed_count += 1 except Exception as err: # pylint: disable=broad-except # broad exception so that index operation does not prevent the rest of the application from working log.exception( @@ -127,9 +131,72 @@ class CoursewareSearchIndexer(object): if raise_on_error and error_list: raise SearchIndexingError(_('Error(s) present during indexing'), error_list) + return indexed_count + + @classmethod + def do_publish_index(cls, modulestore, location, delete=False, raise_on_error=False): + """ + Add to courseware search index published section and children + """ + indexed_count = cls.add_to_search_index(modulestore, location, delete, raise_on_error) + CoursewareSearchIndexer._track_index_request('edx.course.index.published', indexed_count, str(location)) + return indexed_count + @classmethod def do_course_reindex(cls, modulestore, course_key): """ (Re)index all content within the given course """ - return cls.add_to_search_index(modulestore, course_key, delete=False, raise_on_error=True) + indexed_count = cls.add_to_search_index(modulestore, course_key, delete=False, raise_on_error=True) + CoursewareSearchIndexer._track_index_request('edx.course.index.reindexed', indexed_count) + return indexed_count + + @staticmethod + def _track_index_request(event_name, indexed_count, location=None): + """Track content index requests. + + Arguments: + location (str): The ID of content to be indexed. + event_name (str): Name of the event to be logged. + Returns: + None + + """ + + from eventtracking import tracker as track + tracker = track.get_tracker() + tracking_context = tracker.resolve_context() # pylint: disable=no-member + data = { + "indexed_count": indexed_count, + 'category': 'courseware_index', + } + + if location: + data['location_id'] = location + + tracker.emit( + event_name, + data + ) + + try: + if settings.FEATURES.get('SEGMENT_IO_LMS') and hasattr(settings, 'SEGMENT_IO_LMS_KEY'): + #Google Analytics - log index content request + import analytics + + analytics.track( + event_name, + data, + context={ + 'Google Analytics': { + 'clientId': tracking_context.get('client_id') + } + } + ) + except Exception: # pylint: disable=broad-except + # Capturing all exceptions thrown while tracking analytics events. We do not want + # an operation to fail because of an analytics event, so we will capture these + # errors in the logs. + log.exception( + u'Unable to emit {0} event for content indexing.'.format(event_name) + ) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 368095cb0c..5c30f5e379 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -732,7 +732,7 @@ class DraftModuleStore(MongoModuleStore): self.signal_handler.send("course_published", course_key=course_key) # Now it's been published, add the object to the courseware search index so that it appears in search results - CoursewareSearchIndexer.add_to_search_index(self, location) + CoursewareSearchIndexer.do_publish_index(self, location) return self.get_item(as_published(location)) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 0bbcbc360d..7d84e56124 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -357,7 +357,7 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli ) # Now it's been published, add the object to the courseware search index so that it appears in search results - CoursewareSearchIndexer.add_to_search_index(self, location) + CoursewareSearchIndexer.do_publish_index(self, location) return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published), **kwargs) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 44118f9e7a..64d62b2318 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -18,6 +18,8 @@ from lazy import lazy from mock import Mock from operator import attrgetter from path import path +from eventtracking import tracker +from eventtracking.django import DjangoTracker from xblock.field_data import DictFieldData from xblock.fields import ScopeIds, Scope, Reference, ReferenceList, ReferenceValueDict @@ -49,7 +51,6 @@ open_ended_grading_interface = { 'grading_controller': 'grading_controller', } - class TestModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ ModuleSystem for testing @@ -59,6 +60,8 @@ class TestModuleSystem(ModuleSystem): # pylint: disable=abstract-method kwargs.setdefault('id_reader', id_manager) kwargs.setdefault('id_generator', id_manager) kwargs.setdefault('services', {}).setdefault('field-data', DictFieldData({})) + self.tracker = DjangoTracker() + tracker.register_tracker(self.tracker) super(TestModuleSystem, self).__init__(**kwargs) def handler_url(self, block, handler, suffix='', query='', thirdparty=False): diff --git a/lms/static/js/search/collections/search_collection.js b/lms/static/js/search/collections/search_collection.js index bd5689f512..282be95087 100644 --- a/lms/static/js/search/collections/search_collection.js +++ b/lms/static/js/search/collections/search_collection.js @@ -62,7 +62,10 @@ define([ }, error: function (self, xhr) { self.trigger('error'); - } + }, + add: true, + reset: false, + remove: false }); }, diff --git a/lms/static/js/search/views/search_item_view.js b/lms/static/js/search/views/search_item_view.js index 4811087923..b5150529e2 100644 --- a/lms/static/js/search/views/search_item_view.js +++ b/lms/static/js/search/views/search_item_view.js @@ -4,8 +4,9 @@ define([ 'jquery', 'underscore', 'backbone', - 'gettext' -], function ($, _, Backbone, gettext) { + 'gettext', + 'logger' +], function ($, _, Backbone, gettext, Logger) { 'use strict'; return Backbone.View.extend({ @@ -17,6 +18,10 @@ define([ 'aria-label': 'search result' }, + events: { + 'click .search-results-item a': 'logSearchItem', + }, + initialize: function () { var template_name = (this.model.attributes.content_type === "Sequence") ? '#search_item_seq-tpl' : '#search_item-tpl'; this.tpl = _.template($(template_name).html()); @@ -25,9 +30,34 @@ define([ render: function () { this.$el.html(this.tpl(this.model.attributes)); return this; + }, + + logSearchItem: function(event) { + event.preventDefault(); + var target = this.model.id; + var link = $(event.target).attr('href'); + var collection = this.model.collection; + var page = collection.page; + var pageSize = collection.pageSize; + var searchTerm = collection.searchTerm; + var index = collection.indexOf(this.model); + Logger.log("edx.course.search.result_selected", + { + "search_term": searchTerm, + "result_position": (page * pageSize + index), + "result_link": target + }); + window.analytics.track('"edx.course.search.result_selected"', { + category: 'courseware_search', + search_term: searchTerm, + result_position: (page * pageSize + index), + result_link: target + }); + window.location.href = link; } }); }); })(define || RequireJS.define); + diff --git a/lms/static/js/search/views/search_list_view.js b/lms/static/js/search/views/search_list_view.js index eae1230e74..7a8d2c45e5 100644 --- a/lms/static/js/search/views/search_list_view.js +++ b/lms/static/js/search/views/search_list_view.js @@ -57,7 +57,7 @@ define([ var item = new SearchItemView({ model: result }); return item.render().el; }); - this.$el.find('.search-results').append(items); + this.$el.find('.search-results').html(items); }, totalCountMsg: function () { diff --git a/lms/static/js/spec/search/search_spec.js b/lms/static/js/spec/search/search_spec.js index 5130cdce34..4b22afe6b0 100644 --- a/lms/static/js/spec/search/search_spec.js +++ b/lms/static/js/spec/search/search_spec.js @@ -353,6 +353,7 @@ define([ expect(this.listView.$el).toContainHtml('Search Results'); expect(this.listView.$el).toContainHtml('this is a short excerpt'); + searchResults[1] = searchResults[0] this.collection.set(searchResults); this.collection.totalCount = 2; this.listView.renderNext(); From dd34bada713b5df8d014b61f8512610382bbc190 Mon Sep 17 00:00:00 2001 From: Martyn James Date: Thu, 5 Mar 2015 12:40:29 -0500 Subject: [PATCH 2/4] pylinty minty --- common/lib/xmodule/xmodule/modulestore/courseware_index.py | 5 +++-- common/lib/xmodule/xmodule/tests/__init__.py | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/courseware_index.py b/common/lib/xmodule/xmodule/modulestore/courseware_index.py index 182117687c..4375ef3242 100644 --- a/common/lib/xmodule/xmodule/modulestore/courseware_index.py +++ b/common/lib/xmodule/xmodule/modulestore/courseware_index.py @@ -181,7 +181,7 @@ class CoursewareSearchIndexer(object): try: if settings.FEATURES.get('SEGMENT_IO_LMS') and hasattr(settings, 'SEGMENT_IO_LMS_KEY'): - #Google Analytics - log index content request + # Google Analytics - log index content request import analytics analytics.track( @@ -198,5 +198,6 @@ class CoursewareSearchIndexer(object): # an operation to fail because of an analytics event, so we will capture these # errors in the logs. log.exception( - u'Unable to emit {0} event for content indexing.'.format(event_name) + u'Unable to emit %s event for content indexing.', + event_name, ) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 64d62b2318..522999cb5c 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -51,6 +51,7 @@ open_ended_grading_interface = { 'grading_controller': 'grading_controller', } + class TestModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ ModuleSystem for testing From ee90c5fe364afcdda0b9365201097be09db29620 Mon Sep 17 00:00:00 2001 From: Dino Cikatic Date: Fri, 6 Mar 2015 13:37:12 +0100 Subject: [PATCH 3/4] SOL-217 Remove Google Analytics calls --- .../xmodule/modulestore/courseware_index.py | 23 ------------------- .../js/search/views/search_item_view.js | 16 +++++++------ lms/static/js/spec/search/search_spec.js | 12 ++++++++++ 3 files changed, 21 insertions(+), 30 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/courseware_index.py b/common/lib/xmodule/xmodule/modulestore/courseware_index.py index 4375ef3242..48b726d355 100644 --- a/common/lib/xmodule/xmodule/modulestore/courseware_index.py +++ b/common/lib/xmodule/xmodule/modulestore/courseware_index.py @@ -178,26 +178,3 @@ class CoursewareSearchIndexer(object): event_name, data ) - - try: - if settings.FEATURES.get('SEGMENT_IO_LMS') and hasattr(settings, 'SEGMENT_IO_LMS_KEY'): - # Google Analytics - log index content request - import analytics - - analytics.track( - event_name, - data, - context={ - 'Google Analytics': { - 'clientId': tracking_context.get('client_id') - } - } - ) - except Exception: # pylint: disable=broad-except - # Capturing all exceptions thrown while tracking analytics events. We do not want - # an operation to fail because of an analytics event, so we will capture these - # errors in the logs. - log.exception( - u'Unable to emit %s event for content indexing.', - event_name, - ) diff --git a/lms/static/js/search/views/search_item_view.js b/lms/static/js/search/views/search_item_view.js index b5150529e2..53f9f2e79c 100644 --- a/lms/static/js/search/views/search_item_view.js +++ b/lms/static/js/search/views/search_item_view.js @@ -32,6 +32,14 @@ define([ return this; }, + /** + * Redirect to a URL. Mainly useful for mocking out in tests. + * @param {string} url The URL to redirect to. + */ + redirect: function(url) { + window.location.href = url; + }, + logSearchItem: function(event) { event.preventDefault(); var target = this.model.id; @@ -47,13 +55,7 @@ define([ "result_position": (page * pageSize + index), "result_link": target }); - window.analytics.track('"edx.course.search.result_selected"', { - category: 'courseware_search', - search_term: searchTerm, - result_position: (page * pageSize + index), - result_link: target - }); - window.location.href = link; + this.redirect(link); } }); diff --git a/lms/static/js/spec/search/search_spec.js b/lms/static/js/spec/search/search_spec.js index 4b22afe6b0..34ca36f22d 100644 --- a/lms/static/js/spec/search/search_spec.js +++ b/lms/static/js/spec/search/search_spec.js @@ -117,6 +117,18 @@ define([ expect(this.item.$el).toContainHtml(breadcrumbs); }); + it('log request on follow item link', function () { + this.model.collection = new SearchCollection([this.model], { course_id: 'edx101' }); + this.item.render(); + // Mock the redirect call + spyOn(this.item, 'redirect').andCallFake( function() {} ); + spyOn(this.item, 'logSearchItem').andCallThrough(); + var link = this.item.$el.find('a'); + expect(link.length).toBe(1); + link.trigger('click'); + expect(this.item.redirect).toHaveBeenCalled(); + }); + }); From 5d54036090e469b4611ab51ff3d89746f9db4aa7 Mon Sep 17 00:00:00 2001 From: Martyn James Date: Fri, 6 Mar 2015 13:31:01 -0500 Subject: [PATCH 4/4] Fix pylint violation --- .../xmodule/xmodule/modulestore/courseware_index.py | 10 +++------- common/lib/xmodule/xmodule/tests/__init__.py | 5 ++++- lms/static/js/search/views/search_item_view.js | 4 +++- lms/static/js/spec/search/search_spec.js | 3 +++ 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/courseware_index.py b/common/lib/xmodule/xmodule/modulestore/courseware_index.py index 48b726d355..ff55b1a721 100644 --- a/common/lib/xmodule/xmodule/modulestore/courseware_index.py +++ b/common/lib/xmodule/xmodule/modulestore/courseware_index.py @@ -3,10 +3,10 @@ from __future__ import absolute_import import logging -from django.conf import settings from django.utils.translation import ugettext as _ from opaque_keys.edx.locator import CourseLocator from search.search_engine_base import SearchEngine +from eventtracking import tracker from . import ModuleStoreEnum from .exceptions import ItemNotFoundError @@ -139,7 +139,7 @@ class CoursewareSearchIndexer(object): Add to courseware search index published section and children """ indexed_count = cls.add_to_search_index(modulestore, location, delete, raise_on_error) - CoursewareSearchIndexer._track_index_request('edx.course.index.published', indexed_count, str(location)) + cls._track_index_request('edx.course.index.published', indexed_count, str(location)) return indexed_count @classmethod @@ -148,7 +148,7 @@ class CoursewareSearchIndexer(object): (Re)index all content within the given course """ indexed_count = cls.add_to_search_index(modulestore, course_key, delete=False, raise_on_error=True) - CoursewareSearchIndexer._track_index_request('edx.course.index.reindexed', indexed_count) + cls._track_index_request('edx.course.index.reindexed', indexed_count) return indexed_count @staticmethod @@ -162,10 +162,6 @@ class CoursewareSearchIndexer(object): None """ - - from eventtracking import tracker as track - tracker = track.get_tracker() - tracking_context = tracker.resolve_context() # pylint: disable=no-member data = { "indexed_count": indexed_count, 'category': 'courseware_index', diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 522999cb5c..e8771915b4 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -12,6 +12,7 @@ import os import pprint import unittest import inspect +import mock from contextlib import contextmanager from lazy import lazy @@ -21,6 +22,7 @@ from path import path from eventtracking import tracker from eventtracking.django import DjangoTracker + from xblock.field_data import DictFieldData from xblock.fields import ScopeIds, Scope, Reference, ReferenceList, ReferenceValueDict @@ -56,7 +58,8 @@ class TestModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ ModuleSystem for testing """ - def __init__(self, **kwargs): + @mock.patch('eventtracking.tracker.emit') + def __init__(self, mock_emit, **kwargs): # pylint: disable=unused-argument id_manager = CourseLocationManager(kwargs['course_id']) kwargs.setdefault('id_reader', id_manager) kwargs.setdefault('id_generator', id_manager) diff --git a/lms/static/js/search/views/search_item_view.js b/lms/static/js/search/views/search_item_view.js index 53f9f2e79c..30a5565385 100644 --- a/lms/static/js/search/views/search_item_view.js +++ b/lms/static/js/search/views/search_item_view.js @@ -42,6 +42,7 @@ define([ logSearchItem: function(event) { event.preventDefault(); + var self = this; var target = this.model.id; var link = $(event.target).attr('href'); var collection = this.model.collection; @@ -54,8 +55,9 @@ define([ "search_term": searchTerm, "result_position": (page * pageSize + index), "result_link": target + }).always(function() { + self.redirect(link); }); - this.redirect(link); } }); diff --git a/lms/static/js/spec/search/search_spec.js b/lms/static/js/spec/search/search_spec.js index 34ca36f22d..adc3260515 100644 --- a/lms/static/js/spec/search/search_spec.js +++ b/lms/static/js/spec/search/search_spec.js @@ -2,6 +2,7 @@ define([ 'jquery', 'sinon', 'backbone', + 'logger', 'js/common_helpers/template_helpers', 'js/search/views/search_form', 'js/search/views/search_item_view', @@ -14,6 +15,7 @@ define([ $, Sinon, Backbone, + Logger, TemplateHelpers, SearchForm, SearchItemView, @@ -123,6 +125,7 @@ define([ // Mock the redirect call spyOn(this.item, 'redirect').andCallFake( function() {} ); spyOn(this.item, 'logSearchItem').andCallThrough(); + spyOn(Logger, 'log').andReturn($.Deferred().resolve()); var link = this.item.$el.find('a'); expect(link.length).toBe(1); link.trigger('click');