Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use XBlockI18NService js translations #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ Please See the `releases tab <https://github.com/openedx/xblock-lti-consumer/rel

Unreleased
~~~~~~~~~~
9.9.0 (2024-01-24)
---------------------------
* XBlockI18NService js translations support

9.8.3 - 2024-01-23
------------------
* Additional NewRelic traces to functions suspected of causing performance issues.
Expand Down
2 changes: 1 addition & 1 deletion lti_consumer/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
from .apps import LTIConsumerApp
from .lti_xblock import LtiConsumerXBlock

__version__ = '9.8.3'
__version__ = '9.9.0'
35 changes: 31 additions & 4 deletions lti_consumer/lti_xblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
external_user_id_1p1_launches_enabled,
database_config_enabled,
EXTERNAL_ID_REGEX,
DummyTranslationService,
)

log = logging.getLogger(__name__)
Expand Down Expand Up @@ -257,6 +258,7 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
"""

block_settings_key = 'lti_consumer'
i18n_js_namespace = 'LtiI18N'

display_name = String(
display_name=_("Display Name"),
Expand Down Expand Up @@ -663,10 +665,13 @@ def workbench_scenarios():
return scenarios

@staticmethod
def _get_statici18n_js_url(loader): # pragma: no cover
def _get_deprecated_i18n_js_url(loader):
"""
Returns the Javascript translation file for the currently selected language, if any found by
Returns the deprecated JavaScript translation file for the currently selected language, if any found by
`pkg_resources`

This method returns pre-OEP-58 i18n files and is deprecated in favor
of `get_javascript_i18n_catalog_url`.
"""
lang_code = translation.get_language()
if not lang_code:
Expand All @@ -678,10 +683,23 @@ def _get_statici18n_js_url(loader): # pragma: no cover
return text_js.format(lang_code=code)
return None

def _get_statici18n_js_url(self, loader):
"""
Return the JavaScript translation file provided by the XBlockI18NService.
"""
if url_getter_func := getattr(self.i18n_service, 'get_javascript_i18n_catalog_url', None):
if javascript_url := url_getter_func(self):
return javascript_url

if deprecated_url := self._get_deprecated_i18n_js_url(loader):
return self.runtime.local_resource_url(self, deprecated_url)

return None

def validate_field_data(self, validation, data):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmadabuwardeh thanks again!

Please revert all refactoring changes to this pull request. We only need the _get_statici18n_js_url and _get_deprecated_i18n_js_url.

# Validate custom parameters is a list.
if not isinstance(data.custom_parameters, list):
_ = self.runtime.service(self, "i18n").ugettext
_ = self.runtime.service(self, 'i18n').ugettext
validation.add(ValidationMessage(ValidationMessage.ERROR, str(
_("Custom Parameters must be a list")
)))
Expand Down Expand Up @@ -776,6 +794,15 @@ def get_pii_sharing_enabled(self):
# because the service is not defined in those contexts.
return True

@property
def i18n_service(self):
""" Obtains translation service """
i18n_service = self.runtime.service(self, "i18n")
if i18n_service:
return i18n_service
else:
return DummyTranslationService()

@property
def editable_fields(self):
"""
Expand Down Expand Up @@ -1226,7 +1253,7 @@ def student_view(self, context):
fragment.add_javascript(loader.load_unicode('static/js/xblock_lti_consumer.js'))
statici18n_js_url = self._get_statici18n_js_url(loader)
if statici18n_js_url:
fragment.add_javascript_url(self.runtime.local_resource_url(self, statici18n_js_url))
fragment.add_javascript_url(statici18n_js_url)
fragment.initialize_js('LtiConsumerXBlock')
return fragment

Expand Down
16 changes: 16 additions & 0 deletions lti_consumer/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ def _(text):
return text


def ngettext_fallback(text_singular, text_plural, number):
Copy link

@OmarIthawi OmarIthawi Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmadabuwardeh Looking at this change it makes sense to have in a separate pull request.

DummyTranslationService is roughly similar to xblock.xblock.runtime.NullI18nService which can be used.

However, I'd like to understand the motivation behind it and if it's crucial for this pull request.

""" Dummy `ngettext` replacement to make string extraction tools scrape strings marked for translation """
if number == 1:
return text_singular
else:
return text_plural


def get_lti_api_base():
"""
Returns base url to be used as issuer on OAuth2 flows
Expand Down Expand Up @@ -361,3 +369,11 @@ def model_to_dict(model_object, exclude=None):
return object_fields
except (AttributeError, TypeError):
return {}


class DummyTranslationService:
"""
Dummy drop-in replacement for i18n XBlock service
"""
gettext = _
ngettext = ngettext_fallback
Loading