fix: disable video status translation for JSON endpoint

[CefBoud]

Description

This PR is related to GET /videos endpoint in JSON format. It disables server-side translation for this format. This endpoint is consumed by frontend-lib-content-components which is used by frontend-app-course-authoring.
The usecase is a Video Gallery that lists videos and enables the user to filter by video status. The issue arises from the fact that the statuses returned by this endpoint are already translated which makes comparing their values to the filter's predefined values problematic.

The changes introduced here only disable translation but still send the English human-friendly string instead of the canonical status values because there is a one-to-many relationship between the status formats. For instance Failed maps to multiple canonical statues: file_corrupt, pipeline_error, etc. So sending the canonical statues would require duplicating those mapping which is far form ideal. Thus, considering that the English human-friendly status as a reference to be filtered against (and manipulated in other different ways) seemed like a reasonable trade-off.

Supporting information

Private ref: BB-8076

Testing instructions

1. Run make dev.up.cms
2. Enable mock video uploads (create waffle flag contentstore.mock_video_uploads enabled for everyone)
3. Modify def _get_and_validate_course as follows. This a quick and dirty way to circumvent the video pipeline checks.

def _get_and_validate_course(course_key_string, user):
    """
    Given a course key, return the course if it exists, the given user has
    access to it, and it is properly configured for video uploads
    """
    course_key = CourseKey.from_string(course_key_string)

    # For now, assume all studio users that have access to the course can upload videos.
    # In the future, we plan to add a new org-level role for video uploaders.
    return get_course_and_check_access(course_key, user)

4.Modify def handle_videos to always return json to facilitate testing from the browser.

def handle_videos(request, course_key_string, edx_video_id=None):
    .
    .
    if request.method == "GET":
        return videos_index_json(course)
    .
    .

5. Head to Course Videos in Django Admin and create some course videos for the Demo course. Use the Devstack provisionned videos.
   

6. Head to http://localhost:18010/videos/course-v1:edX+DemoX+Demo_Course. You should see a json of the Course Videos you created.

7. Head back to Course Videos in the admin panel and edit the status of the Video that your Course Video links to. You must use canonical values. Set status to ingest or upload_failed which will map to In Progress and Failed respectively.

8. Head to your account and change your language setting. (For some reason, only English was showing in my case. I simply changed the value of the Option HTML tag to fr instead of en and it worked.)

Before applying the changes in this MR, the video status would be translated. After applying it, it is not.

Apologies for the convoluted instructions.

[openedx-webhooks]

Thanks for the pull request, @CefBoud! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[tecoholic]

@CefBoud While the solution works as you have outlined, I find passing skip_translation around so many functions suboptimal. While skip_translation is the appropriate variable name for the StatusDisplayStrings class, it might have to become skip_video_status_translation in places like _get_videos to avoid confusions relating to transcripts and translations.
So I am really trying to avoid it.

Taking a step back, the translations are required only for presentation purposes, which means, it's the responsibility of the presentation layer (HTML templates) to render the correct strings. That seems to be the case in the one instance I found using the status attribute. So, it is possible that just removing the _() gettext call Line 178 alone might be sufficient.

Can we check if that would be a valid solution? Identify the places where the status is used in the templates, underscore, and JS ..etc., and check if it is wrapped with an i18n call? If not, wrap it and remove the translation from the StatusDisplaySettings?

Note:

• Extra Languages can be added by configuring the Dark Language config. I added en,fr,es,de as supported values and English and French showed up in my Account Settings page. I am guessing Spanish and German are beta-languages.

[CefBoud]

@tecoholic Thank you for the review.
I completely agree with you that passing around the parameter on so many functions is far from ideal. it felt somewhat off.

Can we check if that would be a valid solution? Identify the places where the status is used in the templates, underscore, and JS ..etc., and check if it is wrapped with an i18n call? If not, wrap it and remove the translation from the StatusDisplaySettings?

This could work. However, may I bring your attention to the fact that the instance you linked to relates to the active/uploading statuses defined in the frontend and for which translation is available in djangojs.js files unlike the statues impacted by this PR for which translation is only availbale in django.po files.

Therefore, please correct me if I am wrong, gettext won't work right off the bat.

PS: thanks for the language tip, it worked splendidly.

[tecoholic]

@CefBoud Ah! I see. I knew my lazy searching was leaving things out. I didn't realize I had the completely wrong one. I want to ask someone with better knowledge of the system to see if there is a way to make this simpler.

@Agrendalath Can you kindly take a look here? Do you see a way to avoid passing skip_translation around?

[Agrendalath]

@tecoholic, @CefBoud, I see this code for the first time, so I do not have the full context.

Do I understand correctly that we must keep the translated string because some existing platform's code relies on it? Will this code be removed later, during removing the legacy views that the MFEs will replace?

If this is the case, we could return both values, i.e., status and status_key. The latter would contain the non-translated status. We would mark the status field as deprecated so it can be removed some time after we remove the legacy pages.

[CefBoud]

@Agrendalath

Will this code be removed later, during removing the legacy views that the MFEs will replace?

I am not aware of the roadmap regarding the removal of legacy views.

TL;DR

The Problem

• The statuses are handled in the backend:
  1. Status keys are mapped to an English human-friendly format in a N-to-1 fashion e.g. file_corrupt, pipeline_error => Failed (returning only the status_key to the MFE would cause the loss of this mapping)
  2. Then, these English human-friendly statuses are translated in the backend through django.po files.
• The legacy frontend expects and works with the translated statuses.
• The new MFE can't work with the already translated statuses (this is the bug this PR is trying to address) and will ideally handle translation with React.

The solution so far

• As of now, the PR simply disables translation if the request originates from the MFE. The solution works but it lacks elegance/is suboptimal as it passes around a skip_translation arg several function-levels deep.

The counterproposal

• @tecoholic suggested to disable translation in the backend altogether (not just in the case of MFE request) and transfer the status translation to the legacy frontend. ==> This would make the changes in the backend super simple (remove translation) but the problem with that, I believe, is that the status translation files are django.po and gettext won't work in the legacy frontend right off the bat as it needs djangojs.js files.

[Agrendalath]

@CefBoud, using the status_key name was unclear, so let me rephrase my suggestion a bit. Would it work if we removed the translation from the get method and changed this line to something like this?

# The MFEs handle translating the status on their own, so we can remove the second line once the legacy views are deleted.
video['status_nontranslated'] = convert_video_status(video, is_video_encodes_ready)
video['status'] = _(convert_video_status(video, is_video_encodes_ready))

This way, the MFE could use status_nontranslated (this is not the best name), which is the mapped non-translated status.
Plus, if this is what we want to do, we could use the original status value to display the translated string to the user in the MFE. We would not need to add extra translations there.

It would also be fully backward-compatible.

@tecoholic suggested to disable translation in the backend altogether (not just in the case of MFE request) and transfer the status translation to the legacy frontend. ==> This would make the changes in the backend super simple (remove translation) but the problem with that, I believe, is that the status translation files are django.po and gettext won't work in the legacy frontend right off the bat as it needs djangojs.js files.

This is also doable, but IMHO it will introduce more code duplication, as we would need to duplicate these statuses in JS to generate the translation strings for them.

[CefBoud]

@Agrendalath

Thank you for further explaining your suggestion. I love it !!! It amounts to very little change and I feel the introduction of the new status_nontranslated (I couldn't come up with a better name😅) field brings more clarity.

CC @tecoholic

[tecoholic]

@CefBoud 👍

• I tested this: Followed directions in PR, and verified that there is a new status_untranslated field with the expected strings in the JSON response.
• I read through the code
• I checked for accessibility issues
• Includes documentation

[openedx-webhooks]

I don't have a Jira project configured for this repo in the private 2U Jira. Contact Ned Batchelder (@nedbat) to set up a project.

[tecoholic]

@jristau1984 @cablaa77 This is related to the Video Upload block, where the filtering breaks for non-English users.

[farhaanbukhsh]

@mphilbrick211 any update on the review on this PR?

[openedx-webhooks]

@CefBoud 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.