Anton/store video progress

[valera-rozuvan]

persist student progress in video

The following test cases are achieved with this PR:

Student logins to edx, plays video, selects position,
closes browser, opens video, position is restored

Given a video with a start time of :30 and an end time of 1:00
And a student who navigated away from the video was at :45
    (in between start time and end time)
When the student returns to the same video
Then the video should resume playing from :45.

Given a video with a start time of :30 and an end time of 1:00
And a student who navigated away from the video when the slider was at :15
When the student returns to the same video
Then the video should resume playing from :30

Given a video with a start time of :30 and an end time of 1:00
And a student who navigated away from the video after moving the video to 1:15,
When the student returns to the same video
Then the video should resume playing from :30

JIRA issue

BLD-385

Reviewers

• @singingwolfboy
• @polesye

[davestgermain]

isinstance(value, (float, int))

[valera-rozuvan]

@davestgermain You are right! Python docs on isinstance reveals that the second argument can indeed be a tuple of type objects. Thx!

[polesye]

Please add test.

[valera-rozuvan]

@polesye You wrote that I should add tests for https://github.com/edx/edx-platform/pull/2365#discussion-diff-9415398R196 . After working with @auraz , we came up with a solution where the change:

if isinstance(value, (float, int)):  # backward compatibility

is no longer necessary. Please see edx/edx-platform@a2e1e22 .

[polesye]

Is this function used only when we send data to server and its result is not user facing?

[valera-rozuvan]

@polesye Yes. This function was added for the purpose of sending the correctly formatted time string to the server. I decided to add it here in the hope that in the future it might be useful in other places. Code reuse!

[polesye]

I ask it, because if result of this function is planned to be visible for users, a11y should be added.

[valera-rozuvan]

@polesye It will not be user-facing. So no need for internationalization.

[polesye]

Please leave comment about that.

[polesye]

@valera-rozuvan Did you write js unit tests?

[singingwolfboy]

Stylistic nitpick: In Javascript a.b and a['b'] are identical. I prefer the former when you're dealing with static strings. Can you change these lines to:

window.VideoState = {};
window.VideoState.id = {};

[singingwolfboy]

👍

[valera-rozuvan]

@polesye No. I did not write JavaScript unit tests.

[polesye]

Please add tests for saveState and updatePlayTime functions.

[polesye]

And I'm waiting for acceptance tests.

[polesye]

We need round value just when send position to the server.

this.storage.setItem('position', data.position, true);
data.position =  Time.formatFull(data.position);

[valera-rozuvan]

@polesye If the server stores the time as a HH:MM:SS value, and writes position into the data-position attribute of the HTML template as an integer, why do we need to preserve the decimal point on the user's PC? I think it is better to have it rounded before we store using the storage class.

[polesye]

@valera-rozuvan No matter how do we store this value on user's PC (It's just a property of global object, not a cookie), so I don't see a reason to make this redundant rounding. Time.formatFull uses Math.floor under the hood.

[valera-rozuvan]

@polesye Done.

[valera-rozuvan]

@polesye I have implemented acceptance test to check that position in video is stored on page refresh. All tests pass. All comments by you and @singingwolfboy have been addressed. Please list what other tests I have to implement so that @Lyla-Fischer understands how much work on this PR is left to be done.

[polesye]

I believe, that even if you break js method seekTo, this condition will be passed.

[valera-rozuvan]

@polesye I have tried this. I am aware that you can use {} and .format() methods. However, the test fails when you do this. Please check for yourself.

[polesye]

I think you had an issues with curly braces.

Format strings contain “replacement fields” surrounded by curly braces {}. Anything that is not contained in braces is considered literal text, which is copied unchanged to the output. If you need to include a brace character in the literal text, it can be escaped by doubling: {{ and }}.

[valera-rozuvan]

@polesye You were right = ) Done.

[valera-rozuvan]

@polesye The condition will not be passed. This is because world.wait_for() has a default wait time of 5 seconds. If we are waiting for 10 seconds of video to play, the test will fail before it gets to 10 seconds, because it will timeout after 5 seconds.

[valera-rozuvan]

@polesye So the only way this test will pass if the video starts playing from the 10th second.

[polesye]

Ok, I suggest to set timeout explicitly, because default value can be changed. And I think world.wait_for(lambda _: world.css_html('.vidtime')[:4] == '0:11') will be enough.

[valera-rozuvan]

@polesye I have added another acceptance test. How many more should I add?

[polesye]

@valera-rozuvan 2 tests that cover following behaviors:

Given a video with a start time of :30 and an end time of 1:00
And a student who navigated away from the video when the slider was at :15
When the student returns to the same video
Then the video should resume playing from :30

Given a video with a start time of :30 and an end time of 1:00
And a student who navigated away from the video after moving the video to 1:15,
When the student returns to the same video
Then the video should resume playing from :30

[valera-rozuvan]

@polesye I have added the 2 tests you requested, and also addressed your comment

Ok, I suggest to set timeout explicitly, because default value
can be changed. And I think
world.wait_for(lambda _: world.css_html('.vidtime')[:4] == '0:11')
will be enough.

What else?

[valera-rozuvan]

@polesye I have simplified Then I see video starts playing from "0:10" position case. What else?

[polesye]

I should check it manually.

[singingwolfboy]

Seems like rather this code could be improved by using _.every:

toHaveAttrs: function(attrs) {
  if ($.isEmptyObject(attrs)) {
    return false;
  }
  var element = this.actual;
  return _.every(attrs, function(value, name) {
    return element.attr(name) === value;
  });

[polesye]

I found that it doesn't store position sometimes (especially when you're reloading the page without pausing the video).

I think that will be less confusing for users to show them an actual position (in vcr and progress slider position), before the video starts playing. But it shouldn't be a blocker for this PR and could be done in separate PR.

[valera-rozuvan]

@singingwolfboy Done.

[valera-rozuvan]

@polesye Created the JIRA issue "Show start time or starting position on slider and VCR" BLD-823.

[valera-rozuvan]

@singingwolfboy There is a bug in this line. It does not save to actual database. When the browser closes, and you open the browser again, the value of position is once again the default 0. What should I do to update the position value in the database?

[valera-rozuvan]

@nedbat Ned, also please comment on this. @auraz suggested that I ask you.

[nedbat]

I don't know why it wouldn't save to the database. Are you sure there's no exception being thrown? @cpennington any clues?

[valera-rozuvan]

@nedbat What query should I run in mongo shell in order to absolutely be sure that the position is 0. I want to run it when the browser is open, see that it is in fact 0, then trigger the saving of new position via AJAX request to handle_ajax(), and then check once again that the database has position set to 0.

[valera-rozuvan]

@auraz and @polesye I have visually confirmed using the Robomongo tool that position is not being stored in the database. Please look in the following JSON representing the 2 entries in the DB that are modified when I work with the Video:

/* 5 */
{
    "_id" : {
        "tag" : "i4x",
        "org" : "private",
        "course" : "01",
        "category" : "video",
        "name" : "01aadcb12187481788350e558e1ac903",
        "revision" : null
    },
    "definition" : {
        "data" : {
            "data" : ""
        },
        "children" : []
    },
    "metadata" : {
        "display_name" : "Video",
        "sub" : "",
        "start_time" : "00:00:00",
        "html5_sources" : [],
        "youtube_id_1_0" : "OEoXaMPEzfM",
        "end_time" : "00:00:12"
    }
}

/* 6 */
{
    "_id" : {
        "tag" : "i4x",
        "org" : "private",
        "course" : "01",
        "category" : "video",
        "name" : "01aadcb12187481788350e558e1ac903",
        "revision" : "draft"
    },
    "definition" : {
        "data" : {
            "data" : ""
        },
        "children" : []
    },
    "metadata" : {
        "display_name" : "Video",
        "sub" : "",
        "start_time" : "00:00:00",
        "html5_sources" : [],
        "youtube_id_1_0" : "OEoXaMPEzfM",
        "end_time" : "00:00:12"
    }
}

You can see that start_time and end_time are present. When I change the values of start time and end time in the advanced tab of the Editor, the values are updated and I can see the new values in Robomongo. However, I never see the property position in Robomongo.

@cpennington , @singingwolfboy , @nedbat Please help!

[valera-rozuvan]

@nedbat , @cpennington , @singingwolfboy Maybe it has something to do with how position is defined?

position = RelativeTime(
    help="Current position in the video",
    scope=Scope.user_state,
    default=datetime.timedelta(seconds=0)
)

[cpennington]

The position field in video_module.py has scope user_state, which means that it's stored in the relational database, in the courseware_studentmodule table in the lms, and stored in the users session in Studio.

[valera-rozuvan]

@cpennington , @singingwolfboy , @nedbat , @auraz , @polesye I have fixed the bug with position. It turned out that this property was already used in field state in table courseware_studentmodule. The solution was to rename this property. As can be seen, this helped. All tests are passing.

[valera-rozuvan]

@polesye Please finish with your review.

[polesye]

👍