Translation tool shows [% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %] for en/modules/admin/categories.tt
Translation tool shows [% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %] for en/modules/admin/categories.tt and koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt
Created attachment 63626 [details] [review] Bug 18649: Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt Translation tool shows [% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %] for en/modules/admin/categories.tt and koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt This patch fixes it. To test: - Apply patch - Verify that code changes make sense - Verify that template directive is no longer picked by translation https://bugs.koha-community.org/show_bug.cgi?id=18641
Created attachment 63724 [details] [review] Bug 18649: Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt Translation tool shows [% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %] for en/modules/admin/categories.tt and koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt This patch fixes it. To test: - Apply patch - Verify that code changes make sense - Verify that template directive is no longer picked by translation https://bugs.koha-community.org/show_bug.cgi?id=18641 Patch works as expected for me - also this is a really great way to do this kind of fix! We should have this all over Koha. Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
(In reply to Aleisha Amohia from comment #3) (...) > > Patch works as expected for me - also this is a really great way to do > this kind of fix! We should have this all over Koha. > Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Hi Aleisha Thanks for testing and for your comment :-) Some of the translatability issues (not with this bug though) are related to html tags inside of tt directives. I filed an enhancement for that (Bug 18665 - Needs SO). Marc
Hi Marc, What is the advantage of value="[%- category.enrolmentperiod IF category.enrolmentperiod -%]" above e.g. value="[% category.enrolmentperiod %]" ? I guess that if period == 0, the first one will leave the field empty? Null and empty string will probably respond the same. Note that the trailing IF does not increase readability. Before making this kind of changes everywhere, is there a consensus to do so? Also noting that you add chomps here (%-). Do we need them here?
(In reply to Marcel de Rooy from comment #5) > Hi Marc, > > What is the advantage of > value="[%- category.enrolmentperiod IF category.enrolmentperiod -%]" > above e.g. > value="[% category.enrolmentperiod %]" ? > I guess that if period == 0, the first one will leave the field empty? > Null and empty string will probably respond the same. > > Note that the trailing IF does not increase readability. Before making this > kind of changes everywhere, is there a consensus to do so? > > Also noting that you add chomps here (%-). Do we need them here? Hi Marcel - This bug is about translatability, and does not contain a change in behavior. I might be wrong, but the functionality is precisely the same. - At the moment, the translation tool picks the following line for translation: [% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %] That makes no sense for translators, confuses them, and can lead to errors. Translators should not have to care about code internals. To verify, search for that string for any language on translate.koha-community.org for version 17.05 Or create a translation e.g. for a 'language' aa-AA and then search in aa-AA-staff-prog.po for #. INPUT type=text name=enrolmentperiod #: intranet-tmpl/prog/en/modules/admin/categories.tt:92 #: intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt:104 msgid "" "[% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %]" msgstr "" Then apply patch, translate aa-AA again and verify that the line [% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %] no longer appears.
Created attachment 63757 [details] [review] Bug 18649: [QA followup] Remove chomps in admin/categories.tt As of comment #3, the chomps are not needed because the directive has no surrounding white space that should be removed.
Marc, I understand your explanation, but you did not respond to my question about the trailing IF.
(In reply to Marcel de Rooy from comment #8) > Marc, > I understand your explanation, but you did not respond to my question about > the trailing IF. Ah, sorry, I must have skipped the main part... If you do a translate create for a "language" aa-AA and then examine the file, you find: #. INPUT type=text name=enrolmentperiod #: intranet-tmpl/prog/en/modules/admin/categories.tt:92 #: intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt:104 msgid "" "[% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %]" msgstr "" If you apply the patch and translate create again, these line are gone. The problem occurs because there is more than one tt statement inside a value attribute for a form field: value="[% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %]" If the attribute is changed to have one statement only, the translation tool does not pick it. That's why I changed it to have a trailing IF. And yes, we have a little bit less readability for programmers, but no more disturbing tt directives for translators.
(In reply to Marc Véron from comment #9) > (In reply to Marcel de Rooy from comment #8) > > Marc, > > I understand your explanation, but you did not respond to my question about > > the trailing IF. > > Ah, sorry, I must have skipped the main part... > > If you do a translate create for a "language" aa-AA and then examine the > file, you find: > > #. INPUT type=text name=enrolmentperiod > #: intranet-tmpl/prog/en/modules/admin/categories.tt:92 > #: intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt:104 > msgid "" > "[% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %]" > msgstr "" > > If you apply the patch and translate create again, these line are gone. > > The problem occurs because there is more than one tt statement inside a > value attribute for a form field: > value="[% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END > %]" > > If the attribute is changed to have one statement only, the translation tool > does not pick it. That's why I changed it to have a trailing IF. > > And yes, we have a little bit less readability for programmers, but no more > disturbing tt directives for translators. Sure :) But why not just [% category.enrolmentperiod %] ?
Oh... Hmm... That's true for the original line as well...
Created attachment 63937 [details] [review] Bug 18649: [QA followup] Simplify If and remove chomps in admin/categories.tt As of comment #3, the chomps are not needed because the directive has no surrounding white space that should be removed. As of comment #10, the IF statement can be implified (Amended 2017-06-02 mv)
Created attachment 64049 [details] [review] Bug 18649: Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt Translation tool shows [% IF category.enrolmentperiod %][% category.enrolmentperiod %][% END %] for en/modules/admin/categories.tt and koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep2.tt This patch fixes it. To test: - Apply patch - Verify that code changes make sense - Verify that template directive is no longer picked by translation https://bugs.koha-community.org/show_bug.cgi?id=18641 Patch works as expected for me - also this is a really great way to do this kind of fix! We should have this all over Koha. Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Applied the second patch to the first.
There is a change in the behaviour: Before the patch, if the enrolment period == 0, the input was empty, now it shows '0'.
(In reply to Jonathan Druart from comment #15) > There is a change in the behaviour: > Before the patch, if the enrolment period == 0, the input was empty, now it > shows '0'. Ah, that was the reason for the IF I removed as QA follow-up as indicated in comment #10 So the good patch would be the one signed off bei Aleisha: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63724
(In reply to Jonathan Druart from comment #15) > There is a change in the behaviour: > Before the patch, if the enrolment period == 0, the input was empty, now it > shows '0'. I know. But did not find enough reason for a change. 0 == 0
Created attachment 64079 [details] [review] Bug 18649: (Restored earlier version) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt Restored earlier version (patches #63724 + #63757) to get the old behaviour, see comment #15
Resetting to NSO because restored patch lost SO line.
Sounds like there is something wrong with the "0", but if it's a bug we must fix it somewhere else.
(In reply to Jonathan Druart from comment #20) > Sounds like there is something wrong with the "0", but if it's a bug we must > fix it somewhere else. I do not think that there is anything wrong with enrolmentperiod = 0, it is simply not displayed if it is 0. That's the original behaviour, this patch does not change it.
It is wrong to store 0 and it is wrong to display an empty string when 0 is stored. I did not say your last patch modified that, just that it should not be changed here. I was more answering to Marcel than commenting your patch.
Where do we go from here? - The original aim of this bug is not to expose the tt directive to translators (without changing the original behaviour). It is kind of string patch. - Can we agree to let this patch do what it does and move the 0-discussion to a new bug?
I would opt for pushing the two patches as they were in PQA. Not allowing the zero should be solved on another report. Showing the zero here is fine with me. No need to put this in NSO again.
(In reply to Marcel de Rooy from comment #24) > I would opt for pushing the two patches as they were in PQA. > Not allowing the zero should be solved on another report. > Showing the zero here is fine with me. > No need to put this in NSO again. What is the next step? BTW: Still applies on current master
I will look at this one tomorrow during QA.
(In reply to Marcel de Rooy from comment #26) > I will look at this one tomorrow during QA. Sorry, it became manana (issue with issues). Still on my list now !
From an earlier comment: Note that the trailing IF does not increase readability. I am not in favor of the original patch.
(In reply to Marcel de Rooy from comment #28) > From an earlier comment: > Note that the trailing IF does not increase readability. > I am not in favor of the original patch. OK, I will rewrite it.
Created attachment 65494 [details] [review] Bug 18649: (New patch) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt New patch without trailing IF construction in tt directives as requested in comment #28 To test: Verify that translation tool no longer shows tt directives as described in comment #1 New patch, needs SO.
Created attachment 65495 [details] [review] [SIGNED-OFF] Bug 18649: (New patch) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt New patch without trailing IF construction in tt directives as requested in comment #28 To test: Verify that translation tool no longer shows tt directives as described in comment #1 New patch, needs SO. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 65501 [details] [review] Bug 18649: (New patch) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt New patch without trailing IF construction in tt directives as requested in comment #28 To test: Verify that translation tool no longer shows tt directives as described in comment #1 New patch, needs SO. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marc Véron from comment #29) > (In reply to Marcel de Rooy from comment #28) > > From an earlier comment: > > Note that the trailing IF does not increase readability. > > I am not in favor of the original patch. > > OK, I will rewrite it. Thx
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.04
Onboarding is not in 16.11.x, so patch didn't apply cleanly. I've fixed the conflict. This patch has been pushed to 16.11.x and will be in 16.11.12.
Pushed to 16.05.x, for 16.05.17 release