Define custom completion for VideoModule

[jcdyer]

Update the VideoModule to publish a completion event when the player
reaches 95% complete, and submit a BlockCompletion when that event
occurs.

Testing instructions

1. In the lms shell, remove all existing BlockCompletions for video blocks.

   from lms.djangoapps.completion.models import BlockCompletion
   BlockCompletion.objects.filter(block_type='video').delete()

2. Sign in to the LMS and visit the demo course.
3. Navigate to "Example Week 1: Getting Started > Lesson 1 - Getting Started > (Vertical 2) Working with Videos". Open the browser's development panel, and view network traffic.
4. Verify that when the video reach 4m 3s, a POST is sent to a URL for the block ending with "publish_completion", and receives a 200 response.
5. Verify that a BlockCompletion now exists for the video block with completion value == 1.0.

   from lms.djangoapps.completion.models import BlockCompletion
   print(BlockCompletion.objects.filter(block_type='video', completion=1.0))

6. Navigate to the next vertical: "Videos on edX". This one uses more features (a custom start time and end-time). Watch to the end of the video (or jump to a few seconds before the end of the video, and just watch the end).
7. Verify that a publish_completion POST is sent after 7m 17s (Start time is 5m 10s, end time is 7m 24s = 134s; 134 * .95 = 127.3s; 5m 10s + 127s = 7m 17s), and receives a 200 response.
8. Verify that a BlockCompletion object exists for this block as well.

Author concerns

I'm rusty at Javascript, and entirely new to the video player. I may not be doing things the right way.

Reviewers

• @pomegranited
• @robrap

FYI: @yro for video player expertise.

TODO:

• Squash commits
• Rebase PR against edx:master after cliff/complete-default-blocks merges ( Complete default blocks #16234 )

[openedx-webhooks]

Thanks for the pull request, @jcdyer! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

[jcdyer]

jenkins run python
jenkins run quality

[jcdyer]

jenkins run quality

[jcdyer]

jenkins run python

[jcdyer]

jenkins run quality

[pomegranited]

@jcdyer Minor changes requested and a quality issue to fix, but I tested on my devstack, and it does what it says on the box 😄

[pomegranited]

Please replace this comment with a section header like the one above, and a comment for what this setting does.

[pomegranited]

Please provide a section header like the one above, and a comment describing what this setting does.

[pomegranited]

Also, could you add it to [cl]ms.envs.aws too, so we can override it with environment settings as required?

[pomegranited]

This calculation is the same as below -- can it be moved to a separate method, e.g. this.calculateCompleteAfter(duration).

Could even include the if (duration == 0) check from here, to ensure that this.endTime - this.startTime > 0?

[jcdyer]

I think I'll leave the duration check outside the method, for a few reasons:

1. duration === 0 is a special case where the data doesn't exist yet (should probably be returning undefined, but I don't want to mess with it), which is the only case that really needs to be checked.
2. If the video is actually short, it won't hurt to submit the completion anyway. Worst case, the ended event occurs at the same time, and it sends two POSTs instead of one, but the DB is protected from this by a unique index.
3. The check for a zero length video should actually have been if (duration === this.startTime), not if (duration === 0). That's only true for videos that don't have a custom end time.

[pomegranited]

Could you pull that 3.0 into a variable at the top, so we can easily adjust if needed?

[pomegranited]

Cleaner way to post JSON data in 035_video_accessible_menu.js

[jcdyer]

When I try that (data: {completion: 1.0}, dataType: 'json'), it submits the data as x-www-form-encoded, and returns a 400 error. I've changed it back, but left in a commit (edx@cba3592) that tries to do it the way 035_video_accessible_menu does it the way it's done in 035_video_accessible_menu.js, in case you want to take a look and can see something obvious I was missing.

[pomegranited]

This is all a bit mysterious to me, but I can see there's other JS libraries doing something similar, so 👍

[jcdyer]

Yeah. I admit this part was copypasta from one of the other scripts. I think it's just making sure that when the player gets destroyed (maybe when navigating away from the vertical?) all the resources get cleaned up appropriately--no dangling references causing memory leaks. But I didn't track it all down.

[pomegranited]

My only problem with this is that the console logs get filled with 500 errors if the completion tracking waffle switch isn't set. Shouldn't that case fail quietly?

[jcdyer]

Yeah, in that case, the JS should just not send requests to the server. I'll update that.

[jcdyer]

New version passes completionEnabled to the client, and the client only attaches the event handlers if completion is enabled.

[pomegranited]

Nice! Thanks @jcdyer .

[robrap]

Dave just gave a lightning talk on log messages like: "This should never happen.". What about something like the following?:

if not completion_service.completion_tracking_enabled():
    raise JsonHandlerError(400, u"Completion tracking is disabled and api calls should not be made.")
if not completion_service:
    raise JsonHandlerError(500, u"Completion service not found.")

[robrap]

Thanks for splitting these. I still think changing the second from "Completion service not available" to something like "Completion tracking is not enabled and api calls are unexpected" would help someone understand more quickly where to look for the problem.

[pomegranited]

This looks unrelated?

[jcdyer]

It is. I moved it up from below while debugging something because it turns out UnicodeDecodeError is a subclass of ValueError, so that case was getting swallowed by the other except handler block below.

[pomegranited]

Artefact of test debugging -- please remove.

[pomegranited]

Can use get_handler_url method defined below too?

[jcdyer]

jenkins run all

[jcdyer]

jenkins run bokchoy

[jcdyer]

jenkins run lettuce

[jcdyer]

jenkins run all

[jcdyer]

This is ready for another review.

[pomegranited]

👍 @jcdyer

I left one nit about a comment, but it's working well. Thanks for the updates!

• I tested this on my devstack using the PR instructions, and with and without the waffle flag described in https://github.com/edx/edx-platform/pull/16234.
• I read through the code
• I checked for accessibility issues N/A
• Includes documentation -- docstrings only here; asked about waffle flag documentation

[pomegranited]

nit: "How many seconds to wait until a POST fails to try again"

[pomegranited]

Nice! Thanks @jcdyer .

[pomegranited]

@jcdyer I messed around with a couple of the jquery.ajax options for posting data, like processData: false and different options for contentType, but had no luck getting the data parsed properly by the XBlock.json_handler.

Also found this stackoverflow post which suggests stringifying the data like you did.

So no worries here :)

[jcdyer]

@robrap, this is squashed and ready for an edx review.

[jcdyer]

@robrap Do you have a sense of timeline for when you (or someone else at edx) would be able to review this?

[robrap]

@jcdyer: Sorry for the delay. I was trying to clarify the reviewer at edX, and in doing that, I forgot that there was one waiting. Feel free to ping me on HipChat if you don't see action in 1-3 days, even if it is just to get a schedule.

In any case, at this time, I am officially going to review the rest of the block completion PRs. The reviewer for the aggregate PRs is still a question.

For this review, I can get to it in the next couple of days. I'm just wrapping up a story and then I'll pick this up.

Thanks for checking back in.

[robrap]

Thanks @jcdyer. Pretty minor comments in general. Hoping it will be simple to add some jasmine.

[robrap]

How was 09_ chosen? Thanks.

[jcdyer]

By looking at the other files in video. They seem to be roughly ordered by dependency. completion is required by 10_main, so it needed to be less than 10, but I figured not much else depends on it, so it can be added late in the process.

I didn't really understand why the numbering is needed here, but I figured it was better to be consistent with the subsystem I'm working in.

[robrap]

Documentation indentation seems off. Here is what I think it should be:
http://edx.readthedocs.io/projects/edx-developer-guide/en/latest/style_guides/javascript-guidelines.html#documentation

[jcdyer]

Updating throughout.

[robrap]

Nit: this.isComplete would be more consistent with isEnabled.

[robrap]

Nit: completeAfterTime and calculateCompleteAfterTime would be more clear. I was first wondering if this might be returning X minutes or seconds?

[jcdyer]

Updated.

[robrap]

Seems like lastSentAt would be set in markCompletion. On a related note, would handleEnded need the same throttling, or is that a single event?
This makes me think of some other questions:

• If handleEnded ajax call fails is the event repeated?
• If someone fasts forwards to the end of a video, does that make it seem like it was watched?
  Basically, just want to make sure someone can easily mark it as complete if there is a temporary problem.

[jcdyer]

Seems like lastSentAt would be set in markCompletion

I can be down with that. I'll make the update.

On a related note, would handleEnded need the same throttling, or is that a single event?

handleEnded is just a single event, so I thought it would be better to keep it simple.

If handleEnded ajax call fails is the event repeated?

No. That could be useful, but it brings up a few new questions. What if the server's down for five minutes? Or if the user is on a train, and the wifi went out for half an hour? How often and how long should we repeat? Should we use local storage to keep stale completions around until the user is back online? How do we determine when they come back online?

Ultimately, I think the 100% solution is going to be a much bigger one, and I'm not sure we need to hit 100% (because as you suggest, the user can go back and recomplete it fairly easily). The question is how good is good enough? Is a single submission sufficient? Should we retry three times, every five seconds, and call it quits after that?

If someone fasts forwards to the end of a video, does that make it seem like it was watched?
Basically, just want to make sure someone can easily mark it as complete if there is a temporary problem.

Yes. As long as the user is watching a portion of video after the 95% mark when timeupdate gets triggered, the video is considered complete.

[robrap]

Nit: Extra line?

[robrap]

Not sure if the difference is intentional and needed, but just noting the difference between rstrip('/?') and rstrip('?') (i.e. dropped slash). Also, reminder that this call is duplicated depending on if any change is required.

[jcdyer]

Yeah. I tested both ways, and noticed that the trailing / wasn't actually changing the behavior of the call, so I figured why worry about it?

[robrap]

If validating data, do you want to make sure it is a number?

[jcdyer]

Good idea. In python 2, a non-number will just sort outside the range of numbers, but in python 3 it will become an error. Better not to introduce a possible 500 here.

[robrap]

Dave just gave a lightning talk on log messages like: "This should never happen.". What about something like the following?:

if not completion_service.completion_tracking_enabled():
    raise JsonHandlerError(400, u"Completion tracking is disabled and api calls should not be made.")
if not completion_service:
    raise JsonHandlerError(500, u"Completion service not found.")

[robrap]

Seems like there should be tests added to:
https://github.com/edx/edx-platform/tree/master/common/lib/xmodule/xmodule/js/spec/video

[robrap]

Does thumbs-up mean this is coming?

[jcdyer]

Yes. I worked on tests on Friday, but wasn't able to get them all in place. I'll ping you again when they are ready or by end of day at the latest.

[robrap]

@jcdyer: What's the status of this? I'd like to close my review ticket, but I haven't seen you ping me for re-review. Thanks.

[jcdyer]

Hi Robert, sorry for the delay. I've been off this week. I will see if I can get it wrapped up and back on your plate today.

…

On December 12, 2017 3:43:45 PM EST, Robert Raposa ***@***.***> wrote: @jcdyer: What's the status of this? I'd like to close my review ticket, but I haven't seen you ping me for re-review. Thanks. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/edx/edx-platform/pull/16569#issuecomment-351188165

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[robrap]

Thanks for the updates @jcdyer. A couple of remaining. Not sure what the test failures are about and if you picked up a bad commit during a rebase? Anyway, let me know when this is ready again and hopefully have a green build. Thanks.

[robrap]

Thanks for splitting these. I still think changing the second from "Completion service not available" to something like "Completion tracking is not enabled and api calls are unexpected" would help someone understand more quickly where to look for the problem.

[robrap]

Does thumbs-up mean this is coming?

[robrap]

I had proposed completeAfter => completeAfterTime. You instead went with completeAfterSeconds.

I find the code confusing. Since currentTime and this.completeAfterSeconds, they seem to contain the same unit. I'd like to see then consistently named. If they are both times, then please rename the latter to this.completeAfterTime. If they are both in seconds from the beginning, then options might be:

• rename all the "xxxTime" variables to "xxxSeconds", or
• drop a comment where they are defined, and just explain what they contain.

Or if I am missing something entirely, feel free to help clarify and make some other code change that makes this comparison more clear. Thanks.

[jcdyer]

@robrap I was thinking that time and seconds were different, analogous to the way datetime and timedelta are different, but it looks like you're right, and Javascript treats them identically. I think you're right that they should all have the same suffix. I'm leaning toward going with xxxSeconds, even though the names are slightly more awkward, because it makes the semantics clearer in all contexts (including the Javascript code, the Django settings modules, and JSON files that feed into the settings).

[robrap]

@jcdyer What does seconds actually mean? What are sample values for startTime (or startSeconds)? Maybe you could clarify this with a quick comment if the name isn't enough to understand.

[jcdyer]

On further look, I've settled on calling everything "Time." I was hoping to distinguish between DateTime-like objects (which in javascript are represented as "seconds since the epoch", and the number of seconds from the beginning of a video, but even that distinction was going to end up muddied, because the video player exposes "startTime" and "endTime" (which are seconds since the start of the video), so as per your initial recommendation, I'm settling on a consistent use of "Time," with a comment to explain where meaning might be confusing.

[jcdyer]

@robrap Tests are added, and I've addressed your last comments (about naming and error messaging). If the build is green, go ahead and review. JS tests and quality passed locally. I'll check in the morning to be sure Jenkins is happy.

[jcdyer]

This test seems to be flaky. I saw it fail once, and couldn't bear to let the typo survive. I'm not sure what the process is for external contributors marking tests flaky, so I haven't created any tickets about it.

[robrap]

If you know it to be flaky, I don't see any issue with you following the updated flaky test process.

[jcdyer]

There was an error (wher I hadn't changed completeAfterSeconds to xxxTime in the specfile, so I cleaned it up, and added some documentation. In adding the documentation, I noticed that I was registering the destroy handler at the wrong time, so I updated that, too.

[robrap]

Minor comments. Approval after you make some of the more minor additions/changes. Or, ask if you disagree with anything. Thanks!

[robrap]

If you know it to be flaky, I don't see any issue with you following the updated flaky test process.

[robrap]

Consider adding another trigger and checking and changing the check for markCompletion to ensure it was called only once.

[jcdyer]

Done.

[robrap]

This is where I had expected a comment. Maybe you added them elsewhere?

I don't even know if my comment is accurate, but it would be nice to see something like:

// The variables that end with 'Time' are actually in seconds from 0, representing the actual start of the video.

[jcdyer]

Done.

[robrap]

Thanks. The one that is confusing to me is what startTime represents. Is that the startTime of real content, before which if you don't get there it won't be marked complete? It's up to you whether not you enhance comments. I'm just interested.

[jcdyer]

You can actually define an edx video that only includes a subset of the source video, so if you have a 10 minute youtube video, you can include only the slice from 8m0s to 9m0s minute of the video by assigning your block a startTime of 480 (8 * 60), and an endTime of 540 (9 * 60). So startTime is the time from the start of the source video that the video block will use as its start time. There's a video in the demo course of a young man reading an excerpt from the Odyssey (or was it the Iliad?) that uses this feature, if you want to see it in action.

[robrap]

Thanks for explaining that for me. That makes more sense than what I was imagining.

[robrap]

I actually would have left this as repostDelaySeconds, since this is not seconds from the beginning of the video like all the other Time variables. I think it would be more consistent if this went back as it was.

[jcdyer]

Makes sense.

[jcdyer]

Python has apparently flaky paver tests, e.g. https://build.testeng.edx.org/job/edx-platform-python-unittests-pr/46092/testReport/junit/pavelib.paver_tests.test_assets/TestPaverAssetTasks/test_compile_sass_4_____system_lms__/ (+13 others)

All other tests passed. Rebasing against master to rerun tests.

[robrap]

@jcdyer: I'm hopeful that they aren't flaky, but were just broken at one time. Hopefully all will be well after rebase.

[robrap]

@jcdyer: Was this a bad rebase, or do you need to repoint the PR against master or something?

[jcdyer]

Oh wow. I forgot this branch wasn't pointing at master. The target branch merged to master on Nov 28th, so I will repoint the PR to master now.

[robrap]

Also, feel free to squash whenever you are ready. Thanks.

[jcdyer]

Repointed, rebased, resolved conflicts, and squashed.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, December 21, 2017.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.