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)

That looks like a great idea.
What about the interface (for AVs)?

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

Thanks for picking this up Julian, it's looking great so far! :)

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>

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>

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?)

(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.

(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 :)

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>

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>

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.

(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.

Created attachment 102688 [details] [review]
Bug 24975: Fix l10n_source definition in kohastructure.sql

Created attachment 102701 [details] [review]
Bug 24975: Fix syntax issues

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.

> 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.

Are you hoping to continue here Julian.. it's looking so promising :)

(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

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.

(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.

(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 ?

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.

Katrin, Bernardo, can you give us your feeling on this topic please?

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.

(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 ?

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.

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.

(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 ?

(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.

>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

To add detail to that.. it's two different translations for the single word 'Status' in English for the Finnish translation.

(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)

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.

(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

Could we get a work team on this one and target 21.05?

Also very interested in this, can provide testing and QA. A quick discussion on how to approach this would be nice!

Any news here ?

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!

Created attachment 132388 [details] [review]
Bug 24975: Fix bugs

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.

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.

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.

(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?

Ping Martin

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>

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>

Created attachment 149943 [details] [review]
Bug 24975: Fix l10n_source definition in kohastructure.sql

Created attachment 149944 [details] [review]
Bug 24975: Fix syntax issues

Created attachment 149945 [details] [review]
Bug 24975: (follow-up) Rebase for bug 21946

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.

Created attachment 149947 [details] [review]
Bug 24975: Re-introduce translated_description accessor method

Created attachment 149948 [details] [review]
Bug 24975: Add translation_key method example

Created attachment 149949 [details] [review]
Bug 24975: Use translation_key in preference to description

Created attachment 149950 [details] [review]
Bug 24975: Add to Koha::UI::Form::Builder::Item

Created attachment 149951 [details] [review]
Bug 24975: Fix bugs

Created attachment 149952 [details] [review]
Bug 24975: DBIC Build

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.

Created attachment 149954 [details] [review]
Bug 24975: Some more fixes

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

If this has not moved forward in 2 months I won't be back to it.

Created attachment 149956 [details] [review]
Bug 24975: (follow-up) Fix dbrev

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?

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.

Will post some more comments tomorrow.

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.

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.

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.

(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.

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(?)

I haven't spent much time testing this patch, but attaching to get feedback if possible.

TODO tests are missing...

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?

Created attachment 150033 [details] [review]
Bug 24975: to squash - fix search.pl

Created attachment 150034 [details] [review]
Bug 24975: to squash - fix search.pl

(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.

(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.

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.

(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?

Re-reading all comments and patches after a year and a half... the end result doesn't look easier to use that what we currently have in master:

  db_t('itemtype', $itemtype->{translation_key})

I can't precisely tell what is wrong with this, but it's definitely harder to read than:

  $itemtype->translated_description

I think this is because at first, I tried to fit the gettext (POT/PO files) model into database, and then it evolved to use a (primary) key as "source string" instead of the original string, trying to fit this in a system not really designed for it.

So I tried another approach.
Since it appear we want to use the primary key as the "matching point" between an object and its localizations, I tried storing itemtypes localizations in a dedicated `itemtypes_localizations` table and see how much I could generalize it to make it easy to localize other tables data.

I will submit the result soon in a new bug report. It's not finished but I need to get feedback in order to continue.

=> bug 38136

(In reply to Jonathan Druart from comment #114)
> Created attachment 150015 [details] [review] [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(?)

This patch was quite good. Will move it to a separate bug.