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@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.