add transcript credentials api #210
Conversation
DawoudSheraz
force-pushed
the
dsheraz/PROD-1380/add-transcripts-api
branch
from
72c6bcf
to
e052b3c
Compare
edxval/views.py
Outdated
|
|
||
| return Response(status=status_code, data=data) | ||
|
|
||
| def get_cielo_token_response(self, username, api_secure_key): |
There was a problem hiding this comment.
I'm wondering if it would be cleaner to separate out provider-specific stuff like this (and the settings that go with it) into separate source files. Just seems a little out of place to have Cielo-specific stuff here in the middle of views.py. What do you think?
There was a problem hiding this comment.
Sure, it will make the view code more compact. I will update the PR.
There was a problem hiding this comment.
The settings should be left in Django settings. The reason being that Cielo API URL is different in different environments. Right now, we are using a sandboxing URL. In VEDA, the URL is defined in edx-video-worker instance-config.yaml but it is sandboxing URL in edx-video-pipeline. That's why I am thinking that we can override Cielo URL settings at deployment and use actual API URL. Although I will make the code to use defaults if some settings aren't found.
| # Transcript provider settings variables, which will be overridden at deployment if needed | ||
| # NOTE: These settings must be added in edx-platform settings to allow deployment override and val integration | ||
| CIELO24_SETTINGS = dict( | ||
| CIELO24_API_VERSION=1, |
There was a problem hiding this comment.
nit: since these settings will live both on val/platform and vem, we should add a comment suggesting to change the version, whenever we do, in both places (VEM and VAL/Platform) or keep it in some config repo to update it in one place
There was a problem hiding this comment.
These settings aren't utilized by edxval as edxval is to be plugged in edx-platform. I will add the comment when adding the settings in platform.
There was a problem hiding this comment.
I added the settings here to show what set of values are present in it.
|
@zainab-amir / @azarembok Can you take a second look? I have done major code refactoring, mainly move the |
edxval/transcript_utils.py
Outdated
| missing=' and '.join(missing) | ||
| ) | ||
|
|
||
| return TranscriptionProviderErrorType.MISSING_REQUIRED_ATTRIBUTES, error_message |
There was a problem hiding this comment.
Why is it always returning MISSING_REQUIRED_ATTRIBUTES error type? Can we return None if we don't set any error message? then we will not have to explicitly set error_type to None here
085200f#diff-d78075b51c3378064d929ce6515f2181R1248
PROD-1380
Description
This PR is adding two API endpoints for TranscriptCredentials model(added in #208) with the following capabilities:
The purpose of the API is to move the transcription information part out of VEDA and into the VAL as part of VEM development. Most bits of the code are taken from VEDA with necessary changes https://github.com/edx/edx-video-pipeline/blob/master/VEDA_OS01/views.py#L119.
Another important bit that the following settings should be added to platform so that it can be overridden and eventually used by VAL during the deployment: