Bugzilla – Attachment 62097 Details for
Bug 14399
Fix inventory.pl part two (following 12913)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14399: Numerous small refinements to the inventory script
Bug-14399-Numerous-small-refinements-to-the-invent.patch (text/plain), 20.18 KB, created by
Marcel de Rooy
on 2017-04-12 15:00:57 UTC
(
hide
)
Description:
Bug 14399: Numerous small refinements to the inventory script
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2017-04-12 15:00:57 UTC
Size:
20.18 KB
patch
obsolete
>From a7209fe5c77214daf3dc7c279997b395a447a55e Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Mon, 3 Apr 2017 15:16:42 +0200 >Subject: [PATCH] Bug 14399: Numerous small refinements to the inventory script >Content-Type: text/plain; charset=utf-8 > >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> >--- > .../prog/en/modules/tools/inventory.tt | 31 ++-- > tools/inventory.pl | 201 ++++++++++----------- > 2 files changed, 112 insertions(+), 120 deletions(-) > >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt >index 650a510..7f31c93 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt >@@ -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> >diff --git a/tools/inventory.pl b/tools/inventory.pl >index 9efa8c5..76a0925 100755 >--- a/tools/inventory.pl >+++ b/tools/inventory.pl >@@ -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; >+} >-- >2.1.4
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=62097">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 14399
:
61685
|
62096
|
62097
|
62098
|
62099
|
62139
|
62140
|
62733
|
62734
|
62735
|
62736
|
63465
|
63466
|
63467
|
63468
|
67546
|
67547
|
67548
|
67549