Bug 18694 - Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt
Summary: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on: 18734
Blocks: 18331
  Show dependency treegraph
 
Reported: 2017-05-30 06:40 UTC by Marc Véron
Modified: 2018-12-03 20:05 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt (1.98 KB, patch)
2017-05-30 06:52 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt (3.00 KB, patch)
2017-05-30 07:13 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt (3.05 KB, patch)
2017-06-12 15:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt (3.12 KB, patch)
2017-06-12 17:33 UTC, Jonathan Druart
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-30 06:40:47 UTC
The file csv/cash_register_stats.tt exposes two FOREACH loops to translation. Translators should not be confronted with code internals.
Comment 1 Marc Véron 2017-05-30 06:52:52 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2017-05-30 07:13:57 UTC
Created attachment 63836 [details] [review]
Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt

Code exposed is:
%s %s %s [%%- FOREACH field IN row; field IF !loop.last; ItemTypes.GetDescription(field) IF loop.last; sep IF !loop.last; END %%] %s TOTAL [%%- FOREACH field IN total; field; sep IF !loop.last; END %%]

Similar for: csv/orders_by_budget.tt

To test:
- Apply patch
- In staff client, verify that Cash register statistics work as before
  (Home > Reports > Cash register statistics)
- Verify the same for Home > Reports > Orders by fund (triggers orders_by_budget.tt)
- Bonus test: Create a "language" aa-AA (perl translate create aa-AA
  from folder misc/translator, verify that lines mentioned above do
  no longer appear in aa-AA-staff-prog.po )
- Run QA tools (newest version with test for newlines in tt directives)
Comment 3 Jonathan Druart 2017-06-05 19:47:11 UTC
It does not work, the funds are all displayed on the same line.
Comment 4 Marc Véron 2017-06-06 08:17:41 UTC
(In reply to Jonathan Druart from comment #3)
> It does not work, the funds are all displayed on the same line.

Oh, that is a problem for translations, because in the target language the result of the translation process is the same one-liner as in my patch.

Setting importance to normal because it is not only a string change, but functionality in target languages seems to be broken.
Comment 5 Jonathan Druart 2017-06-06 13:05:55 UTC
(In reply to Marc Véron from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > It does not work, the funds are all displayed on the same line.
> 
> Oh, that is a problem for translations, because in the target language the
> result of the translation process is the same one-liner as in my patch.

I only tested on the non-translated interface.
Comment 6 Marc Véron 2017-06-06 14:38:38 UTC
(In reply to Jonathan Druart from comment #3)
> It does not work, the funds are all displayed on the same line.

Hi Jonathan

I took some time to re-test on Home > Reports > Orders by fund, but I can not recreate the issue.

The funds display properly if I choose Output > To screen
The funds are properly imported from csv to LibreOffice Calc if I choose Ooutput > To a file.

Where do you see the funds in one line? Can you pls. provide steps to reproduce and/or a screen shot?

Thanks
Marc
Comment 7 Katrin Fischer 2017-06-06 20:31:13 UTC
Hi Marc, 

make sure the string is not 'fuzzy' but actually translated. I have filed a bug a bit ago for the CSV export on item search being broken again (all in one line) once the header line is actually translated. When we tested the new fix, I think that was missed.
Comment 8 Marc Véron 2017-06-06 20:41:17 UTC
(In reply to Katrin Fischer from comment #7)
> Hi Marc, 
> 
> make sure the string is not 'fuzzy' but actually translated. I have filed a
> bug a bit ago for the CSV export on item search being broken again (all in
> one line) once the header line is actually translated. When we tested the
> new fix, I think that was missed.

The problem Jonathan reports is in the untranslated (English) version, see comment #5, but I can not reproduce.
Comment 9 Katrin Fischer 2017-06-06 20:42:35 UTC
Ah sorry, missed that, it sounded so familiar. - Might still be worth a test.
Comment 10 Marc Véron 2017-06-09 15:19:23 UTC
(In reply to Katrin Fischer from comment #7)
> Hi Marc, 
> 
> make sure the string is not 'fuzzy' but actually translated
(...)
Hi Katrin
If you do for a 'language' aa-AA:
perl translate create aa-AA
...you won't get strings marked as fuzzy
Comment 11 Katrin Fischer 2017-06-12 06:28:00 UTC
But will they not still be untranslated? My point was that if you don't properly translate, sometimes the results are not what you would expect. Running the install step will not create the same file output for an untranslated vs a translated file.
Comment 12 Marc Véron 2017-06-12 06:52:13 UTC
(In reply to Katrin Fischer from comment #11)
> But will they not still be untranslated? My point was that if you don't
> properly translate, sometimes the results are not what you would expect.
> Running the install step will not create the same file output for an
> untranslated vs a translated file.

Maybe there is a misunderstandig about the intention of:
perl translate create aa-AA

It is simply a test to verify that unwanted strings no longer are picked by the translation process (no longer appear in the related aa-AA*.po file).
Comment 13 Katrin Fischer 2017-06-12 07:50:14 UTC
It makes sense, I was just trying to explain something I found out recently that explains why sometimes the CSV exports tests ok, when indeed they are still broken.
Comment 14 Marc Véron 2017-06-12 08:59:02 UTC
(In reply to Jonathan Druart from comment #3)
> It does not work, the funds are all displayed on the same line.

Hi Jonathan

I'd like to move forward this bug, but I still can not reproduce this issue (see my comment #6 )

Can you provide steps to reproduce and/or a screenshot? 

Thanks a lot
Marc
Comment 15 Jonathan Druart 2017-06-12 14:09:57 UTC
The patch does no longer apply, I cannot retest.
Comment 16 Marc Véron 2017-06-12 14:43:44 UTC
(In reply to Jonathan Druart from comment #15)
> The patch does no longer apply, I cannot retest.

Patch applies cleanly here on current master

I did
git pull

HEAD is at: a38ffe2 Bug 18656 - Require confirmation of deletion of files from patron record

git bz apply 18694

63836 - Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt

Yes -> applies cleanly
Comment 17 Jonathan Druart 2017-06-12 15:10:27 UTC
(In reply to Marc Véron from comment #16)
> (In reply to Jonathan Druart from comment #15)
> Yes -> applies cleanly

No it does not, this bug report is marked as dependent on bug 18734 and both patches conflict.
Comment 18 Marc Véron 2017-06-12 15:40:15 UTC
Created attachment 64209 [details] [review]
Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt

Code exposed is:
%s %s %s [%%- FOREACH field IN row; field IF !loop.last; ItemTypes.GetDescription(field) IF loop.last; sep IF !loop.last; END %%] %s TOTAL [%%- FOREACH field IN total; field; sep IF !loop.last; END %%]

Similar for: csv/orders_by_budget.tt

To test:
- Apply patch
- In staff client, verify that Cash register statistics work as before
  (Home > Reports > Cash register statistics)
- Verify the same for Home > Reports > Orders by fund (triggers orders_by_budget.tt)
- Bonus test: Create a "language" aa-AA (perl translate create aa-AA
  from folder misc/translator, verify that lines mentioned above do
  no longer appear in aa-AA-staff-prog.po )
- Run QA tools (newest version with test for newlines in tt directives)

Rebased on top of Bug 18734 2017-12-06 / mv
Comment 19 Jonathan Druart 2017-06-12 17:33:30 UTC
Created attachment 64231 [details] [review]
Bug 18694: Translatability: Get rid of exposing [%% FOREACH in csv/cash_register_stats.tt

Code exposed is:
%s %s %s [%%- FOREACH field IN row; field IF !loop.last; ItemTypes.GetDescription(field) IF loop.last; sep IF !loop.last; END %%] %s TOTAL [%%- FOREACH field IN total; field; sep IF !loop.last; END %%]

Similar for: csv/orders_by_budget.tt

To test:
- Apply patch
- In staff client, verify that Cash register statistics work as before
  (Home > Reports > Cash register statistics)
- Verify the same for Home > Reports > Orders by fund (triggers orders_by_budget.tt)
- Bonus test: Create a "language" aa-AA (perl translate create aa-AA
  from folder misc/translator, verify that lines mentioned above do
  no longer appear in aa-AA-staff-prog.po )
- Run QA tools (newest version with test for newlines in tt directives)

Rebased on top of Bug 18734 2017-12-06 / mv

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2017-06-12 17:34:03 UTC
Sorry, it is actually broken for translated language, with and without your patch.
Skipping QA.
Comment 21 Jonathan Druart 2017-06-12 20:58:51 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 22 Fridolin Somers 2017-06-13 07:19:41 UTC
Pushed to 17.05.x, will be in 17.05.01
Comment 23 Katrin Fischer 2017-06-13 20:42:37 UTC
This patch has been pushed to 16.11.x and will be in 16.11.09.
Comment 24 Mason James 2017-09-19 23:30:14 UTC
depends on bug 6934, 

Blocked by Enhancement, skipping for 16.05.x branch