I wonder about the pref file - the # comment kept all strings for one pref together and we have translated "Do" and "Don't" quite differently for some of them, according to context as it works not really well to put the Verb in front of the sentence in German like that. So it appearing more than once was a feature for us. Can we keep that?

(In reply to Katrin Fischer from comment #2)
> I wonder about the pref file - the # comment kept all strings for one pref
> together and we have translated "Do" and "Don't" quite differently for some
> of them, according to context as it works not really well to put the Verb in
> front of the sentence in German like that. So it appearing more than once
> was a feature for us. Can we keep that?

That feature is kept. msgids will be identical ("Do") but msgctxt won't ("Section > Subsection > Syspref"), allowing translators to give different translations depending on the context.

Thx Julian.

Hi Julian, nice work.

I did a comparison of the msgids on each PO file before and after patch.
pref file needed some touching, I removed the text that now is excluded using
's/\".*#\s/\"/;s/\".*\.pref\s/\"/;'

Comparing only msgid, then doing a diff and counting different lines before (old) and after (new) I got (filename old new)

xx-YY-installer-MARC21.po 0 0
xx-YY-installer.po 0 0
xx-YY-marc-MARC21.po 0 2  *
xx-YY-marc-NORMARC.po 0 0
xx-YY-marc-UNIMARC.po 0 0
xx-YY-messages-js.po 0 0
xx-YY-messages.po 0 2 *
xx-YY-opac-bootstrap.po 4 0 +
xx-YY-pref.po 19 17 #
xx-YY-staff-prog.po 49 0 +

There are a few differences :)
Lines with (*) are new strings, great
Lines with (#), preference file, most differences expected here, most can be ignored
Lines with (+), missing strings after patch

On opac
< msgid "000 "
< msgid "LCCN:"
< msgid "MARC Card View"
< msgid "MARC View"

examples on staff
< msgid "Gone no address flag"
< msgid "Illustrations"
< msgid "Item number"
< msgid "Item number (internal)"
< msgid "Koha itemtype"
< msgid "Koha normalized classification for sorting"
< msgid "Lost card flag"
< msgid "MARC Card View"
< msgid "MARC View"

Thanks Bernardo for this detailed analysis. It looks like I forgot some files in the gulp globs. In particular columns.def and the XSL files that are not related to a specific marc flavour.
I will try to fix that ASAP.

With the last patch I get the same count of msgid than on master for opac-bootstrap and staff-prog

(In reply to Julian Maurice from comment #8)
> With the last patch I get the same count of msgid than on master for
> opac-bootstrap and staff-prog

Yes, they have the same content :)

Last question: some may be used to the old syntax, doing ./translate create|update on misc/translator/, they need to use it anyway to do the install

What do you think of executing 'gulp' from translate script?
and perhaps using "--silent" to suppress messages unless verbose mode is requested?

(In reply to Bernardo Gonzalez Kriegel from comment #9)
> (In reply to Julian Maurice from comment #8)
> > With the last patch I get the same count of msgid than on master for
> > opac-bootstrap and staff-prog
> 
> Yes, they have the same content :)
> 
> Last question: some may be used to the old syntax, doing ./translate
> create|update on misc/translator/, they need to use it anyway to do the
> install
> 
> What do you think of executing 'gulp' from translate script?
> and perhaps using "--silent" to suppress messages unless verbose mode is
> requested?

I think it is not needed, but it can't hurt to have it. Maybe QA has an opinion on that ?

Hi Julian and Bernardo, happy to see the thorough testing of this change. 

If we could make koha-translate and the translate script behave the same with this change than before, I'd be in favor of it.

It seems that koha-translate only uses the 'install' mode of misc/translator/translate, so its behavior should not change.

(In reply to Julian Maurice from comment #15)
> Created attachment 103440 [details] [review] [review]
> Bug 25067: Add translate create|update commands that call gulp

Sorry to bother you with this, but can we left the deprecation notice for verbose mode? 
I would like the 'same' behavior as before
(It's my liking of course, and you can ignore it. I'll sign anyway)

Finally, files gulpfile.js, xgettext-installer, xgettext-pref and xgettext-tt2 need a copyright notice.

(In reply to Bernardo Gonzalez Kriegel from comment #16)
> Sorry to bother you with this, but can we left the deprecation notice for
> verbose mode? 
> I would like the 'same' behavior as before
> (It's my liking of course, and you can ignore it. I'll sign anyway)

In my opinion, people should be warned even if they don't want to, and they can use gulp directly if they don't want to see this warning, right ? :)
But in the end, you are probably the only one directly concerned by this change, so if you think it's better this way, I'll make those changes ASAP

All tested and signed!

All patches rebased on master

I agree with the fact that the translate script should behave the same way than before. But i think it's not possible.

I think of people that will upgrade with the gulp feature and surprisingly note that the script no longer works for create or update if they didn't install the gulp-cli / yarn command.

My feeling is that we should have the command ./translate create|update telling the user: "Creating or updating po process has changed, please use it the way <here>" 

Bernardo, Katrin any thoughts ?

Also, i have concerns about the .pot generated. They are no longer cleaned now.
Do we need these .pot files actually ?

I don't know about the .pot files, but was looking at the test plan again (thx for the write-up!):

- Can 'gulp po:update' be run for a selected language? It takes a long while to do this for all languages, so you might want to do it only for the ones you need/test with.

- In the test plan only installing a language is used with the old syntax, creating and updating are different. I think mixing would not be good. Why is it not possible to move all to the old syntax? and should we them move installing as well with the deprecation note you suggest?

You write:
>- there is no need for $KOHA_CONF to be defined
>  LangInstaller.pm relied on $KOHA_CONF to get the different paths
>  needed. I'm not sure why, since string extraction and PO update should
>  work on source files, not installed files

But aren't the paths to the templates we need to parse different depending on installation type? And also the po files will be stored in a different place?

We need to keep the option for everyone to update/install their own adapted po files. I know a lot of people make local changes. Even we do to a certain extend to have our own XSLT files translated.

Still a bit worried about this one:

>  Now context is put into msgctxt, and the reference is set, which is
>  cleaner

>    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
>    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
>    msgid "automatically display a print dialog for a payment receipt "
>    "when making a payment.."

I think in translation practice using Pootle this could be quite hard on tranlators. :(

Right now you can search in the prefs file for a system preference and it will turn up all strings related to it. They also appear in alphabetic order, so when you start translating, you have the 'context' because the strings appear together with the prefixed note. This was intentional and a 'feature'. 

It doesn't seem that we can search by 'context' in Pootle, so seeing all strings for a pref together will no longer be possible.

To explain: Translating prefs is quite hard because you have to make sure things work grammatically with sentence parts torn apart. This depends on language I am sure, but for German it's not an easy task and we can't translate "Do" and "Don't" just like that. Often we need to move parts of the rest of the sentence into the options for things to make more sense.

Also:

>  The downside is that some messages will have to be re-translated,
>  especially short messages like 'Do', for which msgmerge has a hard
>  time finding the corresponding new msgid.

This is quite a burden for translators, escpecially since you have to look up every single one of them to see what the rest of the sentence looks like. Can't we figure them out? We do have the pref and the current translation.

> - In the test plan only installing a language is used with the old syntax,
> creating and updating are different. I think mixing would not be good. Why
> is it not possible to move all to the old syntax? and should we them move
> installing as well with the deprecation note you suggest?

Realizing that last bit made no sense:

And should we not move installing to the new syntax as well with a deprecation note like you suggest? 

Overall I think we need to keep things as easy as possible since not only experienced Koha users will be using these commands (update, install, create).

(In reply to Katrin Fischer from comment #34)
> I don't know about the .pot files, but was looking at the test plan again
> (thx for the write-up!):
> 
> - Can 'gulp po:update' be run for a selected language? It takes a long while
> to do this for all languages, so you might want to do it only for the ones
> you need/test with.
Yep, just use --lang option:

  gulp po:update --lang de-DE

> You write:
> >- there is no need for $KOHA_CONF to be defined
> >  LangInstaller.pm relied on $KOHA_CONF to get the different paths
> >  needed. I'm not sure why, since string extraction and PO update should
> >  work on source files, not installed files
> 
> But aren't the paths to the templates we need to parse different depending
> on installation type? And also the po files will be stored in a different
> place?
I believe create and update operations should work exclusively on source files. I don't see why we would want to work on the copy installed under /usr/share/koha for instance.
Note that 'install' operation still work the same, translated templates are installed in the right place.

> 
> We need to keep the option for everyone to update/install their own adapted
> po files. I know a lot of people make local changes. Even we do to a certain
> extend to have our own XSLT files translated.
We do too, but local translations are handled in separate PO files.

Do you alter the original PO files ? How do you handle Koha upgrades ?

> 
> Still a bit worried about this one:
> 
> >  Now context is put into msgctxt, and the reference is set, which is
> >  cleaner
> 
> >    #: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/accounting.pref
> >    msgctxt "Accounting > Policy > FinePaymentAutoPopup"
> >    msgid "automatically display a print dialog for a payment receipt "
> >    "when making a payment.."
> 
> I think in translation practice using Pootle this could be quite hard on
> tranlators. :(
> 
> Right now you can search in the prefs file for a system preference and it
> will turn up all strings related to it. They also appear in alphabetic
> order, so when you start translating, you have the 'context' because the
> strings appear together with the prefixed note. This was intentional and a
> 'feature'. 
> 
> It doesn't seem that we can search by 'context' in Pootle, so seeing all
> strings for a pref together will no longer be possible.
It seems that Pootle can search by comment. Would that help if the syspref name and category appear in a comment ?

> 
> To explain: Translating prefs is quite hard because you have to make sure
> things work grammatically with sentence parts torn apart. This depends on
> language I am sure, but for German it's not an easy task and we can't
> translate "Do" and "Don't" just like that. Often we need to move parts of
> the rest of the sentence into the options for things to make more sense.
This will still be possible

> 
> Also:
> 
> >  The downside is that some messages will have to be re-translated,
> >  especially short messages like 'Do', for which msgmerge has a hard
> >  time finding the corresponding new msgid.
> 
> This is quite a burden for translators, escpecially since you have to look
> up every single one of them to see what the rest of the sentence looks like.
> Can't we figure them out? We do have the pref and the current translation.
Probably.
We can also easily revert this change from this patchset and address the issue in a separate bug. This bug brings already too many different things, so if it helps I will gladly remove that. I just thought it would be a nice "clean up" to do at the same time.

(In reply to Katrin Fischer from comment #35)
> > - In the test plan only installing a language is used with the old syntax,
> > creating and updating are different. I think mixing would not be good. Why
> > is it not possible to move all to the old syntax? and should we them move
> > installing as well with the deprecation note you suggest?
> 
> Realizing that last bit made no sense:
> 
> And should we not move installing to the new syntax as well with a
> deprecation note like you suggest? 
> 
> Overall I think we need to keep things as easy as possible since not only
> experienced Koha users will be using these commands (update, install,
> create).
In my opinion, install is really different from create/update so it makes sense for it to be in a different script/called in a different way.
Only install should be called by end users (people who manage Koha instances)
create/update is more for Koha translation maintainer (aka Bernardo)

Last patch tested with fr-FR PO files : msgmerge correctly keep all translations (even short ones like "Do"). The only difference is that long lines are now wrapped.

Hm, I think I don't understand what you are referring to as source files. Will this work for tarball installations, packaging installations and git based installations alike? I believe we don't always have 2 copies of the files? 
I am worried this could break things for some of those?

Adding the info in a comment could help a bit, I tihnk, but it could get quite a lot, maybe just the names of the prefs?

I think it's a wrong assumption that update and create will only be used by our translation manager or developers. I believe libraries have for example changed terms in po files and similar for the normal GUI. 

But I'd be interested in your workflow for separate po for other use cases. :)

(In reply to Julian Maurice from comment #38)
> Last patch tested with fr-FR PO files : msgmerge correctly keep all
> translations (even short ones like "Do"). The only difference is that long
> lines are now wrapped.

That's some good news. :)

(In reply to Katrin Fischer from comment #39)
> Hm, I think I don't understand what you are referring to as source files.
> Will this work for tarball installations, packaging installations and git
> based installations alike? I believe we don't always have 2 copies of the
> files? 
> I am worried this could break things for some of those?
By source files I mean the files that are committed in the git repo. All files that you have directly after a git clone.
IMO create and update should not be run on "installations", but directly on files inside the git repository, independently of any installation.
'create' and 'update' are essentially developer tools to manage PO files within the git repository.

> I think it's a wrong assumption that update and create will only be used by
> our translation manager or developers. I believe libraries have for example
> changed terms in po files and similar for the normal GUI.
I think that we should not support nor encourage that. Even if it's "only" translations, it's still a fork of Koha. Everyone is free to do that, of course, but then they are on their own to maintain their changes.
If translations are wrong, they should be fixed on translate.koha-community.org

> But I'd be interested in your workflow for separate po for other use cases.
> :)
Basically, for each client-specific development that adds new strings, we create a PO file in misc/translator/po/fr-FR-<category>-<ticket-id>.po containing all the new strings and their translation. When we need to update fr-FR templates we call a script that "concatenates" (msgcat) all fr-FR-<category>-*.po and runs misc/translator/translate install

Rebased on master (needed because of bug 26228)

1. Run `yarn install` and install Locale::XGettext::TT2

It looks like Locale::XGettext::TT2 has no packages for Debian yet, can you confirm?
Ended up installatin from CPAN.
Adding Mason.

2. Run `gulp po:update`

$ gulp po:update
-bash: gulp: command not found
I've tried following this:
https://gulpjs.com/docs/en/getting-started/quick-start/

$ gulp po:update
Finishes this time.

3. Verify the contents of updated PO files

I have verified the contents of the de-DE files. 

Problem: It appears that a strings have jumped from one po file to another, requiring a lot of retranslations. Namely this seems to be the value_builder plugins that are no longer in the MARC flavor files but appear in staff now:

Example string:
twentyfour track

This is now in the UNIMARC file, but with this patch appears in staff.

Question: Reviewing the pref po looks like before, correct?

4. Run `cd misc/translator && ./translate install <lang>`

Testing with de-DE again.
Question: do we need a follow-up to change the package commands as well?

Activated the language and the language switcher in the preferences.

5. Verify that all (templates, sysprefs, xslt, installer files) is
   correctly translated

! System preferences: remained English
Staff GUI: OK
OPAC GUI: OK
XSLT in staff and OPAC: OK
messages JS: OK (verified with OPAC cart strings)
messages: OK (verified with "item" and "items" in staff result list)
installer: OK (tested with es-ES)
installed sample files: OK (tested with notices and frameworks)

Preferences don't translate.
Stopping here for now because of the problem with prefs.

6. Run `gulp po:create --lang <lang>` and verify that it created all PO
   files for that language


7. Run `prove t/misc/translator`

Test Summary Report
-------------------
t/misc/translator/xgettext-pref.t     (Wstat: 3840 Tests: 16 Failed: 15)
  Failed tests:  1, 3-16
  Non-zero exit status: 15
Files=3, Tests=56,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.34 cusr  0.07 csys =  0.45 CPU)
Result: FAIL

(In reply to Katrin Fischer from comment #49)
> 1. Run `yarn install` and install Locale::XGettext::TT2
> 
> It looks like Locale::XGettext::TT2 has no packages for Debian yet, can you
> confirm?
That's true. It is not packaged in Debian.

> 3. Verify the contents of updated PO files
> 
> I have verified the contents of the de-DE files. 
> 
> Problem: It appears that a strings have jumped from one po file to another,
> requiring a lot of retranslations. Namely this seems to be the value_builder
> plugins that are no longer in the MARC flavor files but appear in staff now:
> 
> Example string:
> twentyfour track
> 
> This is now in the UNIMARC file, but with this patch appears in staff.
Good catch! Thanks :)
It seems that the update to gulp v4 has some issues with file globs and case-sensitivity. Fixed in one of the latest patches

I compared the number of msgid before and after the patch:

Before:
  de-DE-installer-MARC21.po 1738
  de-DE-installer.po 754
  de-DE-marc-MARC21.po 3152
  de-DE-marc-NORMARC.po 449
  de-DE-marc-UNIMARC.po 1797
  de-DE-messages-js.po 80
  de-DE-messages.po 25
  de-DE-opac-bootstrap.po 2014
  de-DE-pref.po 2538
  de-DE-staff-prog.po 9705
After:
  de-DE-installer-MARC21.po 1738
  de-DE-installer.po 754
  de-DE-marc-MARC21.po 3154   # 2 additional strings picked from koha-tmpl/intranet-tmpl/prog/en/data/marc21_field_CF.xsd, can be excluded if needed
  de-DE-marc-NORMARC.po 449
  de-DE-marc-UNIMARC.po 1797
  de-DE-messages-js.po 80
  de-DE-messages.po 27   # 2 additional strings picked, thanks to the transition to Locale::XGettext::TT2
  de-DE-opac-bootstrap.po 2014
  de-DE-pref.po 2538
  de-DE-staff-prog.po 9705

> Question: Reviewing the pref po looks like before, correct?
Yes, the pref po file content should not change (except for long lines wrapping, and maybe the order of strings ? I did not check)

> 4. Run `cd misc/translator && ./translate install <lang>`
> 
> Testing with de-DE again.
> Question: do we need a follow-up to change the package commands as well?
I haven't tested those, but I don't think a change is needed

> Activated the language and the language switcher in the preferences.
> 
> 5. Verify that all (templates, sysprefs, xslt, installer files) is
>    correctly translated
> 
> ! System preferences: remained English
> Staff GUI: OK
> OPAC GUI: OK
> XSLT in staff and OPAC: OK
> messages JS: OK (verified with OPAC cart strings)
> messages: OK (verified with "item" and "items" in staff result list)
> installer: OK (tested with es-ES)
> installed sample files: OK (tested with notices and frameworks)
> 
> Preferences don't translate.
> Stopping here for now because of the problem with prefs.
Fixed by the last patch

> 7. Run `prove t/misc/translator`
> 
> Test Summary Report
> -------------------
> t/misc/translator/xgettext-pref.t     (Wstat: 3840 Tests: 16 Failed: 15)
>   Failed tests:  1, 3-16
>   Non-zero exit status: 15
> Files=3, Tests=56,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.34 cusr  0.07
> csys =  0.45 CPU)
> Result: FAIL
Fixed

Thank you Katrin for testing !
It's now ready to be tested again :)

Hm, having problems applying this patch set:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 25067: Move PO file manipulation code into gulp tasks
Using index info to reconstruct a base tree...
M	gulpfile.js
M	package.json
M	t/LangInstaller.t
M	yarn.lock
Falling back to patching base and 3-way merge...
Auto-merging yarn.lock
CONFLICT (content): Merge conflict in yarn.lock
Auto-merging t/misc/translator/sample.tt
CONFLICT (modify/delete): t/LangInstaller.t deleted in Bug 25067: Move PO file manipulation code into gulp tasks and modified in HEAD. Version HEAD of t/LangInstaller.t left in tree.
Auto-merging package.json
CONFLICT (content): Merge conflict in package.json
Auto-merging gulpfile.js
error: Failed to merge in the changes.
Patch failed at 0001 Bug 25067: Move PO file manipulation code into gulp tasks
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-25067-Move-PO-file-manipulation-code-into-gulp-ejV37B.patch

Can you please check?

Patches rebased on master

I think I follow Julians thinking here.. the create and update process's are at the developer level and should always take place from a source directory.

We should strongly discourage people from updating live code in /usr/share/koha etc and so I agree with dropping the ability to run the, now gulp tasks, from a live install... 

As far as I understand, everything basically continues to work as they always did (especially with the backward compatable calls inside translate).

I think it's clearer to developers and end users having the distinct processes myself.. one for creating and maintaining translations (via gulp tasks) and a second for 'installing' and 'using' translations from the scripts.

Squashed all patches

Created attachment 111642 [details] [review]
Bug 25067: Move PO file manipulation code into gulp tasks

misc/translator/translate was doing three different things:
- extract translatable strings
- create or update PO files
- install translated templates

This patch separates responsibilities by moving the string extraction
code into several 'xgettext-like' scripts and adds gulp tasks to
automate string extraction and PO files update

This has several benefits:

- gulp runs tasks in parallel, so it's a lot faster (updating all PO
  files is at least 10 times faster with my 4-cores CPU)

- there is no need for $KOHA_CONF to be defined
  LangInstaller.pm relied on $KOHA_CONF to get the different paths
  needed. I'm not sure why, since string extraction and PO update should
  work on source files, not installed files

- string extraction code can be more easily tested

This patch also brings a couple of fixes and improvements:

- TT string extraction (strings wrapped in [% t(...) %]) was done with
  Template::Parser and PPI, which was extremely slow, and had some
  problems (see bug 24797).
  This is now done with Locale::XGettext::TT2 (new dependency) which is
  a lot faster, and fixes bug 24797

- Fix header in 4 PO files

For backward compatibility, 'create' and 'update' commands of
misc/translator/translate can still be used and will execute the
corresponding gulp task

Test plan:
1. Run `yarn install` and install Locale::XGettext::TT2
2. Run `gulp po:update`
3. Verify the contents of updated PO files
4. Run `cd misc/translator && ./translate install <lang>`
5. Verify that all (templates, sysprefs, xslt, installer files) is
   correctly translated
6. Run `gulp po:create --lang <lang>` and verify that it created all PO
   files for that language
7. Run `prove t/misc/translator`

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Need to install yarn & gulp, no errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 111643 [details] [review]
Bug 25067: (QA follow-up) Add +x on test files

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

I am passing QA here, but there are some caveats:

1) This adds a dependency on Locale::XGettext::TT2 that currently has no packages for Debian. 

2) The second part that deals with the translations of system preferences on bug 26547 is not ready yet. I believe both patches should only be pushed together. 

I hope that moving this a step further will be a little push for things to be finished. The other option would be moving it into BLOCKED (leaving that to RM)

(In reply to Katrin Fischer from comment #70)
> 2) The second part that deals with the translations of system preferences on
> bug 26547 is not ready yet. I believe both patches should only be pushed
> together. 

Why ? Translations of system preferences still work without bug 26547. I moved the sysprefs part to bug 26547 for the specific purpose of not blocking this one :-)

(In reply to Julian Maurice from comment #71)
> (In reply to Katrin Fischer from comment #70)
> > 2) The second part that deals with the translations of system preferences on
> > bug 26547 is not ready yet. I believe both patches should only be pushed
> > together. 
> 
> Why ? Translations of system preferences still work without bug 26547. I
> moved the sysprefs part to bug 26547 for the specific purpose of not
> blocking this one :-)

 Maybe this is a misunderstanding then. I'll retest, but what shoudl the pref file look like with only this patch?

(In reply to Katrin Fischer from comment #72)
>  Maybe this is a misunderstanding then. I'll retest, but what shoudl the
> pref file look like with only this patch?

It should look exactly the same, so something like this:

    # Accounting > Policy
    msgid "accounting.pref#FinePaymentAutoPopup# automatically display "
    "a print dialog for a payment receipt when making a payment.."

Mason, Could you have a look at how hard/easy Locale::XGettext::TT2 can be packaged (from 20.11 ideally)?

(In reply to Jonathan Druart from comment #74)
> Mason, Could you have a look at how hard/easy Locale::XGettext::TT2 can be
> packaged (from 20.11 ideally)?

Should it be packaged ? It is a dev-only dependency, so it won't be used in a production environment.
Still better to have it as a deb package, but not required IMO.

(In reply to Julian Maurice from comment #75)
> (In reply to Jonathan Druart from comment #74)
> > Mason, Could you have a look at how hard/easy Locale::XGettext::TT2 can be
> > packaged (from 20.11 ideally)?
> 
> Should it be packaged ? It is a dev-only dependency, so it won't be used in
> a production environment.
> Still better to have it as a deb package, but not required IMO.

Debian packages provide a koha-translate command that remove then install for update. So we are good indeed.

koha-testing-docker related issue: https://gitlab.com/koha-community/koha-testing-docker/-/issues/233

t/misc/translator/xgettext-tt2.t ................. Can't locate Locale/XGettext/TT2.pm in @INC (you may need to install the Locale::XGettext::TT2 module) (@INC contains: /kohadevbox/koha /kohadevbox/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.28.1 /usr/local/share/perl/5.28.1 /usr/lib/x86_64-linux-gnu/perl5/5.28 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.28 /usr/share/perl/5.28 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at /usr/share/perl/5.28/parent.pm line 16.
BEGIN failed--compilation aborted at /kohadevbox/koha/t/misc/translator/../../../misc/translator/xgettext-tt2 line 27.

This will be a problem for Mason, if the non db dependent tests fail.

Created attachment 113252 [details] [review]
Bug 25067: Move the tests to db_dependent

To not break the package workflow

Pushed to master for 20.11, thanks to everybody involved!

Created attachment 113255 [details] [review]
Bug 25067: Adjust paths in tests

Silly me...

Created attachment 113258 [details] [review]
Bug 25067: Fix perlcritic errors

Julian, we have xt/author/translatable-templates.t that is failing.

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove xt/author/translatable-templates.t 
xt/author/translatable-templates.t .. 1/? 
#   Failed test 'opac_bootstrap templates are translatable'
#   at xt/author/translatable-templates.t line 91.
# Unknown action specified.
# 
# Try `./tmpl_process3.pl --help for more information.
# 

#   Failed test 'staff_prog templates are translatable'
#   at xt/author/translatable-templates.t line 91.
# Unknown action specified.
# 
# Try `./tmpl_process3.pl --help for more information.
# 
# Looks like you failed 2 tests of 2.

Any ideas how we could fix that elegantly?

I tried to go back to just before these patches to make xt/author/translatable-templates.t fail, unsuccessfully. I'm not sure what this script is supposed to test, because even with a completely broken template with invalid html and invalid TT markup, the tests pass.
So it looks like the test is useless, and I suggest to remove it.

(In reply to Julian Maurice from comment #75)
> (In reply to Jonathan Druart from comment #74)
> > Mason, Could you have a look at how hard/easy Locale::XGettext::TT2 can be
> > packaged (from 20.11 ideally)?
> 
> Should it be packaged ? It is a dev-only dependency, so it won't be used in
> a production environment.
> Still better to have it as a deb package, but not required IMO.

i've added a package for Locale::XGettext::TT2 to the koha-staging repo

fyi, Locale::XGettext::TT2 0.7 requires a new version of libintl-perl(1.30) to be built for each(16) debian/ubuntu release, so i've gone with 0.6 as a workaround


$ apt-cache policy  libtemplate-plugin-gettext-perl
libtemplate-plugin-gettext-perl:
  Installed: 0.6-1~koha1
  Candidate: 0.6-1~koha1
  Version table:
 *** 0.6-1~koha1 500
        500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages
        100 /var/lib/dpkg/status

$ dpkg -L  libtemplate-plugin-gettext-perl
/.
/usr
/usr/bin
/usr/bin/xgettext-tt2
/usr/share
/usr/share/doc
/usr/share/doc/libtemplate-plugin-gettext-perl
/usr/share/doc/libtemplate-plugin-gettext-perl/NEWS.gz
/usr/share/doc/libtemplate-plugin-gettext-perl/bugs-1.tt
/usr/share/doc/libtemplate-plugin-gettext-perl/bugs.t
/usr/share/doc/libtemplate-plugin-gettext-perl/changelog.Debian.gz
/usr/share/doc/libtemplate-plugin-gettext-perl/changelog.gz
/usr/share/doc/libtemplate-plugin-gettext-perl/copyright
/usr/share/man
/usr/share/man/man1
/usr/share/man/man1/xgettext-tt2.1p.gz
/usr/share/man/man3
/usr/share/man/man3/Locale::XGettext::TT2.3pm.gz
/usr/share/man/man3/Template::Plugin::Gettext.3pm.gz
/usr/share/perl5
/usr/share/perl5/Locale
/usr/share/perl5/Locale/XGettext
/usr/share/perl5/Locale/XGettext/TT2.pm
/usr/share/perl5/Locale/XGettext/TT2.pod
/usr/share/perl5/Template
/usr/share/perl5/Template/Plugin
/usr/share/perl5/Template/Plugin/Gettext.pm
/usr/share/perl5/Template/Plugin/Gettext.pod

(In reply to Julian Maurice from comment #85)
> I tried to go back to just before these patches to make
> xt/author/translatable-templates.t fail, unsuccessfully. I'm not sure what
> this script is supposed to test, because even with a completely broken
> template with invalid html and invalid TT markup, the tests pass.
> So it looks like the test is useless, and I suggest to remove it.

Same here, I decided to remove the file on bug 26971.

(In reply to Jonathan Druart from comment #81)
> Created attachment 113255 [details] [review] [review]
> Bug 25067: Adjust paths in tests

(In reply to Jonathan Druart from comment #83)
> Created attachment 113258 [details] [review] [review]
> Bug 25067: Fix perlcritic errors

Last 2 patches pushed to master for 20.11.

enhancement will not be backported to 20.05.x