Bug 14399: Numerous small refinements to the inventory script
authorMarcel de Rooy <m.de.rooy@rijksmuseum.nl>
Mon, 3 Apr 2017 13:16:42 +0000 (15:16 +0200)
committerKyle M Hall <kyle@bywatersolutions.com>
Wed, 10 May 2017 16:23:55 +0000 (16:23 +0000)
This patch contains the following changes:

[01] Label "Inventory date" reworded to "Last inventory date", adding a
small explanation for its purpose.
[02] Restructured the results: it was an array with items and possible
error messages. Multiple messages duplicated individual items. Now the
results are in a hash, pulling all error messages for one item together.
At the end of the script they are copied to an array. (A helper sub
additemtoresults is added in this regard.) We no longer use array
@items_with_problems.
[03] Both datepickers are no longer connected to the same class. This
prevents changing the set date by filling the last inventory date.
[04] Input markseen in the template and $markseen in the script are
no longer needed.
[05] The paragraph before the detail link in the results table in the
Title column has been removed. Same for problems column. This makes
vertical spacing consistent.
[06] Problem status 'missingitem' is no longer used; the missing items
are marked as 'not_scanned'. Two additional statuses are: no_barcode and
checkedout.
[07] Removed unused $itemtype, $totalrecords and $count. We use variable
$moddatecount to report a count to the template.
[08] The script updated scanned items twice. The first time with ModItem
and the second time with ModDateLastSeen. The second call is removed.
[09] If a book is checked in, we do no longer return an error message when
the checkin is successful (ERR_ONLOAN_RET). The updated datelastseen is
passed to the results.
[10] $wrongplacelist is renamed to $rightplacelist. It is only built when
we need it. (Same for inventorylist now.)
[11] Datelastseen (last inventory date) is always used for building the
inventory list. It allows you to process partial barcode lists or make
a list of items not seen after some date. We do no longer use variable
$paramdatelastseen.
[12] The section where items.datelastseen was compared with the inventory
date has been removed. Scanned items were already updated; to get items
seen before some date, you can now use last inventory date without passing
barcodes.

The form can mainly be used for the following three cases:
[1] Prepare an inventory list or csv file; we do not upload barcodes.
[2] Update items for uploaded barcodes without comparing to inventory.
    Last inventory date is useless in this case.
    Errors wrongplace, checkedout and changestatus are reported.
    Use this scenario for partial scanned barcode lists (all but last).
[3] Update items for uploaded barcodes and compare to inventory, filtered
    by an optional last inventory date.
    Apart from the errors mentioned under [2], this also reports
    not_scanned ("missing") and no_barcode.
    Use this scenario too for the last partial barcode file (together with
    inventory date).

Test plan:
See next patch ("Interface changes").

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt
tools/inventory.pl

index 650a510..7f31c93 100644 (file)
@@ -128,7 +128,7 @@ $(document).ready(function(){
             <legend>Use a barcode file</legend>
      <ol>
             <li><label for="uploadbarcodes">Barcode file: </label> <input type="file" id="uploadbarcodes" name="uploadbarcodes" /></li>
-            <li><label for="setdate">Set inventory date to:</label> <input type="text" id="setdate" name="setdate" value="[% today | $KohaDates %]" class="datepickerfrom" />
+            <li><label for="setdate">Set inventory date to:</label> <input type="text" id="setdate" name="setdate" value="[% today | $KohaDates %]" class="datepicker" />
             </li>
           </ol>
         </fieldset>
@@ -200,8 +200,9 @@ $(document).ready(function(){
           <ol>
         [% END %]
 
-        <li><label for="datelastseen">Inventory date:</label>
-            <input type="text" id="datelastseen" name="datelastseen" value="[% datelastseen | $KohaDates %]" class="datepickerfrom" />
+        <li><label for="datelastseen">Last inventory date:</label>
+            <input type="text" id="datelastseen" name="datelastseen" value="[% datelastseen | $KohaDates %]" class="datepicker" />
+            (Skip records marked as seen on or after this date.)
         </li>
         <li><label for="ignoreissued">Skip items on loan: </label>
             [% IF (ignoreissued) %]
@@ -232,7 +233,6 @@ $(document).ready(function(){
     [% END %]
     [% IF (op) %]
     <form method="post" action="/cgi-bin/koha/tools/inventory.pl" class="checkboxed">
-    <input type="hidden" name="markseen" value="1" />
     <input type="hidden" name="minlocation" value="[% minlocation %]" />
     <input type="hidden" name="maxlocation" value="[% maxlocation %]" />
     <input type="hidden" name="location" value="[% location %]" />
@@ -276,7 +276,8 @@ $(document).ready(function(){
                 [% result.location | html %]
             </td>
             <td>
-            <p><a href="/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=[% result.biblionumber %]" class="openWin">[% result.title | html %]</a></p><p>[% result.author | html %]</p>
+                <a href="/cgi-bin/koha/catalogue/MARCdetail.pl?biblionumber=[% result.biblionumber %]" class="openWin">[% result.title | html %]</a>
+                <p>[% result.author | html %]</p>
             </td>
             <td>
             [% result.notforloan | html %]
@@ -294,14 +295,18 @@ $(document).ready(function(){
             [% result.datelastseen | $KohaDates | html %]
             </td>
             <td>
-            [% IF result.problem == 'wrongplace' %]
-                <p>Item should not have been scanned</p>
-            [% ELSIF result.problem == 'missingitem' %]
-                <p>Item missing</p>
-            [% ELSIF result.problem == 'changestatus' %]
-                <p>Unknown not-for-loan status</p>
-            [% ELSIF result.problem == 'not_scanned' %]
-                <p>Item should have been scanned</p>
+            [% FOREACH problem IN result.problems %]
+                [% IF problem.key == 'wrongplace' %]
+                    Found in wrong place<br/>
+                [% ELSIF problem.key == 'changestatus' %]
+                    Unknown not-for-loan status<br/>
+                [% ELSIF problem.key == 'not_scanned' %]
+                    Missing (not scanned)<br/>
+                [% ELSIF problem.key == 'checkedout' %]
+                    Still checked out<br/>
+                [% ELSIF problem.key == 'no_barcode' %]
+                    No barcode<br/>
+                [% END %]
             [% END %]
             </td>
         </tr>
index 9efa8c5..76a0925 100755 (executable)
@@ -1,7 +1,7 @@
 #!/usr/bin/perl
 
 # Copyright 2000-2009 Biblibre S.A
-#                                         John Soros <john.soros@biblibre.com>
+# John Soros <john.soros@biblibre.com>
 #
 # This file is part of Koha.
 #
@@ -18,8 +18,7 @@
 # You should have received a copy of the GNU General Public License
 # along with Koha; if not, see <http://www.gnu.org/licenses>.
 
-use strict;
-use warnings;
+use Modern::Perl;
 
 #need to open cgi and get the fh before anything else opens a new cgi context (see C4::Auth)
 use CGI qw ( -utf8 );
@@ -40,15 +39,12 @@ use Koha::AuthorisedValues;
 use Koha::BiblioFrameworks;
 use List::MoreUtils qw( none );
 
-
 my $minlocation=$input->param('minlocation') || '';
 my $maxlocation=$input->param('maxlocation');
 $maxlocation=$minlocation.'Z' unless ( $maxlocation || ! $minlocation );
 my $location=$input->param('location') || '';
-my $itemtype=$input->param('itemtype'); # FIXME note, template does not currently supply this
 my $ignoreissued=$input->param('ignoreissued');
-my $datelastseen = $input->param('datelastseen');
-my $markseen = $input->param('markseen');
+my $datelastseen = $input->param('datelastseen'); # last inventory date
 my $branchcode = $input->param('branchcode') || '';
 my $branch     = $input->param('branch');
 my $op         = $input->param('op');
@@ -105,9 +101,8 @@ for my $statfield (qw/items.notforloan items.itemlost items.withdrawn items.dama
     }
 }
 
-
 $template->param( statuses => $statuses );
-my $staton = {};                                #authorized values that are ticked
+my $staton = {}; #authorized values that are ticked
 for my $authvfield (@$statuses) {
     $staton->{$authvfield->{fieldname}} = [];
     for my $authval (@{$authvfield->{values}}){
@@ -130,8 +125,11 @@ $template->param(
     compareinv2barcd         => $compareinv2barcd,
 );
 
+# Walk through uploaded barcodes, report errors, mark as seen, check in
+my $results = {};
 my @scanned_items;
 my @errorloop;
+my $moddatecount = 0;
 if ( $uploadbarcodes && length($uploadbarcodes) > 0 ) {
     my $dbh = C4::Context->dbh;
     my $date = dt_from_string( scalar $input->param('setdate') );
@@ -142,8 +140,6 @@ if ( $uploadbarcodes && length($uploadbarcodes) > 0 ) {
     $strsth="select * from items where items.barcode =? and items.withdrawn = 1";
     my $qwithdrawn = $dbh->prepare($strsth);
 
-    my $count = 0;
-
     my @barcodes;
     my @uploadedbarcodes;
 
@@ -190,101 +186,77 @@ if ( $uploadbarcodes && length($uploadbarcodes) > 0 ) {
         } else {
             my $item = GetItem( '', $barcode );
             if ( defined $item && $item->{'itemnumber'} ) {
-                ModItem( { datelastseen => $date }, undef, $item->{'itemnumber'} );
-                push @scanned_items, $item;
-                $count++;
+                # Modify date last seen for scanned items, remove lost status
+                ModItem( { itemlost => 0, datelastseen => $date }, undef, $item->{'itemnumber'} );
+                $moddatecount++;
+                # update item hash accordingly
+                $item->{itemlost} = 0;
+                $item->{datelastseen} = $date;
                 unless ( $dont_checkin ) {
                     $qonloan->execute($barcode);
                     if ($qonloan->rows){
                         my $data = $qonloan->fetchrow_hashref;
                         my ($doreturn, $messages, $iteminformation, $borrower) =AddReturn($barcode, $data->{homebranch});
-                        if ($doreturn){
-                            push @errorloop, {'barcode'=>$barcode,'ERR_ONLOAN_RET'=>1}
+                        if( $doreturn ) {
+                            $item->{onloan} = undef;
+                            $item->{datelastseen} = dt_from_string;
                         } else {
-                            push @errorloop, {'barcode'=>$barcode,'ERR_ONLOAN_NOT_RET'=>1}
+                            push @errorloop, { barcode => $barcode, ERR_ONLOAN_NOT_RET => 1 };
                         }
                     }
                 }
+                push @scanned_items, $item;
             } else {
-                push @errorloop, {'barcode'=>$barcode,'ERR_BARCODE'=>1};
+                push @errorloop, { barcode => $barcode, ERR_BARCODE => 1 };
             }
         }
-
     }
-
-    $template->param( date => $date, Number => $count );
+    $template->param( date => $date );
     $template->param( errorloop => \@errorloop ) if (@errorloop);
 }
 
-# now build the result list: inventoried items if requested, and mis-placed items -always-
-my $inventorylist;
-my $wrongplacelist;
-my @items_with_problems;
-if ( $markseen or $op ) {
-    # retrieve all items in this range.
-    my $totalrecords;
-
-    # We use datelastseen only when comparing the results to the barcode file.
-    my $paramdatelastseen = ($compareinv2barcd) ? $datelastseen : '';
-    ($inventorylist, $totalrecords) = GetItemsForInventory( {
+# Build inventorylist: used as result list when you do not pass barcodes
+# This list is also used when you want to compare with barcodes
+my ( $inventorylist, $rightplacelist );
+if ( $op && ( !$uploadbarcodes || $compareinv2barcd )) {
+    ( $inventorylist ) = GetItemsForInventory({
       minlocation  => $minlocation,
       maxlocation  => $maxlocation,
       location     => $location,
-      itemtype     => $itemtype,
       ignoreissued => $ignoreissued,
-      datelastseen => $paramdatelastseen,
+      datelastseen => $datelastseen,
       branchcode   => $branchcode,
       branch       => $branch,
       offset       => 0,
-      size         => undef,
       statushash   => $staton,
-    } );
-
-    # For the items that may be marked as "wrong place", we only check the location (callnumbers, location and branch)
-    ($wrongplacelist, $totalrecords) = GetItemsForInventory( {
+    });
+}
+# Build rightplacelist used to check if a scanned item is in the right place.
+if( @scanned_items ) {
+    ( $rightplacelist ) = GetItemsForInventory({
       minlocation  => $minlocation,
       maxlocation  => $maxlocation,
       location     => $location,
-      itemtype     => undef,
       ignoreissued => undef,
       datelastseen => undef,
       branchcode   => $branchcode,
       branch       => $branch,
       offset       => 0,
-      size         => undef,
       statushash   => undef,
-    } );
-
-}
-
-# If "compare barcodes list to results" has been checked, we want to alert for missing items
-if ( $compareinv2barcd ) {
-    # set "missing" flags for all items with a datelastseen (dls) before the chosen datelastseen (cdls)
-    my $dls = output_pref( { dt => dt_from_string( $datelastseen ),
-                             dateformat => 'iso' } );
-    foreach my $item ( @$inventorylist ) {
-        my $cdls = output_pref( { dt => dt_from_string( $item->{datelastseen} ),
-                                  dateformat => 'iso' } );
-        if ( $cdls lt $dls ) {
-            $item->{problem} = 'missingitem';
-            # We have to push a copy of the item, not the reference
-            push @items_with_problems, { %$item };
-        }
-    }
+    });
+    # Convert the structure to a hash on barcode
+    $rightplacelist = {
+        map { $_->{barcode} ? ( $_->{barcode}, $_ ) : (); } @$rightplacelist
+    };
 }
 
-
-
-# insert "wrongplace" to all scanned items that are not supposed to be in this range
-# note this list is always displayed, whatever the librarian has chosen for comparison
-my $moddatecount = 0;
+# Report scanned items that are on the wrong place, or have a wrong notforloan
+# status, or are still checked out.
 foreach my $item ( @scanned_items ) {
+    $item->{notforloancode} = $item->{notforloan}; # save for later use
 
-  # Saving notforloan code before it's replaced by it's authorised value for later comparison
-  $item->{notforloancode} = $item->{notforloan};
-
-  # Populating with authorised values
-  foreach my $field ( keys %$item ) {
+    # Populating with authorised values
+    foreach my $field ( keys %$item ) {
         # If the koha field is mapped to a marc field
         my $fc = $item->{'frameworkcode'} || '';
         my ($f, $sf) = GetMarcFromKohaField("items.$field", $fc);
@@ -299,54 +271,57 @@ foreach my $item ( @scanned_items ) {
         }
     }
 
-    next if $item->{onloan}; # skip checked out items
-
     # If we have scanned items with a non-matching notforloan value
-    if (none { $item->{'notforloancode'} eq $_ } @notforloans) {
-        $item->{problem} = 'changestatus';
-        push @items_with_problems, { %$item };
-    }
-    if (none { $item->{barcode} eq $_->{barcode} && !$_->{'onloan'} } @$wrongplacelist) {
-        $item->{problem} = 'wrongplace';
-        push @items_with_problems, { %$item };
+    if( none { $item->{'notforloancode'} eq $_ } @notforloans ) {
+        $item->{problems}->{changestatus} = 1;
+        additemtoresults( $item, $results );
     }
 
-    # Modify date last seen for scanned items
-    ModDateLastSeen($item->{'itemnumber'});
-    $moddatecount++;
+    # Report an item that is checked out (unusual!) or wrongly placed
+    if( $item->{onloan} ) {
+        $item->{problems}->{checkedout} = 1;
+        additemtoresults( $item, $results );
+        next; # do not modify item
+    } elsif( !exists $rightplacelist->{ $item->{barcode} } ) {
+        $item->{problems}->{wrongplace} = 1;
+        additemtoresults( $item, $results );
+    }
 }
 
+# Compare barcodes with inventory list, report no_barcode and not_scanned.
+# not_scanned can be interpreted as missing
 if ( $compareinv2barcd ) {
     my @scanned_barcodes = map {$_->{barcode}} @scanned_items;
-    for my $should_be_scanned ( @$inventorylist ) {
-        my $barcode = $should_be_scanned->{barcode};
-        unless ( grep /^$barcode$/, @scanned_barcodes ) {
-            $should_be_scanned->{problem} = 'not_scanned';
-            push @items_with_problems, { %$should_be_scanned };
+    for my $item ( @$inventorylist ) {
+        my $barcode = $item->{barcode};
+        if( !$barcode ) {
+            $item->{problems}->{no_barcode} = 1;
+        } elsif ( grep /^$barcode$/, @scanned_barcodes ) {
+            next;
+        } else {
+            $item->{problems}->{not_scanned} = 1;
         }
+        additemtoresults( $item, $results );
     }
 }
 
-for my $item ( @items_with_problems ) {
+# Construct final results, add biblio information
+my $loop = $uploadbarcodes
+    ? [ map { $results->{$_} } keys %$results ]
+    : $inventorylist // [];
+for my $item ( @$loop ) {
     my $biblio = C4::Biblio::GetBiblioData($item->{biblionumber});
     $item->{title} = $biblio->{title};
     $item->{author} = $biblio->{author};
 }
 
-# If a barcode file is given, we want to show problems, else all items
-my @results;
-@results = $uploadbarcodes
-            ? @items_with_problems
-            : $op
-                ? @$inventorylist
-                : ();
-
 $template->param(
     moddatecount => $moddatecount,
-    loop       => \@results,
-    op         => $op
+    loop         => $loop,
+    op           => $op,
 );
 
+# Export to csv
 if (defined $input->param('CSVexport') && $input->param('CSVexport') eq 'on'){
     eval {use Text::CSV};
     my $csv = Text::CSV->new or
@@ -381,22 +356,27 @@ if (defined $input->param('CSVexport') && $input->param('CSVexport') eq 'on'){
     print $csv->string, "\n";
 
     my @keys = qw / title author barcode itemnumber homebranch location itemcallnumber notforloan lost damaged withdrawn stocknumber /;
-    for my $item ( @results ) {
+    for my $item ( @$loop ) {
         my @line;
         for my $key (@keys) {
             push @line, $item->{$key};
         }
-        if ( defined $item->{problem} ) {
-            if ( $item->{problem} eq 'wrongplace' ) {
-                push @line, "wrong place";
-            } elsif ( $item->{problem} eq 'missingitem' ) {
-                push @line, "missing item";
-            } elsif ( $item->{problem} eq 'changestatus' ) {
-                push @line, "change item status";
-            } elsif ($item->{problem} eq 'not_scanned' ) {
-                push @line, "item not scanned";
+        my $errstr = '';
+        foreach my $key ( keys %{$item->{problems}} ) {
+            if( $key eq 'wrongplace' ) {
+                $errstr .= "wrong place,";
+            } elsif( $key eq 'changestatus' ) {
+                $errstr .= "unknown notforloan status,";
+            } elsif( $key eq 'not_scanned' ) {
+                $errstr .= "missing,";
+            } elsif( $key eq 'no_barcode' ) {
+                $errstr .= "no barcode,";
+            } elsif( $key eq 'checkedout' ) {
+                $errstr .= "checked out,";
             }
         }
+        $errstr =~ s/,$//;
+        push @line, $errstr;
         $csv->combine(@line);
         print $csv->string, "\n";
     }
@@ -414,3 +394,10 @@ if (defined $input->param('CSVexport') && $input->param('CSVexport') eq 'on'){
 }
 
 output_html_with_http_headers $input, $cookie, $template->output;
+
+sub additemtoresults {
+    my ( $item, $results ) = @_;
+    my $itemno = $item->{itemnumber};
+    # since the script appends to $item, we can just overwrite the hash entry
+    $results->{$itemno} = $item;
+}