[SE-3248] Add progress_video event to video player

[toxinu]

This pull request is about adding new events to video player about watching progression.

Related pull request to add the new flag: openedx/completion#141

Dependencies: None

Screenshots: None

Sandbox URL:

• LMS
• Studio

Sandbox is being provisioned.

Merge deadline: None

Testing instructions:

The completion.enable_completion_tracking should be enabled.
Should use this edx-completion branch: openedx/completion#141
And then enable the completion. enable_progress_tracking_events switch.

1. Create a video unit
2. Enroll into the course
3. Open the web inspector (network panel)
4. Watch the video
5. Check that events are being fired every decades

Reviewers

• @nizarmah
• edX reviewer[s] TBD

[openedx-webhooks]

Thanks for the pull request, @toxinu! I've created OSPR-4993 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[toxinu]

@natabene I am not sure what is failing here since I have only modified some javascript files, why the Python quality tests are failing too? Thanks :)

[natabene]

@toxinu Thank you for your contribution. Yeah, I don't know. Maybe reviewer from your team will have an idea?

[nizarmah]

👍

• I tested this by playing a video and making sure the events run certain functionality
• I read through the code
• I checked for accessibility issues
• Includes documentation
• I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@toxinu sorry I wasn't able to take a look at why the builds were failing earlier. I didn't have time to dive in deep earlier than today, but you have already fixed them. So nice work 👍

[toxinu]

@natabene Our developer reviewed it and the tests are now green, so it is ready to be reviewed. 😄

[toxinu]

@natabene Friendly reminder that this pull request is ready to be reviewed. 😃

[natabene]

@marcotuts Is this something you want to review before it goes to engineering?

[marcotuts]

What additional data does this provide? Does this overlap with the completion API in any way @toxinu ? I'm not sure where the video progression / completion calculation is done for that API but I wanted to check about potential overlap here

[toxinu]

What additional data does this provide? Does this overlap with the completion API in any way @toxinu ? I'm not sure where the video progression / completion calculation is done for that API but I wanted to check about potential overlap here

Thanks for your question @marcotuts.
The completion API is only sending an event at around 90% of the watch time if I am not mistaken. This pull request changes add events that can overlap with the completion one but will also send events at every 5% of watch time.
The goal of this pull request is mostly to have more generic events that can be used for analytics for example.

[natabene]

@marcotuts Please let me know if this can go to engineering review.

[davidjoy]

Hi @toxinu,

I think if we'd like to proceed with this, that we'll need to put it behind a flag of some sort that's off by default. After asking around a bit, we have some concern about the volume of tracking data that will be created for high traffic sites (like edx.org).

That said, we'd also be fine closing the PR, since we can get a decent approximation with load_video/stop_video, and it's not clear that there's a specific use case or business need for this granularity of data.

[toxinu]

Hi @toxinu,

I think if we'd like to proceed with this, that we'll need to put it behind a flag of some sort that's off by default. After asking around a bit, we have some concern about the volume of tracking data that will be created for high traffic sites (like edx.org).

That said, we'd also be fine closing the PR, since we can get a decent approximation with load_video/stop_video, and it's not clear that there's a specific use case or business need for this granularity of data.

I understand your concern. 👍

Those events are already behind the completion.enable_completion_tracking switch, do we need to add another one for more granularity or is it ok?

[davidjoy]

I think it'd need to be a new switch, something like completion.enable_progress_tracking_events. That way we can have completion on, but keep the progress events off.

[toxinu]

I think it'd need to be a new switch, something like completion.enable_progress_tracking_events. That way we can have completion on, but keep the progress events off.

@davidjoy I have added a new switch (openedx/completion#141) to only call the progression tracking event when it's enabled. The tests and the pull request description have also been updated.

Let me know if you have any questions.

[edx-status-bot]

Your PR has finished running tests. The following contexts failed:

• jenkins/python
• jenkins/quality
• jenkins/a11y

[macdiesel]

@brianhw I would like to get your overall perspective on adding these events. @toxinu Upon initial review It's unclear to me why you need to add these events? Can you explain to me how the current events do not meet your needs? Also It's a bit unclear to me why you don't use the existing events to calculate this post processing? It's something that is already done in the edx-analytics-pipeline code.

IMO these types of things clutter the code and make it more confusing. I do not believe we should merge this PR unless there is a clear edx.org use case for adding these events.

[toxinu]

@brianhw I would like to get your overall perspective on adding these events. @toxinu Upon initial review It's unclear to me why you need to add these events? Can you explain to me how the current events do not meet your needs? Also It's a bit unclear to me why you don't use the existing events to calculate this post processing? It's something that is already done in the edx-analytics-pipeline code.

IMO these types of things clutter the code and make it more confusing. I do not believe we should merge this PR unless there is a clear edx.org use case for adding these events.

@macdiesel We understand your concerns and they are totally legit. We are now discussing internally if we want to put more effort into this pull request in order to give it a more common usage or if we want to cancel it. I will come back to you soon.
Thanks for your patience.

[toxinu]

@brianhw @macdiesel @nizarmah We decided to close this pull request and keep this change as a client-specific need since we have difficulty making it useful and meaningful as a community contribution.

I will also close openedx/completion#141.

Thanks again for your time and your patience. Really appreciate it. 🙇

[openedx-webhooks]

@toxinu Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.