Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populate video duration to edx call #1084

Merged
merged 5 commits into from
Mar 26, 2024

Conversation

rachellougee
Copy link
Contributor

@rachellougee rachellougee commented Mar 22, 2024

What are the relevant tickets?

https://github.com/mitodl/hq/issues/3402

Description (What does it do?)

Adding functionality to populating duration when posting video to open edx. Duration can be found in EncodeJob json response (see https://video-rc.odl.mit.edu/admin/ui/video/165/change/)

How can this be tested?

To test this, you will need a working OVS and open edx (tutur or devstack) instances locally. For OVS setup, follow README instruction, you may need to ask for AWS and CloudFront env variables from devops.

In your local edx instance. e.g. tutor

In your OVS instance

HTTPConnectionPool(host='local.overhang.io', port=80): Max retries exceeded with url: /oauth2/access_token/ (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0xffff7e982b50>: Failed to establish a new connection: [Errno 111] Connection refused'))

I was able to modify post_video_to_edx to test the code I added to make sure duration is returned correctly.

@rachellougee rachellougee marked this pull request as ready for review March 26, 2024 15:05
@mbertrand mbertrand self-assigned this Mar 26, 2024
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some comments but they're pretty minor so approving it, feel free to make the suggested changes or leave as is. I did not test with a local edx instance but added a log statement to dump the JSON that would be sent there and the duration value was as expected.

ui/api.py Outdated
duration = 0.0
if submitted_encode_job:
et_job = get_et_job(submitted_encode_job.id)
duration = et_job["Output"].get("Duration")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor suggestions, feel free to take or leave: you don't necessarily have to run get_et_job, the EncodeJob object in the database should have that info already. But either way should work. Also, when assigning the duration from the encode job, might be good to add or 0 just in case it is null for some reason.

import ast
....

    duration = 0.0
    if submitted_encode_job:
        duration = ast.literal_eval(submitted_encode_job.message).get("Output", {}).get("Duration", 0) or 0 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I've made the change for your suggestion.

@rachellougee rachellougee merged commit 2154f41 into master Mar 26, 2024
3 checks passed
@rachellougee rachellougee deleted the populate-video-duration-to-edx-call branch March 26, 2024 21:05
@rachellougee rachellougee mentioned this pull request Mar 27, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants