Attachment 74052 Details for Bug 14769 – Bug 14769: (QA follow-up) Remove global var $cached_indicators

[patch] Bug 14769: (QA follow-up) Remove global var $cached_indicators

Bug-14769-QA-follow-up-Remove-global-var-cachedind.patch (text/plain), 4.37 KB, created by Marcel de Rooy on 2018-04-12 07:38:06 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2018-04-12 07:38:06 UTC

Size: 4.37 KB

patch

obsolete

>From 54405e6cb02d05afa22583173eabf47d3ce20e16 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Thu, 12 Apr 2018 08:54:34 +0200
>Subject: [PATCH] Bug 14769: (QA follow-up) Remove global var
> $cached_indicators
>Content-Type: text/plain; charset=utf-8
>
>As requested by RM, this patch replaces using the global $cached_indicators
>by saving state temporarily during the (limited) lifetime of the object.
>
>Essentially this affects two places in code:
>[1] blinddetail-biblio-search.pl (loading auth record in editor)
>[2] AuthoritiesMarc::merge (merging authority into biblios)
>
>Concurrent runs of [1] and/or [2] together with a simultaneous pref change
>just in between could cause slight (hypothetical) side-effects.
>The current approach of keeping state in the object makes that a series of
>controlled_indicators calls during an immediate merge of one specific
>authority is not affected by a simultaneous pref change. So the same rules
>are applied to the set of attached biblio record for that authority.
>Note also that the cron job ignores a simultaneous pref change, since
>it reads from the unchanged L1 cache (yes, also hypothetical).
>
>Test plan:
>[1] Run t/Koha/Authority/ControlledIndicators.t
>[2] Run t/db_dependent/Authority/Merge.t
>[3] Run t/db_dependent/Koha/Authorities.t
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> Koha/Authority/ControlledIndicators.pm  | 22 ++++------------------
> t/Koha/Authority/ControlledIndicators.t | 20 +-------------------
> 2 files changed, 5 insertions(+), 37 deletions(-)
>
>diff --git a/Koha/Authority/ControlledIndicators.pm b/Koha/Authority/ControlledIndicators.pm
>index 64a8734..0cd688d 100644
>--- a/Koha/Authority/ControlledIndicators.pm
>+++ b/Koha/Authority/ControlledIndicators.pm
>@@ -20,8 +20,6 @@ package Koha::Authority::ControlledIndicators;
> use Modern::Perl;
> use C4::Context;
> 
>-our $cached_indicators;
>-
> =head1 NAME
> 
> Koha::Authority::ControlledIndicators - Obtain biblio indicators, controlled by authority record
>@@ -39,7 +37,6 @@ Koha::Authority::ControlledIndicators - Obtain biblio indicators, controlled by
> sub new {
>     my ( $class, $params ) = @_;
>     $params = {} if ref($params) ne 'HASH';
>-    $cached_indicators = undef;
>     return bless $params, $class;
> }
> 
>@@ -66,11 +63,11 @@ sub get {
>     $flavour = uc($flavour);
>     $flavour = 'UNIMARC' if $flavour eq 'UNIMARCAUTH';
> 
>-    $cached_indicators //= _load_pref();
>+    $self->{_parsed} //= _load_pref();
>     my $result = {};
>-    return $result if !exists $cached_indicators->{$flavour};
>-    my $rule = $cached_indicators->{$flavour}->{$tag} //
>-        $cached_indicators->{$flavour}->{'*'} //
>+    return $result if !exists $self->{_parsed}->{$flavour};
>+    my $rule = $self->{_parsed}->{$flavour}->{$tag} //
>+        $self->{_parsed}->{$flavour}->{'*'} //
>         {};
>     my $report_fld = $record ? $record->field( $report_tag ) : undef;
> 
>@@ -146,17 +143,6 @@ sub _thesaurus_info {
>     return ( $ind, $sub2 );
> }
> 
>-=head3 clear
>-
>-    Clear internal cache.
>-
>-=cut
>-
>-sub clear {
>-    my ( $self, $params ) = @_;
>-    $cached_indicators = undef;
>-}
>-
> =head1 AUTHOR
> 
>     Marcel de Rooy, Rijksmuseum Amsterdam, The Netherlands
>diff --git a/t/Koha/Authority/ControlledIndicators.t b/t/Koha/Authority/ControlledIndicators.t
>index 8552507..1d51e30 100644
>--- a/t/Koha/Authority/ControlledIndicators.t
>+++ b/t/Koha/Authority/ControlledIndicators.t
>@@ -10,7 +10,7 @@ use t::lib::Mocks;
> use Koha::Authority::ControlledIndicators;
> 
> subtest "Simple tests" => sub {
>-    plan tests => 10;
>+    plan tests => 8;
> 
>     t::lib::Mocks::mock_preference('AuthorityControlledIndicators', q|
> marc21,600,ind1:auth1,ind2:x
>@@ -52,24 +52,6 @@ marc21,800,ind1:,
>     });
>     is( $res->{ind1}, '', 'ind1: clears 1st indicator' );
>     is( exists $res->{ind2}, '', 'Check if 2nd indicator key does not exist' );
>-
>-    # Test caching
>-    t::lib::Mocks::mock_preference('AuthorityControlledIndicators', q{} );
>-    $res = $oInd->get({
>-        flavour => "MARC21",
>-        report_tag  => '100',
>-        auth_record => $record,
>-        biblio_tag  => '700',
>-    });
>-    is( $res->{ind1}, '4', 'Cache not cleared yet' );
>-    $oInd->clear;
>-    $res = $oInd->get({
>-        flavour => "MARC21",
>-        report_tag  => '100',
>-        auth_record => $record,
>-        biblio_tag  => '700',
>-    });
>-    is_deeply( $res, {}, 'Cache cleared' );
> };
> 
> subtest "Tests for sub _thesaurus_info" => sub {
>-- 
>2.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 14769: 42197 | 69343 | 70948 | 70949 | 70950 | 70951 | 71338 | 71339 | 71340 | 71341 | 71342 | 71343 | 71351 | 71352 | 71353 | 71354 | 71355 | 71366 | 71374 | 71375 | 71376 | 71377 | 71378 | 71379 | 71380 | 71381 | 72398 | 72399 | 72400 | 72401 | 72402 | 72403 | 72404 | 73389 | 73773 | 73774 | 73775 | 73776 | 73777 | 73778 | 73779 | 74052