Do these patches make sense?

It seems that some call to getTranslatedLanguages are wrong:
1.
members/memberentry.pl:    my $translated_languages = C4::Languages::getTranslatedLanguages( 'opac', C4::Context->preference('template') );
# template is for the intranet, we should use opacthemes instead

2. tools/letter.pl
159         my $translated_languages =
160           C4::Languages::getTranslatedLanguages( 'opac', 
161             C4::Context->preference('template') );

Additional note: "template" and "opacthemes" are confusing, the same terms must be used

TODO: We should add a warning to koha-translate to restart memcached (or better, clear only what is needed)

(In reply to M. Tompsett from comment #5)
> Comment on attachment 71098 [details] [review] [review]
> Bug 20116: Cache language list
> 
> Review of attachment 71098 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Languages.pm
> @@ +122,3 @@
> >      if ($interface && $interface eq 'opac' ) {
> >          my $htdocs = C4::Context->config('opachtdocs');
> >          @languages = _get_opac_language_dirs( $htdocs, $theme );
> 
> Forgot to delete this line, I think.

Indeed, thanks for catching it!

Created attachment 75382 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Created attachment 75384 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Created attachment 75385 [details] [review]
Bug 20116: Cache language list

Before the patches, C4::Languages::getTranslatedLanguages('intranet', 'prog') took between 0.002 and 0.003s

Now it's 0.0015s (and I guess the difference can be much better when hundred of queries are made at the same time).

(Of course first hit is worse)

Jonathan, it looks great. Will you provide a couple tests for this?

(In reply to Tomás Cohen Arazi from comment #18)
> Jonathan, it looks great. Will you provide a couple tests for this?

I do not think it can be done easily, any ideas?

I don't know if this matters - I gave this a go in a devbox, copied all the files in where they needed to be etc etc, and got this on a koha-translate --install de-DE:


Connection to the memcached servers '__MEMCACHED_SERVERS__' failed. Are the unix socket permissions set properly? Is the host reachable?
If you ignore this warning, you will face performance issues

Connection to the memcached servers '__MEMCACHED_SERVERS__' failed. Are the unix socket permissions set properly? Is the host reachable?
If you ignore this warning, you will face performance issues 

Now, I realise that may be because this feature wasn't installed as part of the package and thusly some data might not have been subbed in like it was supposed to be and this will work fine when it's installed properly via apt. If you think this is OK, I'll sign off on it.

Had a talk with Tomas, he thinks this is fine so I"ll attach a signed off one for QA to have a look at.

Liz

Created attachment 88200 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 88201 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 88202 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

+    my @enabled_languages =
+      ( $interface && $interface eq 'intranet' )
+      ? split ",", C4::Context->preference('language')
+      : split ",", C4::Context->preference('opaclanguage');

Seems to reverse the previous logic. If eq opac then opac else intranet.

(In reply to Marcel de Rooy from comment #25)
> +    my @enabled_languages =
> +      ( $interface && $interface eq 'intranet' )
> +      ? split ",", C4::Context->preference('language')
> +      : split ",", C4::Context->preference('opaclanguage');
> 
> Seems to reverse the previous logic. If eq opac then opac else intranet.

I do not think I get that one, please read again the whole diff:

before:
if $interface == opac:
  use opaclanguages
elsif $interface == intranet:
  use language
else:
  use opaclanguages

after:
if $interface == intranet:
  use language
else:
  use opaclanguages

Looks equal for me.

(In reply to Jonathan Druart from comment #26)
> (In reply to Marcel de Rooy from comment #25)
> > +    my @enabled_languages =
> > +      ( $interface && $interface eq 'intranet' )
> > +      ? split ",", C4::Context->preference('language')
> > +      : split ",", C4::Context->preference('opaclanguage');
> > 
> > Seems to reverse the previous logic. If eq opac then opac else intranet.
> 
> I do not think I get that one, please read again the whole diff:
> 
> before:
> if $interface == opac:
>   use opaclanguages
> elsif $interface == intranet:
>   use language
> else:
>   use opaclanguages
> 
> after:
> if $interface == intranet:
>   use language
> else:
>   use opaclanguages
> 
> Looks equal for me.

Seems so. OK

Hmm What about adding a few tests now? :)

(In reply to Jonathan Druart from comment #19)
> (In reply to Tomás Cohen Arazi from comment #18)
> > Jonathan, it looks great. Will you provide a couple tests for this?
> 
> I do not think it can be done easily, any ideas?

iirc other subs also are tested with mocking cache

Hi Marcel,
I have still no idea how to write useful tests for this change, can you give me more details please?

(In reply to Jonathan Druart from comment #30)
> Hi Marcel,
> I have still no idea how to write useful tests for this change, can you give
> me more details please?

Mock the directory structure too where Koha looks for languages. Remove entries from the cache, etc.

I do not see the point of those tests. The patches are just caching a value, there is nothing to prove.

Asking for another QA feedback.

Comment on attachment 88201 [details] [review]
Bug 20116: Flush the caches

Review of attachment 88201 [details] [review]:
-----------------------------------------------------------------

::: debian/scripts/koha-translate
@@ +233,5 @@
>  
> +flush_cache()
> +{
> +    if [ "$dev" = "" ]; then
> +        koha-foreach --enabled "$KOHA_HOME/misc/bin/clear_cache.pl"

Is $KOHA_HOME/misc/bin going to exist? And is clear_cache.pl going to get put there? Something is fishy for running this live.

In addition to the whole $KOHA_HOME path existing in live problem, perhaps the flushing could be split to a separate patch, so that bug 20884 could depend on that, and not this?

(In reply to M. Tompsett from comment #38)
> Comment on attachment 88201 [details] [review] [review]
> Bug 20116: Flush the caches
> 
> Review of attachment 88201 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: debian/scripts/koha-translate
> @@ +233,5 @@
> >  
> > +flush_cache()
> > +{
> > +    if [ "$dev" = "" ]; then
> > +        koha-foreach --enabled "$KOHA_HOME/misc/bin/clear_cache.pl"
> 
> Is $KOHA_HOME/misc/bin going to exist? And is clear_cache.pl going to get
> put there? Something is fishy for running this live.

How that? Did you `git grep KOHA_HOME`?

(In reply to M. Tompsett from comment #39)
> In addition to the whole $KOHA_HOME path existing in live problem, perhaps
> the flushing could be split to a separate patch, so that bug 20884 could
> depend on that, and not this?

Or we push this and bug 20884 will benefit from it? :)

Still waiting for another QA feedback, back to NQA.

(In reply to Jonathan Druart from comment #42)
> Still waiting for another QA feedback, back to NQA.

I am still hoping for tests :)
Note even that this patch set breaks a test when these patches are applied and does not when not.

t/db_dependent/Languages.t (Wstat: 512 Tests: 18 Failed: 2)
  Failed tests:  11, 13

Created attachment 94995 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 94996 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Created attachment 94997 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

(In reply to Marcel de Rooy from comment #43)
> (In reply to Jonathan Druart from comment #42)
> > Still waiting for another QA feedback, back to NQA.
> 
> I am still hoping for tests :)

Hi Marcel,
Yes I know, but I still not know how to write them usefully. I switched back to SO to get the point of view of an other QAer.
See comments 28 to 32.

> Note even that this patch set breaks a test when these patches are applied
> and does not when not.
> 
> t/db_dependent/Languages.t (Wstat: 512 Tests: 18 Failed: 2)
>   Failed tests:  11, 13

Thanks for catching that, fixed.

I tried testing this, not sure if what I saw is to be expected:

sudo koha-translate --update de-DE --dev kohadev

German is updated, activated via sysprefs

sudo koha-translate --remove de-DE --dev kohadev
sudo koha-translate --list --dev kohadev

Only shows inactive fr-FR.

But: In the OPAC German is still visible and only disappears when I run restart_all.

I reinstalled the language:
sudo koha-translate --install de-DE --dev kohadev

As an additional find, searching the language preferences from the German interface exploded in an error (search for langu):

Can't use an undefined value as an ARRAY reference at /usr/lib/x86_64-linux-gnu/perl5/5.24/YAML/Syck.pm line 76

Jonathan, can you have a look? Just switching to FQA temporarily.

Hi Katrin,
My bet is that you tested without the correct koha-translate file. Are you sure you copied the one patched with this change?
You can use reset_all to make sure it will be copied.

Tested right now and it works as expected.

That's correct, I didn't copy the file. From where to where do you have to copy it? A test plan would be helpful.

(In reply to Katrin Fischer from comment #50)
> That's correct, I didn't copy the file. From where to where do you have to
> copy it? A test plan would be helpful.

Koha scripts are in /usr/sbin/koha-translate (`which koha-translate` tell you where it is).

A test plan could be:
0/ Apply the patch and `reset_all`
1/ Install languages, enable them at staff and OPAC
2/ Confirm you have the languages listed at the bottom
3/ Uninstall one language
4/ Confirm that it does not longer appear in the language at the bottom of the pages and in the prefs

Also play with koha-translate --install, --remove and --list

Additionally, QA will make sure that the changes make sense.

Created attachment 99431 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Created attachment 99432 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Created attachment 99433 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

QAing

Created attachment 103007 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 103008 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 103009 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Nice work everyone!

Pushed to master for 20.05

enhancement not backported to 19.11.x

On a package install I get:

root@koha-debian10:~# koha-translate --install es-ES                                                                                                                                                              
/bin/bash: /usr/share/koha/misc/bin/clear_cache.pl: No such file or directory


The script is in /usr/share/koha/bin/clear_cache.pl

Created attachment 105359 [details] [review]
Bug 20116: Fix package install path for clear_cache.pl

root@koha-debian10:~# koha-translate --install es-ES
/bin/bash: /usr/share/koha/misc/bin/clear_cache.pl: No such file or directory

The script is in /usr/share/koha/bin/clear_cache.pl

Last patch pushed to master for 20.05