From e698cc363f95dbdb38c7fc2720f44e50ef3b6602 Mon Sep 17 00:00:00 2001 From: Omar Al-Ithawi Date: Thu, 21 Dec 2023 12:03:01 +0300 Subject: [PATCH 1/2] chore: bump to xblock[django]==1.9.0 --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 0a7979630521..e627caf38d8c 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -1199,7 +1199,7 @@ wrapt==1.16.0 # via # -r requirements/edx/paver.txt # deprecated -xblock[django]==1.8.1 +xblock[django]==1.9.0 # via # -r requirements/edx/kernel.in # acid-xblock diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index df63fe4ee63b..59e8da40f9e0 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -2177,7 +2177,7 @@ wrapt==1.16.0 # -r requirements/edx/testing.txt # astroid # deprecated -xblock[django]==1.8.1 +xblock[django]==1.9.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 4d880f8f9d2d..1ff39f3c88c0 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1466,7 +1466,7 @@ wrapt==1.16.0 # via # -r requirements/edx/base.txt # deprecated -xblock[django]==1.8.1 +xblock[django]==1.9.0 # via # -r requirements/edx/base.txt # acid-xblock diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 2b321570bd68..b58473c1ce8f 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1591,7 +1591,7 @@ wrapt==1.16.0 # -r requirements/edx/base.txt # astroid # deprecated -xblock[django]==1.8.1 +xblock[django]==1.9.0 # via # -r requirements/edx/base.txt # acid-xblock From a5251cc70550f5342a31e10bde0ea7f593c4482d Mon Sep 17 00:00:00 2001 From: Omar Al-Ithawi Date: Mon, 4 Sep 2023 09:15:04 +0300 Subject: [PATCH 2/2] feat: `atlas pull` for XBlock translations --- .gitignore | 2 + Makefile | 19 ++- .../commands/compile_xblock_translations.py | 22 +++ .../commands/pull_xblock_translations.py | 51 ++++++ .../xblock_django/tests/test_commands.py | 70 ++++++++ .../xblock_django/tests/test_translation.py | 26 +++ .../djangoapps/xblock_django/translation.py | 156 ++++++++++++++++++ .../0019-oep-58-atlas-translations-design.rst | 113 ++++++++++--- openedx/core/djangoapps/plugins/i18n_api.py | 93 +++++++++++ .../core/djangoapps/plugins/tests/__init__.py | 0 .../djangoapps/plugins/tests/test_i18n_api.py | 52 ++++++ xmodule/modulestore/api.py | 46 ++++++ xmodule/modulestore/django.py | 82 +++++++-- xmodule/modulestore/tests/conftest.py | 107 ++++++++++++ xmodule/modulestore/tests/fixtures/django.mo | Bin 0 -> 360 bytes xmodule/modulestore/tests/fixtures/django.po | 22 +++ xmodule/modulestore/tests/test_api.py | 75 +++++++++ .../modulestore/tests/test_django_utils.py | 54 ++++++ 18 files changed, 941 insertions(+), 49 deletions(-) create mode 100644 common/djangoapps/xblock_django/management/commands/compile_xblock_translations.py create mode 100644 common/djangoapps/xblock_django/management/commands/pull_xblock_translations.py create mode 100644 common/djangoapps/xblock_django/tests/test_commands.py create mode 100644 common/djangoapps/xblock_django/tests/test_translation.py create mode 100644 common/djangoapps/xblock_django/translation.py create mode 100644 openedx/core/djangoapps/plugins/i18n_api.py create mode 100644 openedx/core/djangoapps/plugins/tests/__init__.py create mode 100644 openedx/core/djangoapps/plugins/tests/test_i18n_api.py create mode 100644 xmodule/modulestore/api.py create mode 100644 xmodule/modulestore/tests/conftest.py create mode 100644 xmodule/modulestore/tests/fixtures/django.mo create mode 100644 xmodule/modulestore/tests/fixtures/django.po create mode 100644 xmodule/modulestore/tests/test_api.py create mode 100644 xmodule/modulestore/tests/test_django_utils.py diff --git a/.gitignore b/.gitignore index 169296690009..cc568a924c52 100644 --- a/.gitignore +++ b/.gitignore @@ -61,6 +61,8 @@ conf/locale/fake*/LC_MESSAGES/*.po conf/locale/fake*/LC_MESSAGES/*.mo # this was a mistake in i18n_tools, now fixed. conf/locale/messages.mo +conf/plugins-locale/ +/*/static/js/xblock.v1-i18n/ ### Testing artifacts .testids/ diff --git a/Makefile b/Makefile index ff03f36554bc..cc6fa5591376 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,8 @@ docker_auth docker_build docker_tag_build_push_lms docker_tag_build_push_lms_dev \ docker_tag_build_push_cms docker_tag_build_push_cms_dev docs extract_translations \ guides help lint-imports local-requirements migrate migrate-lms migrate-cms \ - pre-requirements pull pull_translations push_translations requirements shell swagger \ + pre-requirements pull pull_xblock_translations pull_translations push_translations \ + requirements shell swagger \ technical-docs test-requirements ubuntu-requirements upgrade-package upgrade # Careful with mktemp syntax: it has to work on Mac and Ubuntu, which have differences. @@ -55,7 +56,15 @@ endif push_translations: ## push source strings to Transifex for translation i18n_tool transifex push -pull_translations: ## pull translations from Transifex +pull_xblock_translations: ## pull xblock translations via atlas + rm -rf conf/plugins-locale # Clean up existing atlas translations + rm -rf lms/static/i18n/xblock.v1 cms/static/i18n/xblock.v1 # Clean up existing xblock compiled translations + mkdir -p conf/plugins-locale/xblock.v1/ lms/static/js/xblock.v1-i18n cms/static/js + python manage.py lms pull_xblock_translations --verbose $(ATLAS_OPTIONS) + python manage.py lms compile_xblock_translations + cp -r lms/static/js/xblock.v1-i18n cms/static/js + +pull_translations: ## pull translations from Transifex git clean -fdX conf/locale ifeq ($(OPENEDX_ATLAS_PULL),) i18n_tool transifex pull @@ -65,13 +74,13 @@ ifeq ($(OPENEDX_ATLAS_PULL),) git clean -fdX conf/locale/rtl git clean -fdX conf/locale/eo i18n_tool validate --verbose - paver i18n_compilejs else + make pull_xblock_translations find conf/locale -mindepth 1 -maxdepth 1 -type d -exec rm -r {} \; - atlas pull $(OPENEDX_ATLAS_ARGS) translations/edx-platform/conf/locale:conf/locale + atlas pull $(ATLAS_OPTIONS) translations/edx-platform/conf/locale:conf/locale i18n_tool generate - paver i18n_compilejs endif + paver i18n_compilejs detect_changed_source_translations: ## check if translation files are up-to-date diff --git a/common/djangoapps/xblock_django/management/commands/compile_xblock_translations.py b/common/djangoapps/xblock_django/management/commands/compile_xblock_translations.py new file mode 100644 index 000000000000..77662a37388b --- /dev/null +++ b/common/djangoapps/xblock_django/management/commands/compile_xblock_translations.py @@ -0,0 +1,22 @@ +""" +Compile the translation files for the XBlocks. +""" + +from django.core.management.base import BaseCommand + +from xmodule.modulestore import api as xmodule_api + +from openedx.core.djangoapps.plugins.i18n_api import compile_po_files + +from ...translation import ( + compile_xblock_js_messages, +) + + +class Command(BaseCommand): + """ + Compile the translation files for the XBlocks. + """ + def handle(self, *args, **options): + compile_po_files(xmodule_api.get_python_locale_root()) + compile_xblock_js_messages() diff --git a/common/djangoapps/xblock_django/management/commands/pull_xblock_translations.py b/common/djangoapps/xblock_django/management/commands/pull_xblock_translations.py new file mode 100644 index 000000000000..3eb3f519e6be --- /dev/null +++ b/common/djangoapps/xblock_django/management/commands/pull_xblock_translations.py @@ -0,0 +1,51 @@ +""" +Download the translations via atlas for the XBlocks. +""" + +from django.core.management.base import BaseCommand, CommandError + +from openedx.core.djangoapps.plugins.i18n_api import ATLAS_ARGUMENTS +from xmodule.modulestore import api as xmodule_api + +from ...translation import xblocks_atlas_pull + + +class Command(BaseCommand): + """ + Pull the XBlock translations via atlas for the XBlocks. + + For detailed information about atlas pull options check the atlas documentation: + + - https://github.com/openedx/openedx-atlas + """ + + def add_arguments(self, parser): + for argument in ATLAS_ARGUMENTS: + parser.add_argument(*argument.get_args(), **argument.get_kwargs()) + + parser.add_argument( + '--verbose|-v', + action='store_true', + default=False, + dest='verbose', + help='Verbose output using `--verbose` argument for `atlas pull`.', + ) + + def handle(self, *args, **options): + xblock_translations_root = xmodule_api.get_python_locale_root() + if list(xblock_translations_root.listdir()): + raise CommandError(f'"{xblock_translations_root}" should be empty before running atlas pull.') + + atlas_pull_options = [] + + for argument in ATLAS_ARGUMENTS: + option_value = options.get(argument.dest) + if option_value is not None: + atlas_pull_options += [argument.flag, option_value] + + if options['verbose']: + atlas_pull_options += ['--verbose'] + else: + atlas_pull_options += ['--silent'] + + xblocks_atlas_pull(pull_options=atlas_pull_options) diff --git a/common/djangoapps/xblock_django/tests/test_commands.py b/common/djangoapps/xblock_django/tests/test_commands.py new file mode 100644 index 000000000000..322ee9384f37 --- /dev/null +++ b/common/djangoapps/xblock_django/tests/test_commands.py @@ -0,0 +1,70 @@ +""" +Tests for the pull_xblock_translations management command. +""" + +from path import Path +from unittest.mock import patch + +from django.core.management import call_command + +from done import DoneXBlock + +from xmodule.modulestore.api import ( + get_python_locale_root, + get_javascript_i18n_file_path, +) +from xmodule.modulestore.tests.conftest import tmp_translations_dir + + +def test_pull_xblock_translations(tmp_path): + """ + Test the compile_xblock_translations management command. + """ + temp_xblock_locale_path = Path(str(tmp_path)) + + with patch('common.djangoapps.xblock_django.translation.get_non_xmodule_xblocks') as mock_get_non_xmodule_xblocks: + with patch('xmodule.modulestore.api.get_python_locale_root') as mock_get_python_locale_root: + with patch('subprocess.run') as mock_run: + mock_get_python_locale_root.return_value = Path(str(temp_xblock_locale_path)) + mock_get_non_xmodule_xblocks.return_value = [('done', DoneXBlock)] + + call_command( + 'pull_xblock_translations', + filter='ar,de_DE,jp', + repository='openedx/custom-translations', + branch='release/redwood', + ) + + assert mock_run.call_count == 1, 'Calls `subprocess.run`' + assert mock_run.call_args.kwargs['args'] == [ + 'atlas', 'pull', + '--expand-glob', + '--filter', 'ar,de_DE,jp', + '--repository', 'openedx/custom-translations', + '--branch', 'release/redwood', + '--silent', + 'translations/*/done/conf/locale:done', + ] + + +def test_compile_xblock_translations(tmp_translations_dir): + """ + Test the compile_xblock_translations management command. + """ + # msgfmt isn't available in test environment, so we mock the `subprocess.run` and copy the django.mo file, + # it to ensure `compile_xblock_js_messages` can work. + with tmp_translations_dir(xblocks=[('done', DoneXBlock)], fixtures_to_copy=['django.po', 'django.mo']): + with patch.object(DoneXBlock, 'i18n_js_namespace', 'TestingDoneXBlockI18n'): + po_file = get_python_locale_root() / 'done/tr/LC_MESSAGES/django.po' + + with patch('subprocess.run') as mock_run: + call_command('compile_xblock_translations') + assert mock_run.call_count == 1, 'Calls `subprocess.run`' + assert mock_run.call_args.kwargs['args'] == [ + 'msgfmt', '--check-format', '-o', str(po_file.with_suffix('.mo')), str(po_file), + ], 'Compiles the .po files' + + js_file_text = get_javascript_i18n_file_path('done', 'tr').text() + assert 'Merhaba' in js_file_text, 'Ensures the JavaScript catalog is compiled' + assert 'TestingDoneXBlockI18n' in js_file_text, 'Ensures the namespace is used' + assert 'gettext' in js_file_text, 'Ensures the gettext function is defined' diff --git a/common/djangoapps/xblock_django/tests/test_translation.py b/common/djangoapps/xblock_django/tests/test_translation.py new file mode 100644 index 000000000000..4e4ed937c513 --- /dev/null +++ b/common/djangoapps/xblock_django/tests/test_translation.py @@ -0,0 +1,26 @@ +""" +Tests for the xblock_django.translation module. +""" + +from done import DoneXBlock + +from ..translation import ( + get_non_xmodule_xblock_module_names, + get_non_xmodule_xblocks, +) + + +def test_get_non_xmodule_xblock_module_names(): + """ + Ensure xmodule isn't returned but other default xblocks are. + """ + assert 'xmodule' not in get_non_xmodule_xblock_module_names() + assert 'done' in get_non_xmodule_xblock_module_names() + assert 'lti_consumer' in get_non_xmodule_xblock_module_names() + + +def test_get_non_xmodule_xblocks(): + """ + Ensures that default XBlocks are included. + """ + assert ('done', DoneXBlock) in get_non_xmodule_xblocks() diff --git a/common/djangoapps/xblock_django/translation.py b/common/djangoapps/xblock_django/translation.py new file mode 100644 index 000000000000..72726221aac1 --- /dev/null +++ b/common/djangoapps/xblock_django/translation.py @@ -0,0 +1,156 @@ +""" +XBlock translations pulling and compilation logic. +""" + +import os +import gettext + +from django.utils.encoding import force_str +from django.views.i18n import JavaScriptCatalog +from django.utils.translation import override, to_locale, get_language +from statici18n.management.commands.compilejsi18n import Command as CompileI18NJSCommand +from xblock.core import XBlock + +from openedx.core.djangoapps.plugins.i18n_api import atlas_pull_by_modules +from xmodule.modulestore import api as xmodule_api + + +class AtlasJavaScriptCatalog(JavaScriptCatalog): + """ + View to return the selected language catalog as a JavaScript library. + + This extends the JavaScriptCatalog class to allow custom domain and locale_dir. + """ + + translation = None + + def get(self, request, *args, **kwargs): + """ + Return the selected language catalog as a JavaScript library. + + This overrides the JavaScriptCatalog.get() method class to allow custom locale_dir. + """ + selected_language = get_language() + locale = to_locale(selected_language) + domain = kwargs['domain'] + locale_dir = kwargs['locale_dir'] + # Using GNUTranslations instead of DjangoTranslation to allow custom locale_dir without needing + # to use a custom `text.mo` translation domain. + self.translation = gettext.translation(domain, localedir=locale_dir, languages=[locale]) + context = self.get_context_data(**kwargs) + return self.render_to_response(context) + + @classmethod + def simulate_get_request(cls, locale, domain, locale_dir): + """ + Simulate a GET request to the JavaScriptCatalog view. + + Return: + str: The rendered JavaScript catalog. + """ + with override(locale): + catalog_view = cls() + response = catalog_view.get( + request=None, # we are passing None as the request, as the request + # object is currently not used by django + domain=domain, + locale_dir=locale_dir, + ) + return force_str(response.content) + + +def mo_file_to_js_namespaced_catalog(xblock_conf_locale_dir, locale, domain, namespace): + """ + Compile .mo to .js gettext catalog and wrap it in a namespace via the `compilejsi18n` command helpers. + """ + rendered_js = AtlasJavaScriptCatalog.simulate_get_request( + locale=locale, + locale_dir=xblock_conf_locale_dir, + domain=domain, + ) + + # The `django-statici18n` package has a non-standard code license, therefore we're using its private API + # to avoid copying the code into this repository and running into licensing issues. + compile_i18n_js_command = CompileI18NJSCommand() + namespaced_catalog_js_code = compile_i18n_js_command._get_namespaced_catalog( # pylint: disable=protected-access + rendered_js=rendered_js, + namespace=namespace, + ) + + return namespaced_catalog_js_code + + +def xblocks_atlas_pull(pull_options): + """ + Atlas pull the translations for the XBlocks that are installed. + """ + xblock_module_names = get_non_xmodule_xblock_module_names() + + atlas_pull_by_modules( + module_names=xblock_module_names, + locale_root=xmodule_api.get_python_locale_root(), + pull_options=pull_options, + ) + + +def compile_xblock_js_messages(): + """ + Compile the XBlock JavaScript messages from .mo file into .js files. + """ + for xblock_module, xblock_class in get_non_xmodule_xblocks(): + xblock_conf_locale_dir = xmodule_api.get_python_locale_root() / xblock_module + i18n_js_namespace = xblock_class.get_i18n_js_namespace() + + for locale_dir in xblock_conf_locale_dir.listdir(): + locale_code = str(locale_dir.basename()) + locale_messages_dir = locale_dir / 'LC_MESSAGES' + js_translations_domain = None + for domain in ['djangojs', 'django']: + po_file_path = locale_messages_dir / f'{domain}.mo' + if po_file_path.exists(): + if not js_translations_domain: + # Select which file to compile to `django.js`, while preferring `djangojs` over `django` + js_translations_domain = domain + + if js_translations_domain and i18n_js_namespace: + js_i18n_file_path = xmodule_api.get_javascript_i18n_file_path(xblock_module, locale_code) + os.makedirs(js_i18n_file_path.dirname(), exist_ok=True) + js_namespaced_catalog = mo_file_to_js_namespaced_catalog( + xblock_conf_locale_dir=xblock_conf_locale_dir, + locale=locale_code, + domain=js_translations_domain, + namespace=i18n_js_namespace, + ) + + with open(js_i18n_file_path, 'w', encoding='utf-8') as f: + f.write(js_namespaced_catalog) + + +def get_non_xmodule_xblocks(): + """ + Returns a list of XBlock classes with their module name excluding edx-platform/xmodule xblocks. + """ + xblock_classes = [] + for _xblock_tag, xblock_class in XBlock.load_classes(): + xblock_module_name = xmodule_api.get_root_module_name(xblock_class) + if xblock_module_name != 'xmodule': + # XBlocks in edx-platform/xmodule are already translated in edx-platform/conf/locale + # So there is no need to add special handling for them. + xblock_classes.append( + (xblock_module_name, xblock_class), + ) + + return xblock_classes + + +def get_non_xmodule_xblock_module_names(): + """ + Returns a list of module names for the plugins that supports translations excluding `xmodule`. + """ + xblock_module_names = set( + xblock_module_name + for xblock_module_name, _xblock_class in get_non_xmodule_xblocks() + ) + + sorted_xblock_module_names = list(sorted(xblock_module_names)) + return sorted_xblock_module_names diff --git a/docs/decisions/0019-oep-58-atlas-translations-design.rst b/docs/decisions/0019-oep-58-atlas-translations-design.rst index 1b7f573e2303..a59e774a0535 100644 --- a/docs/decisions/0019-oep-58-atlas-translations-design.rst +++ b/docs/decisions/0019-oep-58-atlas-translations-design.rst @@ -110,26 +110,12 @@ we will use `openedx-atlas`_ to pull them from the `openedx-translations repo`_. -New ``atlas_pull_plugin_translations`` command +New ``pull_xblock_translations`` commands ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -Introduce new Django commands to the ``edx-platform``: +Introduce new Django command to the ``edx-platform``: -- ``manage.py lms atlas_pull_plugin_translations --list``: List all XBlocks and - Plugins installed in the ``edx-platform`` virtual environment. This will - list the Python *module names* (as opposed to git repository names) of the - installed XBlocks and Plugins e.g.:: - - $ manage.py lms atlas_pull_plugin_translations --list - drag_and_drop_v2 - done - eox_tenant - - This list doesn't include plugins that are bundled within the - ``edx-platform`` repository itself. Translations for bundled plugins - are included in the ``edx-platform`` translation files. - -- ``manage.py lms atlas_pull_plugin_translations``: This command +- ``manage.py lms pull_xblock_translations``: This command will pull translations for installed XBlocks and Plugins by module name:: $ atlas pull --expand-glob \ @@ -181,6 +167,77 @@ Introduce new Django commands to the ``edx-platform``: └── django.po + +Using XBlock python module names instead of repository names +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +There's more than one identifier for XBlocks and Plugins: + +#. **The XBlock/plugin tag:** Python plugins have an entry point name which + is referred to as ``tag`` in Open edX. For example, the + tag in the `Drag and Drop XBlock setup.py file`_ is ``drag-and-drop-v2``:: + + # xblock-drag-and-drop-v2/setup.py + entry_points={ + 'xblock.v1': 'drag-and-drop-v2 = drag_and_drop_v2:DragAndDropBlock', + } + +#. **The git repository name:** Each XBlock has a unique git repository name. + For example, the Drag and Drop XBlock has the ``xblock-drag-and-drop-v2`` + repository name in GitHub: https://github.com/openedx/xblock-drag-and-drop-v2/ + +#. **Python module name:** The python module name appears in the path of + XBlock translations in the `openedx-translations repo`_. For example, + the Drag and Drop XBlock will have ``drag_and_drop_v2`` python module name + in the translations directory structure:: + + translations/xblock-drag-and-drop-v2/drag_and_drop_v2/conf/locale/... + + +The ``pull_xblock_translations`` command will use the Python module name +instead of the repository name to pull translations from the +`openedx-translations repo`_ via ``atlas``. + +Using the Python module name has the following pros and cons: + +**Pros:** + +- The python module name is available without needing to install the XBlock, + or parse the ``setup.py`` file. +- It is available in Python runtime. +- It is available in the `openedx-translations repo`_ + file structure. +- It is unique in the virtual environment which prevents + collisions. +- The python module name of XBlocks doesn't change often if at all. + +**Cons:** + +- The python module name can be confused as the XBlock tag, which can + be different in some XBlocks. +- The unique and stable identifier of XBlocks is the tag, not the + python module name. Therefore, this decision will implicitly make + the python module name another unique identifier for XBlocks. + +The trade-offs are acceptable and this decision is reversible in case +the ``xblock.tag`` needs to be used. However, this will require parsing +the ``setup.py`` file and/or installing the XBlock in order to get the tag +in the `extract-translation-source-files.yml`_ workflow in the +`openedx-translations repo`_. + +Using the ``django`` and ``djangojs`` gettext domains +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +This proposal standardizes the gettext domain for XBlocks and Plugins to +``django`` and ``djangojs``. This helps to unify the file names and avoid the +need to add more complexity to the `openedx-translations repo`_ tooling. + +The `DjangoTranslation class`_ doesn't allow customizing the locale +directory for ``django.mo`` files for caching reasons. Therefore, +the `GNUTranslations class`_ will be used instead in the +``create_js_namespaced_catalog`` helper function for generating +JavaScript catalogs from ``django.mo`` files. + BlockI18nService support for ``atlas`` Python translations ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -201,22 +258,21 @@ Third-party XBlocks that are not included in the `xblocks Transifex project`_, such as the `Lime Survey XBlock`_, will benefit from this backwards compatibility. -New ``compile_plugin_js_translations`` command +New ``compile_xblock_translations`` command ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ An ``XBlock.i18n_js_namespace`` property will be added for -the ``compile_plugin_js_translations`` to generate JavaScript translations +the ``compile_xblock_translations`` to generate JavaScript translations in a centrally managed manner for installed XBlocks. -A ``compile_plugin_js_translations`` command will loop over XBlock +A ``compile_xblock_translations`` command will loop over XBlock modules that has the ``i18n_js_namespace`` property set and compile the JavaScript translations via the `compilejsi18n`_ command. For example if the Drag and Drop XBlock has ``i18n_js_namespace = 'DragAndDropI18N'``, the -``compile_plugin_js_translations`` command will execute the following -commands:: +``compile_xblock_translations`` command will execute the equivalent of the following commands:: i18n_tool generate -v # Generate the .mo files python manage.py compilejsi18n --namespace DragAndDropI18N --output conf/plugins-locale/drag_and_drop_v2/js/ @@ -237,7 +293,7 @@ XBlocks as described in the :ref:`js-translations` section. For example, the `Drag and Drop XBlock get_static_i18n_js_url`_ will need to be updated to support the new ``XBlockI18nService`` -``get_javascript_locale_path`` method and the namespace. +``get_javascript_i18n_catalog_url`` method and the namespace. .. code:: diff @@ -256,10 +312,10 @@ be updated to support the new ``XBlockI18nService`` return None + # TODO: Make this the default once OEP-58 is implemented. - + if hasattr(self.i18n_service, 'get_javascript_locale_path'): - + atlas_locale_path = self.i18n_service.get_javascript_locale_path() - + if atlas_locale_path: - + return atlas_locale_path + + if hasattr(self.i18n_service, 'get_javascript_i18n_catalog_url'): + + i18n_catalog_url = self.i18n_service.get_javascript_i18n_catalog_url() + + if i18n_catalog_url: + + return i18n_catalog_url text_js = 'public/js/translations/{lang_code}/text.js' country_code = lang_code.split('-')[0] @@ -360,3 +416,6 @@ be tackled in the future as part of the .. _xblocks Transifex project: https://www.transifex.com/open-edx/xblocks/ .. _Lime Survey XBlock: https://github.com/eduNEXT/xblock-limesurvey +.. _Drag and Drop XBlock setup.py file: https://github.com/openedx/xblock-drag-and-drop-v2/blame/192ecfc603a2314b2cb1105ebc7ba6991e459250/setup.py#L127-L129 +.. _DjangoTranslation class: https://github.com/django/django/blob/594873befbbec13a2d9a048a361757dd3cf178da/django/utils/translation/trans_real.py#L155-L161 +.. _GNUTranslations class: https://github.com/python/cpython/blob/b4144979934d7b8448f80c1fbee65dc3bfbce005/Lib/gettext.py#L528-L532 diff --git a/openedx/core/djangoapps/plugins/i18n_api.py b/openedx/core/djangoapps/plugins/i18n_api.py new file mode 100644 index 000000000000..923b09e3854f --- /dev/null +++ b/openedx/core/djangoapps/plugins/i18n_api.py @@ -0,0 +1,93 @@ +""" +edx-platform specific i18n helpers for edx-django-utils plugins. +""" + +from dataclasses import dataclass, asdict +import os +from pathlib import Path +import subprocess + + +@dataclass +class ArgparseArgument: + """ + Argument specs to be used with argparse add_argument method. + """ + + flag: str = None # This is passed as a positional argument + dest: str = None + help: str = None + + def get_kwargs(self): + """ + Return keyword arguments for the `add_argument` method . + """ + argument_dict = asdict(self) + argument_dict.pop('flag') + return argument_dict + + def get_args(self): + """ + Return positional arguments for the `add_argument` method . + """ + return [self.flag] + + +# `atlas pull` arguments defintions. +# +# - https://github.com/openedx/openedx-atlas +# +ATLAS_ARGUMENTS = [ + ArgparseArgument( + flag='--filter', + dest='filter', + help='Filter option for `atlas pull` e.g. --filter=ar,fr_CA,de_DE.' + ), + ArgparseArgument( + flag='--repository', + dest='repository', + help='Custom repository slug for `atlas pull` e.g. ' + '--repository=friendsofopenedx/openedx-translations . ' + 'Default is "openedx/openedx-translations".' + ), + ArgparseArgument( + flag='--branch', + dest='branch', + help='Custom branch for "atlas pull" e.g. --branch=release/redwood . Default is "main".', + ), +] + + +def atlas_pull_by_modules(module_names, locale_root, pull_options): + """ + Atlas pull translations by module name instead of repository name. + """ + atlas_pull_args = [ + # Asterisk (*) is used instead of the repository name because it's not known at runtime. + # The `--expand-glob` option is used to expand match any repository name that has the right module name. + f'translations/*/{module_name}/conf/locale:{module_name}' + for module_name in module_names + ] + + subprocess.run( + args=['atlas', 'pull', '--expand-glob', *pull_options, *atlas_pull_args], + check=True, + cwd=locale_root, + ) + + +def compile_po_files(root_dir): + """ + Compile the .po files into .mo files recursively in the given directory. + + Mimics the behavior of `django-admin compilemessages` for the po files but for any directory. + """ + for root, _dirs, files in os.walk(root_dir): + root = Path(root) + for po_file in files: + if po_file.endswith('.po'): + po_file_path = root / po_file + subprocess.run( + args=['msgfmt', '--check-format', '-o', str(po_file_path.with_suffix('.mo')), str(po_file_path)], + check=True, + ) diff --git a/openedx/core/djangoapps/plugins/tests/__init__.py b/openedx/core/djangoapps/plugins/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/plugins/tests/test_i18n_api.py b/openedx/core/djangoapps/plugins/tests/test_i18n_api.py new file mode 100644 index 000000000000..7e541799667b --- /dev/null +++ b/openedx/core/djangoapps/plugins/tests/test_i18n_api.py @@ -0,0 +1,52 @@ +""" +Tests for the plugins.i18n_api module. +""" + +from unittest.mock import patch + +from ..i18n_api import ( + ArgparseArgument, + atlas_pull_by_modules, +) + + +def test_argparse_argument(): + """ + Test the ArgparseArgument utility class. + """ + argument = ArgparseArgument( + flag="--filter", + dest="filter", + help="Some filter", + ) + + assert argument.get_kwargs() == { + "dest": "filter", + "help": "Some filter", + }, 'Should not include `flag` in keyword arguments to match argparse add_argument method' + + assert argument.get_args() == ['--filter'], '--filter should be a positional argument' + + +def test_atlas_pull_by_modules(): + """ + Test the atlas_pull_by_modules's subprocess.run parameters. + """ + module_names = ['done', 'drag_and_drop_v2'] + locale_root = '/tmp/locale' + + with patch('subprocess.run') as mock_run: + atlas_pull_by_modules(module_names, locale_root, ['--filter', 'ar,jp_JP', '--silent']) + + mock_run.assert_called_once_with( + args=[ + 'atlas', 'pull', + '--expand-glob', + '--filter', 'ar,jp_JP', + '--silent', + 'translations/*/done/conf/locale:done', + 'translations/*/drag_and_drop_v2/conf/locale:drag_and_drop_v2', + ], + check=True, + cwd=locale_root, + ) diff --git a/xmodule/modulestore/api.py b/xmodule/modulestore/api.py new file mode 100644 index 000000000000..8ef471bd0226 --- /dev/null +++ b/xmodule/modulestore/api.py @@ -0,0 +1,46 @@ +""" +Python APIs for the xmodule.modulestore module. +""" + +from django.conf import settings + + +def get_root_module_name(class_or_function): + """ + Return the root module name for the given class or function. + """ + module_path = class_or_function.__module__ + return module_path.split('.')[0] + + +def get_xblock_root_module_name(block): + """ + Return the XBlock Python module name. + """ + # `xblock.unmixed_class` is a property added by the XBlock library to add mixins to the class which conceals + # the original class properties. + xblock_original_class = getattr(block, 'unmixed_class', block.__class__) + return get_root_module_name(xblock_original_class) + + +def get_python_locale_root(): + """ + Return the XBlock locale root directory for OEP-58 translations. + """ + return settings.REPO_ROOT / 'conf/plugins-locale/xblock.v1' + + +def get_javascript_i18n_file_name(xblock_module, locale): + """ + Return the relative path to the JavaScript i18n file. + + Relative to the /static/ directory. + """ + return f'js/xblock.v1-i18n/{xblock_module}/{locale}.js' + + +def get_javascript_i18n_file_path(xblock_module, locale): + """ + Return the absolute path to the JavaScript i18n file. + """ + return settings.STATICI18N_ROOT / get_javascript_i18n_file_name(xblock_module, locale) diff --git a/xmodule/modulestore/django.py b/xmodule/modulestore/django.py index 9e78b21a4ce6..f36c0c35a90d 100644 --- a/xmodule/modulestore/django.py +++ b/xmodule/modulestore/django.py @@ -19,6 +19,7 @@ if not settings.configured: settings.configure() +from django.contrib.staticfiles.storage import staticfiles_storage # lint-amnesty, pylint: disable=wrong-import-position from django.core.cache import caches, InvalidCacheBackendError # lint-amnesty, pylint: disable=wrong-import-position import django.dispatch # lint-amnesty, pylint: disable=wrong-import-position import django.utils # lint-amnesty, pylint: disable=wrong-import-position @@ -30,6 +31,13 @@ from xmodule.modulestore.mixed import MixedModuleStore # lint-amnesty, pylint: disable=wrong-import-position from xmodule.util.xmodule_django import get_current_request_hostname # lint-amnesty, pylint: disable=wrong-import-position +from .api import ( # lint-amnesty, pylint: disable=wrong-import-position + get_javascript_i18n_file_name, + get_javascript_i18n_file_path, + get_python_locale_root, + get_xblock_root_module_name, +) + # We also may not always have the current request user (crum) module available try: from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService @@ -365,12 +373,13 @@ class XBlockI18nService: has ugettext, ungettext, etc), so we can use it directly as the runtime i18n service. + This service supports OEP-58 translations (https://docs.openedx.org/en/latest/developers/concepts/oep58.html) + that are pulled via atlas. """ def __init__(self, block=None): """ - Attempt to load an XBlock-specific GNU gettext translator using the XBlock's own domain - translation catalog, currently expected to be found at: - /conf/locale//LC_MESSAGES/.po|mo + Attempt to load an XBlock-specific GNU gettext translation using the XBlock's own domain + translation catalog. If we can't locate the domain translation catalog then we fall-back onto django.utils.translation, which will point to the system's own domain translation catalog This effectively achieves translations by coincidence for an XBlock which does not provide @@ -378,21 +387,60 @@ def __init__(self, block=None): """ self.translator = django.utils.translation if block: - xblock_class = getattr(block, 'unmixed_class', block.__class__) - xblock_resource = xblock_class.__module__ - xblock_locale_dir = 'translations' - xblock_locale_path = resource_filename(xblock_resource, xblock_locale_dir) - xblock_domain = 'text' + xblock_locale_domain, xblock_locale_dir = self.get_python_locale(block) selected_language = get_language() - try: - self.translator = gettext.translation( - xblock_domain, - xblock_locale_path, - [to_locale(selected_language if selected_language else settings.LANGUAGE_CODE)] - ) - except OSError: - # Fall back to the default Django translator if the XBlock translator is not found. - pass + + if xblock_locale_dir: + try: + self.translator = gettext.translation( + xblock_locale_domain, + xblock_locale_dir, + [to_locale(selected_language if selected_language else settings.LANGUAGE_CODE)] + ) + except OSError: + # Fall back to the default Django translator if the XBlock translator is not found. + pass + + def get_python_locale(self, block): + """ + Return the XBlock locale directory with the domain name. + + Return: + (domain, locale_path): A tuple of the domain name and the XBlock locale directory. + + This method looks for translations in two locations: + - First it looks for `atlas` translations in get_python_locale_root(). + - Alternatively, it looks for bundled translations in the XBlock pip package which are + found at /conf/locale//LC_MESSAGES/.po|mo + """ + xblock_module_name = get_xblock_root_module_name(block) + xblock_locale_path = get_python_locale_root() / xblock_module_name + + # OEP-58 translations are pulled via atlas and takes precedence if exists. + if xblock_locale_path.isdir(): + # The `django` domain is used for XBlocks consistent with the other repositories. + return 'django', xblock_locale_path + + # Pre-OEP-58 translations within the XBlock pip packages are deprecated but supported. + deprecated_xblock_locale_path = resource_filename(xblock_module_name, 'translations') + # The `text` domain was used for XBlocks pre-OEP-58. + return 'text', deprecated_xblock_locale_path + + def get_javascript_i18n_catalog_url(self, block): + """ + Return the XBlock compiled JavaScript translations catalog static url. + + Return: + str: The static url to the JavaScript translations catalog, otherwise None. + """ + xblock_module_name = get_xblock_root_module_name(block) + language_name = get_language() # Returns language name e.g. `de` or `de-de`. + locale = to_locale(language_name) # Use the `de` or `de_DE` format for the locale directory. + + if get_javascript_i18n_file_path(xblock_module_name, locale).exists(): + relative_file_path = get_javascript_i18n_file_name(xblock_module_name, locale) + return staticfiles_storage.url(relative_file_path) + return None def __getattr__(self, name): name = 'gettext' if name == 'ugettext' else name diff --git a/xmodule/modulestore/tests/conftest.py b/xmodule/modulestore/tests/conftest.py new file mode 100644 index 000000000000..3743512aba47 --- /dev/null +++ b/xmodule/modulestore/tests/conftest.py @@ -0,0 +1,107 @@ +""" +Test fixture for the `xmodule.modulestore` module. +""" + +import shutil +from contextlib import contextmanager +from unittest.mock import Mock, patch + +import pytest +from path import Path + +from django.test.utils import override_settings + +from xmodule.modulestore.api import get_python_locale_root + + +@pytest.fixture +def tmp_translations_dir(tmp_path, settings): + """ + Pytest fixture to create a temporary directory for translations. + + Returns: + (function): Context manager to be used with the `with tmp_translations_dir(...):` statement. + """ + + @contextmanager + def _tmp_translations_dir(xblocks, fixtures_to_copy=None): + """ + Context manager to create temporary directory for translations. + + Args: + xblocks: A list of tuples of (module_name, xblock_class) to patch `get_non_xmodule_xblocks` for consistent + test runs. + + fixtures_to_copy: A list of `modulestore/tests/fixtures` file names to copy to the XBlocks directory. + + Yields: + Path: The temporary edx-platform directory path. + + The temp directory will have the following structure: + + edx-platform/ + ├── conf + │ └── plugins-locale + │ └── xblock.v1 + │ └── done + │ └── tr + │ └── LC_MESSAGES + │ └── django.po + └── lms + └── static + """ + # tmp_path represents settings.REPO_ROOT + # Converting to `path.path()` to be compatible with the `settings.REPO_ROOT` type. + original_repo_root = settings.REPO_ROOT + repo_root = Path(str(tmp_path / 'edx-platform')) + + project_dir_name = settings.PROJECT_ROOT.basename() # lms or cms + static_i18n_root = repo_root / f'{project_dir_name}/static' + + with override_settings(REPO_ROOT=repo_root, STATICI18N_ROOT=static_i18n_root): + gettext_fixtures = original_repo_root / 'xmodule/modulestore/tests/fixtures' + + python_root = get_python_locale_root() + python_root.makedirs_p() + static_i18n_root.makedirs_p() + + if fixtures_to_copy: + for module_name, _xblock in xblocks: + for fixture in fixtures_to_copy: + dest_dir = python_root / module_name / 'tr/LC_MESSAGES' + dest_dir.makedirs_p() + shutil.copyfile(gettext_fixtures / fixture, dest_dir / fixture) + + with patch('common.djangoapps.xblock_django.translation.get_non_xmodule_xblocks', return_value=xblocks): + yield repo_root + + return _tmp_translations_dir + + +def create_mock_xblock(module_name): + """ + Create a mocked XBlock with the given module name. + """ + block = Mock() + block.unmixed_class.__module__ = module_name + return block + + +@pytest.fixture +def mock_modern_xblock(tmp_translations_dir): + """ + Mocks a successful `atlas pull` for `my_modern_xblock` xblock. + + Yields: + dict: A dictionary of mocked XBlocks: + - modern_xblock: A mocked XBlock atlas translations. + - legacy_xblock: A mocked XBlock without atlas translations. + """ + with tmp_translations_dir( + xblocks=[('my_modern_xblock', Mock())], + fixtures_to_copy=['django.po', 'django.mo'], + ): + yield { + 'legacy_xblock': create_mock_xblock('my_legacy_xblock'), + 'modern_xblock': create_mock_xblock('my_modern_xblock'), + } diff --git a/xmodule/modulestore/tests/fixtures/django.mo b/xmodule/modulestore/tests/fixtures/django.mo new file mode 100644 index 0000000000000000000000000000000000000000..ea0a617ee90693c9e55ae3a3e2e20fd1482ee975 GIT binary patch literal 360 zcmaKm&q~BF5XPe@N{*gA?usZk8swvx-l1`sI~JLE{Cvz>DD)9+Mt8EDQwx8N{Q*Z37n\n" +"Language-Team: Tr \n" +"Language: tr\n" +"MIME-Version: 1.0\n" +"Content-Type: text/plain; charset=utf-8\n" +"Content-Transfer-Encoding: 8bit\n" + +msgid "Hello" +msgstr "Merhaba" diff --git a/xmodule/modulestore/tests/test_api.py b/xmodule/modulestore/tests/test_api.py new file mode 100644 index 000000000000..e42d3ded46c2 --- /dev/null +++ b/xmodule/modulestore/tests/test_api.py @@ -0,0 +1,75 @@ +""" +Tests for the modulestore and XBlock python APIs. +""" +from unittest.mock import Mock + +from django.conf import settings + +from lti_consumer.lti_xblock import LtiConsumerXBlock +from done import DoneXBlock +from xblock.field_data import DictFieldData + +from xblock.test.tools import TestRuntime +from xblock.test.test_runtime import TestSimpleMixin +from xmodule.video_block import VideoBlock +from xmodule.modulestore.api import ( + get_javascript_i18n_file_name, + get_javascript_i18n_file_path, + get_python_locale_root, + get_root_module_name, + get_xblock_root_module_name, +) + + +def test_get_root_module_name(): + """ + Ensure the module name function works with different xblocks. + """ + assert get_root_module_name(LtiConsumerXBlock) == 'lti_consumer' + assert get_root_module_name(VideoBlock) == 'xmodule' + assert get_root_module_name(DoneXBlock) == 'done' + + +def test_get_xblock_root_module_name(): + """ + Ensure the get_root_module_name works with mixed XBlocks. + + The XBlock uses a little-known Mixologist class which changes the final + XBlock object class. See the XBlock.construct_xblock_from_class method + for more information about this behavior. + """ + field_data = DictFieldData({ + 'field_a': 5, + 'field_x': [1, 2, 3], + }) + runtime = TestRuntime(Mock(), mixins=[TestSimpleMixin], services={'field-data': field_data}) + + mixed_done_xblock = runtime.construct_xblock_from_class(DoneXBlock, Mock()) + + assert mixed_done_xblock.__module__ == 'xblock.internal' # Mixed classes has a runtime generated module name. + assert mixed_done_xblock.unmixed_class == DoneXBlock, 'The unmixed_class property retains the original property.' + + assert get_xblock_root_module_name(mixed_done_xblock) == 'done' + + +def test_file_paths_api(): + """ + Test the `get_python_locale_root` returned path. + """ + root = get_python_locale_root() + assert root.endswith('edx-platform/conf/plugins-locale/xblock.v1'), 'Needs to match Makefile and other code' + + +def test_get_javascript_i18n_file_name(): + """ + Test get_javascript_i18n_file_name relative path to `/static` URL. + """ + assert get_javascript_i18n_file_name('lti_consumer', 'ar') == 'js/xblock.v1-i18n/lti_consumer/ar.js' + + +def test_get_javascript_i18n_file_path(): + """ + Test get_javascript_i18n_file_path absolute file path. + """ + path = str(get_javascript_i18n_file_path('done', 'eo')) + assert path.endswith(f'{settings.PROJECT_ROOT}/static/js/xblock.v1-i18n/done/eo.js') diff --git a/xmodule/modulestore/tests/test_django_utils.py b/xmodule/modulestore/tests/test_django_utils.py new file mode 100644 index 000000000000..86c3a08df503 --- /dev/null +++ b/xmodule/modulestore/tests/test_django_utils.py @@ -0,0 +1,54 @@ +""" +Tests for the modulestore.django module +""" + +from unittest.mock import patch + +import django.utils.translation + +from xmodule.modulestore.django import XBlockI18nService + + +def test_get_python_locale_with_atlas_oep58_translations(mock_modern_xblock): + """ + Test that the XBlockI18nService.get_python_locale() method finds the atlas locale if it exists. + + More on OEP-58 and atlas pull: https://docs.openedx.org/en/latest/developers/concepts/oep58.html. + """ + i18n_service = XBlockI18nService() + block = mock_modern_xblock['modern_xblock'] + domain, locale_path = i18n_service.get_python_locale(block) + + assert locale_path.endswith('conf/plugins-locale/xblock.v1/my_modern_xblock'), 'Uses atlas locale if found.' + assert domain == 'django', 'Uses django domain when atlas locale is found.' + + +@patch('xmodule.modulestore.django.resource_filename', return_value='/lib/my_legacy_xblock/translations') +def test_get_python_locale_with_bundled_translations(mock_modern_xblock): + """ + Ensure that get_python_locale() falls back to XBlock internal translations if atlas translations weren't pulled. + + Pre-OEP-58 translations were stored in the `translations` directory of the XBlock which is + accessible via the `pkg_resources.resource_filename` function. + """ + i18n_service = XBlockI18nService() + block = mock_modern_xblock['legacy_xblock'] + domain, path = i18n_service.get_python_locale(block) + + assert path == '/lib/my_legacy_xblock/translations', 'Backward compatible with pe-OEP-58.' + assert domain == 'text', 'Use the legacy `text` domain for backward compatibility with old XBlocks.' + + +def test_i18n_service_translator_with_modern_xblock(mock_modern_xblock): + """ + Ensure the XBlockI18nService uses the atlas translations if found. + """ + block = mock_modern_xblock['modern_xblock'] + + with django.utils.translation.override('fr'): + i18n_service = XBlockI18nService(block) + assert i18n_service.translator is django.utils.translation, 'French is not pulled by `mock_modern_xblock`.' + + with django.utils.translation.override('tr'): + i18n_service = XBlockI18nService(block) + assert i18n_service.translator is not django.utils.translation, 'Turkish is pulled by `mock_modern_xblock`.'