chamilo / chamilo-lms

Chamilo is a learning management system focused on ease of use and accessibility
https://chamilo.org
GNU General Public License v3.0
798 stars 480 forks source link

Broken locale inclusion for jquery-timeago #2204

Open lonesomewalker opened 6 years ago

lonesomewalker commented 6 years ago

Current behavior / Resultado actual / Résultat actuel

When posting some stuff at social network, date/time is only shown in english. No translation available. (example: 7 minutes ago)

Expected behavior / Resultado esperado / Résultat attendu

Also this part of Chamilo should have strings which can be translated.

Chamilo Version / Versión de Chamilo / Version de Chamilo

1.11.4

lonesomewalker commented 6 years ago

Digging further...

Seems the jquery-timeago doesn't load the locales. Referenced in main/inc/lib/template.lib.php AND main/social/profile.php. But both not working.

Also seen in generated page source code that the jQuery timeago is loaded 2 times...?

Sorry, this is a total mess, there is really no code quality since in main/social/profile.php the include is done in 2 different ways:

$htmlHeadXtra[] = api_get_asset('jquery-timeago/jquery.timeago.js');
$timeAgoLocaleDir = $javascriptDir.'jquery-timeago/locales/jquery.timeago.'.$locale.'.js';
if (file_exists($timeAgoLocaleDir)) {
    $htmlHeadXtra[] = api_get_js('jquery-timeago/locales/jquery.timeago.'.$locale.'.js');
}

Seriously, which one is right? api_get_asset or api_get_js?

Both seem to have a similar base, since the function is declared in main/inc/lib/api.lib.php

function api_get_js($file) {
    return '<script type="text/javascript" src="'.api_get_path(WEB_LIBRARY_PATH).'javascript/'.$file.'"></script>'."\n";
}

/**
 * Returns the <script> HTML tag
 * @return string
 */
function api_get_asset($file)
{
    return '<script type="text/javascript" src="'.api_get_path(WEB_PATH).'web/assets/'.$file.'"></script>'."\n";
}
ywarnier commented 6 years ago

Assets are dynamically generated by our JS dependencies manager (bower) from the web/assets/ directory, while api_get_js() loads files from the more persistent main/inc/lib/ directory. Version 1.11 is a transition between 1.10 and a more Symfony-adapted files structure in 2.0. Feel free to submit pull requests to improve the documentation. This is in your reach.

ywarnier commented 6 years ago

Just as a precision, in the code you mentioned, what we do is we load the generic timeago lib, and then we check if the localized version (basically the translations JSON) exists for the current language. If it does, we load it. Otherwise we don't. This means that if you want the German translations to be available, you need the file web/assets/jquery-timeago/locales/jquery.timeago.de.js to exist. I see in the latest code that it exists, but maybe that was an addition after 1.11.4. I haven't checked.

lonesomewalker commented 6 years ago

No, it is available, but it doesn't get loaded... So i think the if statement doesn't get "true". Will debug this later to see what's happening.

lonesomewalker commented 6 years ago

Okay, as i thought: $timeAgoLocaleDir = $javascriptDir.'jquery-timeago/locales/jquery.timeago.'.$locale.'.js';

becomes false, because it is the wrong directory... (it resolves to: /main/inc/lib/javascript/jquery-timeago/locales/jquery.timeago.de.js).

So, no enhancement, it is a bug which can be reproduced. Pull request sent. #2207

ywarnier commented 6 years ago

Fair enough. Did you try changing it in the code on your platform already to see if that works?

lonesomewalker commented 6 years ago

Sure :-) works as expected, now all the timestamps get translated.

ywarnier commented 6 years ago

Fancy sending your own pull request? Or too much to learn for such a little change?

lonesomewalker commented 6 years ago

Didn't i do it right? Or do i have to pull to chamilo:1.11.x ?

ywarnier commented 6 years ago

A pull request is its own big thing, the first time you do it (you have to start from your own fork of the project on Github). I suggest you check our Contributing.md file for explanations: https://github.com/chamilo/chamilo-lms/blob/1.11.x/CONTRIBUTING.md

ywarnier commented 6 years ago

Actually, sorry, this is more of this link: https://guides.github.com/activities/contributing-to-open-source/ (which is included in CONTRIBUTING.md but more focused on your case).

lonesomewalker commented 6 years ago

Sorry, but did you see #2207 ? Forked, changed, pull.

ywarnier commented 6 years ago

You're right, I missed it. In general, if you report an issue, then you can associate the commit in your PR with the issue by simply mentioning #2204. That would have attached your commit to this report, but that's OK for now. I'll check it and merge it (if alright) tomorrow.

jmontoyaa commented 6 years ago

The change in #2207 should be send to branch 1.11.x not yet to master.

lonesomewalker commented 6 years ago

But it is related to both. It is a wrong path so do i have to commit it to both?

jmontoyaa commented 6 years ago

@lonesomewalker then you have to send the commit to both branches

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.