From 919355cab1cc626731c0c671c9e8866fcbf62243 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Tue, 20 Feb 2018 13:36:32 -0500 Subject: [PATCH] Revert "Track viewing of individual blocks." This reverts commit 4946b6b296be943b9830f8543a71ee2a21985f20. --- .../public/js/vertical_student_view.js | 71 +++---- .../xmodule/xmodule/library_content_module.py | 1 - .../xmodule/xmodule/tests/test_vertical.py | 11 -- common/lib/xmodule/xmodule/vertical_block.py | 18 +- lms/envs/aws.py | 2 - lms/envs/common.py | 1 - lms/static/completion/js/.eslintrc.js | 7 - lms/static/completion/js/ViewedEvent.js | 182 ------------------ .../completion/js/spec/ViewedEvent_spec.js | 95 --------- lms/static/karma_lms.conf.js | 4 +- lms/static/lms/js/spec/main.js | 1 - lms/templates/vert_module.html | 6 +- requirements/edx/base.txt | 2 +- webpack.common.config.js | 1 - 14 files changed, 35 insertions(+), 367 deletions(-) delete mode 100644 lms/static/completion/js/.eslintrc.js delete mode 100644 lms/static/completion/js/ViewedEvent.js delete mode 100644 lms/static/completion/js/spec/ViewedEvent_spec.js diff --git a/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js b/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js index ca2dbc5240..66c37af33b 100644 --- a/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js +++ b/common/lib/xmodule/xmodule/assets/vertical/public/js/vertical_student_view.js @@ -1,7 +1,6 @@ /* JavaScript for Vertical Student View. */ -/* global Set:false */ // false means do not assign to Set -/* global ViewedEventTracker:false */ +/* global Set:false */ // false means do not assign to Set // The vertical marks blocks complete if they are completable by viewing. The // global variable SEEN_COMPLETABLES tracks blocks between separate loads of @@ -9,7 +8,6 @@ // navigates back within a given sequential) to protect against duplicate calls // to the server. - var SEEN_COMPLETABLES = new Set(); window.VerticalStudentView = function(runtime, element) { @@ -17,6 +15,7 @@ window.VerticalStudentView = function(runtime, element) { RequireJS.require(['course_bookmarks/js/views/bookmark_button'], function(BookmarkButton) { var $element = $(element); var $bookmarkButtonElement = $element.find('.bookmark-button'); + return new BookmarkButton({ el: $bookmarkButtonElement, bookmarkId: $bookmarkButtonElement.data('bookmarkId'), @@ -25,46 +24,32 @@ window.VerticalStudentView = function(runtime, element) { apiUrl: $bookmarkButtonElement.data('bookmarksApiUrl') }); }); - RequireJS.require(['bundles/ViewedEvent'], function() { - var tracker, vertical, viewedAfter; - var completableBlocks = []; - var vertModDivs = element.getElementsByClassName('vert-mod'); - if (vertModDivs.length === 0) { - return; - } - vertical = vertModDivs[0]; - $(element).find('.vert').each(function(idx, block) { - if (block.dataset.completableByViewing !== undefined) { - completableBlocks.push(block); - } - }); - if (completableBlocks.length > 0) { - viewedAfter = parseInt(vertical.dataset.completionDelayMs, 10); - if (!(viewedAfter >= 0)) { - // parseInt will return NaN if it fails to parse, which is not >= 0. - viewedAfter = 5000; - } - tracker = new ViewedEventTracker(completableBlocks, viewedAfter); - tracker.addHandler(function(block, event) { - var blockKey = block.dataset.id; + $(element).find('.vert').each( + function(idx, block) { + var blockKey = block.dataset.id; - if (blockKey && !SEEN_COMPLETABLES.has(blockKey)) { - if (event.elementHasBeenViewed) { - $.ajax({ - type: 'POST', - url: runtime.handlerUrl(element, 'publish_completion'), - data: JSON.stringify({ - block_key: blockKey, - completion: 1.0 - }) - }).then( - function() { - SEEN_COMPLETABLES.add(blockKey); - } - ); - } - } - }); + if (!block.dataset.completableByViewing) { + return; + } + // TODO: EDUCATOR-1778 + // * Check if blocks are in the browser's view window or in focus + // before marking complete. This will include a configurable + // delay so that blocks must be seen for a few seconds before + // being marked complete, to prevent completion via rapid + // scrolling. (OC-3358) + // * Limit network traffic by batching and throttling calls. + // (OC-3090) + if (blockKey && !SEEN_COMPLETABLES.has(blockKey)) { + $.ajax({ + type: 'POST', + url: runtime.handlerUrl(element, 'publish_completion'), + data: JSON.stringify({ + block_key: blockKey, + completion: 1.0 + }) + }); + SEEN_COMPLETABLES.add(blockKey); + } } - }); + ); }; diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 5f23e442aa..babd349a5f 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -339,7 +339,6 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): 'xblock_context': context, 'show_bookmark_button': False, 'watched_completable_blocks': set(), - 'completion_delay_ms': None, })) return fragment diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index aab72a565b..8995849162 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -20,8 +20,6 @@ from .xml import factories as xml from ..x_module import STUDENT_VIEW, AUTHOR_VIEW -COMPLETION_DELAY = 9876 - JsonRequest = namedtuple('JsonRequest', ['method', 'body']) @@ -43,7 +41,6 @@ class StubCompletionService(object): def __init__(self, enabled, completion_value): self._enabled = enabled self._completion_value = completion_value - self.delay = COMPLETION_DELAY def completion_tracking_enabled(self): """ @@ -59,12 +56,6 @@ class StubCompletionService(object): """ return {candidate: self._completion_value for candidate in candidates} - def get_completion_by_viewing_delay_ms(self): - """ - Return the completion-by-viewing delay in milliseconds. - """ - return self.delay - class BaseVerticalBlockTest(XModuleXmlImportTest): """ @@ -126,7 +117,6 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): """ self.module_system._services['bookmarks'] = Mock() self.module_system._services['user'] = StubUserService() - self.module_system._services['completion'] = StubCompletionService(enabled=True, completion_value=0.0) html = self.module_system.render( self.vertical, STUDENT_VIEW, self.default_context if context is None else context @@ -134,7 +124,6 @@ class VerticalBlockTestCase(BaseVerticalBlockTest): self.assertIn(self.test_html_1, html) self.assertIn(self.test_html_2, html) self.assert_bookmark_info_in(html) - self.assertIn(six.text_type(COMPLETION_DELAY), html) @staticmethod def _render_completable_blocks(template, context): # pylint: disable=unused-argument diff --git a/common/lib/xmodule/xmodule/vertical_block.py b/common/lib/xmodule/xmodule/vertical_block.py index 7a081c315b..8625b4cff3 100644 --- a/common/lib/xmodule/xmodule/vertical_block.py +++ b/common/lib/xmodule/xmodule/vertical_block.py @@ -60,13 +60,14 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse show_in_read_only_mode = True - def get_completable_by_viewing(self, completion_service): + def get_completable_by_viewing(self): """ Return a set of descendent blocks that this vertical still needs to mark complete upon viewing. Completed blocks are excluded to reduce network traffic from clients. """ + completion_service = self.runtime.service(self, 'completion') if completion_service is None: return set() if not completion_service.completion_tracking_enabled(): @@ -79,15 +80,6 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse completions = completion_service.get_completions(blocks) return {six.text_type(block_key) for block_key in blocks if completions[block_key] < 1.0} - def get_completion_delay_ms(self, completion_service): - """ - Do not mark blocks as complete until they have been visible to the user - for the returned amount of time (in milliseconds). - """ - if completion_service is None: - return 0 - return completion_service.get_completion_by_viewing_delay_ms() - def student_view(self, context): """ Renders the student view of the block in the LMS. @@ -107,9 +99,8 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse user_service = self.runtime.service(self, 'user') child_context['username'] = user_service.get_current_user().opt_attrs['edx-platform.username'] - completion_service = self.runtime.service(self, 'completion') - child_context['child_of_vertical'] = True + is_child_of_vertical = context.get('child_of_vertical', False) # pylint: disable=no-member @@ -129,8 +120,7 @@ class VerticalBlock(SequenceFields, XModuleFields, StudioEditableBlock, XmlParse 'show_bookmark_button': child_context.get('show_bookmark_button', not is_child_of_vertical), 'bookmarked': child_context['bookmarked'], 'bookmark_id': u"{},{}".format(child_context['username'], unicode(self.location)), # pylint: disable=no-member - 'watched_completable_blocks': self.get_completable_by_viewing(completion_service), - 'completion_delay_ms': self.get_completion_delay_ms(completion_service), + 'watched_completable_blocks': self.get_completable_by_viewing(), })) fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/vertical_student_view.js')) diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 564ccab362..2d9c9a9427 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -1095,8 +1095,6 @@ COMPLETION_VIDEO_COMPLETE_PERCENTAGE = ENV_TOKENS.get( 'COMPLETION_VIDEO_COMPLETE_PERCENTAGE', COMPLETION_VIDEO_COMPLETE_PERCENTAGE, ) -# The time a block needs to be viewed to be considered complete, in milliseconds. -COMPLETION_BY_VIEWING_DELAY_MS = ENV_TOKENS.get('COMPLETION_BY_VIEWING_DELAY_MS', COMPLETION_BY_VIEWING_DELAY_MS) ############### Settings for django-fernet-fields ################## FERNET_KEYS = AUTH_TOKENS.get('FERNET_KEYS', FERNET_KEYS) diff --git a/lms/envs/common.py b/lms/envs/common.py index bfc3191af7..0478c1fcad 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3447,7 +3447,6 @@ EDX_PLATFORM_REVISION = 'unknown' # Once a user has watched this percentage of a video, mark it as complete: # (0.0 = 0%, 1.0 = 100%) COMPLETION_VIDEO_COMPLETE_PERCENTAGE = 0.95 -COMPLETION_BY_VIEWING_DELAY_MS = 5000 ############### Settings for Django Rate limit ##################### RATELIMIT_ENABLE = True diff --git a/lms/static/completion/js/.eslintrc.js b/lms/static/completion/js/.eslintrc.js deleted file mode 100644 index 12cb26eef9..0000000000 --- a/lms/static/completion/js/.eslintrc.js +++ /dev/null @@ -1,7 +0,0 @@ -module.exports = { - extends: 'eslint-config-edx', - root: true, - settings: { - 'import/resolver': 'webpack', - }, -}; diff --git a/lms/static/completion/js/ViewedEvent.js b/lms/static/completion/js/ViewedEvent.js deleted file mode 100644 index d119abcb1b..0000000000 --- a/lms/static/completion/js/ViewedEvent.js +++ /dev/null @@ -1,182 +0,0 @@ -/** Ensure that a function is only run once every `wait` milliseconds */ -function throttle(fn, wait) { - let time = 0; - function delay() { - // Do not call the function until at least `wait` seconds after the - // last time the function was called. - const now = Date.now(); - if (time + wait < now) { - time = now; - fn(); - } - } - return delay; -} - - -export class ElementViewing { - /** - * A wrapper for an HTMLElement that tracks whether the element has been - * viewed or not. - */ - constructor(el, viewedAfterMs, callback) { - this.el = el; - this.viewedAfterMs = viewedAfterMs; - this.callback = callback; - - this.topSeen = false; - this.bottomSeen = false; - this.seenForMs = 0; - this.becameVisibleAt = undefined; - this.hasBeenViewed = false; - } - - getBoundingRect() { - return this.el.getBoundingClientRect(); - } - - /** This element has become visible on screen. - * - * (may be called even when already on screen though) - */ - handleVisible() { - if (!this.becameVisibleAt) { - this.becameVisibleAt = Date.now(); - // We're now visible; after viewedAfterMs, if the top and bottom have been - // seen, this block will count as viewed. - setTimeout( - () => { - this.checkIfViewed(); - }, - this.viewedAfterMs - this.seenForMs, - ); - } - } - - handleNotVisible() { - if (this.becameVisibleAt) { - this.seenForMs = Date.now() - this.becameVisibleAt; - } - this.becameVisibleAt = undefined; - } - - markTopSeen() { - // If this element has been seen for enough time, but the top wasn't visible, it may now be - // considered viewed. - this.topSeen = true; - this.checkIfViewed(); - } - - markBottomSeen() { - this.bottomSeen = true; - this.checkIfViewed(); - } - - getTotalTimeSeen() { - if (this.becameVisibleAt) { - return this.seenForMs + (Date.now() - this.becameVisibleAt); - } - return this.seenForMs; - } - - areViewedCriteriaMet() { - return this.topSeen && this.bottomSeen && (this.getTotalTimeSeen() >= this.viewedAfterMs); - } - - checkIfViewed() { - // User can provide a "now" value for testing purposes. - if (this.hasBeenViewed) { - return; - } - if (this.areViewedCriteriaMet()) { - this.hasBeenViewed = true; - // Report to the tracker that we have been viewed - this.callback(this.el, { elementHasBeenViewed: this.hasBeenViewed }); - } - } -} - - -export class ViewedEventTracker { - /** - * When the top or bottom of an element is first viewed, and the entire - * element is viewed for a specified amount of time, the callback is called, - * passing the element that was viewed, and an event object having the - * following field: - * - * * hasBeenViewed (bool): true if all the conditions for being - * considered "viewed" have been met. - */ - constructor(elements, viewedAfterMs) { - this.viewedAfterMs = viewedAfterMs; - this.elementViewings = new Set(); - this.handlers = []; - - this.interval = undefined; - elements.forEach((el) => { - this.elementViewings.add( - new ElementViewing( - el, - viewedAfterMs, - (element, event) => this.callHandlers(element, event), - ), - ); - }); - this.registerDomHandlers(); - } - - /** Register a new handler to be called when an element has been viewed. */ - addHandler(handler) { - this.handlers.push(handler); - } - - /** Mark which elements are currently visible. - * - * Also marks when an elements top or bottom has been seen. - * */ - updateVisible() { - this.elementViewings.forEach((elv) => { - if (elv.hasBeenViewed) { - return; - } - - const now = Date.now(); // Use the same "now" for all calculations - const rect = elv.getBoundingRect(); - let visible = false; - - if (rect.top > 0 && rect.top < window.innerHeight) { - elv.markTopSeen(now); - visible = true; - } - if (rect.bottom > 0 && rect.bottom < window.innerHeight) { - elv.markBottomSeen(now); - visible = true; - } - if (rect.top < 0 && rect.bottom > window.innerHeight) { - visible = true; - } - - if (visible) { - elv.handleVisible(now); - } else { - elv.handleNotVisible(now); - } - }); - } - - registerDomHandlers() { - window.onscroll = throttle(() => this.updateVisible(), 100); - window.onresize = throttle(() => this.updateVisible(), 100); - this.updateVisible(); - } - - /** Call the handlers for all newly-viewed elements and pause tracking - * for recently disappeared elements. - */ - callHandlers(el, event) { - this.handlers.forEach((handler) => { - handler(el, event); - }); - } -} - diff --git a/lms/static/completion/js/spec/ViewedEvent_spec.js b/lms/static/completion/js/spec/ViewedEvent_spec.js deleted file mode 100644 index 30368e7cf9..0000000000 --- a/lms/static/completion/js/spec/ViewedEvent_spec.js +++ /dev/null @@ -1,95 +0,0 @@ -import { ElementViewing, ViewedEventTracker } from '../ViewedEvent'; - - -describe('ViewedTracker', () => { - let existingHTML; - beforeEach(() => { - existingHTML = document.body.innerHTML; - }); - - afterEach(() => { - document.body.innerHTML = existingHTML; - }); - - it('calls the handlers when an element is viewed', () => { - document.body.innerHTML = '
'; - const tracker = new ViewedEventTracker(Array.from(document.getElementsByTagName('div')), 1000); - const handlerSpy = jasmine.createSpy('handlerSpy'); - tracker.addHandler(handlerSpy); - const elvIter = tracker.elementViewings.values(); - // Pick two elements, and mock them so that one has met the criteria to be viewed, - // and the other hasn't. - const viewed = elvIter.next().value; - spyOn(viewed, 'areViewedCriteriaMet').and.returnValue(true); - viewed.checkIfViewed(); - expect(handlerSpy).toHaveBeenCalledWith(viewed.el, { - elementHasBeenViewed: true, - }); - const unviewed = elvIter.next().value; - spyOn(unviewed, 'areViewedCriteriaMet').and.returnValue(false); - unviewed.checkIfViewed(); - expect(handlerSpy).not.toHaveBeenCalledWith(unviewed.el, jasmine.anything()); - }); -}); - -describe('ElementViewing', () => { - beforeEach(() => { - jasmine.clock().install(); - }); - - afterEach(() => { - jasmine.clock().uninstall(); - }); - - it('calls checkIfViewed when enough time has elapsed', () => { - const viewing = new ElementViewing({}, 500, () => {}); - spyOn(viewing, 'checkIfViewed').and.callThrough(); - viewing.seenForMs = 250; - viewing.handleVisible(); - jasmine.clock().tick(249); - expect(viewing.checkIfViewed).not.toHaveBeenCalled(); - jasmine.clock().tick(1); - expect(viewing.checkIfViewed).toHaveBeenCalled(); - }); - - it('has been viewed after the specified number of milliseconds', () => { - const viewing = new ElementViewing({}, 500, () => {}); - viewing.seenForMs = 250; - spyOn(Date, 'now').and.returnValue(750); - viewing.handleVisible(); - viewing.markTopSeen(); - viewing.markBottomSeen(); - Date.now.and.returnValue(999); - viewing.checkIfViewed(); - expect(viewing.hasBeenViewed).toBeFalsy(); - Date.now.and.returnValue(1000); - jasmine.clock().tick(250); - expect(viewing.hasBeenViewed).toBeTruthy(); - }); - - it('has not been viewed if the bottom has not been seen', () => { - const viewing = new ElementViewing(undefined, 500, () => {}); - viewing.markTopSeen(); - viewing.seenForMs = 500; - expect(viewing.areViewedCriteriaMet()).toBeFalsy(); - viewing.checkIfViewed(); - expect(viewing.hasBeenViewed).toBeFalsy(); - }); - - it('has not been viewed if the top has not been seen', () => { - const viewing = new ElementViewing(undefined, 500, () => {}); - viewing.markBottomSeen(); - viewing.seenForMs = 500; - expect(viewing.areViewedCriteriaMet()).toBeFalsy(); - viewing.checkIfViewed(); - expect(viewing.hasBeenViewed).toBeFalsy(); - }); - - it('does not update time seen if lastSeen is undefined', () => { - const viewing = new ElementViewing(undefined, 500, () => {}); - viewing.becameVisibleAt = undefined; - expect(viewing.becameVisibleAt).toBeUndefined(); - viewing.handleVisible(); - expect(viewing.becameVisibleAt).not.toBeUndefined(); - }); -}); diff --git a/lms/static/karma_lms.conf.js b/lms/static/karma_lms.conf.js index a2e1456591..a3bdc64bb0 100644 --- a/lms/static/karma_lms.conf.js +++ b/lms/static/karma_lms.conf.js @@ -34,14 +34,12 @@ var options = { {pattern: 'learner_profile/**/!(*spec).js'}, {pattern: 'lms/js/**/!(*spec).js'}, {pattern: 'support/js/**/!(*spec).js'}, - {pattern: 'teams/js/**/!(*spec).js'}, - {pattern: 'completion/js/**/!(*spec).js'} + {pattern: 'teams/js/**/!(*spec).js'} ], specFiles: [ // Define the Webpack-built spec files first {pattern: 'course_experience/js/**/*_spec.js', webpack: true}, - {pattern: 'completion/js/**/*_spec.js', webpack: true}, // Add all remaining spec files to be used without Webpack {pattern: '../**/*spec.js'} diff --git a/lms/static/lms/js/spec/main.js b/lms/static/lms/js/spec/main.js index 9bc5ba3f0c..d81b26dae4 100644 --- a/lms/static/lms/js/spec/main.js +++ b/lms/static/lms/js/spec/main.js @@ -690,7 +690,6 @@ }); testFiles = [ - 'completion/js/spec/ViewedEvent_spec.js', 'course_bookmarks/js/spec/bookmark_button_view_spec.js', 'course_bookmarks/js/spec/bookmarks_list_view_spec.js', 'course_bookmarks/js/spec/course_bookmarks_factory_spec.js', diff --git a/lms/templates/vert_module.html b/lms/templates/vert_module.html index cee2c635e0..cc7cfd569a 100644 --- a/lms/templates/vert_module.html +++ b/lms/templates/vert_module.html @@ -8,11 +8,7 @@ <%include file='bookmark_button.html' args="bookmark_id=bookmark_id, is_bookmarked=bookmarked"/> % endif -
+
% for idx, item in enumerate(items):