Bug 18649 - Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt
Summary: Translatability: Get rid of tt directive in translation for admin/categories....
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Marc Véron
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-22 15:56 UTC by Marc Véron
Modified: 2017-09-15 22:57 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18649: Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt (3.16 KB, patch)
2017-05-22 16:08 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18649: Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt (3.33 KB, patch)
2017-05-26 04:07 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 18649: [QA followup] Remove chomps in admin/categories.tt (1.55 KB, patch)
2017-05-27 05:02 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18649: [QA followup] Simplify If and remove chomps in admin/categories.tt (2.83 KB, patch)
2017-06-02 11:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18649: Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt (3.37 KB, patch)
2017-06-06 13:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18649: (Restored earlier version) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt (2.77 KB, patch)
2017-06-07 13:11 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18649: (New patch) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt (3.58 KB, patch)
2017-08-04 09:01 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18649: (New patch) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt (3.65 KB, patch)
2017-08-04 12:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 18649: (New patch) Translatability: Get rid of tt directive in translation for admin/categories.tt and onboardingstep2.tt (3.73 KB, patch)
2017-08-04 14:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2017-05-22 15:56:11 UTC Comment hidden (obsolete)
Comment 1 Marc Véron 2017-05-22 16:01:53 UTC
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
Comment 2 Marc Véron 2017-05-22 16:08:47 UTC Comment hidden (obsolete)
Comment 3 Aleisha Amohia 2017-05-26 04:07:51 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2017-05-26 07:41:15 UTC
(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
Comment 5 Marcel de Rooy 2017-05-26 10:26:24 UTC
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?
Comment 6 Marc Véron 2017-05-26 13:09:55 UTC
(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.
Comment 7 Marc Véron 2017-05-27 05:02:22 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2017-06-02 09:33:24 UTC
Marc,
I understand your explanation, but you did not respond to my question about the trailing IF.
Comment 9 Marc Véron 2017-06-02 10:49:26 UTC
(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.
Comment 10 Marcel de Rooy 2017-06-02 10:51:40 UTC
(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 %] ?
Comment 11 Marc Véron 2017-06-02 11:11:03 UTC
Oh... Hmm... That's true for the original line as well...
Comment 12 Marc Véron 2017-06-02 11:19:11 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2017-06-06 13:09:19 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2017-06-06 15:03:42 UTC
Applied the second patch to the first.
Comment 15 Jonathan Druart 2017-06-06 19:51:36 UTC
There is a change in the behaviour:
Before the patch, if the enrolment period == 0, the input was empty, now it shows '0'.
Comment 16 Marc Véron 2017-06-06 20:14:51 UTC
(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
Comment 17 Marcel de Rooy 2017-06-07 04:50:02 UTC
(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
Comment 18 Marc Véron 2017-06-07 13:11:23 UTC
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
Comment 19 Marc Véron 2017-06-07 13:13:58 UTC
Resetting to NSO because restored patch lost SO line.
Comment 20 Jonathan Druart 2017-06-07 14:19:39 UTC
Sounds like there is something wrong with the "0", but if it's a bug we must fix it somewhere else.
Comment 21 Marc Véron 2017-06-07 15:47:50 UTC
(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.
Comment 22 Jonathan Druart 2017-06-07 16:06:53 UTC
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.
Comment 23 Marc Véron 2017-06-07 16:32:11 UTC
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?
Comment 24 Marcel de Rooy 2017-06-08 04:57:49 UTC
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.
Comment 25 Marc Véron 2017-07-20 08:17:46 UTC
(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
Comment 26 Marcel de Rooy 2017-07-20 08:21:21 UTC
I will look at this one tomorrow during QA.
Comment 27 Marcel de Rooy 2017-07-21 10:18:10 UTC
(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 !
Comment 28 Marcel de Rooy 2017-08-04 05:51:52 UTC
From an earlier comment:
Note that the trailing IF does not increase readability.
I am not in favor of the original patch.
Comment 29 Marc Véron 2017-08-04 08:12:13 UTC
(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.
Comment 30 Marc Véron 2017-08-04 09:01:17 UTC
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.
Comment 31 Owen Leonard 2017-08-04 12:19:29 UTC
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>
Comment 32 Marcel de Rooy 2017-08-04 14:18:55 UTC
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>
Comment 33 Marcel de Rooy 2017-08-04 14:19:39 UTC
(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
Comment 34 Jonathan Druart 2017-08-30 19:58:16 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 35 Fridolin SOMERS 2017-09-12 12:50:02 UTC
Pushed to 17.05.x, will be in 17.05.04
Comment 36 Katrin Fischer 2017-09-15 22:57:27 UTC
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.