Bug 24975

Summary: Refactor database translations
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: I18N/L10NAssignee: Bugs List <koha-bugs>
Status: NEW --- QA Contact:
Severity: enhancement    
Priority: P5 - low CC: bgkriegel, caroline.cyr-la-rose, dcook, f.demians, jonathan.druart, joonas.kylmala, katrin.fischer, lauren_denny, m.de.rooy, marjorie.barry-vila, martin.renvoize, patrick.robitaille, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20307
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35200
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 24977    
Attachments: Bug 24975: Refactor database translations
Bug 24975: Keep the translations, prevent duplicates, and other fixes
Bug 24975: Refactor database translations
Bug 24975: Keep the translations, prevent duplicates, and other fixes
Bug 24975: Refactor database translations
Bug 24975: Keep the translations, prevent duplicates, and other fixes
Bug 24975: Fix l10n_source definition in kohastructure.sql
Bug 24975: Fix syntax issues
Bug 24975: Fix bugs
Bug 24975: Refactor database translations
Bug 24975: Keep the translations, prevent duplicates, and other fixes
Bug 24975: Fix l10n_source definition in kohastructure.sql
Bug 24975: Fix syntax issues
Bug 24975: (follow-up) Rebase for bug 21946
Bug 24975: Use 'key' for translation lookup
Bug 24975: Re-introduce translated_description accessor method
Bug 24975: Add translation_key method example
Bug 24975: Use translation_key in preference to description
Bug 24975: Add to Koha::UI::Form::Builder::Item
Bug 24975: DBIC Build
Bug 24975: Fix bugs
Bug 24975: Pass transation_key in GetItemsInfo
Bug 24975: Refactor database translations
Bug 24975: Keep the translations, prevent duplicates, and other fixes
Bug 24975: Fix l10n_source definition in kohastructure.sql
Bug 24975: Fix syntax issues
Bug 24975: (follow-up) Rebase for bug 21946
Bug 24975: Use 'key' for translation lookup
Bug 24975: Re-introduce translated_description accessor method
Bug 24975: Add translation_key method example
Bug 24975: Use translation_key in preference to description
Bug 24975: Add to Koha::UI::Form::Builder::Item
Bug 24975: DBIC Build
Bug 24975: Fix bugs
Bug 24975: Pass transation_key in GetItemsInfo
Bug 24975: Fix bug
Bug 24975: Refactor database translations
Bug 24975: Keep the translations, prevent duplicates, and other fixes
Bug 24975: Fix l10n_source definition in kohastructure.sql
Bug 24975: Fix syntax issues
Bug 24975: (follow-up) Rebase for bug 21946
Bug 24975: Use 'key' for translation lookup
Bug 24975: Re-introduce translated_description accessor method
Bug 24975: Add translation_key method example
Bug 24975: Use translation_key in preference to description
Bug 24975: Add to Koha::UI::Form::Builder::Item
Bug 24975: Fix bugs
Bug 24975: Pass transation_key in GetItemsInfo
Bug 24975: DBIC Build
Bug 24975: Fix db_t
Bug 24975: Refactor database translations
Bug 24975: Keep the translations, prevent duplicates, and other fixes
Bug 24975: Fix l10n_source definition in kohastructure.sql
Bug 24975: Fix syntax issues
Bug 24975: (follow-up) Rebase for bug 21946
Bug 24975: Use 'key' for translation lookup
Bug 24975: Re-introduce translated_description accessor method
Bug 24975: Add translation_key method example
Bug 24975: Use translation_key in preference to description
Bug 24975: Add to Koha::UI::Form::Builder::Item
Bug 24975: Fix bugs
Bug 24975: DBIC Build
Bug 24975: Fix db_t
Bug 24975: Some more fixes
Bug 24975: (follow-up) Fix dbrev
Bug 24975: WIP remove db_t from controllers
Bug 24975: Simplify and uniform calls to get the translation
Bug 24975: to squash - fix search.pl
Bug 24975: to squash - fix search.pl

Description Julian Maurice 2020-03-25 13:22:35 UTC
This is an attempt to make the process of translating database strings (like itemtype descriptions) easier for developers and translators.
Comment 1 Julian Maurice 2020-03-25 13:24:53 UTC
Created attachment 101703 [details] [review]
Bug 24975: Refactor database translations

This is an attempt to make the process of translating database strings
(like itemtype descriptions) easier for developers and translators.

For developers by reducing the amount of code needed to enable
translations for a new kind of object (authorised values, letters, news,
...).
For translators by providing a single entry point for all translatable
strings

Table `localization` is replaced by two new tables `l10n_source` and
`l10n_target`. `l10n_source` contains the original text while
`l10n_target` contains the corresponding translations

`l10n_source` is automatically updated whenever an itemtype is
inserted, updated, or deleted, so it is always up to date (this is done
in Koha::Schema::Result::ItemType).
It means that if you delete an itemtype, translations get deleted too.
It also means that if you change an itemtype's description, translations
are lost (not sure if this should be fixed, since if description
changes, translations should probably be changed too)

It adds a new subroutine in Koha::I18N, `db_t`, which returns the
translation for a given context and string, according to the current
language. Translations are stored in memcached (if enabled).

So instead of having to declare a new subroutine
Koha::ItemType::translated_description, you can write
    db_t('itemtype', $itemtype->description)

In templates you can write
    [% ItemTypes.t(itemtype.description) %]

There's also a new page, accessible from the administration main page,
"Localization". It displays all translatable strings and allow to
translate them for all enabled languages.

Test plan:
1. Before applying the patch, make sure you have translated some
   itemtypes. Translations should be kept by the updatedatabase script.
   Before applying the updatedatabase you may want to comment the
   'DROP TABLE localization' statement if you want to keep this table
2. Apply patch, run updatedatabase.pl, run update_dbix_class_files.pl
   and reload starman
3. Go to Admin » Localization, verify that you have your itemtypes and
   their translations.
4. Verify that the filter is working (it filters on the source column
   only).
5. Translate some strings by clicking on one of the 'Translate' link,
   entering some text and clicking on the 'Save' button. You can remove
   a translation by emptying the text box.
6. Add a new item type. Verify that it appears in the Localization page
7. Change the description of this item type. Verify that the new
   description appears in Localization page and that the old description
   is gone
8. Delete this item type and verify that its description no longer
   appears in Localization page
9. Navigate through Koha (intranet and opac) in multiple languages and
   verify that item types are translated (if not, please verify in
   master too, not all occurrences were translated)
Comment 2 Jonathan Druart 2020-03-25 13:32:01 UTC
That looks like a great idea.
What about the interface (for AVs)?
Comment 3 Julian Maurice 2020-03-26 08:28:08 UTC
Created attachment 101786 [details] [review]
Bug 24975: Keep the translations, prevent duplicates, and other fixes

Translations are no longer lost when you change an itemtype's
description, thanks to a new `l10n_source.key` column which must be
unique within it's "text group" ('itemtype' is a text group)
This also prevent duplicate entries in l10n_source

Columns were renamed to better match their purpose:
* context -> group
* source -> text

Use Koha::Cache::Memory::Lite if other caching systems are not available

Prepend 'l10n:' to the cache key to avoid conflicts

Test plan:
1. DROP TABLE IF EXISTS l10n_target, l10n_source
2. Follow the test plan from previous patch
Comment 4 Martin Renvoize 2020-03-26 12:10:57 UTC
Thanks for picking this up Julian, it's looking great so far! :)
Comment 5 Martin Renvoize 2020-04-03 13:52:33 UTC
Created attachment 102387 [details] [review]
Bug 24975: Refactor database translations

This is an attempt to make the process of translating database strings
(like itemtype descriptions) easier for developers and translators.

For developers by reducing the amount of code needed to enable
translations for a new kind of object (authorised values, letters, news,
...).
For translators by providing a single entry point for all translatable
strings

Table `localization` is replaced by two new tables `l10n_source` and
`l10n_target`. `l10n_source` contains the original text while
`l10n_target` contains the corresponding translations

`l10n_source` is automatically updated whenever an itemtype is
inserted, updated, or deleted, so it is always up to date (this is done
in Koha::Schema::Result::ItemType).
It means that if you delete an itemtype, translations get deleted too.
It also means that if you change an itemtype's description, translations
are lost (not sure if this should be fixed, since if description
changes, translations should probably be changed too)

It adds a new subroutine in Koha::I18N, `db_t`, which returns the
translation for a given context and string, according to the current
language. Translations are stored in memcached (if enabled).

So instead of having to declare a new subroutine
Koha::ItemType::translated_description, you can write
    db_t('itemtype', $itemtype->description)

In templates you can write
    [% ItemTypes.t(itemtype.description) %]

There's also a new page, accessible from the administration main page,
"Localization". It displays all translatable strings and allow to
translate them for all enabled languages.

Test plan:
1. Before applying the patch, make sure you have translated some
   itemtypes. Translations should be kept by the updatedatabase script.
   Before applying the updatedatabase you may want to comment the
   'DROP TABLE localization' statement if you want to keep this table
2. Apply patch, run updatedatabase.pl, run update_dbix_class_files.pl
   and reload starman
3. Go to Admin » Localization, verify that you have your itemtypes and
   their translations.
4. Verify that the filter is working (it filters on the source column
   only).
5. Translate some strings by clicking on one of the 'Translate' link,
   entering some text and clicking on the 'Save' button. You can remove
   a translation by emptying the text box.
6. Add a new item type. Verify that it appears in the Localization page
7. Change the description of this item type. Verify that the new
   description appears in Localization page and that the old description
   is gone
8. Delete this item type and verify that its description no longer
   appears in Localization page
9. Navigate through Koha (intranet and opac) in multiple languages and
   verify that item types are translated (if not, please verify in
   master too, not all occurrences were translated)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-04-03 13:52:36 UTC
Created attachment 102388 [details] [review]
Bug 24975: Keep the translations, prevent duplicates, and other fixes

Translations are no longer lost when you change an itemtype's
description, thanks to a new `l10n_source.key` column which must be
unique within it's "text group" ('itemtype' is a text group)
This also prevent duplicate entries in l10n_source

Columns were renamed to better match their purpose:
* context -> group
* source -> text

Use Koha::Cache::Memory::Lite if other caching systems are not available

Prepend 'l10n:' to the cache key to avoid conflicts

Test plan:
1. DROP TABLE IF EXISTS l10n_target, l10n_source
2. Follow the test plan from previous patch

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-04-03 14:02:23 UTC
I like this, and it's working pretty well in testing.. few comments however

1/ The 'Localization' option appears in the admin home screen whether or not you have any translations installed.. if you then navigate into it you get an ajax request that fails for the datatable if there are no translations available.
2/ You switch from Koha::Object classes to plain DBIx ResultSets.. was that deliberate and if so can you explain why (I'm betting it was a performance perspective?)
Comment 8 Martin Renvoize 2020-04-03 14:44:19 UTC
(In reply to Martin Renvoize from comment #7)
> I like this, and it's working pretty well in testing.. few comments however
> 
> 1/ The 'Localization' option appears in the admin home screen whether or not
> you have any translations installed.. if you then navigate into it you get
> an ajax request that fails for the datatable if there are no translations
> available.
> 2/ You switch from Koha::Object classes to plain DBIx ResultSets.. was that
> deliberate and if so can you explain why (I'm betting it was a performance
> perspective?)

Both points answered on IRC.. 1/ was a mistake my end.. forgot to rebuild the schema's after applying the patches on second test run.

Signing off.
Comment 9 Julian Maurice 2020-04-03 14:46:55 UTC
(In reply to Martin Renvoize from comment #7)
> 2/ You switch from Koha::Object classes to plain DBIx ResultSets.. was that
> deliberate and if so can you explain why (I'm betting it was a performance
> perspective?)

(pasting my answer on IRC here for reference:)
I had performance in mind (but i did not measure it), but also, some parts of Koha use DBIx::Class directly, so by putting the code at DBIX::Class level it should work whether you use dbix::class or koha::objects.

Thanks for testing :)
Comment 10 Jonathan Druart 2020-04-07 12:37:13 UTC
Created attachment 102513 [details] [review]
Bug 24975: Refactor database translations

This is an attempt to make the process of translating database strings
(like itemtype descriptions) easier for developers and translators.

For developers by reducing the amount of code needed to enable
translations for a new kind of object (authorised values, letters, news,
...).
For translators by providing a single entry point for all translatable
strings

Table `localization` is replaced by two new tables `l10n_source` and
`l10n_target`. `l10n_source` contains the original text while
`l10n_target` contains the corresponding translations

`l10n_source` is automatically updated whenever an itemtype is
inserted, updated, or deleted, so it is always up to date (this is done
in Koha::Schema::Result::ItemType).
It means that if you delete an itemtype, translations get deleted too.
It also means that if you change an itemtype's description, translations
are lost (not sure if this should be fixed, since if description
changes, translations should probably be changed too)

It adds a new subroutine in Koha::I18N, `db_t`, which returns the
translation for a given context and string, according to the current
language. Translations are stored in memcached (if enabled).

So instead of having to declare a new subroutine
Koha::ItemType::translated_description, you can write
    db_t('itemtype', $itemtype->description)

In templates you can write
    [% ItemTypes.t(itemtype.description) %]

There's also a new page, accessible from the administration main page,
"Localization". It displays all translatable strings and allow to
translate them for all enabled languages.

Test plan:
1. Before applying the patch, make sure you have translated some
   itemtypes. Translations should be kept by the updatedatabase script.
   Before applying the updatedatabase you may want to comment the
   'DROP TABLE localization' statement if you want to keep this table
2. Apply patch, run updatedatabase.pl, run update_dbix_class_files.pl
   and reload starman
3. Go to Admin » Localization, verify that you have your itemtypes and
   their translations.
4. Verify that the filter is working (it filters on the source column
   only).
5. Translate some strings by clicking on one of the 'Translate' link,
   entering some text and clicking on the 'Save' button. You can remove
   a translation by emptying the text box.
6. Add a new item type. Verify that it appears in the Localization page
7. Change the description of this item type. Verify that the new
   description appears in Localization page and that the old description
   is gone
8. Delete this item type and verify that its description no longer
   appears in Localization page
9. Navigate through Koha (intranet and opac) in multiple languages and
   verify that item types are translated (if not, please verify in
   master too, not all occurrences were translated)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Jonathan Druart 2020-04-07 12:37:16 UTC
Created attachment 102514 [details] [review]
Bug 24975: Keep the translations, prevent duplicates, and other fixes

Translations are no longer lost when you change an itemtype's
description, thanks to a new `l10n_source.key` column which must be
unique within it's "text group" ('itemtype' is a text group)
This also prevent duplicate entries in l10n_source

Columns were renamed to better match their purpose:
* context -> group
* source -> text

Use Koha::Cache::Memory::Lite if other caching systems are not available

Prepend 'l10n:' to the cache key to avoid conflicts

Test plan:
1. DROP TABLE IF EXISTS l10n_target, l10n_source
2. Follow the test plan from previous patch

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Jonathan Druart 2020-04-07 12:38:43 UTC
It does not work for me. The table does not load on admin/localization.pl

DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'me.context' in 'field list' at /kohadevbox/koha/svc/localization line 35

I rebased the patches because of conflicts with bug 4944.
Comment 13 Julian Maurice 2020-04-10 08:56:00 UTC
(In reply to Jonathan Druart from comment #12)
> It does not work for me. The table does not load on admin/localization.pl
> 
> DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'me.context' in
> 'field list' at /kohadevbox/koha/svc/localization line 35

You should probably recreate the DBIx schema files, 'context' has been renamed 'group' in the last patch.
Comment 14 Julian Maurice 2020-04-10 09:32:18 UTC
Created attachment 102688 [details] [review]
Bug 24975: Fix l10n_source definition in kohastructure.sql
Comment 15 Jonathan Druart 2020-04-10 11:40:11 UTC
Created attachment 102701 [details] [review]
Bug 24975: Fix syntax issues
Comment 16 Jonathan Druart 2020-04-10 12:33:46 UTC
I've talked a bit with Julian, and there is something that is not correct IMO.

The translation for the item type "BK/Books" is picked using its description, "Books". Which means that if we have another itemtype with the same description "BK2/Books", its translation string will be used for BK.

For instance:
 BK/Books translated in fr-FR => "Livre"
 BK2/Books translated in fr-FR => "Autre livre"

Item type BK will be display "Autre livre" in French.

I should also note that the collation for the 2 new tables is utf8mb4_bin (instead of the usual utf8mb4_unicode_ci), to differentiate "Book" and "book".
But I have tried:
 BK/Books translated in fr-FR => "Livre"
 BK2/books translated in fr-FR => "Autre livre"
An I still see "Autre livre" for "BK/Books".

I know it does not make a lot a sense in a real-life situation, but I think we hit a design issue. This problem might be highlighted even more for AV, when we will need to take into account the "interface".

I think we should rethink this a bit and have (add back) a method at Koha::ItemType level to retrieve the translated string. That will give us more flexibility for future changes (now we need to update all the occurrences of db_t).

I still think this implementation is better than the original one, but it still need a bit of tuning to be ready IMO.


Failing QA/In Discussion.
Comment 17 Julian Maurice 2020-04-10 14:51:20 UTC
> I still think this implementation is better than the original one, but
> it still need a bit of tuning to be ready IMO.

I think there is no perfect solution, but maybe the best solution, and
the simplest one, is to go back to using the code of itemtype/AV,
instead of the description, like you suggested.
With codes there will be no duplicates, and no collation issues.
Comment 18 Martin Renvoize 2020-04-16 07:48:26 UTC
Are you hoping to continue here Julian.. it's looking so promising :)
Comment 19 Julian Maurice 2020-04-16 10:07:08 UTC
(In reply to Martin Renvoize from comment #18)
> Are you hoping to continue here Julian.. it's looking so promising :)

Yes. I don't know when but I will continue
Comment 20 Julian Maurice 2020-09-04 13:10:08 UTC
I've been rethinking about this.

If we fetch a translation using a key like "itemtype-BK", or "authorisedvalue-CCODE-FIC-OPAC" it might work well for these two type of entities, because there would be a 1:1 relationship between every string and its translation, and there should not be too much duplicated translations.
But what if we want to make MARC frameworks translatable too ? Say I have 10 frameworks that share 95% of their field/subfield descriptions, I'll have to translate each of these strings 10 times, because there would be a translation for "framework-FW1-245a-OPAC" and another for "framework-FW2-245a-OPAC" and so on. Waste of time and DB space.

So I think it would be better to fetch a translation using the text, for instance "framework-Title". That way every field/subfield description in a MARC framework that is "Title" would be translated the same way and the translator would need to translate it only once. Bonus: if you create a new framework it will already be translated.

Now there is still a design issue with the current implementation (comment 16) where Koha allows to give multiple translations to the same original text, but only one of those will be picked and the others will never show up. I think it can be solved by using a different db schema, but before going into more details I would like to hear what people think about the "key vs text" topic.

By the way, I think the collation issue in comment 16 was in fact a cache issue. The cache is not always cleared when it should be, and I tested the exact same example and it worked for me.
Comment 21 Martin Renvoize 2020-09-04 16:25:25 UTC
(In reply to Julian Maurice from comment #20)
> I've been rethinking about this.
> 
> If we fetch a translation using a key like "itemtype-BK", or
> "authorisedvalue-CCODE-FIC-OPAC" it might work well for these two type of
> entities, because there would be a 1:1 relationship between every string and
> its translation, and there should not be too much duplicated translations.
> But what if we want to make MARC frameworks translatable too ? Say I have 10
> frameworks that share 95% of their field/subfield descriptions, I'll have to
> translate each of these strings 10 times, because there would be a
> translation for "framework-FW1-245a-OPAC" and another for
> "framework-FW2-245a-OPAC" and so on. Waste of time and DB space.
> 
> So I think it would be better to fetch a translation using the text, for
> instance "framework-Title". That way every field/subfield description in a
> MARC framework that is "Title" would be translated the same way and the
> translator would need to translate it only once. Bonus: if you create a new
> framework it will already be translated.
> 
> Now there is still a design issue with the current implementation (comment
> 16) where Koha allows to give multiple translations to the same original
> text, but only one of those will be picked and the others will never show
> up. I think it can be solved by using a different db schema, but before
> going into more details I would like to hear what people think about the
> "key vs text" topic.
> 
> By the way, I think the collation issue in comment 16 was in fact a cache
> issue. The cache is not always cleared when it should be, and I tested the
> exact same example and it worked for me.

I'm confused.. you seem to suggest shifting from code based keys to description based keys and then revert again in the next paragraph.

I feel like we should be using code based keys for flexability/reliability but allow for a fallback chain for cases like frameworks..  so for example 'framework-FW2-245a-OPAC' could fallback to a 'framework-*-245a-OPAC' for a case where the FW2 specific case doesn't exist?

I need to look at the code again to really get to grips with that suggestion so please feel free to cast it aside if it's non-sense.
Comment 22 Julian Maurice 2020-09-04 16:35:22 UTC
(In reply to Martin Renvoize from comment #21)
> (In reply to Julian Maurice from comment #20)
> > I've been rethinking about this.
> > 
> > If we fetch a translation using a key like "itemtype-BK", or
> > "authorisedvalue-CCODE-FIC-OPAC" it might work well for these two type of
> > entities, because there would be a 1:1 relationship between every string and
> > its translation, and there should not be too much duplicated translations.
> > But what if we want to make MARC frameworks translatable too ? Say I have 10
> > frameworks that share 95% of their field/subfield descriptions, I'll have to
> > translate each of these strings 10 times, because there would be a
> > translation for "framework-FW1-245a-OPAC" and another for
> > "framework-FW2-245a-OPAC" and so on. Waste of time and DB space.
> > 
> > So I think it would be better to fetch a translation using the text, for
> > instance "framework-Title". That way every field/subfield description in a
> > MARC framework that is "Title" would be translated the same way and the
> > translator would need to translate it only once. Bonus: if you create a new
> > framework it will already be translated.
> > 
> > Now there is still a design issue with the current implementation (comment
> > 16) where Koha allows to give multiple translations to the same original
> > text, but only one of those will be picked and the others will never show
> > up. I think it can be solved by using a different db schema, but before
> > going into more details I would like to hear what people think about the
> > "key vs text" topic.
> > 
> > By the way, I think the collation issue in comment 16 was in fact a cache
> > issue. The cache is not always cleared when it should be, and I tested the
> > exact same example and it worked for me.
> 
> I'm confused.. you seem to suggest shifting from code based keys to
> description based keys and then revert again in the next paragraph.
Sorry for the confusion... where did I lost you exactly ? :)
Just to be clear, I suggest description based keys.

> I feel like we should be using code based keys for flexability/reliability
> but allow for a fallback chain for cases like frameworks..  so for example
> 'framework-FW2-245a-OPAC' could fallback to a 'framework-*-245a-OPAC' for a
> case where the FW2 specific case doesn't exist?
But what if the description of 245a for FW2 is different from the 245a of default framework ? Won't we have a wrong translation ?
Comment 23 Martin Renvoize 2020-09-14 13:18:07 UTC
I'm coming back around to keying on the original text now rather than a definitive unique key.

However, I think we should allow for the key to be passed in additional and perhaps other variables too eventually.

My thinking behind this is all the various options we have now for plural forms and the likes in the rest of the translation system. It's those cases I could see requiring additional options to refine which version of a translation to 'pick'.

So.. my thinking is along the lines

db_t($itype->itemtype, $itype->description, { key => $itype->code, count => $x });

Where the first two params are required, but the hashref or additional arguments is optional depending on the situation.

This way, we can key on the description text and allow for passing the code as an option to allow for more specific selection it it exists but fallback to just description as the key if it does not. Something similar to the way we allow falling back through rules in the circ rules system.. if the more specific case doesn't exist we trap the less specific case.

I'll have a little play with the code but I think this could end up flexible enough to support future expansions and allow for more and less specific cases in translation as required.
Comment 24 Jonathan Druart 2020-09-28 09:18:13 UTC
Katrin, Bernardo, can you give us your feeling on this topic please?
Comment 25 Martin Renvoize 2020-09-28 16:57:21 UTC
Code speaks more than words.. I've been having a play with a couple of followups to use a 'key' rather than rely on the string as the key for lookup, adding 'fuzzy' support and re-introducing an accessor method on the Koha::ItemType object:

https://gitlab.com/mrenvoize/Koha/-/commits/bug_24975

Still a work in progress but would love some feedback as to whether this might be a route to adopt.
Comment 26 Julian Maurice 2020-09-29 07:25:37 UTC
(In reply to Martin Renvoize from comment #25)
> Code speaks more than words.. I've been having a play with a couple of
> followups to use a 'key' rather than rely on the string as the key for
> lookup, adding 'fuzzy' support and re-introducing an accessor method on the
> Koha::ItemType object:
> 
> https://gitlab.com/mrenvoize/Koha/-/commits/bug_24975
> 
> Still a work in progress but would love some feedback as to whether this
> might be a route to adopt.

Does it solve the problem we mentioned with framework translations ?
Comment 27 Martin Renvoize 2020-09-29 07:36:05 UTC
We key on new 'key' field. So it leaves it up to the person whome implements the next translatable feature to pick an appropriate key.

For frameworks I would suggest a group of 'framework' and a key of 'marc field'.. thus allowing to translate at the default framework level but keep marc fields distinct for the cases as mentioned by Jonas on IRC.
Comment 28 Martin Renvoize 2020-09-29 08:03:39 UTC
What I'm also tempted to do is add a standardly named accessor be to objects with translatable fields.. ->translation_key which would produce the right key from the internal fields behind the scenes. So for itemtype it simply passes through itemtype, but for a it might pass $code_$interface for example.
Comment 29 Julian Maurice 2020-09-29 08:15:15 UTC
(In reply to Martin Renvoize from comment #27)
> We key on new 'key' field. So it leaves it up to the person whome implements
> the next translatable feature to pick an appropriate key.
> 
> For frameworks I would suggest a group of 'framework' and a key of 'marc
> field'.. thus allowing to translate at the default framework level but keep
> marc fields distinct for the cases as mentioned by Jonas on IRC.

I'm confused. Do you mean something like
  db_t('framework', 'marc field', { marc_field => '245a', type => 'opac' })
or
  db_t('framework', '245a-opac')
or
  db_t('framework-FW1', '245a-opac')
or something else ?

I could not find what Jonas mentioned on IRC, that probably doesn't help me to understand :) Can you paste it or summarize it here ?
Comment 30 Martin Renvoize 2020-09-29 08:41:29 UTC
(In reply to Julian Maurice from comment #29)
> (In reply to Martin Renvoize from comment #27)
> > We key on new 'key' field. So it leaves it up to the person whome implements
> > the next translatable feature to pick an appropriate key.
> > 
> > For frameworks I would suggest a group of 'framework' and a key of 'marc
> > field'.. thus allowing to translate at the default framework level but keep
> > marc fields distinct for the cases as mentioned by Jonas on IRC.
> 
> I'm confused. Do you mean something like
>   db_t('framework', 'marc field', { marc_field => '245a', type => 'opac' })
> or
>   db_t('framework', '245a-opac')
> or
>   db_t('framework-FW1', '245a-opac')
> or something else ?
> 
> I could not find what Jonas mentioned on IRC, that probably doesn't help me
> to understand :) Can you paste it or summarize it here ?

db_t('framework', '245a');

or if you need it at an interface level too

db_t('framework', '245a-opac');

i.e. We leave defining the granularity of the key to the developer adding the translation option to the component and we discus that granularity there.

What I've not done is any form of fallback system.. i.e. if we wanted to allow one to optionally define translations at a more granular level, for per framework for example.

example

db_t('framework', 'FA-245a');.. fall back to just '245a' if the 'FA-' translation doesn't exist.  I felt that was out of scope for here.. and perhaps not really required.

I'll go dig out the IRC log.
Comment 31 Martin Renvoize 2020-09-29 08:45:29 UTC
>kohaputti
maybe 013e and 583l could use different translations in finnish, 583l = "Kunto" and 013l = "Tila"
I mean you could translate Kunto also as Tila I guess
so not the best example but someone could do it like this...
>kohaputti
a safe bet here would be frameworks+field+subfield, the meaning cannot change between different frameworks, can it? Also, why even allow changing the descriptions for each framework separately
Comment 32 Martin Renvoize 2020-09-29 08:46:14 UTC
To add detail to that.. it's two different translations for the single word 'Status' in English for the Finnish translation.
Comment 33 Julian Maurice 2020-09-29 08:50:37 UTC
(In reply to Martin Renvoize from comment #30)
> db_t('framework', 'FA-245a');.. fall back to just '245a' if the 'FA-'
> translation doesn't exist.  I felt that was out of scope for here.. and
> perhaps not really required.

The problem with that is that we don't know if we should fall back. 245a may have a completely different description in FA than in default framework. (Well maybe not for 245a, but you get the point)
Comment 34 Martin Renvoize 2020-09-29 08:55:00 UTC
My point is.. I've tried to make it flexible enough without introducing too much additional overhead/code complexity here. I suggest we should not second guess the level of translation requirements/granularity of different components here but rather defer that to each bug introducing additional translation options.
Comment 35 Julian Maurice 2020-09-29 11:53:16 UTC
(In reply to Martin Renvoize from comment #28)
> What I'm also tempted to do is add a standardly named accessor be to objects
> with translatable fields.. ->translation_key which would produce the right
> key from the internal fields behind the scenes. So for itemtype it simply
> passes through itemtype, but for a it might pass $code_$interface for
> example.

That means Koha::Object(s) should be aware of the interface (opac/staff) ? :/

(In reply to Martin Renvoize from comment #34)
> My point is.. I've tried to make it flexible enough without introducing too
> much additional overhead/code complexity here. I suggest we should not
> second guess the level of translation requirements/granularity of different
> components here but rather defer that to each bug introducing additional
> translation options.
We have the same goal, just different opinions on how to achieve that. If we do it right from the beginning, making more kind of objects translatable will be easier. I think it's worth taking some time to think about it.

By using codes as translation key, it can work for MARC frameworks too, but it will require to translate the same strings multiple times. To avoid that, we would need to implement some kind of fallback mechanism.
By using descriptions as translation key, there is no need for fallback. Every string have to be translated only once. However it doesn't allow to translate the same string in different ways (like in comment 31). Doing that will also require a fallback mechanism.

So, do we want to use a code as key (eg. "245a") and refine with other codes ("FW2"),
or do we want to use a description as key (eg. "Title") and refine with other codes ("FW2", "245a") ?
I'd say the fallback for a description-based key is more likely to be accurate ("Kunto" is not very different in meaning from "Tila"), whereas a fallback for a code-based key can be simply wrong if descriptions are different.

Now that I think about it, this refinement system could also include the group ("itemtype", "framework", ...) so we'd have to translate "Title" only once, and it would be translated everywhere the same way, except if one add a different translation for [framework, FW2, 245a] for instance.

That being said, I recognize that frameworks are a particular case, and it may not be worth it to add all these translation features just for that. But having a separate translation system for frameworks is not ideal either... It would be simpler if all frameworks shared their descriptions :)

BTW, I really like the idea of "fuzzy" you've added
Comment 36 Jonathan Druart 2020-11-11 08:17:47 UTC
Could we get a work team on this one and target 21.05?
Comment 37 Katrin Fischer 2020-11-11 21:31:46 UTC
Also very interested in this, can provide testing and QA. A quick discussion on how to approach this would be nice!
Comment 38 Marcel de Rooy 2022-03-09 14:46:58 UTC
Any news here ?
Comment 39 Martin Renvoize 2022-03-28 14:07:03 UTC
I think my branch contained the latest in progress work.. but it needs a non-trivial rebase.

My hope is to catc Jonathan and Julian at Hackfest to work through the final details... it feels so close!
Comment 40 Jonathan Druart 2022-03-29 08:55:22 UTC
Created attachment 132388 [details] [review]
Bug 24975: Fix bugs
Comment 41 Jonathan Druart 2022-03-29 08:57:13 UTC
This patch fixes 3 500 errors, to apply on top of Martin's remote branch.

I translated Books to "Livres" in fr-FR, install templates for fr-FR but there is nothing displayed. At least on the item table of the catalogue/detail.pl page.
Comment 42 Martin Renvoize 2022-03-30 06:37:33 UTC Comment hidden (obsolete)
Comment 43 Martin Renvoize 2022-03-30 06:37:40 UTC Comment hidden (obsolete)
Comment 44 Martin Renvoize 2022-03-30 06:37:45 UTC Comment hidden (obsolete)
Comment 45 Martin Renvoize 2022-03-30 06:37:51 UTC Comment hidden (obsolete)
Comment 46 Martin Renvoize 2022-03-30 06:37:56 UTC Comment hidden (obsolete)
Comment 47 Martin Renvoize 2022-03-30 06:38:02 UTC Comment hidden (obsolete)
Comment 48 Martin Renvoize 2022-03-30 06:38:08 UTC Comment hidden (obsolete)
Comment 49 Martin Renvoize 2022-03-30 06:38:13 UTC Comment hidden (obsolete)
Comment 50 Martin Renvoize 2022-03-30 06:38:19 UTC Comment hidden (obsolete)
Comment 51 Martin Renvoize 2022-03-30 06:38:25 UTC Comment hidden (obsolete)
Comment 52 Martin Renvoize 2022-03-30 06:38:30 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize 2022-03-30 06:38:35 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize 2022-03-30 06:38:40 UTC Comment hidden (obsolete)
Comment 55 Martin Renvoize 2022-03-30 06:43:07 UTC Comment hidden (obsolete)
Comment 56 Martin Renvoize 2022-03-30 06:43:12 UTC Comment hidden (obsolete)
Comment 57 Martin Renvoize 2022-03-30 06:43:17 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize 2022-03-30 06:43:22 UTC Comment hidden (obsolete)
Comment 59 Martin Renvoize 2022-03-30 06:43:27 UTC Comment hidden (obsolete)
Comment 60 Martin Renvoize 2022-03-30 06:43:33 UTC Comment hidden (obsolete)
Comment 61 Martin Renvoize 2022-03-30 06:43:38 UTC Comment hidden (obsolete)
Comment 62 Martin Renvoize 2022-03-30 06:43:43 UTC Comment hidden (obsolete)
Comment 63 Martin Renvoize 2022-03-30 06:43:48 UTC Comment hidden (obsolete)
Comment 64 Martin Renvoize 2022-03-30 06:43:53 UTC Comment hidden (obsolete)
Comment 65 Martin Renvoize 2022-03-30 06:43:59 UTC Comment hidden (obsolete)
Comment 66 Martin Renvoize 2022-03-30 06:44:04 UTC Comment hidden (obsolete)
Comment 67 Martin Renvoize 2022-03-30 06:44:09 UTC Comment hidden (obsolete)
Comment 68 Martin Renvoize 2022-03-30 06:50:55 UTC
Right... that's my branch rebased and a few bugs fixed and all uploaded to here.

This includes the work I did on translation_key as an example.. i'm envisaging something along the lines of below for AV's as another example.

sub translation_key {
    return $self->code . ":" . C4::Context->interface;
}

In the itemtypes case I've cheated slighty with translation key stuff.. as it is just retunring itemtype behind the scenes I didn't go ahead and expose it more widely in C4::Search for example.

Still need to expose the 'fuzzy' flagging in the UI... but that should be trivial to do.

I think the 'code' vs 'description' issues may have gone away with the frameworks having been moved into pootle translations now.

Hopefully this gives us a point to delve back in.
Comment 69 Martin Renvoize 2022-03-30 09:35:22 UTC Comment hidden (obsolete)
Comment 70 Martin Renvoize 2022-03-30 11:58:04 UTC Comment hidden (obsolete)
Comment 71 Martin Renvoize 2022-03-30 11:58:10 UTC Comment hidden (obsolete)
Comment 72 Martin Renvoize 2022-03-30 11:58:15 UTC Comment hidden (obsolete)
Comment 73 Martin Renvoize 2022-03-30 11:58:20 UTC Comment hidden (obsolete)
Comment 74 Martin Renvoize 2022-03-30 11:58:26 UTC Comment hidden (obsolete)
Comment 75 Martin Renvoize 2022-03-30 11:58:32 UTC Comment hidden (obsolete)
Comment 76 Martin Renvoize 2022-03-30 11:58:37 UTC Comment hidden (obsolete)
Comment 77 Martin Renvoize 2022-03-30 11:58:43 UTC Comment hidden (obsolete)
Comment 78 Martin Renvoize 2022-03-30 11:58:48 UTC Comment hidden (obsolete)
Comment 79 Martin Renvoize 2022-03-30 11:58:53 UTC Comment hidden (obsolete)
Comment 80 Martin Renvoize 2022-03-30 11:58:58 UTC Comment hidden (obsolete)
Comment 81 Martin Renvoize 2022-03-30 11:59:04 UTC Comment hidden (obsolete)
Comment 82 Martin Renvoize 2022-03-30 11:59:09 UTC Comment hidden (obsolete)
Comment 83 Martin Renvoize 2022-03-30 11:59:15 UTC Comment hidden (obsolete)
Comment 84 Martin Renvoize 2022-03-30 12:02:32 UTC
Ready for testing..

Code wise, we should merge lots of these commits.. they're messy right now but show the thought process as they are.

Testing wise, ensure the itemtype translations continue to all work as expected.
Comment 85 Joonas Kylmälä 2022-08-28 17:33:35 UTC
(In reply to Martin Renvoize from comment #84)
> Ready for testing..

Martin, I assume the patches attached to this bug report from you are the latest. They don't apply anymore. Could you do a rebase?
Comment 86 Marcel de Rooy 2022-08-31 14:53:00 UTC
Ping Martin
Comment 87 Jonathan Druart 2023-04-20 12:44:18 UTC
Created attachment 149941 [details] [review]
Bug 24975: Refactor database translations

This is an attempt to make the process of translating database strings
(like itemtype descriptions) easier for developers and translators.

For developers by reducing the amount of code needed to enable
translations for a new kind of object (authorised values, letters, news,
...).
For translators by providing a single entry point for all translatable
strings

Table `localization` is replaced by two new tables `l10n_source` and
`l10n_target`. `l10n_source` contains the original text while
`l10n_target` contains the corresponding translations

`l10n_source` is automatically updated whenever an itemtype is
inserted, updated, or deleted, so it is always up to date (this is done
in Koha::Schema::Result::ItemType).
It means that if you delete an itemtype, translations get deleted too.
It also means that if you change an itemtype's description, translations
are lost (not sure if this should be fixed, since if description
changes, translations should probably be changed too)

It adds a new subroutine in Koha::I18N, `db_t`, which returns the
translation for a given context and string, according to the current
language. Translations are stored in memcached (if enabled).

So instead of having to declare a new subroutine
Koha::ItemType::translated_description, you can write
    db_t('itemtype', $itemtype->description)

In templates you can write
    [% ItemTypes.t(itemtype.description) %]

There's also a new page, accessible from the administration main page,
"Localization". It displays all translatable strings and allow to
translate them for all enabled languages.

Test plan:
1. Before applying the patch, make sure you have translated some
   itemtypes. Translations should be kept by the updatedatabase script.
   Before applying the updatedatabase you may want to comment the
   'DROP TABLE localization' statement if you want to keep this table
2. Apply patch, run updatedatabase.pl, run update_dbix_class_files.pl
   and reload starman
3. Go to Admin » Localization, verify that you have your itemtypes and
   their translations.
4. Verify that the filter is working (it filters on the source column
   only).
5. Translate some strings by clicking on one of the 'Translate' link,
   entering some text and clicking on the 'Save' button. You can remove
   a translation by emptying the text box.
6. Add a new item type. Verify that it appears in the Localization page
7. Change the description of this item type. Verify that the new
   description appears in Localization page and that the old description
   is gone
8. Delete this item type and verify that its description no longer
   appears in Localization page
9. Navigate through Koha (intranet and opac) in multiple languages and
   verify that item types are translated (if not, please verify in
   master too, not all occurrences were translated)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 88 Jonathan Druart 2023-04-20 12:44:22 UTC
Created attachment 149942 [details] [review]
Bug 24975: Keep the translations, prevent duplicates, and other fixes

Translations are no longer lost when you change an itemtype's
description, thanks to a new `l10n_source.key` column which must be
unique within it's "text group" ('itemtype' is a text group)
This also prevent duplicate entries in l10n_source

Columns were renamed to better match their purpose:
* context -> group
* source -> text

Use Koha::Cache::Memory::Lite if other caching systems are not available

Prepend 'l10n:' to the cache key to avoid conflicts

Test plan:
1. DROP TABLE IF EXISTS l10n_target, l10n_source
2. Follow the test plan from previous patch

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 89 Jonathan Druart 2023-04-20 12:44:26 UTC
Created attachment 149943 [details] [review]
Bug 24975: Fix l10n_source definition in kohastructure.sql
Comment 90 Jonathan Druart 2023-04-20 12:44:29 UTC
Created attachment 149944 [details] [review]
Bug 24975: Fix syntax issues
Comment 91 Jonathan Druart 2023-04-20 12:44:33 UTC
Created attachment 149945 [details] [review]
Bug 24975: (follow-up) Rebase for bug 21946
Comment 92 Jonathan Druart 2023-04-20 12:44:37 UTC
Created attachment 149946 [details] [review]
Bug 24975: Use 'key' for translation lookup

This patch switches from using the original text as a key for
translations to using a dedicated 'key' field.  We also add in a 'fuzzy'
flag to denote when the source text has changed and as such the
pre-existing translations may be incorrect.

ToDo:
1/ Expose 'Fuzzy' on the 'Localization' admin page
2/ Add a 'translation_key' method to Koha::Itemtype to encourage the
adoption of a standardly names accessor to get the 'translation_key'
from a Koha::Object derived class.
Comment 93 Jonathan Druart 2023-04-20 12:44:40 UTC
Created attachment 149947 [details] [review]
Bug 24975: Re-introduce translated_description accessor method
Comment 94 Jonathan Druart 2023-04-20 12:44:44 UTC
Created attachment 149948 [details] [review]
Bug 24975: Add translation_key method example
Comment 95 Jonathan Druart 2023-04-20 12:44:47 UTC
Created attachment 149949 [details] [review]
Bug 24975: Use translation_key in preference to description
Comment 96 Jonathan Druart 2023-04-20 12:44:51 UTC
Created attachment 149950 [details] [review]
Bug 24975: Add to Koha::UI::Form::Builder::Item
Comment 97 Jonathan Druart 2023-04-20 12:44:54 UTC
Created attachment 149951 [details] [review]
Bug 24975: Fix bugs
Comment 98 Jonathan Druart 2023-04-20 12:44:58 UTC
Created attachment 149952 [details] [review]
Bug 24975: DBIC Build
Comment 99 Jonathan Druart 2023-04-20 12:45:01 UTC
Created attachment 149953 [details] [review]
Bug 24975: Fix db_t

The query here is difficult.. I wanted to return FULL LEFT JOIN, but
filtering on the joined data (l10n_targets.language) means we drop the
l10n_source rows where there are no corresponding l10n_targets with
language code.

I tried setting `l10n_targets.language => [ $language, undef ]` to
include null values, but that doesn't work either.  I then tried coming
up with a correlate subquery instead of the join and couldn't get dbic
to play nicely there either.

As such, I've fallen back to multiple queries over the relationship..
I'm sure we can do better... but this works for now.
Comment 100 Jonathan Druart 2023-04-20 12:45:05 UTC
Created attachment 149954 [details] [review]
Bug 24975: Some more fixes
Comment 101 Jonathan Druart 2023-04-20 12:45:47 UTC
Patches rebased, not without troubles...

IMO there are inconsistencies in the way we deal with itemtypes, and we should correct that in this patch set. We should always either call db_t in the script and pass the translated version to the template, or pass the object and let the template deals with it.

Ready for testing (spoiler: it is full of bugs).

Remote branch pushed to https://gitlab.com/joubu/Koha/-/commits/bug_24975
Comment 102 Jonathan Druart 2023-04-20 12:46:29 UTC
If this has not moved forward in 2 months I won't be back to it.
Comment 103 Marcel de Rooy 2023-04-20 12:59:25 UTC
Created attachment 149956 [details] [review]
Bug 24975: (follow-up) Fix dbrev
Comment 104 Marcel de Rooy 2023-04-20 13:04:44 UTC Comment hidden (obsolete)
Comment 105 Jonathan Druart 2023-04-20 13:08:03 UTC
Created attachment 149957 [details] [review]
Bug 24975: WIP remove db_t from controllers

This is not done yet, for discussion. I don't think we should call db_t
in controllers, or we do it better, but now it's confusing.

This patch may impact performances, as we are not caching the itemtype
object.

Ideas?
Comment 106 Marcel de Rooy 2023-04-20 13:09:13 UTC
An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::I18N':
        current merge results [
                Koha::I18N,
        ]
        merging failed on 'Exporter' at /app/qatools/koha-qa.pl line 112.
Comment 107 Marcel de Rooy 2023-04-20 14:28:54 UTC
Will post some more comments tomorrow.
Comment 108 Marcel de Rooy 2023-04-21 06:56:03 UTC
Some findings in random order:

I am missing an Add button on admin/localization. You can clear a record and remove it; but perhaps adding a Delete button would be nice too.

Not sure why we are using DBIC here instead of Koha objects. No performance figures are given to justify. Koha::Object should be our standard.

opac/opac-search.pl
     my $description = $itemtypes->{$itemtype}->{iscat}
       ? $itemtypes->{$itemtype}->{description}
-      : db_t('itemtype', Koha::ItemTypes->find($itemtype)->itemtype);
+      : Koha::ItemTypes->find($itemtype)->translated_description;
     $itemtypes->{$itemtype}->{description} = $description || $itemtypes->{$itemtype}->{description} || q{};
Weird code.

package Koha::Template::Plugin::ItemTypes;
sub GetDescription {
    return $parent ? $self->t($parent->description) . "->" . $self->t($itemtype->description) : $self->t($itemtype->description);
Omission for switching to itemtype.
Comment 109 Marcel de Rooy 2023-04-21 06:56:16 UTC
There is still something confusing in the table design:
select * from l10n_source;
+----------------+----------+-----+------------------------+
| l10n_source_id | group    | key | text                   |
+----------------+----------+-----+------------------------+
|              1 | itemtype | BK  | Books                  |
|              2 | itemtype | BKS | Books                  |
|              3 | itemtype | CF  | Computer Files         |
select * from l10n_target;
+----------------+----------------+----------+-----------------+-------+
| l10n_target_id | l10n_source_id | language | translation     | fuzzy |
+----------------+----------------+----------+-----------------+-------+
|              2 |              1 | nl-NL    | Boek            |     0 |
|              3 |              2 | en       | BKS_EN          |     0 |
|              4 |              2 | nl-NL    | Boek            |     0 |
The source seems to be the English text. But I can translate it into EN again via target. Easiest seems to be: prevent having EN in the target table.
Shouldnt we check if we are calling db_t btw for an English translation? We should not be looking for the English translation of itemtype if we have the description already in our object.
Comment 110 Marcel de Rooy 2023-04-21 06:58:12 UTC
db_t could be made more flexible? Now we look for key and we removed description. But I think we should support both.

Not sure how the concept of localization group will grow. But it might be too big soon for loading all its sources and finding corresponding translations. (The code needs a join here.) For instance, thinking about AV, I am probably only interested in a particular category and not in all entries under 'authorised_values' if that was the group. Other areas may have other needs again.
This is a quite fundamental issue on the design of db_t. It feels not complete.
Comment 111 Jonathan Druart 2023-04-21 08:51:24 UTC Comment hidden (obsolete)
Comment 112 Jonathan Druart 2023-04-21 08:54:02 UTC Comment hidden (obsolete)
Comment 113 Jonathan Druart 2023-04-21 10:18:50 UTC
(In reply to Marcel de Rooy from comment #108)
> Some findings in random order:
> 
> I am missing an Add button on admin/localization. You can clear a record and
> remove it; but perhaps adding a Delete button would be nice too.

What would you like to add? You are not supposed to need it, at least for now.

> Not sure why we are using DBIC here instead of Koha objects. No performance
> figures are given to justify. Koha::Object should be our standard.

It avoids 2 new packages, Koha::I18N::L10nSource[s]?
I would go without them to start with this approach, please keep things as simple as possible.

> opac/opac-search.pl
>      my $description = $itemtypes->{$itemtype}->{iscat}
>        ? $itemtypes->{$itemtype}->{description}
> -      : db_t('itemtype', Koha::ItemTypes->find($itemtype)->itemtype);
> +      : Koha::ItemTypes->find($itemtype)->translated_description;
>      $itemtypes->{$itemtype}->{description} = $description ||
> $itemtypes->{$itemtype}->{description} || q{};
> Weird code.

All this "itemtype category" code is ugly, but why is this part weird? (there is a comment above about non-translatability of item type categories)

> package Koha::Template::Plugin::ItemTypes;
> sub GetDescription {
>     return $parent ? $self->t($parent->description) . "->" .
> $self->t($itemtype->description) : $self->t($itemtype->description);
> Omission for switching to itemtype.

Should be fixed now.

(In reply to Marcel de Rooy from comment #109)
> There is still something confusing in the table design:
> select * from l10n_source;
> +----------------+----------+-----+------------------------+
> | l10n_source_id | group    | key | text                   |
> +----------------+----------+-----+------------------------+
> |              1 | itemtype | BK  | Books                  |
> |              2 | itemtype | BKS | Books                  |
> |              3 | itemtype | CF  | Computer Files         |
> select * from l10n_target;
> +----------------+----------------+----------+-----------------+-------+
> | l10n_target_id | l10n_source_id | language | translation     | fuzzy |
> +----------------+----------------+----------+-----------------+-------+
> |              2 |              1 | nl-NL    | Boek            |     0 |
> |              3 |              2 | en       | BKS_EN          |     0 |
> |              4 |              2 | nl-NL    | Boek            |     0 |
> The source seems to be the English text. But I can translate it into EN
> again via target. Easiest seems to be: prevent having EN in the target table.

The source (l10n_source.text) also contain the default in case there is no translation. We should keep it.

> Shouldnt we check if we are calling db_t btw for an English translation? We
> should not be looking for the English translation of itemtype if we have the
> description already in our object.

If we are not doing worst with this patch than before, I would not recommend to add more to it. First step should be to have something pushed that is stable.
Then we can iterate and improve.

(In reply to Marcel de Rooy from comment #110)
> db_t could be made more flexible? Now we look for key and we removed
> description. But I think we should support both.

No, that's the whole point of the discussion earlier. We should go with the key.
At least not for now.

> Not sure how the concept of localization group will grow. But it might be
> too big soon for loading all its sources and finding corresponding
> translations. (The code needs a join here.) For instance, thinking about AV,
> I am probably only interested in a particular category and not in all
> entries under 'authorised_values' if that was the group. Other areas may
> have other needs again.
> This is a quite fundamental issue on the design of db_t. It feels not
> complete.

Well, it's cached, I would not care much about performances here, that should not be a problem if we are doing things correctly.
Comment 114 Jonathan Druart 2023-04-21 10:28:18 UTC
Created attachment 150015 [details] [review]
Bug 24975: Simplify and uniform calls to get the translation

This patch add a caching mecanism for ItemType objects. We are also
caching the result of the image location and the translation key so that
we can retrieve from the cache and pass them to the temmplate.

We should avoid call to db_t in the controller scripts, and prefer to
pass the itemtype object and let the template does what it needs
(retrieve the translated description, display the image)

Note that the new Koha::Template::Plugin::ItemTypes->GetSorted should
certainly be used in other places, for instance in
 * catalogue/search.pl sub prepare_adv_search_types
 * opac/opac-search.pl
but those 2 occurrences are tricky and we would like to prevent
regressions (as much as possible...)

A major change is the modification of the prototype of search_compat, I
am not sure this patch is updating all the template when the result of
the search is passed.

Note that Koha::SearchEngine::Elasticsearch::Search could use
Koha::ItemTypes->get_from_cache, and certainly other places as well(?)
Comment 115 Jonathan Druart 2023-04-21 10:29:03 UTC
I haven't spent much time testing this patch, but attaching to get feedback if possible.

TODO tests are missing...
Comment 116 Jonathan Druart 2023-04-21 11:22:04 UTC
TODO:
* Add the entries into l10n_* tables when a new item type is created.
* Remove them when the item type is deleted?
* What to do when the description of an item type is modified?
Comment 117 Jonathan Druart 2023-04-21 11:26:13 UTC
Created attachment 150033 [details] [review]
Bug 24975: to squash - fix search.pl
Comment 118 Jonathan Druart 2023-04-21 11:35:19 UTC
Created attachment 150034 [details] [review]
Bug 24975: to squash - fix search.pl
Comment 119 Jonathan Druart 2023-04-21 12:02:22 UTC
(In reply to Jonathan Druart from comment #116)
> TODO:
> * Add the entries into l10n_* tables when a new item type is created.
> * Remove them when the item type is deleted?
> * What to do when the description of an item type is modified?

There are stuffs in Koha/Schema/Result/Itemtype.pm to deal with that actually. I need to retest.
Comment 120 Katrin Fischer 2023-04-21 18:24:48 UTC
(In reply to Jonathan Druart from comment #116)
> TODO:
> * Add the entries into l10n_* tables when a new item type is created.

Does this mean there have to be entries even if you don't translate? (sorry, I should maybe know from the rest of the patch/bug)

> * Remove them when the item type is deleted?

Removing translations when the itemtype is deleted makes sense to me.

> * What to do when the description of an item type is modified?

I think, when an itemtype description is modified, this should not change the translations or remove them. Libraries will take care of it and a typo fix or modification should not cause any changes.
Comment 121 Jonathan Druart 2023-04-25 12:38:52 UTC
See previous comment, that's not relevant.

(In reply to Katrin Fischer from comment #120)
> > * What to do when the description of an item type is modified?
> 
> I think, when an itemtype description is modified, this should not change
> the translations or remove them. Libraries will take care of it and a typo
> fix or modification should not cause any changes.

This will need to be confirmed however.
Comment 122 Katrin Fischer 2023-04-27 21:44:20 UTC
(In reply to Jonathan Druart from comment #121)
> See previous comment, that's not relevant.
> 
> (In reply to Katrin Fischer from comment #120)
> > > * What to do when the description of an item type is modified?
> > 
> > I think, when an itemtype description is modified, this should not change
> > the translations or remove them. Libraries will take care of it and a typo
> > fix or modification should not cause any changes.
> 
> This will need to be confirmed however.

Confirmed?