Skip to content

MCKIN-21527 Problem Builder (FTE, Assessment, MCQ, MRQ) - On opening these modules, none of the text is translated on notifications dropdown. - #298

Merged
xitij2000 merged 4 commits into
open-craft:problem-builder-v3from
wasifarbisoft:MCKIN-21527-v1
Aug 28, 2020
Merged

MCKIN-21527 Problem Builder (FTE, Assessment, MCQ, MRQ) - On opening these modules, none of the text is translated on notifications dropdown.#298
xitij2000 merged 4 commits into
open-craft:problem-builder-v3from
wasifarbisoft:MCKIN-21527-v1

Conversation

@wasifarbisoft

@wasifarbisoft wasifarbisoft commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

MCKIN-21527 Problem Builder (FTE, Assessment, MCQ, MRQ) - On opening these modules, none of the text is translated on notifications dropdown.

Based on the Previous Closed Pull Request comments #294

@wasifarbisoft I just tested this from scratch, and I don't think this approach will work. While it does solve the problem from browser side, it breaks extracting translations. I think the best approach here is to make the gettext and ngettext declaraton from being global to local to the function, instead of renaming them everywhere, which would mean that translations can no longer be extracted because the tooling searches for gettext not pb_gettext.

var data = $('.attempts', element).data();
$('.attempts', element).html(attemptsTemplate(data));
if (data != undefined && data.max_attempts > 0) {
var message = _.template(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please note that template string is now pulled out of mentoring_attempts.underscore because it was using global ngettext but we require local problem builder ngettext for translation.

…these modules, none of the text is translated on notifications dropdown.
@wasifarbisoft

Copy link
Copy Markdown
Contributor Author

@xitij2000 Please review this Pull Request

@xitij2000 xitij2000 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 This is good to merge once the review comments have been addressed.

This should also come with a corresponding v4/master PR.

  • I tested this: tested with multiple languages, present, not present, with and without a dash
  • I read through the code

Comment thread problem_builder/instructor_tool.py Outdated
Comment thread problem_builder/public/js/mentoring.js Outdated
function renderAttempts() {
var data = $('.attempts', element).data();
$('.attempts', element).html(attemptsTemplate(data));
if (data != undefined && data.max_attempts > 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be duplicated. The template is already performing this check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the template!

Comment thread problem_builder/public/js/mentoring.js Outdated
ngettext("You have used {num_used} of 1 submission.", "You have used {num_used} of {max_attempts} submissions.", data.max_attempts),
{num_used: _.min([data.num_attempts, data.max_attempts]), max_attempts: data.max_attempts}, {interpolate: /\{(.+?)\}/g}
);
data.message = message;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

message is defined inside the if, but used outside.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

message is used within the if statement,

Comment thread setup.py Outdated
@wasifarbisoft

wasifarbisoft commented Aug 28, 2020

Copy link
Copy Markdown
Contributor Author

@xitij2000 ,
Pull Request Updated!
Ready to be reviewed again!

Version 4 Pull Request https://github.com/open-craft/problem-builder/pull/295/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants