Attachment 38622 Details for Bug 14057 – Bug 14057 - A Rough Start

[patch] Bug 14057 - A Rough Start

Bug-14057---A-Rough-Start.patch (text/plain), 15.37 KB, created by Mark Tompsett on 2015-04-29 03:12:39 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Tompsett

Created: 2015-04-29 03:12:39 UTC

Size: 15.37 KB

patch

obsolete

>From d6ea06f9d61066498d30183c29026b63a2d03566 Mon Sep 17 00:00:00 2001
>From: Mark Tompsett <mtompset@hotmail.com>
>Date: Fri, 24 Apr 2015 00:40:46 -0400
>Subject: [PATCH] Bug 14057 - A Rough Start
>
>This patch is a rough start. I believe it runs exponentially
>faster, but its equality to the previous version needs to be
>tested before I clean it up to acceptable standards.
>
>Nested hashes of hashes was being a debugging nightmare.
>
>Moved the SQL select to C4::Koha.
>
>Changed the GetItemsForInventory to have a hashref parameter.
>Added interface, in case there is a need for 'opac' vs. 'staff'.
>
>Added t/db_dependent/Items/GetItemsForInventory.t
>Added t/db_dependent/Koha/GetKohaAuthorisedValuesMapping.t
>---
> C4/Items.pm                                        |  41 ++++--
> C4/Koha.pm                                         |  45 ++++++
> t/db_dependent/Items/GetItemsForInventory.t        | 156 +++++++++++++++++++++
> .../Koha/GetKohaAuthorisedValuesMapping.t          |  54 +++++++
> tools/inventory.pl                                 |  30 +++-
> 5 files changed, 316 insertions(+), 10 deletions(-)
> create mode 100755 t/db_dependent/Items/GetItemsForInventory.t
> create mode 100755 t/db_dependent/Koha/GetKohaAuthorisedValuesMapping.t
>
>diff --git a/C4/Items.pm b/C4/Items.pm
>index 11ff4ca..d1a866b 100644
>--- a/C4/Items.pm
>+++ b/C4/Items.pm
>@@ -1024,7 +1024,20 @@ sub GetLostItems {
> 
> =head2 GetItemsForInventory
> 
>-($itemlist, $iTotalRecords)  = GetItemsForInventory($minlocation, $maxlocation, $location, $itemtype, $ignoreissued, $datelastseen, $branchcode, $offset, $size, $statushash);
>+($itemlist, $iTotalRecords) = GetItemsForInventory( {
>+  minlocation  => $minlocation,
>+  maxlocation  => $maxlocation,
>+  location     => $location,
>+  itemtype     => $itemtype,
>+  ignoreissued => $ignoreissued,
>+  datelastseen => $datelastseen,
>+  branchcode   => $branchcode,
>+  branch       => $branch,
>+  offset       => $offset,
>+  size         => $size,
>+  stautshash   => $statushash
>+  interface    => $interface,
>+} );
> 
> Retrieve a list of title/authors/barcode/callnumber, for biblio inventory.
> 
>@@ -1042,7 +1055,20 @@ $iTotalRecords is the number of rows that would have been returned without the $
> =cut
> 
> sub GetItemsForInventory {
>-    my ( $minlocation, $maxlocation,$location, $itemtype, $ignoreissued, $datelastseen, $branchcode, $branch, $offset, $size, $statushash ) = @_;
>+    my ( $parameters ) = @_;
>+    my $minlocation  = $parameters->{'minlocation'}  // '';
>+    my $maxlocation  = $parameters->{'maxlocation'}  // '';
>+    my $location     = $parameters->{'location'}     // '';
>+    my $itemtype     = $parameters->{'itemtype'}     // '';
>+    my $ignoreissued = $parameters->{'ignoreissued'} // '';
>+    my $datelastseen = $parameters->{'datelastseen'} // '';
>+    my $branchcode   = $parameters->{'branchcode'}   // '';
>+    my $branch       = $parameters->{'branch'}       // '';
>+    my $offset       = $parameters->{'offset'}       // '';
>+    my $size         = $parameters->{'size'}         // '';
>+    my $statushash   = $parameters->{'statushash'}   // '';
>+    my $interface    = $parameters->{'interface'}    // '';
>+
>     my $dbh = C4::Context->dbh;
>     my ( @bind_params, @where_strings );
> 
>@@ -1121,16 +1147,15 @@ sub GetItemsForInventory {
>     $sth->execute( @bind_params );
>     my ($iTotalRecords) = $sth->fetchrow_array();
> 
>+    my $avmapping = C4::Koha::GetKohaAuthorisedValuesMapping( {
>+                      interface => $interface
>+                    } );
>     foreach my $row (@$tmpresults) {
> 
>         # Auth values
>         foreach (keys %$row) {
>-            # If the koha field is mapped to a marc field
>-            my ($f, $sf) = GetMarcFromKohaField("items.$_", $row->{'frameworkcode'});
>-            if ($f and $sf) {
>-                # We replace the code with it's description
>-                my $authvals = C4::Koha::GetKohaAuthorisedValuesFromField($f, $sf, $row->{'frameworkcode'});
>-                $row->{$_} = $authvals->{$row->{$_}} if defined $authvals->{$row->{$_}};
>+            if (defined($avmapping->{"items.$_,".$row->{'frameworkcode'}.",".$row->{$_}})) {
>+                $row->{$_} = $avmapping->{"items.$_,".$row->{'frameworkcode'}.",".$row->{$_}};
>             }
>         }
>         push @results, $row;
>diff --git a/C4/Koha.pm b/C4/Koha.pm
>index 23fbfee..cb0ed5a 100644
>--- a/C4/Koha.pm
>+++ b/C4/Koha.pm
>@@ -62,6 +62,7 @@ BEGIN {
>                 &IsAuthorisedValueCategory
> 		&GetKohaAuthorisedValues
> 		&GetKohaAuthorisedValuesFromField
>+    &GetKohaAuthorisedValuesMapping
>     &GetKohaAuthorisedValueLib
>     &GetAuthorisedValueByCode
>     &GetKohaImageurlFromAuthorisedValues
>@@ -1327,6 +1328,50 @@ sub GetKohaAuthorisedValuesFromField {
>   }
> }
> 
>+=head2 GetKohaAuthorisedValuesMapping
>+
>+Takes a hash as a parameter. The interface key indicates the
>+description to use in the mapping.
>+
>+Returns hashref of:
>+ "{kohafield},{frameworkcode},{authorised_value}" => "{description}"
>+for all the kohafields, frameworkcodes, and authorised values.
>+
>+Returns undef if nothing is found.
>+
>+=cut
>+
>+sub GetKohaAuthorisedValuesMapping {
>+    my ($parameter) = @_;
>+    my $interface = $parameter->{'interface'} // '';
>+
>+    my $query_mapping = q{
>+SELECT TA.kohafield,TA.authorised_value AS category,
>+       TA.frameworkcode,TB.authorised_value,
>+       IF(TB.lib_opac>'',TB.lib_opac,TB.lib) AS OPAC,
>+       TB.lib AS Intranet,TB.lib_opac
>+FROM marc_subfield_structure AS TA JOIN
>+     authorised_values as TB ON
>+     TA.authorised_value=TB.category
>+WHERE TA.kohafield>'' AND TA.authorised_value>'';
>+    };
>+    my $dbh = C4::Context->dbh;
>+    my $sth = $dbh->prepare($query_mapping);
>+    $sth->execute();
>+    my $avmapping;
>+    if ($interface eq 'opac') {
>+        while (my $row = $sth->fetchrow_hashref) {
>+            $avmapping->{$row->{kohafield}.",".$row->{frameworkcode}.",".$row->{authorised_value}} = $row->{OPAC};
>+        }
>+    }
>+    else {
>+        while (my $row = $sth->fetchrow_hashref) {
>+            $avmapping->{$row->{kohafield}.",".$row->{frameworkcode}.",".$row->{authorised_value}} = $row->{Intranet};
>+        }
>+    }
>+    return $avmapping;
>+}
>+
> =head2 xml_escape
> 
>   my $escaped_string = C4::Koha::xml_escape($string);
>diff --git a/t/db_dependent/Items/GetItemsForInventory.t b/t/db_dependent/Items/GetItemsForInventory.t
>new file mode 100755
>index 0000000..3a0a4d3
>--- /dev/null
>+++ b/t/db_dependent/Items/GetItemsForInventory.t
>@@ -0,0 +1,156 @@
>+#!/usr/bin/perl
>+#
>+# This file is part of Koha.
>+#
>+# Copyright (c) 2015   Mark Tompsett
>+#
>+# Koha is free software; you can redistribute it and/or modify it
>+# under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# Koha is distributed in the hope that it will be useful, but
>+# WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http://www.gnu.org/licenses>.
>+
>+use Modern::Perl;
>+use Test::More tests => 6;
>+
>+$| = 1;
>+
>+BEGIN {
>+    use_ok('C4::Context');
>+    use_ok('C4::Items');
>+    use_ok('C4::Biblio');
>+    use_ok('C4::Koha');
>+}
>+
>+can_ok('C4::Items','GetItemsForInventory');
>+
>+my $dbh = C4::Context->dbh;
>+$dbh->{AutoCommit} = 0;
>+$dbh->{RaiseError} = 1;
>+
>+diag("This could take a while for large datasets...");
>+
>+my ($oldResults, $oldCount) = OldWay($dbh);
>+my ($newResults, $newCount) = GetItemsForInventory( { interface => 'staff' } );
>+
>+is_deeply($newResults,$oldResults,"Inventory results unchanged.");
>+
>+$dbh->rollback;
>+
>+sub OldWay {
>+    my ($tdbh)       = @_;
>+    my $ldbh         = $tdbh;
>+    my $minlocation  = '';
>+    my $maxlocation  = '';
>+    my $location     = '';
>+    my $itemtype     = '';
>+    my $ignoreissued = '';
>+    my $datelastseen = '';
>+    my $branchcode   = '';
>+    my $branch       = '';
>+    my $offset       = '';
>+    my $size         = '';
>+    my $statushash   = '';
>+    my $interface    = '';
>+
>+    my ( @bind_params, @where_strings );
>+
>+    my $select_columns = q{
>+        SELECT items.itemnumber, barcode, itemcallnumber, title, author, biblio.biblionumber, biblio.frameworkcode, datelastseen, homebranch, location, notforloan, damaged, itemlost, withdrawn, stocknumber
>+    };
>+    my $select_count = q{SELECT COUNT(*)};
>+    my $query = q{
>+        FROM items
>+        LEFT JOIN biblio ON items.biblionumber = biblio.biblionumber
>+        LEFT JOIN biblioitems on items.biblionumber = biblioitems.biblionumber
>+    };
>+    if ($statushash){
>+        for my $authvfield (keys %$statushash){
>+            if ( scalar @{$statushash->{$authvfield}} > 0 ){
>+                my $joinedvals = join ',', @{$statushash->{$authvfield}};
>+                push @where_strings, "$authvfield in (" . $joinedvals . ")";
>+            }
>+        }
>+    }
>+
>+    if ($minlocation) {
>+        push @where_strings, 'itemcallnumber >= ?';
>+        push @bind_params, $minlocation;
>+    }
>+
>+    if ($maxlocation) {
>+        push @where_strings, 'itemcallnumber <= ?';
>+        push @bind_params, $maxlocation;
>+    }
>+
>+    if ($datelastseen) {
>+        $datelastseen = format_date_in_iso($datelastseen);
>+        push @where_strings, '(datelastseen < ? OR datelastseen IS NULL)';
>+        push @bind_params, $datelastseen;
>+    }
>+
>+    if ( $location ) {
>+        push @where_strings, 'items.location = ?';
>+        push @bind_params, $location;
>+    }
>+
>+    if ( $branchcode ) {
>+        if($branch eq "homebranch"){
>+        push @where_strings, 'items.homebranch = ?';
>+        }else{
>+            push @where_strings, 'items.holdingbranch = ?';
>+        }
>+        push @bind_params, $branchcode;
>+    }
>+
>+    if ( $itemtype ) {
>+        push @where_strings, 'biblioitems.itemtype = ?';
>+        push @bind_params, $itemtype;
>+    }
>+
>+    if ( $ignoreissued) {
>+        $query .= "LEFT JOIN issues ON items.itemnumber = issues.itemnumber ";
>+        push @where_strings, 'issues.date_due IS NULL';
>+    }
>+
>+    if ( @where_strings ) {
>+        $query .= 'WHERE ';
>+        $query .= join ' AND ', @where_strings;
>+    }
>+    $query .= ' ORDER BY items.cn_sort, itemcallnumber, title';
>+    my $count_query = $select_count . $query;
>+    $query .= " LIMIT $offset, $size" if ($offset and $size);
>+    $query = $select_columns . $query;
>+    my $sth = $ldbh->prepare($query);
>+    $sth->execute( @bind_params );
>+
>+    my @results = ();
>+    my $tmpresults = $sth->fetchall_arrayref({});
>+    $sth = $ldbh->prepare( $count_query );
>+    $sth->execute( @bind_params );
>+    my ($iTotalRecords) = $sth->fetchrow_array();
>+
>+    foreach my $row (@$tmpresults) {
>+
>+        # Auth values
>+        foreach my $field (sort keys %$row) {
>+            # If the koha field is mapped to a marc field
>+            my ($f, $sf) = C4::Biblio::GetMarcFromKohaField("items.$field", $row->{'frameworkcode'});
>+            if (defined($f) and defined($sf)) {
>+                # We replace the code with it's description
>+                my $authvals = C4::Koha::GetKohaAuthorisedValuesFromField($f, $sf, $row->{'frameworkcode'});
>+                $row->{$field} = $authvals->{$row->{$field}} if defined $authvals && defined $row->{$field} && defined $authvals->{$row->{$field}};
>+            }
>+        }
>+        push @results, $row;
>+    }
>+
>+    return (\@results, $iTotalRecords);
>+}
>diff --git a/t/db_dependent/Koha/GetKohaAuthorisedValuesMapping.t b/t/db_dependent/Koha/GetKohaAuthorisedValuesMapping.t
>new file mode 100755
>index 0000000..2cdb3b6
>--- /dev/null
>+++ b/t/db_dependent/Koha/GetKohaAuthorisedValuesMapping.t
>@@ -0,0 +1,54 @@
>+#!/usr/bin/perl
>+#
>+# This file is part of Koha.
>+#
>+# Copyright (c) 2015   Mark Tompsett
>+#
>+# Koha is free software; you can redistribute it and/or modify it
>+# under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# Koha is distributed in the hope that it will be useful, but
>+# WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http://www.gnu.org/licenses>.
>+
>+use Modern::Perl;
>+
>+use C4::Context;
>+
>+use Test::More tests => 8;
>+
>+BEGIN {
>+    use_ok('C4::Context');
>+    use_ok('C4::Koha');
>+}
>+
>+can_ok('C4::Koha','GetKohaAuthorisedValuesMapping');
>+
>+my $avMappingOPAC;
>+my $avMappingStaff;
>+my $avMappingUndef1;
>+my $avMappingUndef2;
>+my $avMappingUndef3;
>+SKIP: {
>+    my $dbh = C4::Context->dbh;
>+    my $count = $dbh->selectrow_arrayref("SELECT COUNT(*) FROM marc_subfield_structure WHERE kohafield LIKE 'item%';");
>+    skip "Lacking item mappings in marc_subfield_structure",5 unless ($count && $count->[0]>0);
>+    $count = $dbh->selectrow_arrayref("SELECT COUNT(*) FROM authorised_values;");
>+    skip "Lacking authorised_values",5 unless ($count && $count->[0]>0);
>+    $avMappingOPAC   = GetKohaAuthorisedValuesMapping( { interface => 'opac' });
>+    $avMappingStaff  = GetKohaAuthorisedValuesMapping( { interface => 'staff' });
>+    $avMappingUndef1 = GetKohaAuthorisedValuesMapping( { interface => undef });
>+    $avMappingUndef2 = GetKohaAuthorisedValuesMapping( { } );
>+    $avMappingUndef3 = GetKohaAuthorisedValuesMapping();
>+    is_deeply($avMappingUndef1,$avMappingStaff,"Undefined interface = Staff test 1");
>+    is_deeply($avMappingUndef2,$avMappingStaff,"Undefined interface = Staff test 2");
>+    is_deeply($avMappingUndef3,$avMappingStaff,"Undefined interface = Staff test 3");
>+    isnt($avMappingOPAC ,undef,"OPAC has a mapping");
>+    isnt($avMappingStaff,undef,"Staff has a mapping");
>+}
>diff --git a/tools/inventory.pl b/tools/inventory.pl
>index d463d19..015d0be 100755
>--- a/tools/inventory.pl
>+++ b/tools/inventory.pl
>@@ -234,10 +234,36 @@ if ( $markseen or $op ) {
> 
>     # We use datelastseen only when comparing the results to the barcode file.
>     my $paramdatelastseen = ($compareinv2barcd) ? $datelastseen : '';
>-    ($inventorylist, $totalrecords) = GetItemsForInventory($minlocation, $maxlocation, $location, $itemtype, $ignoreissued, $paramdatelastseen, $branchcode, $branch, 0, undef, $staton);
>+    ($inventorylist, $totalrecords) = GetItemsForInventory( {
>+      minlocation  => $minlocation,
>+      maxlocation  => $maxlocation,
>+      location     => $location,
>+      itemtype     => $itemtype,
>+      ignoreissued => $ignoreissued,
>+      datelastseen => $paramdatelastseen,
>+      branchcode   => $branchcode,
>+      branch       => $branch,
>+      offset       => 0,
>+      size         => undef,
>+      statushash   => $staton,
>+      interface    => 'staff',
>+    } );
> 
>     # For the items that may be marked as "wrong place", we only check the location (callnumbers, location and branch)
>-    ($wrongplacelist, $totalrecords) = GetItemsForInventory($minlocation, $maxlocation, $location, undef, undef, undef, $branchcode, $branch, 0, undef, undef);
>+    ($wrongplacelist, $totalrecords) = GetItemsForInventory( {
>+      minlocation  => $minlocation,
>+      maxlocation  => $maxlocation,
>+      location     => $location,
>+      itemtype     => undef,
>+      ignoreissued => undef,
>+      datelastseen => undef,
>+      branchcode   => $branchcode,
>+      branch       => $branch,
>+      offset       => 0,
>+      size         => undef,
>+      statushash   => undef,
>+      interface    => 'staff',
>+    } );
> 
> }
> 
>-- 
>1.9.1

Actions: View | Diff | Splinter Review

Attachments on bug 14057: 38472 | 38482 | 38483 | 38500 | 38506 | 38507 | 38525 | 38621 | 38622 | 38636 | 38733