feat: code changes for ai_languages field #4586
Conversation
zawan-ila
changed the title
zawan-ila
changed the title
| @@ -1262,3 +1263,42 @@ def get_course_run_statuses(statuses, course_runs): | |||
| else: | |||
| statuses.add(course_run.status) | |||
| return statuses | |||
|
|
|||
| AI_LANG_SCHEMA = { | |||
There was a problem hiding this comment.
Not too sure about this change. I like it, as it ensures that we don't put any ugly stuff accidentally in our field. I am a little concerned about having the validation on every save though.
There was a problem hiding this comment.
Maybe we should only do the validation in the command, and not on every save?
There was a problem hiding this comment.
I think doing validation at the command level would be fine.
| @@ -3531,6 +3532,8 @@ def clean(self): | |||
| self.enrollment_count = 0 | |||
| if self.recent_enrollment_count is None: | |||
| self.recent_enrollment_count = 0 | |||
There was a problem hiding this comment.
I can't comment on unchanged code. Can I safely remove the translation_languages field from the model here (but not commit the migration)? I think yes, but would love to know if I am wrong.
| # Remove any keys other than `code` and `label` | ||
| available_translation_languages = [{'code': lang['code'], 'label': lang['label']} for lang in available_translation_languages] | ||
|
|
||
| # Add the labels for the codes. Currently we set the code as the label. We will be fixing this in a follow-up PR |
There was a problem hiding this comment.
Right now I am setting the code as the label for transcription_languages. We definitely need to improve this. I intend to leave it as is for now, and handle this in another ticket
| resource_url = urljoin(self.lms_url, resource) | ||
|
|
||
| try: | ||
| response = self.client.get(resource_url, params={'course_id': course_run_id}) | ||
| response.raise_for_status() | ||
| return response.json() | ||
| except RequestException as e: | ||
| logger.exception(f'Failed to fetch translation data for course run [{course_run_id}]: {e}') | ||
| logger.exception(f'Failed to fetch data for course run [{course_run_id}]: {e}') |
There was a problem hiding this comment.
Log message can be specific i.e. translations_and_transcriptions data for course run.
| "additionalProperties": False | ||
| } | ||
| Draft202012Validator.check_schema(AI_LANG_SCHEMA) | ||
| def validate_ai_languages(ai_langs): |
There was a problem hiding this comment.
Some docstrings would be nice that explains what it does.
| Draft202012Validator.check_schema(AI_LANG_SCHEMA) | ||
| def validate_ai_languages(ai_langs): | ||
| try: | ||
| Draft202012Validator(AI_LANG_SCHEMA).validate(ai_langs) |
There was a problem hiding this comment.
Can you explain the working on Draft202012Validator? Do we really need to send AI_LANG_SCHEMA both here and in check_schema?
| @@ -66,6 +66,7 @@ gspread | |||
| html2text | |||
| lxml[html_clean] | |||
| jsonfield | |||
| jsonschema | |||
There was a problem hiding this comment.
You'll need to run make upgrade locally to generate the .txt files.
| @@ -1262,3 +1263,42 @@ def get_course_run_statuses(statuses, course_runs): | |||
| else: | |||
| statuses.add(course_run.status) | |||
| return statuses | |||
|
|
|||
| AI_LANG_SCHEMA = { | |||
| "type": "object", | |||
There was a problem hiding this comment.
nit: move this lang schema to constants.py file
PROD-4306
This is a continuation of 4585
This PR adds the relevant code changes for shifting from the
translation_languagesfield to theai_languagesfield. Since we do not care about preserving/copying existing data in thetranslation_languagesfield, I have chosen to direct all reads/writes only to the new fieldai_languages