https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20307
--- Comment #97 from Jonathan Druart <[email protected]> --- (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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
