https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20307 --- Comment #97 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Julian Maurice from comment #96) Thanks Julian!
A few complaints from QA script:
FAIL Koha/Schema/Result/AuthorisedValueLocalization.pm FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
=> False positive, there is no sub there.
FAIL installer/data/mysql/atomicupdate/bug_20307.sql FAIL git manipulation The file has been added and deleted in the same patchset
So, maybe patches should be squashed.
Different authors.
Also: 1) The value stored in localization.code is a concatenation of the AV category code, a '_', and the AV code. Which means that if we have a category A with a value B_C, and a category A_B with a value C, they will share their translations.
I can pick something else than '_', but the problem will exist for any characters. % or $ would fit better?
2) It feels weird to have the ability to translate for all languages. I mean, the original description is already written in one of those languages, right ? So if I add a translation for each language, the original description will never be used. Not really a problem though, as it allows to have the original description in the language we want, and we can just not add a translation for that language.
This development basically mimic what is done for item types. So if there is a change to do in the way they are translated, that should be done before or after this development to affect both screen. I do not think it's a problem anyway. The "default" (the one in the authorised_value table) is used when there is no translation available.
3) There is some places where translated descriptions are not used (the items table in cataloguing/additem.pl for instance) but that can be added later (not blocking)
I can commit on fixing any bugs we are going to find after those patches are pushed. I will take a look at additem.
4) The new column 'interface' will be only useful for authorized value, right ? Why not append the interface to the entity name instead of adding a new column ("authorised_values:opac" for instance) ? Actually, the AV category code could be added there too to avoid (1) ("authorised_values:opac:CCODE").
I think it makes sense to have the interface. The idea is to provide an interface in the admin module to have a global view of the different entities that are translated. So we could imagine a filter to display only opac/staff. Also it could make sense to have the interface for some of the system preferences. As the feature is here, I prefer to keep it and see how it could be useful that add work to drop it then re-add it later. Do not you agree?
5) Please add a test plan :) Even if it's not exhaustive it really helps to have one.
As I said already, a test plan is hard to provide. One could be: - Pick an authorised value that is displayed in lot of places (LOST/DAMAGED for instance) - Provide a translation for some of the values - Go to different pages where the AV is displayed (opac and staff) and make sure the values you translated is the one that is displayed.
6) Koha::AuthorisedValues::search_with_localization does something really strange : it sets package variables $Koha::Schema::Result::AuthorisedValue::LANGUAGE and $Koha::Schema::Result::AuthorisedValue::INTERFACE. I understand that they are then used in the has_many subroutine for 'localization', and that the same thing is done for itemtypes, but that feels wrong to use global variables like that. Can't we use a "normal" join in search_with_localization ? And why has_many ? An authorised value should have only 1 (or 0) translation, given a particular language and interface.
They are used actually in the view (Koha/Schema/Result/AuthorisedValueLocalization.pm). Take a look at this commit where I tried to explain it commit 80018625cf03ccd00957035f99065ed5793b80e3 Bug 17835: Create a ItemtypeLocalization view I spent quite a lot of time to find a correct solution when I implemented bug 17835. Maybe there is a better way to do it, but here I am following an existing pattern, that works so far. I think the description also answer why 1-N (has_many): 1 entry in authorised_value has several entries in the view. -- You are receiving this mail because: You are watching all bug changes.