|
Description
Julian Maurice
2024-10-10 12:03:41 UTC
Awesome to see some new activity on this topic. Go Julian go! Created attachment 172585 [details] [review] Bug 38136: Refactor database translations (alternative) This has the same goals of bug 24975 but with a different approach. Bug 24975 tried the "POT/PO files as database tables" approach but it appeared that is not what best fits our needs. Instead of a "source text / translated text" mapping, we need translations to be attached to a specific "object" (or table row), which has the added benefit of keeping translations unaffected when the original string changes. It may or may not be what you want but at least this avoids the problem where fixing an error in the original string forces users to re-translate everything. The solution proposed here is to store translations in separate tables, one table for each object type (so, one for itemtypes, another for authorised values, ...). While it may seem redundant and tedious to add another table with the same structure for each localizable object type, this has the benefit of being able to add foreign key constraints and thus having integrity checks "for free". Also this patch adds all the necessary tools so that the tedium of localizing a new object type is reduced to a minimum. In summary, we will only need to: * create the table structure (atomicupdate, kohastructure.sql) * add two method calls in the "result class" corresponding to the localizable object type * in the admin interface, add a link to the the translation popup. Unlike bug 24975 there is not a page where you can translate everything. I don't know if it is really useful (probably better to translate itemtypes when you are on the itemtype page). It can be added later if needed. To give an example, this is what needs to be added manually in Koha/Schema/Result/Itemtype.pm: __PACKAGE__->load_components('+Koha::DBIx::Class::Localization'); __PACKAGE__->localization_add_relationships( 'itemtypes_localizations', 'itemtype' => 'itemtype', 'description' ); (see POD in Koha/DBIx/Class/Localization.pm for more information) Final notes: * If having several localization tables is really a problem, I believe the current implementation can be changed easily to use a single table (just need to change the dbic relationship conditions) * I wanted this patch to have as few changes as possible so I kept methods like `search_with_localization`, `children_with_localization` even if I believe they can be removed (replaced by search/children) Localizations do not need to be prefetched/joined unless you want to sort on them. For the same reason, $itemtype->unblessed now always return a `translated_description` key. This can be fixed in followup patches * There is cache involved (see Koha::DBIx::Class::Localization::localization). Localizations in cache are grouped by object type and language (keys look like 'KOHA:localization:Itemtype:en') Changing status to Needs Signoff, but really this patch needs QA eyes more than anything. Before testing, backup your localization table as the atomicupdate deletes it. Alternatively, comment the DROP TABLE query before running updatedatabase.pl (In reply to Julian Maurice from comment #3) > Changing status to Needs Signoff, but really this patch needs QA eyes more > than anything. > > Before testing, backup your localization table as the atomicupdate deletes > it. Alternatively, comment the DROP TABLE query before running > updatedatabase.pl Definitely interested in getting this further. Great! Hope to have a look soon. This is not clear from the patch description, but this is capable of managing translations for several properties of the same object. In other words, we could easily make itemtype's "checking message" and "summary" translatable. (In reply to Julian Maurice from comment #5) > This is not clear from the patch description, but this is capable of > managing translations for several properties of the same object. In other > words, we could easily make itemtype's "checking message" and "summary" > translatable. Thanks for pointing out, I had been wondering about the normal and OPAC description for AV values. I will have a look as well, next week. Some things I have in mind right now (so more a note for myself) * can we answer the need of translatability for plugins? Bug 37472 * to_string for the rest api (+search +search on embed) * confirm the key problem we had on the other bugs is taken into account (same string same table, different translations) * amount of remaining work to apply it to AVs > It may or may not be what you want but at least this avoids the problem where fixing an error in the original string forces users to re-translate everything. Then it's fuzzy (In reply to Jonathan Druart from comment #7) > * can we answer the need of translatability for plugins? Bug 37472 This would require Koha::Schema to include plugins' result classes. Can it do that ? > * confirm the key problem we had on the other bugs is taken into account > (same string same table, different translations) If I understand the problem correctly, you're talking about having (eg.) two itemtypes with the same description, and the problem is that you have to translate both ? That's the problem I tried to avoid with my first approach but it had other issues. I'm not sure how you can have translations attached to one object AND avoid this problem. So the response is: no, it's not taken into account. But... if I remember correctly this was only a problem when we talked about translating MARC frameworks. If so, I believe it would be possible to have localizations managed differently only for MARC framework (by using a different DBIC component - or adapting the exiting one - to handle a special case where all similar objects share the same translations), while keeping the same public API (Koha::Object::localization). (I'm thinking as I write, so sorry if it's not clear) > > It may or may not be what you want but at least > this avoids the problem where fixing an error in the original string > forces users to re-translate everything. > > Then it's fuzzy Just to be clear, the proposed patch here have no concept of fuzziness. Is it useful ? Can it be replaced by a warning ? ("You modified description, which has translations. Make sure the translations are still correct: <link to translations>") Library limit for item types search is broken DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'branchcode' in 'where clause' at /kohadevbox/koha/Koha/Objects.pm line 321 http://localhost:8081/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=4&itemnumber=18#edititem Koha/UI/Form/Builder/Item.pm 214 $itemtypes = Koha::ItemTypes->search_with_localization( 215 { branchcode => $branch_limit } ); Created attachment 172804 [details] [review] Bug 38136: Fix call to search_with_localization with branchcode param It was the only place where this parameter was used Created attachment 173195 [details] [review] Bug 38136: [TO SQUASH] Remove unecessary 'use Koha::Caches' from Koha::ItemType Created attachment 173196 [details] [review] Bug 38136: Show item type when adding translations We are retrieving the value from the wrong cache in Koha/Template/Plugin/ItemTypes.pm, it is using Koha::Cache::Memory::Lite. If you have a translation, remove it, add another one: the old one will be displayed (at least on catalogue/detail.pl in the items table). (In reply to Jonathan Druart from comment #13) > We are retrieving the value from the wrong cache in > Koha/Template/Plugin/ItemTypes.pm, it is using Koha::Cache::Memory::Lite. > > If you have a translation, remove it, add another one: the old one will be > displayed (at least on catalogue/detail.pl in the items table). Nope sorry, this does not make sense. The wrong fetch happens, but obviously not from the TT plugin (Memory::Lite it's not persistent from one request to another) (In reply to Jonathan Druart from comment #14) > Nope sorry, this does not make sense. The wrong fetch happens, but obviously > not from the TT plugin (Memory::Lite it's not persistent from one request to > another) The cache should be cleared when a translation is added, updated or removed. I'll check. It's C4::Biblio::GetAuthorisedValueDesc that needs adjustments. Created attachment 173197 [details] [review] Bug 38136: Fix 'Delete' link When a new entry has been added the Delete link was wrong. Also fix translatability of "Delete" Created attachment 173198 [details] [review] Bug 38136: Return '404 Not Found' when trying to delete a non-existent translation 1. not blocker (personal preference) - I don't think we should have one localization table per table we want to support. The data integrity for free can be discussed as it would be easy to have a single line in Koha::Object->delete to delete the linked localizations. However I don't want to discuss this more or change the implementation, I can live with that. OTOH it will be faster for the JOINs. 2. It would be nice to have a POC for AVs (on a separate bug report). 3. Not fond of Koha::DBIx, I would keep this under Koha::Schema (personal preference again, not blocker) I agree that it feels better to not add a table for each translation category. Created attachment 174578 [details] [review] Bug 38136: Move DBIC component to Koha::Schema namespace Created attachment 174579 [details] [review] Bug 38136: Fix C4::Biblio::GetAuthorisedValueDesc for itemtypes Store itemtypes in memory cache so that we can use translated_description, which will use the right cache Created attachment 174580 [details] [review] Bug 38136: Fix cache not being cleared when updating a translation Created attachment 174581 [details] [review] Bug 38136: Fix behavior of lang selector in translations editor > 2. It would be nice to have a POC for AVs (on a separate bug report).
I'm on it.
Created attachment 174598 [details] [review] Bug 38136: Fix localization cache Created attachment 174599 [details] [review] Bug 38136: Die early if the localization relationship does not exist confess shows the whole stacktrace which is useful to spot where we made a mistake (croak showed the wrong file) (In reply to Julian Maurice from comment #25) > > 2. It would be nice to have a POC for AVs (on a separate bug report). > I'm on it. bug 38460 |