Description
Jonathan Druart
2018-02-27 21:34:39 UTC
Is there any work yet planned or done on this? (In reply to Katrin Fischer from comment #1) > Is there any work yet planned or done on this? I have started something, but we need bug 15799 first (move the last C4::Koha::GetAuthorisedValues to Koha::AuthorisedValues) There are local patches (very wip) on my laptop, but I have no idea when I will get back to it. The rewrite/move patches are to slow to get pushed and need too much work to rebase. Ok, a little confused by the graph - didn't spot any patch with patches still needing work. Can you point me the right direction? (In reply to Katrin Fischer from comment #3) > Ok, a little confused by the graph - didn't spot any patch with patches > still needing work. Can you point me the right direction? I do not plan to get back to this soon. Should we continue on the same way of item types (translation from the UI), or extract the strings from the DB then use the new Koha::I18 module (translation from POs)? (In reply to Jonathan Druart from comment #5) > Should we continue on the same way of item types (translation from the UI), > or extract the strings from the DB then use the new Koha::I18 module > (translation from POs)? If I am not mistaken the translation from POs would always require to run jobs on the server, maintain your own po files... put them in the right place. I think we need something that can be done in the GUI to manage the translations. I am not sure if that could be achieved differently than what you have done for the itemtypes. (In reply to Jonathan Druart from comment #5) > Should we continue on the same way of item types (translation from the UI), > or extract the strings from the DB then use the new Koha::I18 module > (translation from POs)? I think going the item type route is the proper way to do it. Doing it through PO:s wouldn't really work. Libraries within the same language area (sv-SE in my case) will not use the same names for values like LOC, and most likely not have the same values at all, and you can't have locally edited PO files for each library. Translations for item types should be fairly similar to translations for authorised values. Patron categories and library names should also benefit from such a system. Created attachment 86622 [details] [review] Bug 20307: Add DB column localization.interface Created attachment 86623 [details] [review] Bug 20307: DBIC Schema changes Created attachment 86624 [details] [review] Bug 20307: Update the localization unique key Created attachment 86625 [details] [review] Bug 20307: DBIC Schema changes 2 Created attachment 86626 [details] [review] Bug 20307: Language overlay for authorised values Created attachment 86627 [details] [review] Bug 20307: Some more fixes I have cleaned and rebased my local patches, as well as fixing obvious problems. I have no idea how big is the remaining work to make it ready to be pushed. However it can be tested and having a list of what is ok and what is not will help (and motivate) me to finish this enhancement. Oh, sorry, I hadn't spotted that this one had come to life.. I'll try hard to find a moment to have a look at it. Updating the status to Needs Signoff (to get more love). Created attachment 89538 [details] [review] Bug 20307: (QA follow-up) Move DB update to perl This works, and I'm not against it.. but I can't help but feel there must be a simpler approach. I'm going to go away and work on an alternative implementation as a way of getting my head around this code as part of a SO/QA effort. I'll be using some of this as foundation work.. so see it as a golfing effort. Hello Martin, I do not know how easier it could be, what do you have in mind? :) We already have a way to translate stuffs (only item types so far), we must reuse it. In those patches I am just mimicking what we already have for item types. Testers do please go ahead and test this however.. The work I'm doing is more as preparation for doing a QA.. I felt working through the thought process involved in thinking about an alternative would be the best way for me to get up to speed with this implementation. Hi, Is there a test plan for this? It's hard to provide a complete test plan, you will need to show imagination to find bugs :) The idea is to translate authorised values and confirm that they are displayed translated for translated templates. Created attachment 91645 [details] [review] Bug 20307: Add DB column localization.interface Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 91646 [details] [review] Bug 20307: DBIC Schema changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 91647 [details] [review] Bug 20307: Update the localization unique key Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 91648 [details] [review] Bug 20307: DBIC Schema changes 2 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 91649 [details] [review] Bug 20307: Language overlay for authorised values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 91650 [details] [review] Bug 20307: Some more fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 91651 [details] [review] Bug 20307: (QA follow-up) Move DB update to perl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Rebased on master and signing off here.. I couldn't come up with a cleaner way and don't want to stop this one in it's tracks.. I'm minded to go ahead and push it and deal with consequences over the rest of the cycle as it feels pretty solid, but it would be nice to get a QA eye on it really.. Created attachment 91653 [details] [review] Bug 20307: (QA follow-up) Move DB update to perl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Needs unit tests ;) Hi Jonathan, starting QA here - not only a RM priority, but one of mine as well. I will try to come back at this as long as necessary: 1) QA test tools: There are some about POD, but I think those are valid: FAIL admin/authorised_values.pl FAIL valid Can't locate Carp/Always.pm in @INC (you may need to install the Carp::Always module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins) FAIL Koha/AuthorisedValues.pm FAIL valid Name "Koha::Schema::Result::AuthorisedValue::LANGUAGE" used only once: possible typo 2) Code review a) C4/Search.pm + $facet_label_value = $av->count ? $av->next->opac_translated_description : ''; # FIXME: Why are we displaying the opac description for staff? I believe you are right here - this is a bug. Could be fixed here or later. Why are we not dealing with this in the templates? b) Koha/AuthorisedValue.pm opac_translated_description and tranlated_description seem to have a lot of duplicated code. Coudl it make sense to merge them with an interface parameter? + my $descriptions = { + lib => $av->translated_description, # Rename with opac_translated_description to avoid confusion + opac_description => $av->opac_translated_description # Same with translated_description + }; Not sure what you meant with your comments here? 3) Testing The admin page currently explodes: Could not compile /home/vagrant/kohaclone/admin/authorised_values.pl: Can't locate Carp/Always.pm in @INC (you may need to install the Carp::Always module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/kohaclone/installer /home/vagrant/kohaclone/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins) at /home/vagrant/kohaclone/admin/authorised_values.pl line 27. BEGIN failed--compilation aborted at /home/vagrant/kohaclone/admin/authorised_values.pl line 27. at /usr/share/perl5/CGI/Compile.pm line 132 As I understand it, this is just the 'base' and then we will have to work through the different AV categories... that about right? (In reply to Katrin Fischer from comment #33) > Hi Jonathan, Hi Katrin, thanks for your feedback! > starting QA here - not only a RM priority, but one of mine as well. I will > try to come back at this as long as necessary: > > 1) QA test tools: > > There are some about POD, but I think those are valid: > > FAIL admin/authorised_values.pl > FAIL valid > Can't locate Carp/Always.pm in @INC (you may need to install the > Carp::Always module) (@INC contains: /home/vagrant/kohaclone > /home/vagrant/qa-test-tools /etc/perl > /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 > /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 > /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 > /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base > /var/lib/koha/kohadev/plugins) All issues related with Carp::Always will be resolved by a follow-up patch. > FAIL Koha/AuthorisedValues.pm > > FAIL valid > Name "Koha::Schema::Result::AuthorisedValue::LANGUAGE" used only once: > possible typo Not valid, it's what we are doing for Itemtype.pm > 2) Code review > > a) C4/Search.pm > + $facet_label_value = $av->count ? > $av->next->opac_translated_description : ''; # FIXME: Why are we displaying > the opac description for staff? > > I believe you are right here - this is a bug. Could be fixed here or later. > Why are we not dealing with this in the templates? Because we are in C4::Search :-/ > b) Koha/AuthorisedValue.pm > > opac_translated_description and tranlated_description seem to have a lot of > duplicated code. Coudl it make sense to merge them with an interface > parameter? IMO it's more readable as it, but could be discussed. > + my $descriptions = { > + lib => $av->translated_description, # Rename with > opac_translated_description to avoid confusion > + opac_description => $av->opac_translated_description # Same with > translated_description > + }; > > Not sure what you meant with your comments here? They do not make sense, I guess it's a c/p failure from Koha/Template/Plugin/AuthorisedValues.pm > 3) Testing > > The admin page currently explodes: > > Could not compile /home/vagrant/kohaclone/admin/authorised_values.pl: Can't > locate Carp/Always.pm in @INC (you may need to install the Carp::Always > module) (@INC contains: /home/vagrant/kohaclone > /home/vagrant/kohaclone/installer /home/vagrant/kohaclone/lib/installer > /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 > /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 > /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 > /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base > /var/lib/koha/kohadev/plugins) at > /home/vagrant/kohaclone/admin/authorised_values.pl line 27. > BEGIN failed--compilation aborted at > /home/vagrant/kohaclone/admin/authorised_values.pl line 27. > at /usr/share/perl5/CGI/Compile.pm line 132 Because of missing Carp::Always, but I removed it now. > As I understand it, this is just the 'base' and then we will have to work > through the different AV categories... that about right? Nope, it should work for any AV. Created attachment 92192 [details] [review] Bug 20307: Some more fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 92193 [details] [review] Bug 20307: Remove debug statement Carp::Always Back here... :) Sorry Jonathan, not getting far again: When I click on a category in authorised values to edit existing values: Template process failed: undef error - The method Koha::AuthorisedValue->translated_descriptions is not covered by tests! Sorry, my fault... forgot a restart_all. I have problems with adding end editing of translations: 1) Adding translations the same time as adding a new authorised value doesn't work. The first time you try, it seems to save, but doesn't display the values later. And the second time you try, it claims values already exist. I think the easiest way for now would be to not display the translate links when adding, only on editing. 2) The entity shown in the table of translations is "avs" - not sure if this is a code or should be translatable? It might not be understood well in other context. 3) After adding some values and translations to TERM I have switched to DEPARTMENT. Adding an OPAC description translation worked, but now I get an error when I try to add a staff translation: DBD::mysql::st execute failed: Duplicate entry 'avs-Department1-de-DE' for key 'entity_code_lang' [for Statement "INSERT INTO `localization` ( `code`, `entity`, `interface`, `lang`, `translation`) VALUES ( ?, ?, ?, ?, ? )" with ParamValues: 0="Department1", 1="avs", 2="intranet", 3="de-DE", 4="depds"] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. Duplicate ID at /usr/share/perl5/Exception/Class/Base.pm line 88 Exception::Class::Base::throw('Koha::Exceptions::Object::DuplicateID', 'error', 'Duplicate ID', 'duplicate_id', 'entity_code_lang') called at /home/vagrant/kohaclone/Koha/Object.pm line 160 I am not yet sure why this happens, my table looks like this: +-----------------+-----------+-----------+-------------+-------+-------------+ | localization_id | entity | interface | code | lang | translation | +-----------------+-----------+-----------+-------------+-------+-------------+ | 1 | avs | opac | | de-DE | 2019d | | 2 | avs | opac | | en | test | | 3 | avs | intranet | 2019 | de-DE | 2019d | | 5 | avs | intranet | 2019 | en | t | | 6 | itemtypes | NULL | BK | de-DE | Bücher | | 8 | avs | opac | Department1 | de-DE | dep_do | +-----------------+-----------+-----------+-------------+-------+-------------+ 4) Looking at the db structure: We know the code and that it is an authorised value, but we don't know the category! I think this is a mistake. Not always the tanslation for the same code is the same. Think about the numeric ones like Damaged, Lost... Created attachment 92359 [details] [review] Bug 20307: Disable the translate links if the object has not been saved yet We do not have the code yet (or can be edited after the translation has been done) Created attachment 92360 [details] [review] Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion And so we need to extend the size of localization.entity Created attachment 92361 [details] [review] Bug 20307: Code for AVs must be category and value And not only the authorized value, otherwise we could not handle the case where 2 categories have an identical authorized value. FIXME: When editing an AV, if you translate then change the AV's value, we will have inconsistent data, we should add some JS code to disable the translate link if the AV's value is modified. I would suggest to do it on a follow-up bug. (In reply to Katrin Fischer from comment #40) > I have problems with adding end editing of translations: > > 1) Adding translations the same time as adding a new authorised value > doesn't work. The first time you try, it seems to save, but doesn't display > the values later. And the second time you try, it claims values already > exist. > I think the easiest way for now would be to not display the translate links > when adding, only on editing. Indeed, I removed the links on adding a new AV or itemtype. > 2) The entity shown in the table of translations is "avs" - not sure if this > is a code or should be translatable? It might not be understood well in > other context. It's an internal code, I have renamed it anyway. > 3) After adding some values and translations to TERM I have switched to > DEPARTMENT. Adding an OPAC description translation worked, but now I get an > error when I try to add a staff translation: > > DBD::mysql::st execute failed: Duplicate entry 'avs-Department1-de-DE' for > key 'entity_code_lang' [for Statement "INSERT INTO `localization` ( `code`, > `entity`, `interface`, `lang`, `translation`) VALUES ( ?, ?, ?, ?, ? )" with > ParamValues: 0="Department1", 1="avs", 2="intranet", 3="de-DE", 4="depds"] > at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. > Duplicate ID at /usr/share/perl5/Exception/Class/Base.pm line 88 > Exception::Class::Base::throw('Koha::Exceptions::Object::DuplicateID', > 'error', 'Duplicate ID', 'duplicate_id', 'entity_code_lang') called at > /home/vagrant/kohaclone/Koha/Object.pm line 160 > > I am not yet sure why this happens, my table looks like this: > > +-----------------+-----------+-----------+-------------+-------+------------ > -+ > | localization_id | entity | interface | code | lang | > translation | > +-----------------+-----------+-----------+-------------+-------+------------ > -+ > | 1 | avs | opac | | de-DE | 2019d > | > | 2 | avs | opac | | en | test > | > | 3 | avs | intranet | 2019 | de-DE | 2019d > | > | 5 | avs | intranet | 2019 | en | t > | > | 6 | itemtypes | NULL | BK | de-DE | Bücher > | > | 8 | avs | opac | Department1 | de-DE | dep_do > | > +-----------------+-----------+-----------+-------------+-------+------------ > -+ > > 4) Looking at the db structure: We know the code and that it is an > authorised value, but we don't know the category! I think this is a mistake. > Not always the tanslation for the same code is the same. Think about the > numeric ones like Damaged, Lost... Indeed, big design issue. Thanks Katrin! It should be fixed now. I started with testing the DEPARTMENT again. I added a de-DE translation for the DEPARTMENT. None for en. 1) The language de-DE is not activated, but it starts to show the translation I added for German in the English interface. dep-dei - internal staff description in the table below, shows in the English OPAC GUI. Staff/OPAC might be a template issue, but still wondering why it shows up in the English GUI? +-----------------+-------------------+-----------+------------------------+-------+-------------+ | localization_id | entity | interface | code | lang | translation | +-----------------+-------------------+-----------+------------------------+-------+-------------+ | 1 | itemtypes | NULL | BK | de-DE | Bücher | | 2 | authorised_values | intranet | DEPARTMENT_Department1 | de-DE | Dep-dei | | 5 | authorised_values | opac | DEPARTMENT_Department1 | de-DE | dep-deo | +-----------------+-------------------+-----------+------------------------+-------+-------------+ 2) In staff admin: My translation for OPAC description shows as description in the OPAC summary (the OPAC description column being empty). When I edit the OPAC description, the translation shows there. So maybe just something wrong on the list of values? (In reply to Katrin Fischer from comment #45) > I started with testing the DEPARTMENT again. > > I added a de-DE translation for the DEPARTMENT. None for en. > > 1) The language de-DE is not activated, but it starts to show the > translation I added for German in the English interface. dep-dei - internal > staff description in the table below, shows in the English OPAC GUI. > Staff/OPAC might be a template issue, but still wondering why it shows up in > the English GUI? Where? When editing/adding a course reserve I see the default (untranslated) value. > 2) In staff admin: > My translation for OPAC description shows as description in the OPAC summary > (the OPAC description column being empty). When I edit the OPAC description, > the translation shows there. So maybe just something wrong on the list of > values? I do not get that one, sorry. Could you provide a screenshot? Created attachment 94245 [details] [review] Bug 20307: Add DB column localization.interface Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94246 [details] [review] Bug 20307: DBIC Schema changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94247 [details] [review] Bug 20307: Update the localization unique key Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94248 [details] [review] Bug 20307: DBIC Schema changes 2 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94249 [details] [review] Bug 20307: Language overlay for authorised values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94250 [details] [review] Bug 20307: (QA follow-up) Move DB update to perl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94251 [details] [review] Bug 20307: Some more fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 94252 [details] [review] Bug 20307: Remove debug statement Carp::Always Created attachment 94253 [details] [review] Bug 20307: Disable the translate links if the object has not been saved yet We do not have the code yet (or can be edited after the translation has been done) Created attachment 94254 [details] [review] Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion And so we need to extend the size of localization.entity Created attachment 94255 [details] [review] Bug 20307: Code for AVs must be category and value And not only the authorized value, otherwise we could not handle the case where 2 categories have an identical authorized value. FIXME: When editing an AV, if you translate then change the AV's value, we will have inconsistent data, we should add some JS code to disable the translate link if the AV's value is modified. I would suggest to do it on a follow-up bug. Created attachment 94256 [details] [review] Bug 20307: Few more fixes due to bad rebase conflict resolutions Created attachment 94700 [details] [review] Bug 20307: Add DB column localization.interface Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94701 [details] [review] Bug 20307: DBIC Schema changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94702 [details] [review] Bug 20307: Update the localization unique key Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94703 [details] [review] Bug 20307: DBIC Schema changes 2 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94704 [details] [review] Bug 20307: Language overlay for authorised values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94705 [details] [review] Bug 20307: (QA follow-up) Move DB update to perl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94706 [details] [review] Bug 20307: Some more fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94707 [details] [review] Bug 20307: Remove debug statement Carp::Always Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94708 [details] [review] Bug 20307: Disable the translate links if the object has not been saved yet We do not have the code yet (or can be edited after the translation has been done) Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94709 [details] [review] Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion And so we need to extend the size of localization.entity Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94710 [details] [review] Bug 20307: Code for AVs must be category and value And not only the authorized value, otherwise we could not handle the case where 2 categories have an identical authorized value. FIXME: When editing an AV, if you translate then change the AV's value, we will have inconsistent data, we should add some JS code to disable the translate link if the AV's value is modified. I would suggest to do it on a follow-up bug. Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 94711 [details] [review] Bug 20307: Few more fixes due to bad rebase conflict resolutions Signed-off-by: Michal Denar <black23@gmail.com> QA: This is not a priority for 19.11, it's too late. But please keep it in mind for early 19.12 ;) (In reply to Jonathan Druart from comment #71) > QA: This is not a priority for 19.11, it's too late. But please keep it in > mind for early 19.12 ;) +1! I've added a new keyword for things we should target early next cycle.. rel_20_05_target and christened it by adding it to this bug :) This is looking pretty solid to me at the moment.. would love to see a QA stamp on it soon if anyone has the time to take a look :) (In reply to Martin Renvoize from comment #74) > This is looking pretty solid to me at the moment.. would love to see a QA > stamp on it soon if anyone has the time to take a look :) Am I allowed to remove the use of C4::Language in Koha::*? (In reply to Tomás Cohen Arazi from comment #75) > (In reply to Martin Renvoize from comment #74) > > This is looking pretty solid to me at the moment.. would love to see a QA > > stamp on it soon if anyone has the time to take a look :) > > Am I allowed to remove the use of C4::Language in Koha::*? I agree this should be pushed. And until we have a guideline for this, we can deal with fixing it on a separate bug. Could you please add unit tests for the new subs? Created attachment 97074 [details] [review] Bug 20307: Fix existing tests Created attachment 97075 [details] [review] Bug 20307: Fix logic in search_with_localization and add tests Maybe we will want to replace this method at some point. There was something wrong in the logic. I did not manage to implement what I really needed here. The real SQL query we need is: SELECT SELECT *, COALESCE( `localization_intranet`.`translation`, `me`.`lib` ) AS `translated_description`, COALESCE( `localization_opac`.`translation`, `me`.`lib` ) AS `opac_translated_description` FROM `authorised_values` `me` LEFT JOIN (SELECT localization_id, code, lang, translation FROM localization WHERE entity='authorised_values' AND interface='intranet') `localization_intranet` ON ( `localization_intranet`.`lang` = 'es-ES' AND CONCAT(me.category, '_', `me`.`authorised_value`) = `localization_intranet`.`code` ) LEFT JOIN (SELECT localization_id, code, lang, translation FROM localization WHERE entity='authorised_values' AND interface='opac') `localization_opac` ON ( `localization_opac`.`lang` = 'es-ES' AND CONCAT(me.category, '_', `me`.`authorised_value`) = `localization_opac`.`code` ) WHERE me.category="?" ORDER BY `translated_description`; So 2 left join on the same table, with different on clause (to deal with the 2 interfaces) That would give us something like: | id | category | authorised_value | lib | lib_opac | imageurl | translated_description | opac_translated_description | I would like another signoff here. Created attachment 98863 [details] [review] Bug 20307: Add DB column localization.interface Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98864 [details] [review] Bug 20307: DBIC Schema changes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98865 [details] [review] Bug 20307: Update the localization unique key Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98866 [details] [review] Bug 20307: DBIC Schema changes 2 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98867 [details] [review] Bug 20307: Language overlay for authorised values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98868 [details] [review] Bug 20307: (QA follow-up) Move DB update to perl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98869 [details] [review] Bug 20307: Some more fixes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98870 [details] [review] Bug 20307: Remove debug statement Carp::Always Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98871 [details] [review] Bug 20307: Disable the translate links if the object has not been saved yet We do not have the code yet (or can be edited after the translation has been done) Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98872 [details] [review] Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion And so we need to extend the size of localization.entity Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98873 [details] [review] Bug 20307: Code for AVs must be category and value And not only the authorized value, otherwise we could not handle the case where 2 categories have an identical authorized value. FIXME: When editing an AV, if you translate then change the AV's value, we will have inconsistent data, we should add some JS code to disable the translate link if the AV's value is modified. I would suggest to do it on a follow-up bug. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98874 [details] [review] Bug 20307: Few more fixes due to bad rebase conflict resolutions Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98875 [details] [review] Bug 20307: Fix existing tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 98876 [details] [review] Bug 20307: Fix logic in search_with_localization and add tests Maybe we will want to replace this method at some point. There was something wrong in the logic. I did not manage to implement what I really needed here. The real SQL query we need is: SELECT SELECT *, COALESCE( `localization_intranet`.`translation`, `me`.`lib` ) AS `translated_description`, COALESCE( `localization_opac`.`translation`, `me`.`lib` ) AS `opac_translated_description` FROM `authorised_values` `me` LEFT JOIN (SELECT localization_id, code, lang, translation FROM localization WHERE entity='authorised_values' AND interface='intranet') `localization_intranet` ON ( `localization_intranet`.`lang` = 'es-ES' AND CONCAT(me.category, '_', `me`.`authorised_value`) = `localization_intranet`.`code` ) LEFT JOIN (SELECT localization_id, code, lang, translation FROM localization WHERE entity='authorised_values' AND interface='opac') `localization_opac` ON ( `localization_opac`.`lang` = 'es-ES' AND CONCAT(me.category, '_', `me`.`authorised_value`) = `localization_opac`.`code` ) WHERE me.category="?" ORDER BY `translated_description`; So 2 left join on the same table, with different on clause (to deal with the 2 interfaces) That would give us something like: | id | category | authorised_value | lib | lib_opac | imageurl | translated_description | opac_translated_description | Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Re-tested and all seems to be working well.. Signing off... 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 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. 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. 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. 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) 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"). 5) Please add a test plan :) Even if it's not exhaustive it really helps to have one. 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. Failing QA for (1) that should be fixed, but I really would like to know your opinions on the other points (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. Created attachment 101070 [details] [review] Bug 20307: Replace separator with # To avoid colision we replace the separator with something that is supposed to be less used than underscore The problematic case was explaing by Julian on comment 96: """ 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. """ (In reply to Jonathan Druart from comment #97) > I can pick something else than '_', but the problem will exist for any > characters. That's why I suggested appending the AV code to the entity name, instead of prepending it to the AV value. Have you considered this ? Created attachment 101450 [details] [review] [DO NOT APPLY/PUSH] Bug 20307: Move av.category to loc.entity (In reply to Julian Maurice from comment #99) > (In reply to Jonathan Druart from comment #97) > > I can pick something else than '_', but the problem will exist for any > > characters. > > That's why I suggested appending the AV code to the entity name, instead of > prepending it to the AV value. Have you considered this ? I failed to make it work, see last patch. Maybe there is something obvious I missed. (In reply to Jonathan Druart from comment #98) > Created attachment 101070 [details] [review] [review] > Bug 20307: Replace separator with # > > To avoid colision we replace the separator with something that is > supposed to be less used than underscore > > The problematic case was explaing by Julian on comment 96: > """ > 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. > """ It sounds unlikely as our categories should be unique enough, aren't they? (In reply to Katrin Fischer from comment #102) > It sounds unlikely as our categories should be unique enough, aren't they? But users can create their own categories. I agree that it is unlikely, but it is still possible. Marked as blocked by bug 24975 that offers a more elegant way to localize our DB strings. (In reply to Jonathan Druart from comment #104) > Marked as blocked by bug 24975 that offers a more elegant way to localize > our DB strings. Too bad that this one is Blocked and the other one is Does not apply. (In reply to Marcel de Rooy from comment #105) > (In reply to Jonathan Druart from comment #104) > > Marked as blocked by bug 24975 that offers a more elegant way to localize > > our DB strings. > > Too bad that this one is Blocked and the other one is Does not apply. Can you help please? Jonathan is absent for a couple months. Ping people Give your opinion on the 2 implementations, that's the only way to make things move forward. (In reply to Jonathan Druart from comment #108) > Give your opinion on the 2 implementations, that's the only way to make > things move forward. Is this something we could maybe gain more attention for by putting it on the roadmap or on the agenda for a meeting? I really want it to happen, but not sure if I could give helpful feedback on implementation. See bug 24977 comment 9, it was the last movement for this. I am out however. (In reply to Jonathan Druart from comment #110) > See bug 24977 comment 9, it was the last movement for this. > > I am out however. Does it mean we should follow this approach? Would this benefit from putting some heads together at the hackfest? (In reply to Katrin Fischer from comment #111) > (In reply to Jonathan Druart from comment #110) > > See bug 24977 comment 9, it was the last movement for this. > > > > I am out however. > > Does it mean we should follow this approach? It means what it says. There are FIXMEs and bad perf impact possible. It needed feedback and help. |