Bug 18754

Summary: Translatability: Get rid of exposed tt directives in opac-detail.tt
Product: Koha Reporter: Marc Véron <veron>
Component: I18N/L10NAssignee: Marc Véron <veron>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: f.demians, fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy, mtj
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 19913    
Attachments: Bug 18754: Translatability: Get rid of exposed tt directives in opac-detail.tt
Bug 18754: Translatability: Get rid of exposed tt directives in opac-detail.tt
Bug 18754: Translatability: Get rid of exposed tt directives in opac-detail.tt
Bug 18754: Translatability: Get rid of exposed tt directives in opac-detail.tt
Bug 18754: [QA Follow-up] Tiny corrections

Description Marc Véron 2017-06-08 04:55:54 UTC
The file opac-detail.tt exposes a lot of template directives to translation where translators should not be confronted with.

Some examples from po file are:

$[% SUBFIELD.code %] [% SUBFIELD.value %]

%s [%% INCLUDE \"openlibrary-readapi.inc\" bib = { normalized_isbn => "
"normalized_isbn, lccn => lccn, normalized_oclc => normalized_oclc } %%]

See: [% IF ( nextTitle ) %][% nextTitle |html %][% ELSE %]next biblio[% END "
"%]
Comment 1 Marc Véron 2017-06-09 13:45:28 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2017-06-11 06:58:58 UTC
Created attachment 64178 [details] [review]
Bug 18754: Translatability: Get rid of exposed tt directives in opac-detail.tt

The file opac-detail.tt exposes a lot of template directives to translation where translators should not be confronted with.

Some examples from po file are:

$[% SUBFIELD.code %] [% SUBFIELD.value %]

%s [%% INCLUDE \"openlibrary-readapi.inc\" bib = { normalized_isbn => "
"normalized_isbn, lccn => lccn, normalized_oclc => normalized_oclc } %%]

See: [% IF ( nextTitle ) %][% nextTitle |html %][% ELSE %]next biblio[% END "
"%]

To test:
- Apply patch
- Do a search in OPAC that has more than 1 results
- Go to the detail page of one of the items found
- Verify that the details display as before and that you can
  browse the results with Previous and Next
- In staff client, change OPACXSLTDetailsDisplay from 'default' to
  empty for "no xslt" and repeat steps above
- In staff client, set HTML5MediaEnabled to 'OPAC' or 'OPAC and staff client'
- Verify that media catalogued in field 856 still work
- Create a new translation for a 'language' aa-AA (perl translate create aa-AA)
- Verify that template directives ar no longer exposed in aa-AA-opac-bootstrap.po
Comment 3 Marc Véron 2017-07-20 08:10:52 UTC
Still applies on current master
Comment 4 Alex Buckley 2017-08-02 05:09:11 UTC
Created attachment 65412 [details] [review]
Bug 18754: Translatability: Get rid of exposed tt directives in opac-detail.tt

The file opac-detail.tt exposes a lot of template directives to translation where translators should not be confronted with.

Some examples from po file are:

$[% SUBFIELD.code %] [% SUBFIELD.value %]

%s [%% INCLUDE \"openlibrary-readapi.inc\" bib = { normalized_isbn => "
"normalized_isbn, lccn => lccn, normalized_oclc => normalized_oclc } %%]

See: [% IF ( nextTitle ) %][% nextTitle |html %][% ELSE %]next biblio[% END "
"%]

To test:
- Apply patch
- Do a search in OPAC that has more than 1 results
- Go to the detail page of one of the items found
- Verify that the details display as before and that you can
  browse the results with Previous and Next
- In staff client, change OPACXSLTDetailsDisplay from 'default' to
  empty for "no xslt" and repeat steps above
- In staff client, set HTML5MediaEnabled to 'OPAC' or 'OPAC and staff client'
- Verify that media catalogued in field 856 still work
- Create a new translation for a 'language' aa-AA (perl translate create aa-AA)
- Verify that template directives ar no longer exposed in aa-AA-opac-bootstrap.po

Followed test plan which works as intended

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 5 Marcel de Rooy 2017-08-04 06:17:53 UTC
I am wondering if constructions like those below should be considered as improvements to the template (recognizing translation benefits). But as always, we have to find some balance.

[% IF ( collectionvolume ) %] [% ' ; ' _ collectionvolume %] [% END %]

[% ' ( ' _ (itemloop.size || 0) _ ' )' %]

Another QA/RM opinion please.
Comment 6 Marcel de Rooy 2017-08-04 06:19:00 UTC
Given the number of hard-to-read lines, I would not mark this one as trivial btw.
Comment 7 Marc Véron 2017-08-04 07:56:56 UTC
(In reply to Marcel de Rooy from comment #6)
> Given the number of hard-to-read lines, I would not mark this one as trivial
> btw.

Who can better deal with code - translators or developers? We should avoid to expose code or code fragents to translators IMO, otherwise we risk that translators break something.
Comment 8 Owen Leonard 2017-08-04 11:42:47 UTC
I would vote for added complexity in the templates in order to make translation easier.
Comment 9 Marcel de Rooy 2017-08-04 14:31:00 UTC
Just a note about BLOCK..INCLUDE versus BLOCK..PROCESS:

The PROCESS directive is slightly faster than INCLUDE because it avoids the need to localise (i.e. copy) the variable stash before processing the template.
Comment 10 Marcel de Rooy 2017-08-04 14:49:48 UTC
Created attachment 65504 [details] [review]
Bug 18754: Translatability: Get rid of exposed tt directives in opac-detail.tt

The file opac-detail.tt exposes a lot of template directives to translation where translators should not be confronted with.

Some examples from po file are:

$[% SUBFIELD.code %] [% SUBFIELD.value %]

%s [%% INCLUDE \"openlibrary-readapi.inc\" bib = { normalized_isbn => "
"normalized_isbn, lccn => lccn, normalized_oclc => normalized_oclc } %%]

See: [% IF ( nextTitle ) %][% nextTitle |html %][% ELSE %]next biblio[% END "
"%]

To test:
- Apply patch
- Do a search in OPAC that has more than 1 results
- Go to the detail page of one of the items found
- Verify that the details display as before and that you can
  browse the results with Previous and Next
- In staff client, change OPACXSLTDetailsDisplay from 'default' to
  empty for "no xslt" and repeat steps above
- In staff client, set HTML5MediaEnabled to 'OPAC' or 'OPAC and staff client'
- Verify that media catalogued in field 856 still work
- Create a new translation for a 'language' aa-AA (perl translate create aa-AA)
- Verify that template directives ar no longer exposed in aa-AA-opac-bootstrap.po

Followed test plan which works as intended

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-08-04 14:49:53 UTC
Created attachment 65505 [details] [review]
Bug 18754: [QA Follow-up] Tiny corrections

Converted one INCLUDE directive to PROCESS; we are not changing variables here. (The PROCESS directive is slightly faster than INCLUDE because it avoids the need to localise (i.e. copy) the variable stash before processing the template.)

Removed one vim inserted letter i.

Error in [% IF ( XISBN.publicationyear ) _ ', ' _ XISBN.publicationyear %][% END %] The concatenation became part of the condition.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-08-04 14:51:34 UTC
(In reply to Marc Véron from comment #7)
> (In reply to Marcel de Rooy from comment #6)
> > Given the number of hard-to-read lines, I would not mark this one as trivial
> > btw.
> 
> Who can better deal with code - translators or developers? We should avoid
> to expose code or code fragents to translators IMO, otherwise we risk that
> translators break something.

(In reply to Owen Leonard from comment #8)
> I would vote for added complexity in the templates in order to make
> translation easier.

Okay. Send it to the next level.
Comment 13 Jonathan Druart 2017-08-30 19:59:35 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 14 Fridolin Somers 2017-09-29 13:45:24 UTC
Pushed to 17.05.x, will be in 17.05.05.
Comment 15 Katrin Fischer 2017-10-01 22:28:05 UTC
These patches have been pushed to 16.11.x and will be in 16.11.13.
Comment 16 Mason James 2017-11-23 22:10:31 UTC
Pushed to 16.05.x, for 16.05.18 release