mattdrayer/xblock-translations: I18N/L10N for XBlocks

[mattdrayer]

Add I18N/L10N support to XBlocks via the XBlock runtime (ModuleI18nService)

@asadiqbal08 @nedbat @cpennington @bradenmacdonald FYI

[mattdrayer]

I'm not sure that this line is necessary -- we can probably remove it.

[nedbat]

Please don't ever use plain str() where the value could be a unicode string. If you do, then non-ascii characters will cause a UnicodeEncodeError to be thrown.

[nedbat]

Is this needed if we are using gettext directly to load XBlock translations? LOCALE_PATHS is only a Django thing, so if we aren't using Django machinery to get the XBlock translations, then we won't have to tweak it.

[mattdrayer]

@asadiqbal08 and I discussed this late last night and we're also thinking it's not needed.

[mattdrayer]

Okay, I've done some housecleaning on this PR to get rid of a lot of the extraneous stuff. @asadiqbal08 I noted that we could move some things into settings params, and we could also consider supporting multiple values for both domain and the xblock locale which would allow for some flexibility on the XBlock developer's side. However these are not required, and per a previous conversation between @nedbat and me, it might be easier to simply support a single locale/domain and document it as such, which would give people an easier way to provide translations with their XBlocks.

[mattdrayer]

I'm not entirely sure we want to remove getattr here -- are there other instances of ModuleI18nService calls which would require support for a clear pass-through to django.utils.translation? Otherwise an AttributeError would be raised and the operation would need to be added to the service at that point, right? Of course it could still be a simple pass-through to d.u.t., but we're moving from an implicit approach to an explicit approach here, no?

[asadiqbal08]

Ok @mattdrayer , I think we should keep __getattr__ here. I am not sure but may be it is possible that some xBlocks trying to access some other attribute of django.utils.translation instead of ugettext

[nedbat]

I checked all the XBlocks we know about: they use ugettext, ungettext, strftime, and with an attribute check, get_language_bidi.

[nedbat]

We should document the methods we support on the i18n service in the XBlock docs.

[nedbat]

I don't understand where in this code the XBlock's translations are found. The xblock_root argument seems like a temporary thing, right?

[nedbat]

If we are skipping the Django logic, shouldn't we use a different domain name?

[asadiqbal08]

@nedbat xblock_root is not a temporary thing. we are passing this info ( pointing the LOCALE folder )
from xblock to this method in **kwargs

[asadiqbal08]

@mattdrayer can you look here ? each xBlock can specify its own domain as well.

[mattdrayer]

LGTM!

[asadiqbal08]

hope build will be green now !

[nedbat]

It doesn't make sense to me to change the signature of ugettext. What is xblock_root for, and why should XBlocks have to pass it in? Why would an XBlock want to specify its own domain? If it does want to, why is ugettext the right place to do it? I don't feel like we've considered the API we're presenting to the XBlock here.

[mattdrayer]

If there's a way for the runtime to determine the path/location of the XBlock's installation, then we could remove the need to specify the xblock_root from the XBlock's side of things. As far as domain and locale go, these are not required, but we were thinking it might be useful to allow an XBlock developer to specify an alternative location and domain (ie, the name of the PO/MO files) of the translations if they choose. We could certainly remove that flexibility, however, if you think it's unnecessary or confusing.

[nedbat]

I would like to provide this capability in an opinionated simple way. We don't need to provide for all possibilities, we need to make it possible to provide translations.

But even if we want to provide those options, is ugettext the right place? Remember, we also need to implement a few other methods here. And there's a self.runtime.service call that can help with this stuff.

[mattdrayer]

Yes, I agree that ugettext is too specific of a place -- no reason to reimplement for every additional gettext operation we want to support.

@asadiqbal08 I'm looking into the ResourceManager API from setuptools, which was a recommendation from @cpennington which may allow us to avoid forcing the XBlock to inform the runtime of its location (ie, xblock_root). If we can crack that, then we can drop the entire extra_info approach and document the conventions for locale and domain.

See this link for info on setuptools.ResourceManager: https://pythonhosted.org/setuptools/pkg_resources.html#resourcemanager-api

[mattdrayer]

@cpennington @nedbat -- getting closer to a real implementation -- this changeset now utilizes the pkg_resources library on the runtime-side to determine the XBlock installation location. The XBlock runtime's service locator is overridden for LMS and Studio. Let us know what you think and if the approach is sound we can work on cleaning things up, proper test coverage, etc.

[mattdrayer]

We can drop this import now

[nedbat]

@mattdrayer I like where this is going :) We're keeping the XBlock API simple and easy to use.

[nedbat]

We'll need to write instructions for XBlocks, and the layout will look different than the way edx-platform and edx-ora2 both do it. The "conf/locale" directory won't be at the root of the project, but inside one of the package directories.

[asadiqbal08]

@nedbat fyi. let me know about it.

[asadiqbal08]

jenkins run python

[cpennington]

Couldn't we just change __getattr__ to read from self.translator (which is either xblock or django), and then not have to override ugettext at all?

[mattdrayer]

Yes, it appears we can -- just pushed a commit to remove the now-unnecessary override -- good catch!

[mattdrayer]

Squashed a lot of the working PRs into the main commit -- will be working on an implementation of the "callable" suggestion today.

[mattdrayer]

@cpennington @nedbat I've pushed a commit that reflects the suggested 'callable' approach -- if you can take a look and let us know what you think, that would be great -- @asadiqbal08 FYI

[cpennington]

Yeah, this looks like I was expecting it to.

[asadiqbal08]

jenkins run bokchoy

[mattdrayer]

@bradenmacdonald @nedbat @cpennington -- I've done another round of rebasing+squashing, and @asadiqbal08 has changed the locale/domain values to be more along the lines of Flask-Babel -- if you can take another look at the PR and note any additional concerns that would be great -- and if you're good with the state of things we'd love to start seeing some +1's 😄

[bradenmacdonald]

@mattdrayer I've only looked at the code and done some quick tests in Studio, but I like this how this approach has shaped up, so 👍 from me. I'd be happy to do a more in depth review of the Drag and Drop XBlock integration with this code once it's updated.

[cpennington]

Shouldn't this still have the ModuleI18nService installed?

[mattdrayer]

@asadiqbal08 -- I caught up with @cpennington today to understand more about why this particular line is necessary. It seems there is an LTI use case we need to support, which has to do with when the LTI XBlock is loaded. The XBlock needs to re-bind to the user associated with the authentication information passed along with the XBlock handler contents. The good news is that the fix should be as easy as restoring the above line.

In addition to reverting the above change, however, we should ensure that the LTI use case is not impacted in the future by protecting it with a unit test. The test can utilize a simple XBlock that calls the re-bind method and confirms the proper services are both defined and the same before and after the re-binding process is complete.

@cpennington, if there's any more context/information to add here so @asadiqbal08 can write up the proper test, please feel free to share -- thanks!

[asadiqbal08]

@mattdrayer I was trying to get your point here but sorry I cannot get it, what is the dependency of LTI xblock on i18n service ? We are already initializing the ModuleI18nService in init method of LmsModuleSystem .
here
what is meant by re-bind method ? are you pointing out to -- Bind a module to another student / user ?

[asadiqbal08]

@mattdrayer is that you required to test ?

def assert_services(self, module):
        """
        assert possible services
        """
        self.assertEqual(module.runtime._services.get('i18n'), ModuleI18nService)
        self.assertTrue(module.runtime._services.get('fs', False))
        self.assertTrue(module.runtime._services.get('user', False))
        self.assertTrue(module.runtime._services.get('reverification', False))
        self.assertTrue(module.runtime._services.get('proctoring', False))
        self.assertTrue(module.runtime._services.get('credit', False))
        self.assertTrue(module.runtime._services.get('bookmarks', False))

def test_services_before_and_after_module_rebinding(self):
        """
        LTI XBlock that calls the re-bind method and confirms the proper services.
        """
        module = self.get_module_for_user(self.anon_user)
        # assert services before rebinding
        self.assert_services(module)
        user2 = UserFactory.create()
        module.system.rebind_noauth_module_to_user(module, user2)
        # assert services after rebinding
        self.assert_services(self.get_module_for_user(user2))

[mattdrayer]

@cpennington, is this what you had in mind? @asadiqbal08, does this new
test fail if you remove the ModuleI18nService() call above?
On Mar 9, 2016 7:45 AM, "Asad Iqbal" [email protected] wrote:

In lms/djangoapps/courseware/module_render.py
https://github.com/edx/edx-platform/pull/11575#discussion_r55512583:

@@ -713,7 +713,6 @@ def rebind_noauth_module_to_user(module, real_user):
wrappers=block_wrappers,
get_real_user=user_by_anonymous_id,
services={

•        'i18n': ModuleI18nService(),
       'fs': FSService(),
       'field-data': field_data,
       'user': DjangoXBlockUserService(user, user_is_staff=user_is_staff),
  

@mattdrayer https://github.com/mattdrayer is that you required to test
?

def assert_services(self, module):
"""
assert possible services
"""
self.assertTrue(module.runtime._services.get('i18n', False))
self.assertTrue(module.runtime._services.get('fs', False))
self.assertTrue(module.runtime._services.get('user', False))
self.assertTrue(module.runtime._services.get('reverification', False))
self.assertTrue(module.runtime._services.get('proctoring', False))
self.assertTrue(module.runtime._services.get('credit', False))
self.assertTrue(module.runtime._services.get('bookmarks', False))

def test_services_before_and_after_module_rebinding(self):
"""
LTI XBlock that calls the re-bind method and confirms the proper services.
"""
module = self.get_module_for_user(self.anon_user)
# assert services before rebinding
self.assert_services(module)
user2 = UserFactory.create()
module.system.rebind_noauth_module_to_user(module, user2)
# assert services after rebinding
self.assert_services(self.get_module_for_user(user2))

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/11575/files#r55512583.

[asadiqbal08]

@mattdrayer I cannot understand why is it required to revert the above change ? talking about
i18n': ModuleI18nService because we are already declaring it in init of LmsModuleSystem .
In this new test I am explicitly testing for ModuleI18nService, I am sure if we will not declare the i18n: ModuleI18nService then test would be failed

[mattdrayer]

I think we're going to have to ask @cpennington to provide some more clarity here -- I unfortunately don't have all the context around this requirement, either.

[cpennington]

Oh, @asadiqbal08, I'm sorry, you're right. I misread this code, and didn't realize that this line was being removed and the line during __init__ was being added to replace it.

[mattdrayer]

AH, okay then :) So we are good to go from @cpennington's perspective at this point -- I'd like to get one last +1 from @nedbat and then we should be clear to merge.

[cpennington]

👍 after my remaining comment is fixed

[mattdrayer]

Caught up with @nedbat and we're good to go on this PR with the caveat that we'll finish up the WIP RTD documentation in the very near future -- going ahead with the merge.