Bug 20116 - Cache language list
Summary: Cache language list
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 20115
Blocks: 7923
  Show dependency treegraph
 
Reported: 2018-01-31 18:20 UTC by Jonathan Druart
Modified: 2019-06-20 01:20 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20116: Prepare the ground (3.72 KB, patch)
2018-01-31 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.73 KB, patch)
2018-01-31 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.73 KB, patch)
2018-04-02 14:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.72 KB, patch)
2018-04-04 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.73 KB, patch)
2018-04-04 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.72 KB, patch)
2018-05-16 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.83 KB, patch)
2018-05-16 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Flush the caches (2.37 KB, patch)
2018-05-16 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.89 KB, patch)
2018-05-16 20:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.76 KB, patch)
2019-04-17 14:55 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20116: Flush the caches (2.42 KB, patch)
2019-04-17 14:55 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.93 KB, patch)
2019-04-17 14:55 UTC, Liz Rea
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-01-31 18:20:45 UTC
C4::Languages::getTranslatedLanguages calls _get_language_dirs that list directories to know the available themes.
It sounds like we could avoid that and cache it instead.
Comment 1 Jonathan Druart 2018-01-31 18:21:36 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2018-01-31 18:21:39 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2018-01-31 18:25:06 UTC
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
Comment 4 Jonathan Druart 2018-01-31 18:27:22 UTC
TODO: We should add a warning to koha-translate to restart memcached (or better, clear only what is needed)
Comment 5 M. Tompsett 2018-04-02 02:18:42 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2018-04-02 14:09:42 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2018-04-02 14:09:59 UTC
(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!
Comment 8 Josef Moravec 2018-04-04 11:28:23 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2018-04-04 12:54:59 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2018-04-04 12:55:03 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2018-04-04 12:55:31 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2018-05-16 20:34:22 UTC
Created attachment 75382 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable
Comment 13 Jonathan Druart 2018-05-16 20:34:28 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2018-05-16 20:34:32 UTC
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
Comment 15 Jonathan Druart 2018-05-16 20:40:25 UTC
Created attachment 75385 [details] [review]
Bug 20116: Cache language list
Comment 16 Jonathan Druart 2018-05-16 20:58:27 UTC
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).
Comment 17 Jonathan Druart 2018-05-16 21:00:20 UTC
(Of course first hit is worse)
Comment 18 Tomás Cohen Arazi 2018-08-20 12:40:28 UTC
Jonathan, it looks great. Will you provide a couple tests for this?
Comment 19 Jonathan Druart 2018-08-24 18:38:49 UTC
(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?
Comment 20 Liz Rea 2019-04-16 17:36:01 UTC
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.
Comment 21 Liz Rea 2019-04-17 14:32:41 UTC
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
Comment 22 Liz Rea 2019-04-17 14:55:15 UTC
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>
Comment 23 Liz Rea 2019-04-17 14:55:24 UTC
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>
Comment 24 Liz Rea 2019-04-17 14:55:27 UTC
Created attachment 88202 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 25 Marcel de Rooy 2019-04-26 08:55:39 UTC
+    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.
Comment 26 Jonathan Druart 2019-04-26 22:14:02 UTC
(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.
Comment 27 Marcel de Rooy 2019-05-03 09:44:20 UTC
(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
Comment 28 Marcel de Rooy 2019-05-03 09:51:50 UTC
Hmm What about adding a few tests now? :)
Comment 29 Marcel de Rooy 2019-05-03 09:52:42 UTC
(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
Comment 30 Jonathan Druart 2019-05-03 12:57:29 UTC
Hi Marcel,
I have still no idea how to write useful tests for this change, can you give me more details please?
Comment 31 Marcel de Rooy 2019-05-06 06:30:06 UTC
(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.
Comment 32 Jonathan Druart 2019-05-06 15:36:05 UTC
I do not see the point of those tests. The patches are just caching a value, there is nothing to prove.
Comment 33 Guillaume 2019-05-12 18:53:44 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2019-05-13 00:11:34 UTC Comment hidden (obsolete)
Comment 35 Guillaume 2019-05-13 18:11:05 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2019-05-13 23:40:38 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2019-06-11 16:44:31 UTC
Asking for another QA feedback.
Comment 38 M. Tompsett 2019-06-17 16:10:35 UTC
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.
Comment 39 M. Tompsett 2019-06-17 16:11:53 UTC
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?
Comment 40 Jonathan Druart 2019-06-20 01:18:46 UTC
(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`?
Comment 41 Jonathan Druart 2019-06-20 01:20:28 UTC
(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? :)