-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
@melvinsoft do you mind taking a look here? |
60d1dc6
to
8d76a64
Compare
@OmarIthawi Thanks for creating the PR. The code looks amazing, but following your advice I've been trying to run the XBlock in my devstack. I installed your feature branch and the first error I have is the following:
I think you're working on a newer version of I tried upgrading the
Can you point me to some instructions to install it? Or if we need to upgrade the dependencies version (we're in still on Ficus) we will need to plan that, to make sure others XBlocks still work. Thanks! |
@melvinsoft You'd need the same version of XBlock in Ficus:
and this specific XBlock utils: Please also make sure you're using the same branch of this PR I've just tried it and it works fine, please try again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OmarIthawi Thank for pointing me to the right versions.
I think I've found the problem, the templates folder didn't exist in the installation. I added the templates
and translations
folders int he package data and re installed, and started to work.
package_data=package_data("oppia", ["static", "templates", "public", "translations"]),
But then I found another problem that I commented in the code inline, when I change the language I get this error:
[Errno 2] No such file or directory: '/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/oppia/public/js/translations/en/textjs.js'
Pretty similar to the previous one. Looks like the public folder doesn't exists in the XBlock, and isn't in the package_data
either.
Please advise.
oppia/oppia.py
Outdated
lang=utils.translation.get_language(), | ||
)) | ||
except IOError: | ||
return self.resource_string('public/js/translations/en/textjs.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have the public folder in the XBlock, should it be created on the fly in the installation? or this default should point to static
?
Thanks @melvinsoft, I'll take a look and let you know. |
8d76a64
to
a564d10
Compare
@melvinsoft Please take another look. I've fixed the data and |
@OmarIthawi Thanks for addressing the change! It's working now. I'm testing the Makefile commands and I've found a couple of issues. All the other commands work like a charm. pull_translationsIf you run
Do you think we should add some check in the Make command and return a proper error message and some instructions if the file isn't there, instead of returning the exception directly? make testThis command is throwing an error, here is the stack trace I'm getting.
make quality
|
a564d10
to
4e17df3
Compare
@melvinsoft I've removed Regarding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OmarIthawi Thanks for addressing all the changes. Great work here!
Thanks @melvinsoft! I'll merge it. |
@seanlip FYI. |
Note: @oppia don't have the time to review the upstream pull request (oppia#2), hence they can't get it merged. Opening it here as an intermediate solution. I've also switch to Transifex.
We'd like this XBlock to be translated to Japanese and other languages, I've also supported JavaScript translations via
django-statici18n
.Before you continue, take a look at edx/i18n-tools and edx/cookiecutter-django-app.
Summary of the Changes
locale
link tooppia/translations
to allow thei18n_tools
find the po/mo filesoppia/translations/config.yaml
which works as the configuration file of thei18n_tools
commandrequirements-dev.txt
to pin development packages. Used for now only for translations.TODO
textjs.po
Cheese
Studio
LMS