feat: atlas pull for XBlock translations | FC-0012

[OmarIthawi]

Implements the following proposal:

• doc: design for atlas pull for the edx-platform and its plugins FC-0012 #33166

Known limitations

• Test i18n translations:
  • JavaScript
  • Python
• This won't pull other plugin translations such as edx-val and any non-XBlock library. A follow up pull request will be created.
• Address translation.to_locale(lang_code) vs translation.to_locale(translation.get_language())
• Fix get_statici18n_js_url
• Refactor settings location and shared variables
• Consider a dedicated plugin_translations edX Platform module independent of XBlock module
• Fix failed tests
• Update the method and directory names to match the python implementation in the 0019-oep-58-atlas-translations-design.rst document
• Upgrade XBlock to 1.9.0:
  • feat: add catalog url and js namespace for OEP-58 JS translations | FC-0012 XBlock#691
  • Python Requirements Update #33948 (done manually via this PR)
• Ensure non--verbose silent the output is respected in pull_xblock_translations, otherwise remove the command

Testing instructions

Install the Drag and Drop v2 fork which supports OEP-58 translations:

devstack $ make cms-shell
edx-platform # pip uninstall xblock-drag-and-drop-v2
edx-platform # pip install -e https://github.com/Zeit-Labs/xblock-drag-and-drop-v2/archive/atlas-translations.zip
edx-platform # make OPENEDX_ATLAS_PULL=yes pull_xblock_translations
devstack $ make cms-restart

Then add a Drag and Drop XBlock instance and it should use the translations from the atlas pull results instead of the bundled ones.

Of course, it'll be difficult to distinguish which is which, therefore I've hacked the process as described in the Testing results.

Testing results

I've manually replaced the atlas pull French translations with Chinese so I ensure that I'm not just seeing stale translations.

This is a temp. hack during testing and it looks like the Drag and Drop v2 translation is being pulled correctly via atlas.

These are the lines that I've added after the atlas pull command:

def atlas_pull_xblock_by_module_names():
    ...
    # This hack purpose is to make it obvious that the translations are working by showing Chinese instead of Arabic.
    for dummy_lang in ['ar', 'fr']:
        subprocess.run(
            args=f'rm -rf {dummy_lang} && cp -r zh_CN {dummy_lang}',
            shell=True,
            cwd=f'{XBLOCK_TRANSLATIONS_ROOT}/drag_and_drop_v2/conf/locale',
            check=True,
        )

Screenshots

Python code translations

JavaScript code translations

Refs

This pull request is part of the FC-0012 project which implements the Translation Infrastructure update OEP-58.

[openedx-webhooks]

Thanks for the pull request, @OmarIthawi! 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.

[feanil]

Here you're returning a path type but above you're returning a string, is there a reason for that?

[OmarIthawi]

Generally, Path and str are interchangeable and get_javascript_i18n_file_name is used for URLs as well as file system paths. Therefore, it makes sense to use str imo.

[OmarIthawi]

@feanil @kdmccormick Thanks a lot for the code review yesterday!

I've addressed most of the review comments already and added the rest as TODO check list items in the GitHub pull request description. Feel free to take a look now or later after tests are fixed.

I'll fix tests and tag you for a review before I start adding new test cases.

cc: @brian-smith-tcril

[feanil]

Changes look good to me, I like the more general management command names also so it will be good to update the docs to match as you are already planning to do.

[OmarIthawi]

Thanks @feanil for the review. I'll finish the remainder TODOs, add tests and let you know when I'm done for a final review.

[OmarIthawi]

@feanil this is ready again for review. I've added tests and refactored the work into reusable modules for future work (within FC-0012).

[feanil]

Hey Omar, sounds good. FYI, I'm going on vacation for the next week so I probably won't get to this till the new year but I'll review it as soon as I can after I'm back. Looks like there are still some test failures to deal with. Let me know if you want me to wait for fixes or if I should start review as soon as I can.

[feanil]

Generally looks like it's ready to go, I had one question about an empty test file.

[feanil]

Suggested change

	pull_xblock_translations:
	pull_xblock_translations: ## pull xblock translations via atlas

This comment becomes the help text for make help so it's useful to have something.

[feanil]

Is this file supposed to be empty?

[OmarIthawi]

no, i forgot to add test for this one

[feanil]

@OmarIthawi it looks like this is good to go, is there anything else we need to do before we merge this?

[OmarIthawi]

@feanil I think it's ready, I've pushed it with few more tests in test_i18n_api.py.

[OmarIthawi]

@feanil @brian-smith-tcril A gentle reminder that this pull request is ready to be merged.

[brian-smith-tcril]

Left a couple small docs suggestions but overall this LGTM!

[feanil]

@OmarIthawi if the suggestions look good to you, can you add them in? I can merge this PR tomorrow morning our time.

[feanil]

Looks like this needs a rebase and conflict resolution as well now that the other atlas PR landed.

[OmarIthawi]

Thanks @feanil for the quick triage.

Conflicts has been resolved and I've tested it again and it seems to be playing well.

I've changed this line to match the Tutor option to have one less surprise for operators.

[openedx-webhooks]

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