Bug 20307 - Language overlay for authorized values
Summary: Language overlay for authorized values
Status: BLOCKED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords: rel_20_11_target, RM_priority
Depends on: 14100
Blocks: 11815 26403 22610
  Show dependency treegraph
 
Reported: 2018-02-27 21:34 UTC by Jonathan Druart
Modified: 2020-09-09 14:21 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20307: Add DB column localization.interface (1.47 KB, patch)
2019-03-14 18:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes (1.61 KB, patch)
2019-03-14 18:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Update the localization unique key (1.57 KB, patch)
2019-03-14 18:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes 2 (1.44 KB, patch)
2019-03-14 18:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Language overlay for authorised values (20.83 KB, patch)
2019-03-14 18:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Some more fixes (15.63 KB, patch)
2019-03-14 18:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: (QA follow-up) Move DB update to perl (1.87 KB, patch)
2019-05-10 10:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Add DB column localization.interface (1.53 KB, patch)
2019-07-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes (1.67 KB, patch)
2019-07-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Update the localization unique key (1.63 KB, patch)
2019-07-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes 2 (1.50 KB, patch)
2019-07-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Language overlay for authorised values (20.83 KB, patch)
2019-07-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Some more fixes (15.56 KB, patch)
2019-07-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: (QA follow-up) Move DB update to perl (1.93 KB, patch)
2019-07-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: (QA follow-up) Move DB update to perl (1.93 KB, patch)
2019-07-19 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Some more fixes (15.53 KB, patch)
2019-08-14 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Remove debug statement Carp::Always (659 bytes, patch)
2019-08-14 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Disable the translate links if the object has not been saved yet (4.55 KB, patch)
2019-08-19 20:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion (6.26 KB, patch)
2019-08-19 20:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Code for AVs must be category and value (4.31 KB, patch)
2019-08-19 20:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Add DB column localization.interface (1.53 KB, patch)
2019-10-16 07:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes (1.67 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Update the localization unique key (1.63 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes 2 (1.50 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Language overlay for authorised values (20.63 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: (QA follow-up) Move DB update to perl (1.93 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Some more fixes (15.51 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Remove debug statement Carp::Always (659 bytes, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Disable the translate links if the object has not been saved yet (4.55 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion (6.25 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Code for AVs must be category and value (4.30 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Few more fixes due to bad rebase conflict resolutions (4.36 KB, patch)
2019-10-16 07:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Add DB column localization.interface (1.57 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes (1.72 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Update the localization unique key (1.68 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes 2 (1.55 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Language overlay for authorised values (20.68 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: (QA follow-up) Move DB update to perl (1.98 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Some more fixes (15.55 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Remove debug statement Carp::Always (707 bytes, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Disable the translate links if the object has not been saved yet (4.60 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion (6.30 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Code for AVs must be category and value (4.35 KB, patch)
2019-10-24 21:44 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Few more fixes due to bad rebase conflict resolutions (4.40 KB, patch)
2019-10-24 21:45 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20307: Fix existing tests (3.18 KB, patch)
2020-01-09 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Fix logic in search_with_localization and add tests (12.21 KB, patch)
2020-01-09 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20307: Add DB column localization.interface (1.64 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes (1.78 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Update the localization unique key (1.74 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: DBIC Schema changes 2 (1.61 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Language overlay for authorised values (20.67 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: (QA follow-up) Move DB update to perl (2.04 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Some more fixes (15.55 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Remove debug statement Carp::Always (772 bytes, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Disable the translate links if the object has not been saved yet (4.65 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Use 'auhorised_values' instead of 'avs' to avoid confusion (6.33 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Code for AVs must be category and value (4.40 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Few more fixes due to bad rebase conflict resolutions (4.44 KB, patch)
2020-02-13 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Fix existing tests (3.26 KB, patch)
2020-02-13 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Fix logic in search_with_localization and add tests (12.25 KB, patch)
2020-02-13 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20307: Replace separator with # (5.25 KB, patch)
2020-03-20 10:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
[DO NOT APPLY/PUSH] Bug 20307: Move av.category to loc.entity (5.85 KB, patch)
2020-03-23 14:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-02-27 21:34:39 UTC

    
Comment 1 Katrin Fischer 2018-06-22 12:49:20 UTC
Is there any work yet planned or done on this?
Comment 2 Jonathan Druart 2018-06-22 13:09:57 UTC
(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.
Comment 3 Katrin Fischer 2018-07-01 18:55:18 UTC
Ok, a little confused by the graph - didn't spot any patch with patches still needing work. Can you point me the right direction?
Comment 4 Jonathan Druart 2018-07-02 13:52:00 UTC
(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.
Comment 5 Jonathan Druart 2019-03-02 21:19:19 UTC
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)?
Comment 6 Katrin Fischer 2019-03-05 06:48:35 UTC
(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.
Comment 7 Stefan Berndtsson 2019-03-13 13:05:07 UTC
(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.
Comment 8 Jonathan Druart 2019-03-14 18:02:20 UTC
Created attachment 86622 [details] [review]
Bug 20307: Add DB column localization.interface
Comment 9 Jonathan Druart 2019-03-14 18:02:25 UTC
Created attachment 86623 [details] [review]
Bug 20307: DBIC Schema changes
Comment 10 Jonathan Druart 2019-03-14 18:02:29 UTC
Created attachment 86624 [details] [review]
Bug 20307: Update the localization unique key
Comment 11 Jonathan Druart 2019-03-14 18:02:35 UTC
Created attachment 86625 [details] [review]
Bug 20307: DBIC Schema changes 2
Comment 12 Jonathan Druart 2019-03-14 18:02:41 UTC
Created attachment 86626 [details] [review]
Bug 20307: Language overlay for authorised values
Comment 13 Jonathan Druart 2019-03-14 18:02:45 UTC
Created attachment 86627 [details] [review]
Bug 20307: Some more fixes
Comment 14 Jonathan Druart 2019-03-14 18:06:16 UTC
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.
Comment 15 Martin Renvoize 2019-04-24 20:29:17 UTC
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.
Comment 16 Jonathan Druart 2019-05-09 15:53:34 UTC
Updating the status to Needs Signoff (to get more love).
Comment 17 Martin Renvoize 2019-05-10 10:13:11 UTC
Created attachment 89538 [details] [review]
Bug 20307: (QA follow-up) Move DB update to perl
Comment 18 Martin Renvoize 2019-05-10 10:14:46 UTC
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.
Comment 19 Jonathan Druart 2019-05-10 12:54:25 UTC
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.
Comment 20 Martin Renvoize 2019-05-10 17:47:53 UTC
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.
Comment 21 Hayley Mapley 2019-06-12 01:43:37 UTC
Hi,

Is there a test plan for this?
Comment 22 Jonathan Druart 2019-06-20 01:34:06 UTC
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.
Comment 23 Martin Renvoize 2019-07-19 12:00:15 UTC
Created attachment 91645 [details] [review]
Bug 20307: Add DB column localization.interface

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2019-07-19 12:00:19 UTC
Created attachment 91646 [details] [review]
Bug 20307: DBIC Schema changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2019-07-19 12:00:22 UTC
Created attachment 91647 [details] [review]
Bug 20307: Update the localization unique key

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2019-07-19 12:00:26 UTC
Created attachment 91648 [details] [review]
Bug 20307: DBIC Schema changes 2

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2019-07-19 12:00:29 UTC
Created attachment 91649 [details] [review]
Bug 20307: Language overlay for authorised values

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2019-07-19 12:00:33 UTC
Created attachment 91650 [details] [review]
Bug 20307: Some more fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2019-07-19 12:00:36 UTC
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>
Comment 30 Martin Renvoize 2019-07-19 12:09:02 UTC
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..
Comment 31 Martin Renvoize 2019-07-19 12:18:33 UTC
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>
Comment 32 Martin Renvoize 2019-07-19 13:30:21 UTC
Needs unit tests ;)
Comment 33 Katrin Fischer 2019-08-09 08:15:52 UTC
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?
Comment 34 Jonathan Druart 2019-08-14 16:31:33 UTC
(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.
Comment 35 Jonathan Druart 2019-08-14 16:32:11 UTC
Created attachment 92192 [details] [review]
Bug 20307: Some more fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Jonathan Druart 2019-08-14 16:32:15 UTC
Created attachment 92193 [details] [review]
Bug 20307: Remove debug statement Carp::Always
Comment 37 Katrin Fischer 2019-08-16 05:54:16 UTC
Back here... :)
Comment 38 Katrin Fischer 2019-08-16 06:03:53 UTC
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!
Comment 39 Katrin Fischer 2019-08-16 06:08:42 UTC
Sorry, my fault... forgot a restart_all.
Comment 40 Katrin Fischer 2019-08-16 06:26:17 UTC
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...
Comment 41 Jonathan Druart 2019-08-19 20:20:58 UTC
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)
Comment 42 Jonathan Druart 2019-08-19 20:21:02 UTC
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
Comment 43 Jonathan Druart 2019-08-19 20:21:09 UTC
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.
Comment 44 Jonathan Druart 2019-08-19 20:22:51 UTC
(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.
Comment 45 Katrin Fischer 2019-08-20 21:10:17 UTC
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?
Comment 46 Jonathan Druart 2019-09-01 14:57:21 UTC
(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?
Comment 47 Jonathan Druart 2019-10-16 07:42:56 UTC
Created attachment 94245 [details] [review]
Bug 20307: Add DB column localization.interface

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Jonathan Druart 2019-10-16 07:43:00 UTC
Created attachment 94246 [details] [review]
Bug 20307: DBIC Schema changes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Jonathan Druart 2019-10-16 07:43:04 UTC
Created attachment 94247 [details] [review]
Bug 20307: Update the localization unique key

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Jonathan Druart 2019-10-16 07:43:08 UTC
Created attachment 94248 [details] [review]
Bug 20307: DBIC Schema changes 2

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Jonathan Druart 2019-10-16 07:43:13 UTC
Created attachment 94249 [details] [review]
Bug 20307: Language overlay for authorised values

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Jonathan Druart 2019-10-16 07:43:17 UTC
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>
Comment 53 Jonathan Druart 2019-10-16 07:43:21 UTC
Created attachment 94251 [details] [review]
Bug 20307: Some more fixes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Jonathan Druart 2019-10-16 07:43:25 UTC
Created attachment 94252 [details] [review]
Bug 20307: Remove debug statement Carp::Always
Comment 55 Jonathan Druart 2019-10-16 07:43:29 UTC
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)
Comment 56 Jonathan Druart 2019-10-16 07:43:33 UTC
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
Comment 57 Jonathan Druart 2019-10-16 07:43:37 UTC
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.
Comment 58 Jonathan Druart 2019-10-16 07:43:41 UTC
Created attachment 94256 [details] [review]
Bug 20307: Few more fixes due to bad rebase conflict resolutions
Comment 59 Michal Denar 2019-10-24 21:44:06 UTC
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>
Comment 60 Michal Denar 2019-10-24 21:44:11 UTC
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>
Comment 61 Michal Denar 2019-10-24 21:44:15 UTC
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>
Comment 62 Michal Denar 2019-10-24 21:44:20 UTC
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>
Comment 63 Michal Denar 2019-10-24 21:44:25 UTC
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>
Comment 64 Michal Denar 2019-10-24 21:44:30 UTC
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>
Comment 65 Michal Denar 2019-10-24 21:44:35 UTC
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>
Comment 66 Michal Denar 2019-10-24 21:44:41 UTC
Created attachment 94707 [details] [review]
Bug 20307: Remove debug statement Carp::Always

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 67 Michal Denar 2019-10-24 21:44:45 UTC
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>
Comment 68 Michal Denar 2019-10-24 21:44:51 UTC
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>
Comment 69 Michal Denar 2019-10-24 21:44:56 UTC
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>
Comment 70 Michal Denar 2019-10-24 21:45:01 UTC
Created attachment 94711 [details] [review]
Bug 20307: Few more fixes due to bad rebase conflict resolutions

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 71 Jonathan Druart 2019-10-25 09:00:21 UTC
QA: This is not a priority for 19.11, it's too late. But please keep it in mind for early 19.12 ;)
Comment 72 Katrin Fischer 2019-10-28 06:55:05 UTC
(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!
Comment 73 Martin Renvoize 2019-10-28 09:36:33 UTC
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 :)
Comment 74 Martin Renvoize 2020-01-08 14:08:45 UTC
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 :)
Comment 75 Tomás Cohen Arazi 2020-01-08 14:19:42 UTC
(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::*?
Comment 76 Tomás Cohen Arazi 2020-01-08 14:33:38 UTC
(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.
Comment 77 Kyle M Hall 2020-01-08 14:36:23 UTC
Could you please add unit tests for the new subs?
Comment 78 Jonathan Druart 2020-01-09 13:19:26 UTC
Created attachment 97074 [details] [review]
Bug 20307: Fix existing tests
Comment 79 Jonathan Druart 2020-01-09 13:19:30 UTC
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 |
Comment 80 Jonathan Druart 2020-01-09 13:20:08 UTC
I would like another signoff here.
Comment 81 Martin Renvoize 2020-02-13 16:09:13 UTC
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>
Comment 82 Martin Renvoize 2020-02-13 16:09:17 UTC
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>
Comment 83 Martin Renvoize 2020-02-13 16:09:21 UTC
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>
Comment 84 Martin Renvoize 2020-02-13 16:09:25 UTC
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>
Comment 85 Martin Renvoize 2020-02-13 16:09:29 UTC
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>
Comment 86 Martin Renvoize 2020-02-13 16:09:32 UTC
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>
Comment 87 Martin Renvoize 2020-02-13 16:09:36 UTC
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>
Comment 88 Martin Renvoize 2020-02-13 16:09:40 UTC
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>
Comment 89 Martin Renvoize 2020-02-13 16:09:44 UTC
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>
Comment 90 Martin Renvoize 2020-02-13 16:09:48 UTC
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>
Comment 91 Martin Renvoize 2020-02-13 16:09:51 UTC
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>
Comment 92 Martin Renvoize 2020-02-13 16:09:55 UTC
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>
Comment 93 Martin Renvoize 2020-02-13 16:10:01 UTC
Created attachment 98875 [details] [review]
Bug 20307: Fix existing tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 94 Martin Renvoize 2020-02-13 16:10:05 UTC
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>
Comment 95 Martin Renvoize 2020-02-13 16:13:16 UTC
Re-tested and all seems to be working well.. Signing off...
Comment 96 Julian Maurice 2020-02-26 16:44:27 UTC
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
Comment 97 Jonathan Druart 2020-03-06 11:13:25 UTC
(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.
Comment 98 Jonathan Druart 2020-03-20 10:55:39 UTC
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.
"""
Comment 99 Julian Maurice 2020-03-23 12:27:13 UTC
(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 ?
Comment 100 Jonathan Druart 2020-03-23 14:24:17 UTC
Created attachment 101450 [details] [review]
[DO NOT APPLY/PUSH] Bug 20307: Move av.category to loc.entity
Comment 101 Jonathan Druart 2020-03-23 14:24:45 UTC
(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.
Comment 102 Katrin Fischer 2020-03-23 14:53:16 UTC
(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?
Comment 103 Julian Maurice 2020-03-23 14:58:42 UTC
(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.
Comment 104 Jonathan Druart 2020-03-25 13:56:42 UTC
Marked as blocked by bug 24975 that offers a more elegant way to localize our DB strings.