Hi Jonathan,

I tested the proof of concept. It works as described and I think it is really the way to go.

To make it work I had to remove the column 'gstrate' from queries in C4/Koha.pm and ad in/itemtypes.pl (maybe I should have applied te patch on top of an other patch?)

One minor thing is the 'mapping' between the language and the translation. You do it with the main part of the language code, e.g. de for de-DE  or fr for fr-FR. Maybe it is better to do the mapping on the whole language code? - I think that there could be a need to differentiate between sublanguages, and I remeber that it was somehow an issue with the news.

Looking forward for the next steps...

I don't plan to go further without any feedbacks.
Please test.

(In reply to Jonathan Druart from comment #3)
> I don't plan to go further without any feedbacks.
> Please test.

Hi Jonathan, I tested the proof of concept. It works as described and I think it is really the way to go. (See comment #2)

Yes true, I would have said "I don't plan to go further without any *other* feedbacks" ;)

Hello Jonathan,

I tested the code as well. I had the same "problem" with itemtypes.gstrate, maybe the patch was built on the top of bug 12993?

Anyway, everything works as described. I agree with Marc about the language code.

Another useful improvement I can imagine is to show number of existed translations in the table on main itemtypes page. And I think that "translate" link could by placed straight in action column in main itemtypes table. It does make a bit more sense to me, because it's another mode not closely linked to standard item type editing.

What do you think?

I like this feature very much :)

I agree with Marc regarding the language code. (A good example for this would be zh_CN and zh_TW where the two variants don't use the same characters.)

Right now the tool is offering you a list of languages to choose from that doesn't match the templates you have installed. I think this will confuse users. (It certainly confused me.)

Jonathan explained me that his idea was to make the languages available even if they are not installed yet, so that you can translate and it will work immediately when you decide to install them.
In my experience users decide which translation they want at the very beginning of a project, and set everything up from there onwards. There should be a fallback mechanism to a translated string if none exists for the selected language though.

Long story short, i think we should display a list that fits the installed templates (like for news).

I agree with Josef's suggestion of displaying the translations directly in the table with the list of itemtypes. This will make it easy to spot what is missing a translated string. I doubt users will ever install more than a handful of translations, so it should remain manageable.
Maybe they could all be displayed directly in the description column like this:

Audio tape (en)
Cassette audio (fr-FR)
Audiokassette (de-DE)

The language corresponding to the currently selected language could be highlighted. And obviously on other pages where we don't have to manage the translation, we would only see the translation for the currently selected language.

Adding a translate link in a separate column doesn't look über-important to me, since i think this will be part of setting up itemtypes (and others) at the beginning, and not something we will keep coming back to. But i don't mind.

Here a version to sign off.

Changes done in the last patch:
1/ Remove the gstrate in C4/Koha.pm and admin/itemtypes.pl. Sorry about that.
2/ Display the translated languages only. The full language codes are displayed. fr-FR and fr-CA can be translated.
3/ Display the translated string at several places. There are certainly some missing, please find them.

What it's not done:
Change the link from the edit to the list page. Because I did not find an ergonomic way to do. I don't want to update the list view when a translation is added in ajax.

Looks good, the localization works as expected.

I found some pages, where the localization is not used: 

Intranet:
admin/smart-rules.pl
authorities/authorities.pl
catalogue/detail.pl
catalogue/itemsearch.pl
catalogue/moredetail.pl
reports/acquisitions_stats.pl
reports/bor_issues_top.pl
reports/cat_issues_top.pl
reports/catalogue_out.pl
reports/catalogue_stats.pl
reports/issues_avg_stats.pl
reports/issues_stats.pl
reports/itemslost.pl
reports/manager.pl
reports/reserves_stats.pl
suggestion/suggestion.pl
tools/export.pl

Opac:
opac-detail.pl
opac-MARCdetail.pl
opac-search.pl


It isn't propably the complete list, but I tried my best ;)

(In reply to Josef Moravec from comment #11)
> Looks good, the localization works as expected.
> 
> I found some pages, where the localization is not used: 
> 
> Intranet:
> admin/smart-rules.pl

Fixed.

> authorities/authorities.pl

Looks good here, please detail.

> catalogue/detail.pl
> catalogue/itemsearch.pl
> catalogue/moredetail.pl
> reports/acquisitions_stats.pl
> reports/bor_issues_top.pl
> reports/cat_issues_top.pl
> reports/catalogue_out.pl
> reports/catalogue_stats.pl
> reports/issues_avg_stats.pl
> reports/issues_stats.pl
> reports/itemslost.pl
> reports/manager.pl
> reports/reserves_stats.pl
> suggestion/suggestion.pl
> tools/export.pl

Fixed.

> Opac:
> opac-detail.pl
> opac-MARCdetail.pl
> opac-search.pl

Fixed.

> It isn't propably the complete list, but I tried my best ;)

Thanks for this list!

Thanks for the quick fix!

I tried authorities.pl again and it looks good for me too, propably my mistake. I'm going to test it once again and probably sign off, if there will be some other places with non-translated itemtypes we still could file new bug for specific occurences.

First patch does not apply:

CONFLICT (content): Merge conflict in admin/itemtypes.pl

Sorry, patch no longer applies, can you please fix?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14100: Generic solution for language overlay - Item types
Using index info to reconstruct a base tree...
M	C4/Items.pm
M	cataloguing/additem.pl
M	installer/data/mysql/kohastructure.sql
M	tools/batchMod.pl
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
	installer/data/mysql/atomicupdate/Bug_14100-add_table_localization.sql
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 14100: Generic solution for language overlay - Item types
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.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-14100-Generic-solution-for-language-overlay----vNN_6L.patch

conflict on kohastructure.sql fixed.

Argh... late evening QA'ing...
error: The following untracked working tree files would be overwritten by merge:
	installer/data/mysql/atomicupdate/Bug_14100-add_table_localization.sql
Sorry!

(In reply to Katrin Fischer from comment #24)
> Argh... late evening QA'ing...
> error: The following untracked working tree files would be overwritten by
> merge:
> 	installer/data/mysql/atomicupdate/Bug_14100-add_table_localization.sql
> Sorry!

You need remove the file before applying the patches.

Yep, I missed that my workspace wasn't clean. I will fix.

Working on this one now :)

Hi Joubu,

I started testing with the itemtypes administration page and the new modal for adding the translations. Some first findings:

1) I think "Translate for other languages" would work better as "Translate into other languages".
2) I added "Bücher" as the German translation for "Books" and the umlauts in the table with the translated strings are broken.
3) I accidentally added the "Bücher" string twice - should we allow multiple translations for the same lang and entity combination?
4) There should be a way to delete strings accidentally added, but I don't see one. There is an empty last cell in the last column, only when you open the modal a second time the X appears.
5) I think it would be good to switch the sequence and have the entry field above the table. 
6) It would be good to show the entity and code you are translating for in the window somewhere.

http://localhost:8080/cgi-bin/koha/tools/batchMod.pl and issues_avg_stats.pl
If you have multiple translations for a entity-code-language combination, they all show up in the pull down.
In other pull downs, like on the reserve_stats.pl only one translation shows.

(In reply to Katrin Fischer from comment #28)
> Hi Joubu,

Thanks Katrin for QAing this!

> I started testing with the itemtypes administration page and the new modal
> for adding the translations. Some first findings:
> 
> 1) I think "Translate for other languages" would work better as "Translate
> into other languages".

Fixed.

> 2) I added "Bücher" as the German translation for "Books" and the umlauts in
> the table with the translated strings are broken.

Oops, fixed!

> 3) I accidentally added the "Bücher" string twice - should we allow multiple
> translations for the same lang and entity combination?

I added a check on update/insert. You should now get a warning/error message.

> 4) There should be a way to delete strings accidentally added, but I don't
> see one. There is an empty last cell in the last column, only when you open
> the modal a second time the X appears.

It should yes, but... I would prefer to see this feature push as it and receive feedback: is it useful? how could we improve it? etc.
This will take me ages (because js...) and I don't think the added value is high. And there is a workaround...

> 5) I think it would be good to switch the sequence and have the entry field
> above the table. 
> 6) It would be good to show the entity and code you are translating for in
> the window somewhere.

Done in a separate patch.

Hi Jonathan,

thx for the changes. I continue with checking the pages and scripts changed, some more findings:

1) The pull down on reports/bor_issues_top.pl is empty apart from the 'any' entry.

2) There is something weird about sorting going on. Example: With the sample data, translate only MP = Maps to "Test" for another language. Go to the item search page - check the pull down: Test is sorted, as if it was still Maps. I think it would make more sense to sort always by the displayed description.

3) The intranet result list doesn't show the right description in the right column with the item information. It still lists 'Books' when it should be 'Bücher'

4) Same for the itemtype facet descriptions in staff and the OPAC - the new translations are not used. It would be good to at least solve this for the OPAC side.

5) Not working on OPAC advanced search.

6) Not working for opac-topissues.pl filters and result list.

Only opac page working seems to be the opac/detail.pl page? Lost patch?

7) The duplicate check only seems to work if entity - code - langauge - description are the same - but it should only be entity - code - langauge identical instead. Multiple German translations for the same itemtype make no sense :)

(In reply to Katrin Fischer from comment #37)
> Hi Jonathan,
> 
> thx for the changes. I continue with checking the pages and scripts changed,
> some more findings:
> 
> 1) The pull down on reports/bor_issues_top.pl is empty apart from the 'any'
> entry.

Will be fixed.

> 2) There is something weird about sorting going on. Example: With the sample
> data, translate only MP = Maps to "Test" for another language. Go to the
> item search page - check the pull down: Test is sorted, as if it was still
> Maps. I think it would make more sense to sort always by the displayed
> description.

Yes, sometimes it's ordered by 'itemtype' and sometimes by 'description'.
I have not changed the behavior when it's ordered by itemtype here, but it could be done on top of bug 14828.

> 3) The intranet result list doesn't show the right description in the right
> column with the item information. It still lists 'Books' when it should be
> 'Bücher'

I have not found this info (unimarc?) but I think I have fixed it.

> 4) Same for the itemtype facet descriptions in staff and the OPAC - the new
> translations are not used. It would be good to at least solve this for the
> OPAC side.

Same.

> 5) Not working on OPAC advanced search.

Fixed.

> 6) Not working for opac-topissues.pl filters and result list.

Fixed.

> Only opac page working seems to be the opac/detail.pl page? Lost patch?

I don't think so :)

(In reply to Katrin Fischer from comment #38)
> 7) The duplicate check only seems to work if entity - code - langauge -
> description are the same - but it should only be entity - code - langauge
> identical instead. Multiple German translations for the same itemtype make
> no sense :)

Yes! And I have added the unique key at the DB level.

*** Bug 14063 has been marked as a duplicate of this bug. ***

Hi Jonathan, 

this looks very good by now - I am optimistic that this is the last list:

Not working yet:
- opac-suggestions.pl
- opac-readingrecord.pl
- opac-reserve.pl
I think it would be really nice to cover the OPAC with the initial patch.

- suggestion/suggestion.pl
  Organize by itemtype - tab descriptions are not correct

I think I have checked all files touched by this bug - at least I tried really hard - everything else looked fine to me.

---

Could go on a separate bug to be fixed later:
- Allow editing of entries, instead of having to delete and readd them

Further testing notes - out of scope for this bug:
- circ/returns.pl
  Shows code of homebranch and hodingbranch
  Show code of itemtype, incorrect label: Type should be Itemtype

- suggestion/suggestion.pl
  Edit page: Pull down needs empty entry, when no itemtype was 
  selected in the OPAC on adding it. Otherwise first entry will 
  accidentally be saved on editing.
  Summary page: Show description instead of itemtype code

- Guided reports - Definitions
  Shows itemtype code instead of description in pull down

(In reply to Katrin Fischer from comment #44)
> Hi Jonathan, 
> 
> this looks very good by now - I am optimistic that this is the last list:
> 
> Not working yet:
> - opac-suggestions.pl
> - opac-readingrecord.pl
> - opac-reserve.pl
> I think it would be really nice to cover the OPAC with the initial patch.

Fixed.


> - suggestion/suggestion.pl
>   Organize by itemtype - tab descriptions are not correct

Not so easy, because of C4::Koha::GetSupportName
I would prefer to fix it later, on top of bug 14828.

> I think I have checked all files touched by this bug - at least I tried
> really hard - everything else looked fine to me.

Thanks a lot for QAing this!

> Could go on a separate bug to be fixed later:
> - Allow editing of entries, instead of having to delete and readd them

It's possible! It's not obvious but... you can click on the table to edit the lang or the translation.

*** Bug 7789 has been marked as a duplicate of this bug. ***

Created attachment 44085 [details] [review]
Bug 14100: Generic solution for language overlay - Item types

Test plan:
1/ update the Schema (misc/devel/update_dbix_class_files.pl)
2/ Translate templates for some languages (es-DE, de-DE for instance)
3/ Enable them in the pref (search for 'lang') for the staff interface
4/ Go on the item type admin page (admin/itemtypes.pl)
5/ Edit one
6/ Click on the 'translate for other languages' link
7/ You are now on the interface to translate the item type's description
in the languages you want. So translate some :)
8/ Go back on the item type list view (admin/itemtypes.pl)
9/ You should see the original description (non translated)
10/ Switch the language
11/ You should see the translated description in the correct language.
If the description is non translated, the original description is
displayed.

12/ On the different page where the item type is displayed, confirm that
the translated description appears.

Think further / Todo:
1/ Update all occurrences of the item type's description (DONE)
2/ Implement for authorised values
3/ Implement for syspref value (at least textarea)
4/ Implement for branch names
5/ Centralize all the translation on a single page in the admin area
...
N/ Implement a webservice to centralize all the translations and give
the ability to sync the item types/authorised values description with
the rest of the world (push and pull).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

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

Created attachment 44086 [details] [review]
Bug 14100: (follow-up) Language overlay for item types

Display the translated description for item types in the following pages:
> admin/smart-rules.pl
> catalogue/detail.pl
> catalogue/itemsearch.pl
> catalogue/moredetail.pl
> reports/acquisitions_stats.pl
> reports/bor_issues_top.pl
> reports/cat_issues_top.pl
> reports/catalogue_out.pl
> reports/catalogue_stats.pl
> reports/issues_avg_stats.pl
> reports/issues_stats.pl
> reports/itemslost.pl
> reports/manager.pl
> reports/reserves_stats.pl
> suggestion/suggestion.pl
> tools/export.pl

> Opac:
> opac-detail.pl
> opac-MARCdetail.pl
> opac-search.pl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

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

Created attachment 44087 [details] [review]
Bug 14100: Better errors handling

This patch mainly improves the errors handling.
It's now not possible to add several translations for the same entity-code-lang
combination.

It also:
- wording: Translate for other languages => into
- fixes encoding issues on add/update (was just display)
- add entity and code to the title of the pop-up window

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

Created attachment 44088 [details] [review]
Bug 14100: Move add form to the top

Not sure this is the best: now the messages will make the table moves.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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

Created attachment 44089 [details] [review]
Bug 14100: Fix some missing occurrences

This patch fixes:
- reports/bor_issues_top.pl
- sort order
- adv search and search results
- opac-topissues.pl

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

Created attachment 44090 [details] [review]
Bug 14100: Fix 3 occurrences more

- opac-suggestions.pl
- opac-readingrecord.pl
- opac-reserve.pl

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

Created attachment 44094 [details] [review]
Bug 14100: (QA followup) fix wrong comment in kohastructure.sql

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Nice feature! Patches pushed to master.

Thanks Jonathan!

Created attachment 44108 [details] [review]
Bug 14100: Fix test xt/author/valid_templates complaining on opac-topissues.tt

undef error - Undefined subroutine &C4::Languages::getlanguage called at C4/Koha.pm line 250.

Test plan:
    prove xt/author/valid-templates.t

Created attachment 44109 [details] [review]
Bug 14100: Fix t/db_dependent/Items.t

prove t/db_dependent/Items.t
Undefined subroutine &C4::Languages::getlanguage called at C4/Items.pm line 1304.

Created attachment 44119 [details] [review]
Bug 14100: Fix Search.t tests

So, this one is VERY weird, let me try to explain what I have
understood.

Bisecting using run prove t/db_dependent/Search.t, I have found that the
following commit make the test fail:
  commit 0f63f89f66e40cc01ef02da3654fcfb404c9001d
    Bug 14100: Generic solution for language overlay - Item types
The error is
  DBI bind_columns: invalid number of arguments: got handle + 0, expected handle + between 1 and -1
  Usage: $h->bind_columns(\$var1 [, \$var2, ...]) at /usr/lib/i386-linux-gnu/perl5/5.20/DBI.pm line 2065.

Note that the interface (admin/itemtypes.pl) which calls the same
subroutine with the same parameter (style => 'array') works great.

The problem comes from the change in C4::Search::searchResults, if I
only apply the change done to this subroutine on 0f63f89f^1, I reproduce
the issue.

Looking closely at how %itemtypes is built, we could actually call
GetItemTypes with the style => 'hash' to get exactly what we want.
The following piece prove it for you:
    use Test::More;
    use C4::Koha;
    my $i = GetItemTypes;
    my $j = GetItemTypes(style => 'array');
    my %itemtypes;
    for my $itemtype ( @$j ) {
        $itemtypes{ $itemtype->{itemtype} } = $itemtype;
    }
    is_deeply( \%itemtypes, $i);

So changing the code accordingly and just forget this last hour...

Created attachment 44151 [details] [review]
Bug 14100: (QA followup) add missing mocked sysprefs

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

updating summary for release notes

I am not sure if this is not confusing for the release notes - we only implemented itemtypes so far.

*** Bug 14064 has been marked as a duplicate of this bug. ***