From c565816fa43b99c611fce30c26625179edb63c07 Mon Sep 17 00:00:00 2001 From: Alex Sassmannshausen Date: Tue, 7 Mar 2017 16:11:19 +0100 Subject: [PATCH] Bug 11897: Bugfixes, unit tests, feedback, documentation. A (squashed) patch implementing significant changes to many parts of the module. Also adds thorough unit tests. Signed-off-by: Kathleen Milne --- Koha/Stockrotationitem.pm | 202 +++++++++++++++-------------------- Koha/Stockrotationitems.pm | 59 ++++++++++ Koha/Stockrotationrota.pm | 59 ++++++++-- Koha/Stockrotationrotas.pm | 37 +++++++ Koha/Stockrotationstage.pm | 120 ++++++++++++++++++++- Koha/Stockrotationstages.pm | 22 ++++ misc/cronjobs/stockrotation.pl | 180 ++++--------------------------- t/db_dependent/Stockrotationitems.t | 154 ++++++++++++++++++++++++-- t/db_dependent/Stockrotationrotas.t | 2 +- t/db_dependent/Stockrotationstages.t | 183 ++++++++++++++++++++++++++++++- 10 files changed, 726 insertions(+), 292 deletions(-) diff --git a/Koha/Stockrotationitem.pm b/Koha/Stockrotationitem.pm index 0e06862b0d..b4aaaeca7b 100644 --- a/Koha/Stockrotationitem.pm +++ b/Koha/Stockrotationitem.pm @@ -84,59 +84,31 @@ sub stage { return Koha::Stockrotationstage->_new_from_dbic( $rs ); } -sub needs_initiating { - my ( $self ) = @_; - my $completed = $self->itemnumber->_result->branchtransfers->search; - if ( !$completed->count ) { - return 1; - } else { - my $stage_branch = $self->stage->branchcode_id; - my $home_branch = $self->itemnumber->homebranch; - return !( $stage_branch eq $home_branch ); - } -} - =head3 needs_repatriating - my $status = $item->needs_repatriating; + 1|0 = $item->needs_repatriating; Return 1 if this item is currently not at the library it should be at according to our stockrotation plan. =cut -# Check following things: -# -# Homebranch == stockrotationstage.branch -# If not, check if Homebranch == stockrotationstage.stageafter.branch -# If so, presumably in transit :=> 0 -# Else, :=> 1 -# -# Holdingbranch == Homebranch -# If not, :=> 1 - sub needs_repatriating { my ( $self ) = @_; my ( $item, $stage ) = ( $self->itemnumber, $self->stage ); - if ( $item->homebranch ne $stage->branchcode_id ) { - # We're either in transit to new branch or we got corrupted. - my $next_stage = $stage->next_sibling; - ( $next_stage && $item->homebranch eq $next_stage->branchcode_id ) - ? return 0 # We seem to be in transit. - : return 1; # We got corrupted, gotta be repatriated! - } - # Check if we are where we should be… - if ( $item->holdingbranch eq $item->homebranch - || $self->itemnumber->get_transfer ) { - return 0; # Yup, all good + if ( $self->itemnumber->get_transfer ) { + return 0; # We're in transit. + } elsif ( $item->holdingbranch ne $stage->branchcode_id + || $item->homebranch ne $stage->branchcode_id ) { + return 1; # We're not where we should be. } else { - return 1; # Nope, gotta be repatriated + return 0; # We're at home. } } =head3 needs_advancing - my $status = $item->needs_advancing; + 1|0 = $item->needs_advancing; Return 1 if this item is ready to be moved on to the next stage in its rota. @@ -144,59 +116,32 @@ Return 1 if this item is ready to be moved on to the next stage in its rota. sub needs_advancing { my ( $self ) = @_; - my $intransfers = $self->itemnumber->get_transfer; - # Check whether we are in transfer - if ( !$intransfers ) { - my $completed = $self->itemnumber->_result->branchtransfers->search( - {}, - { - order_by => { -desc => 'datearrived' } - } + return 0 if $self->itemnumber->get_transfer; # intransfer: don't advance. + return 1 if $self->fresh; # Just on rota: advance. + my $completed = $self->itemnumber->_result->branchtransfers->search( + { 'comments' => "StockrotationAdvance" }, + { order_by => { -desc => 'datearrived' } } + ); + # Do maths on whether we need to be moved on. + if ( $completed->count ) { + my $arrival = dt_from_string( + $completed->next->datearrived, 'iso' ); - # Do maths on whether we need to be moved on. - if ( $completed->count ) { - my $arrival = dt_from_string( - $completed->next->datearrived, 'iso' - ); - my $duration = DateTime::Duration - ->new( days => $self->stage->duration ); - if ( $arrival + $duration le DateTime->now ) { - return 1; - } else { - return 0; - } + my $duration = DateTime::Duration + ->new( days => $self->stage->duration ); + if ( $arrival + $duration le DateTime->now ) { + return 1; } else { - die "We have no historical branch transfer; this should not have happened!"; + return 0; } - } - # Currently in transfer, do not advance. - return 0; -} - -sub initiate { - my ( $self ) = @_; - my $stage_branch = $self->stage->branchcode_id; - my $home_branch = $self->itemnumber->homebranch; - my $holding_branch = $self->itemnumber->holdingbranch; - if ( !( ( $stage_branch eq $home_branch ) - && ( $holding_branch eq $home_branch ) ) ) { - return $self->repatriate("StockrotationInit"); } else { - $self->itemnumber->homebranch($self->stage->branchcode_id)->store; - return Koha::Item::Transfer->new({ - 'itemnumber' => $self->itemnumber_id, - 'frombranch' => $self->stage->branchcode_id, - 'tobranch' => $self->stage->branchcode_id, - 'datesent' => DateTime->now, - 'datearrived' => DateTime->now, - 'comments' => "StockrotationInit" - })->store; + die "We have no historical branch transfer; this should not have happened!"; } } =head3 repatriate - $sritem = $sritem->repatriate + 1|0 = $sritem->repatriate Put this item into branch transfer with 'StockrotationCorrection' comment, so that it may return to it's stage.branch to continue its rota as normal. @@ -219,7 +164,7 @@ sub repatriate { =head3 advance - $sritem = $sritem->advance + 1|0 = $sritem->advance; Put this item into branch transfer with 'StockrotationAdvance' comment, to transfer it to the next stage in its rota. @@ -235,67 +180,90 @@ advance the item as per usual. sub advance { my ( $self ) = @_; - + my $item = $self->itemnumber; my $transfer = Koha::Item::Transfer->new({ 'itemnumber' => $self->itemnumber_id, - 'frombranch' => $self->itemnumber->holdingbranch, + 'frombranch' => $item->holdingbranch, 'datesent' => DateTime->now, 'comments' => "StockrotationAdvance" }); - if ( $self->indemand ) { + if ( $self->indemand && !$self->fresh ) { $self->indemand(0)->store; # De-activate indemand $transfer->tobranch($self->stage->branchcode_id); $transfer->datearrived(DateTime->now); } else { # Find and update our stage. - my $current_stage = $self->stage; + my $stage = $self->stage; my $new_stage; - # Last stage in rota? - if ( !$current_stage->last_sibling ) { - # Special rules: Cyclical rota? - if ( $current_stage->rota->cyclical ) { + if ( $self->fresh ) { # Just added to rota + $new_stage = $self->stage->first_sibling || $self->stage; + $transfer->tobranch($new_stage->branchcode_id); + $transfer->datearrived(DateTime->now) # Already at first branch + if $item->holdingbranch eq $new_stage->branchcode_id; + $self->fresh(0)->store; # Reset fresh + } elsif ( !$stage->last_sibling ) { # Last stage + if ( $stage->rota->cyclical ) { # Cyclical rota? # Revert to first stage. - my $xstg = $current_stage->first_sibling; - # Is this the first (as well as last) stage? - if ( $xstg ) { - # ... No, so return to first stage. - $new_stage = $xstg; - } else { - # ... Yes, so we create a dummy transfer (for statistics) - my $transfer_stored = Koha::Item::Transfer->new({ - 'itemnumber' => $self->itemnumber_id, - 'frombranch' => $self->itemnumber->holdingbranch, - 'tobranch' => $self->itemnumber->holdingbranch, - 'datesent' => DateTime->now, - 'datearrived' => DateTime->now, - 'comments' => "StockrotationAdvance" - })->store; - # and return. - return 1; - } - $new_stage = ( $xstg ) ? $xstg : $current_stage; + $new_stage = $stage->first_sibling || $stage; + $transfer->tobranch($new_stage->branchcode_id); + $transfer->datearrived(DateTime->now); } else { - # Stockrotationitem no longer needs to exist. - $self->delete; + $self->delete; # Stockrotationitem is done. return 1; } } else { # Just advance. $new_stage = $self->stage->next_sibling; } - - # Update our stage. - $self->stage_id($new_stage->stage_id)->store; - # Update our homebranch. - $self->itemnumber->homebranch($self->stage->branchcode_id)->store; - #Update the transfer - $transfer->tobranch($new_stage->branchcode_id); + $self->stage_id($new_stage->stage_id)->store; # Set new stage + $item->homebranch($new_stage->branchcode_id)->store; # Update homebranch + $transfer->tobranch($new_stage->branchcode_id); # Send to new branch } return $transfer->store; } +=head3 investigate + + my $report = $item->investigate; + +Return the base set of information, namely this individual item's report, for +generating stockrotation reports about this stockrotationitem. + +=cut + +sub investigate { + my ( $self ) = @_; + my $item_report = { + title => $self->itemnumber->_result->biblioitem + ->biblionumber->title, + author => $self->itemnumber->_result->biblioitem + ->biblionumber->author, + callnumber => $self->itemnumber->itemcallnumber, + location => $self->itemnumber->location, + onloan => $self->itemnumber->onloan, + barcode => $self->itemnumber->barcode, + itemnumber => $self->itemnumber_id, + branch => $self->itemnumber->_result->holdingbranch, + object => $self, + }; + my $reason; + if ( $self->fresh ) { + $reason = 'initiation'; + } elsif ( $self->needs_repatriating ) { + $reason = 'repatriation'; + } elsif ( $self->needs_advancing ) { + $reason = 'advancement'; + $reason = 'in-demand' if $self->indemand; + } else { + $reason = 'not-ready'; + } + $item_report->{reason} = $reason; + + return $item_report; +} + 1; =head1 AUTHOR diff --git a/Koha/Stockrotationitems.pm b/Koha/Stockrotationitems.pm index 06f2a47b62..98c9dbb200 100644 --- a/Koha/Stockrotationitems.pm +++ b/Koha/Stockrotationitems.pm @@ -55,6 +55,65 @@ sub object_class { return 'Koha::Stockrotationitem'; } +=head3 investigate + + my $report = $items->investigate; + +Return a stockrotation report about this set of stockrotationitems. + +In this part of the overall investigation process we split individual item +reports into appropriate action segments of our items report and increment +some counters. + +The report generated here will be used on the stage level to slot our item +reports into appropriate sections of the branched report. + +For details of intent and context of this procedure, please see +Koha::Stockrotationrota->investigate. + +=cut + +sub investigate { + my ( $self ) = @_; + + my $items_report = { + items => [], + log => [], + initiable_items => [], + repatriable_items => [], + advanceable_items => [], + indemand_items => [], + actionable => 0, + stationary => 0, + }; + while ( my $item = $self->next ) { + my $report = $item->investigate; + if ( $report->{reason} eq 'initiation' ) { + $items_report->{initiable}++; + $items_report->{actionable}++; + push @{$items_report->{items}}, $report; + push @{$items_report->{initiable_items}}, $report; + } elsif ( $report->{reason} eq 'repatriation' ) { + $items_report->{repatriable}++; + $items_report->{actionable}++; + push @{$items_report->{items}}, $report; + push @{$items_report->{repatriable_items}}, $report; + } elsif ( $report->{reason} eq 'advancement' ) { + $items_report->{actionable}++; + push @{$items_report->{items}}, $report; + push @{$items_report->{advanceable_items}}, $report; + } elsif ( $report->{reason} eq 'in-demand' ) { + $items_report->{actionable}++; + push @{$items_report->{items}}, $report; + push @{$items_report->{indemand_items}}, $report; + } else { + $items_report->{stationary}++; + push @{$items_report->{log}}, $report; + } + } + + return $items_report; +} 1; diff --git a/Koha/Stockrotationrota.pm b/Koha/Stockrotationrota.pm index 42128929d7..f96c1f021c 100644 --- a/Koha/Stockrotationrota.pm +++ b/Koha/Stockrotationrota.pm @@ -72,15 +72,15 @@ a rota, move it from that rota to this rota. sub add_item { my ( $self, $itemnumber ) = @_; my $sritem = Koha::Stockrotationitems->find($itemnumber); - my $stages = $self->stockrotationstages; - die "Rota is missing stages." unless ( $stages->count > 0 ); if ($sritem) { - $sritem->stage_id($stages->next->stage_id)->store; + $sritem->stage_id($self->first_stage->stage_id) + ->indemand(0)->fresh(1)->store; } else { $sritem = Koha::Stockrotationitem->new({ itemnumber_id => $itemnumber, - stage_id => $stages->next->stage_id, + stage_id => $self->first_stage->stage_id, indemand => 0, + fresh => 1, })->store; } return $self; @@ -102,9 +102,9 @@ sub first_stage { return ( $stage ) ? $stage : $guess; } -=head3 items +=head3 stockrotationitems - my $items = $rota->items; + my $items = $rota->stockrotationitems; Return all items associated with this rota via its stages. @@ -118,6 +118,53 @@ sub stockrotationitems { return $rs; } +=head3 investigate + + my $report = $rota->investigate($report_so_far); + +Aim here is to return $report augmented with content for this rota. We +delegate to $stage->investigate. + +The report will include some basic information and 2 primary reports: + +- per rota report in 'rotas'. This report is mainly used by admins to do check + & compare results. + +- branched report in 'branched'. This is the workhorse: emails to libraries + are compiled from these reports, and they will have the actionable work. + +Both reports are generated in stage based investigations; the rota report is +then glued into place at this stage. + +=cut + +sub investigate { + my ( $self, $report ) = @_; + my $count = $self->stockrotationitems->count; + $report->{sum_items} += $count; + + if ( $self->active ) { + $report->{rotas_active}++; + # stockrotationstages->investigate augments $report with the stage's + # content. This is how 'branched' slowly accumulates all items. + $report = $self->stockrotationstages->investigate($report); + # Add our rota report to the full report. + push @{$report->{rotas}}, { + name => $self->title, + id => $self->rota_id, + items => $report->{tmp_items}, + log => $report->{tmp_log}, + }; + delete $report->{tmp_items}; + delete $report->{tmp_log}; + } else { # Rota is not active. + $report->{rotas_inactive}++; + $report->{items_inactive} += $count; + } + + return $report; +} + =head3 _type =cut diff --git a/Koha/Stockrotationrotas.pm b/Koha/Stockrotationrotas.pm index 8851bb3d13..6981b6a133 100644 --- a/Koha/Stockrotationrotas.pm +++ b/Koha/Stockrotationrotas.pm @@ -43,6 +43,43 @@ Standard Koha::Objects definitions, and additional methods. =cut +=head3 investigate + + my $report = $rotas->investigate; + +Return a report detailing the current status and required actions for all +relevant items spread over rotas. + +See Koha::Stockrotationrota->investigate for details. + +=cut + +sub investigate { + my ( $self ) = @_; + + my $report = { + actionable => 0, + advanceable => 0, + initiable => 0, + items_inactive => 0, + repatriable => 0, + rotas_active => 0, + rotas_inactive => 0, + stationary => 0, + sum_items => 0, + sum_rotas => $self->count, + branched => {}, + rotas => [], + items => [], + }; + + while ( my $rota = $self->next ) { + $report = $rota->investigate($report) + } + + return $report; +} + =head3 _type =cut diff --git a/Koha/Stockrotationstage.pm b/Koha/Stockrotationstage.pm index 4752613a58..b167dcc7e6 100644 --- a/Koha/Stockrotationstage.pm +++ b/Koha/Stockrotationstage.pm @@ -21,7 +21,6 @@ use Modern::Perl; use Koha::Database; use Koha::Library; -use Koha::Stockrotationitems; use Koha::Stockrotationrota; use base qw(Koha::Object); @@ -286,6 +285,125 @@ sub delete { return $self->_result->delete; } +=head3 investigate + + my $report = $stage->investigate($report_so_far); + +Return a stage based report. This report will mutate and augment the report +that is passed to it. It slots item reports into the branched and temporary +rota sections of the report. It also increments a number of counters. + +For details of intent and context of this procedure, please see +Koha::Stockrotationrota->investigate. + +=cut + +sub investigate { + my ( $self, $report ) = @_; + my $new_stage = $self->next_sibling; + my $duration = $self->duration; + # Generate stage items report + my $items_report = $self->stockrotationitems->investigate; + + # Merge into general report + + ## Branched indexes + ### The branched indexes work as follows: + ### - They contain information about the relevant branch + ### - They contain an index of actionable items for that branch + ### - They contain an index of non-actionable items for that branch + + ### Items are assigned to a particular branched index as follows: + ### - 'advanceable' : assigned to branch of the current stage + ### (this should also be the current holding branch) + ### - 'log' items are always assigned to branch of current stage. + ### - 'indemand' : assigned to branch of current stage + ### (this should also be the current holding branch) + ### - 'initiable' : assigned to the current holding branch of item + ### - 'repatriable' : assigned to the current holding branch of item + + ### 'Advanceable', 'log', 'indemand': + + # Set up our stage branch info. + my $stagebranch = $self->_result->branchcode; + my $stagebranchcode = $stagebranch->branchcode; + + # Initiate our stage branch index if it does not yet exist. + if ( !$report->{branched}->{$stagebranchcode} ) { + $report->{branched}->{$stagebranchcode} = { + code => $stagebranchcode, + name => $stagebranch->branchname, + email => $stagebranch->branchemail, + phone => $stagebranch->branchphone, + items => [], + log => [], + }; + } + + push @{$report->{branched}->{$stagebranchcode}->{items}}, + @{$items_report->{advanceable_items}}; + push @{$report->{branched}->{$stagebranchcode}->{log}}, + @{$items_report->{log}}; + push @{$report->{branched}->{$stagebranchcode}->{items}}, + @{$items_report->{indemand_items}}; + + ### 'Initiable' & 'Repatriable' + foreach my $ireport (@{$items_report->{initiable_items}}) { + my $branch = $ireport->{branch}; + my $branchcode = $branch->branchcode; + if ( !$report->{branched}->{$branchcode} ) { + $report->{branched}->{$branchcode} = { + code => $branchcode, + name => $branch->branchname, + email => $branch->branchemail, + phone => $branch->branchphone, + items => [], + log => [], + }; + } + push @{$report->{branched}->{$branchcode}->{items}}, $ireport; + } + + foreach my $ireport (@{$items_report->{repatriable_items}}) { + my $branch = $ireport->{branch}; + my $branchcode = $branch->branchcode; + if ( !$report->{branched}->{$branchcode} ) { + $report->{branched}->{$branchcode} = { + code => $branchcode, + name => $branch->branchname, + email => $branch->branchemail, + phone => $branch->branchphone, + items => [], + log => [], + }; + } + push @{$report->{branched}->{$branchcode}->{items}}, $ireport; + } + + ## Per rota indexes + ### Per rota indexes are item reports pushed into the index for the + ### current rota. We don't know where that index is yet as we don't know + ### about the current rota. To resolve this we assign our items and log + ### to tmp indexes. They will be merged into the proper rota index at the + ### rota level. + push @{$report->{tmp_items}}, @{$items_report->{items}}; + push @{$report->{tmp_log}}, @{$items_report->{log}}; + + ## Collection of items + ### Finally we just add our collection of items to the full item index. + push @{$report->{items}}, @{$items_report->{items}}; + + ## Assemble counters + $report->{actionable} += $items_report->{actionable}; + $report->{indemand} += scalar @{$items_report->{indemand_items}}; + $report->{advanceable} += scalar @{$items_report->{advanceable_items}}; + $report->{initiable} += scalar @{$items_report->{initiable_items}}; + $report->{repatriable} += scalar @{$items_report->{repatriable_items}}; + $report->{stationary} += scalar @{$items_report->{log}}; + + return $report; +} + 1; =head1 AUTHOR diff --git a/Koha/Stockrotationstages.pm b/Koha/Stockrotationstages.pm index b1ad81e0c9..22545f0b79 100644 --- a/Koha/Stockrotationstages.pm +++ b/Koha/Stockrotationstages.pm @@ -43,6 +43,28 @@ Standard Koha::Objects definitions, and additional methods. =cut +=head3 investigate + + my $report = $stages->investigate($rota_so_far); + +Return a report detailing the current status and required actions for all +relevant items spread over the set of stages. + +For details of intent and context of this procedure, please see +Koha::Stockrotationrota->investigate. + +=cut + +sub investigate { + my ( $self, $report ) = @_; + + while ( my $stage = $self->next ) { + $report = $stage->investigate($report); + } + + return $report; +} + =head3 _type =cut diff --git a/misc/cronjobs/stockrotation.pl b/misc/cronjobs/stockrotation.pl index 8a021507ff..df1a9c7290 100755 --- a/misc/cronjobs/stockrotation.pl +++ b/misc/cronjobs/stockrotation.pl @@ -117,146 +117,6 @@ $send_email++ if ( $send_all ); # if we send all, then we must want emails. =head2 Helpers -=head3 investigate - - my $report = investigate($rotas); - -Return a datastructure which contains a wealth of information about the -current state of the stockrotation submodule, and which can be used by -`report_full`, `report_email` and `execute` to either emit reports or perform -database updates. - -$ROTAS should be an iterable set of Koha::Stockrotationrotas. - -No data in the database is manipulated by this procedure. - -=cut - -sub investigate { - my ( $rotas ) = @_; - # Initiate return variables. - my $all_relevant_items = []; - my $all_relevant_rotas = []; - my $branched = {}; - my $rotas_active = 0; - my $rotas_inactive = 0; - my $sum_items = 0; - my $items_inactive = 0; - my $stationary = 0; - my $advanceable = 0; - my $expulseable = 0; - my $repatriable = 0; - my $initiable = 0; - my $actionable = 0; - - # Each 'rota' entry in `rotas` will contain an `items` section with items - # to be moved, and a `logs` section, with items that will not be moved, - # with a reason for not moving it. - while ( my $rota = $rotas->next ) { - if ( $rota->active ) { - $rotas_active++; - my $items = []; - my $log = []; - my $stages = $rota->stockrotationstages; - while ( my $stage = $stages->next) { - my $old_stage = $stage; - my $new_stage = $stage->next_sibling; - my $duration = $stage->duration; - my $branch = $old_stage->_result->branchcode; - my $branchcode = $branch->branchcode; - $branched->{$branchcode} = { - code => $branchcode, - name => $branch->branchname, - email => $branch->branchemail, - phone => $branch->branchphone, - items => [], - }; - my $stage_items = $stage->stockrotationitems; - while ( my $item = $stage_items->next ) { - $sum_items++; - # Generate item details - my $item_report = { - title => $item->itemnumber->_result->biblioitem - ->biblionumber->title, - author => $item->itemnumber->_result->biblioitem - ->biblionumber->author, - callnumber => $item->itemnumber->itemcallnumber, - location => $item->itemnumber->location, - onloan => $item->itemnumber->onloan, - barcode => $item->itemnumber->barcode, - itemnumber => $item->itemnumber_id, - old_stage => $old_stage, - new_stage => $new_stage, - object => $item, - }; - - # Test whether we need to perform an action on item. - my $flag = 0; - if ( $item->needs_initiating ) { - $flag++; - $initiable++; - $actionable++; - $item_report->{reason} = 'initiation'; - } elsif ( $item->needs_repatriating ) { - $flag++; - $repatriable++; - $actionable++; - $item_report->{reason} = 'repatriation'; - } elsif ( $item->needs_advancing ) { - $flag++; - $actionable++; - if ( !$new_stage ) { - $expulseable++; - } else { - $advanceable++; - } - $item_report->{reason} = - ( $item->indemand ) ? 'in-demand' : 'advancement'; - } - - if ( $flag ) { # Item will be moved. - push @{$items}, $item_report; - push @{$all_relevant_items}, $item_report; - push @{$branched->{$branchcode}->{items}}, $item_report; - } else { # Item not ready to be moved. - $stationary++; - $item_report->{reason} = 'not-ready'; - push @{$log}, $item_report; - } - } - } - # Add our rota report to the full report. - push @{$all_relevant_rotas}, { - name => $rota->title, - id => $rota->rota_id, - items => $items, - log => $log, - }; - } else { # Rota is not active. - $rotas_inactive++; - $sum_items += $rota->stockrotationitems->count; - $items_inactive += $rota->stockrotationitems->count; - } - } - - return { - sum_rotas => $rotas->count, - rotas_inactive => $rotas_inactive, - rotas_active => $rotas_active, - sum_items => $sum_items, - items_inactive => $items_inactive, - stationary => $stationary, - actionable => $actionable, - initiable => $initiable, - repatriable => $repatriable, - advanceable => $advanceable, - expulseable => $expulseable, - rotas => $all_relevant_rotas, - items => $all_relevant_items, - branched => $branched, - } -} - =head3 execute undef = execute($report); @@ -280,11 +140,10 @@ sub execute { # Carry out db updates foreach my $item (@{$data->{items}}) { my $reason = $item->{reason}; - if ( $reason eq 'initiation' ) { - $item->{object}->initiate; - } elsif ( $reason eq 'repatriation' ) { + if ( $reason eq 'repatriation' ) { $item->{object}->repatriate; - } elsif ( $reason eq 'in-demand' || $reason eq 'advancement' ) { + } elsif ( grep { $reason eq $_ } + qw/in-demand advancement initiation/ ) { $item->{object}->advance; } } @@ -325,12 +184,11 @@ STOCKROTATION REPORT Total items to be initiated: %5u Total items to be repatriated: %5u Total items to be advanced: %5u - Total items to be expulsed: %5u\n\n", + Total items in demand: %5u\n\n", $data->{sum_rotas}, $data->{rotas_inactive}, $data->{rotas_active}, - $data->{sum_items}, $data->{items_inactive}, - $data->{stationary}, $data->{actionable}, - $data->{initiable}, $data->{repatriable}, - $data->{advanceable}, $data->{expulseable}; + $data->{sum_items}, $data->{items_inactive}, $data->{stationary}, + $data->{actionable}, $data->{initiable}, $data->{repatriable}, + $data->{advanceable}, $data->{indemand}; if (@{$data->{rotas}}) { # Per Rota details $body .= sprintf "ROTAS DETAIL\n------------\n\n"; @@ -441,7 +299,9 @@ sub _report_per_branch { } else { $out .= sprintf "No items to be processed for this branch.\n\n"; } - $out .= join("", map { _print_item($_) } @{$per_branch->{items}}); + $out .= join("", map { + _print_item($_) unless $_->{reason} eq 'in-demand' + } @{$per_branch->{items}}); return { body => $out, # The body of the report email_address => $per_branch->{email}, # The branch email address @@ -465,17 +325,19 @@ No data in the database is manipulated by this procedure. sub _print_item { my ( $item ) = @_; return sprintf " - Title: %s - Author: %s - Callnumber: %s - Location: %s - Barcode: %s - On loan?: %s - Status: %s\n\n", + Title: %s + Author: %s + Callnumber: %s + Location: %s + Barcode: %s + On loan?: %s + Status: %s + Current Library: %s [%s]\n\n", $item->{title} || "N/A", $item->{author} || "N/A", $item->{callnumber} || "N/A", $item->{location} || "N/A", $item->{barcode} || "N/A", $item->{onloan} ? 'Yes' : 'No', - $item->{reason} || "N/A"; + $item->{reason} || "N/A", $item->{branch}->branchname, + $item->{branch}->branchcode; } =head3 emit @@ -557,7 +419,7 @@ sub emit { # Compile Stockrotation Report data my $rotas = Koha::Stockrotationrotas->search; -my $data = investigate($rotas); +my $data = $rotas->investigate; # Perform db updates if requested execute($data) if ( $execute ); diff --git a/t/db_dependent/Stockrotationitems.t b/t/db_dependent/Stockrotationitems.t index 11d5cde13d..4a80f35ecf 100644 --- a/t/db_dependent/Stockrotationitems.t +++ b/t/db_dependent/Stockrotationitems.t @@ -20,11 +20,12 @@ use Modern::Perl; use DateTime; +use DateTime::Duration; use Koha::Database; use Koha::Item::Transfer; use t::lib::TestBuilder; -use Test::More tests => 7; +use Test::More tests => 8; my $schema = Koha::Database->new->schema; @@ -146,12 +147,23 @@ subtest "Tests for repatriate." => sub { }; subtest "Tests for needs_advancing." => sub { - plan tests => 3; + plan tests => 6; $schema->storage->txn_begin; - # Setup a pristine stockrotation context. - my $sritem = $builder->build({ source => 'Stockrotationitem' }); + # Test behaviour of item freshly added to rota. + my $sritem = $builder->build({ + source => 'Stockrotationitem', + value => { 'fresh' => 1, }, + }); my $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + is($dbitem->needs_advancing, 1, "An item that is fresh will always need advancing."); + + # Setup a pristine stockrotation context. + $sritem = $builder->build({ + source => 'Stockrotationitem', + value => { 'fresh' => 0,} + }); + $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id); $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id); $dbitem->stage->position(1); @@ -163,15 +175,23 @@ subtest "Tests for needs_advancing." => sub { 'tobranch' => $dbitem->stage->branchcode_id, 'datesent' => DateTime->now, 'datearrived' => undef, - 'comments' => "Test item", + 'comments' => "StockrotationAdvance", })->store; # Test item will not be advanced if in transit. is($dbitem->needs_advancing, 0, "Not ready to advance: in transfer."); + # Test item will not be advanced if in transit even if fresh. + $dbitem->fresh(1)->store; + is($dbitem->needs_advancing, 0, "Not ready to advance: in transfer (fresh)."); + $dbitem->fresh(0)->store; # Test item will not be advanced if it has not spent enough time. $dbtransfer->datearrived(DateTime->now)->store; is($dbitem->needs_advancing, 0, "Not ready to advance: Not spent enough time."); + # Test item will be advanced if it has not spent enough time, but is fresh. + $dbitem->fresh(1)->store; + is($dbitem->needs_advancing, 1, "Advance: Not spent enough time, but fresh."); + $dbitem->fresh(0)->store; # Test item will be advanced if it has spent enough time. $dbtransfer->datesent( # Item was sent 100 days ago... @@ -186,10 +206,13 @@ subtest "Tests for needs_advancing." => sub { }; subtest "Tests for advance." => sub { - plan tests => 12; + plan tests => 15; $schema->storage->txn_begin; - my $sritem = $builder->build({ source => 'Stockrotationitem' }); + my $sritem = $builder->build({ + source => 'Stockrotationitem', + value => { 'fresh' => 1 } + }); my $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id); my $dbstage = $dbitem->stage; @@ -200,6 +223,12 @@ subtest "Tests for advance." => sub { # Sanity check is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage sanity check."); + # Test if an item is fresh, always move to first stage. + is($dbitem->fresh, 1, "Fresh is correct."); + $dbitem->advance; + is($dbitem->stage->stage_id, $dbstage->stage_id, "Stage is first stage after fresh advance."); + is($dbitem->fresh, 0, "Fresh reset after advance."); + # Test cases of single stage $dbstage->rota->cyclical(1)->store; # Set Rota to cyclical. ok($dbitem->advance, "Single stage cyclical advance done."); @@ -250,4 +279,115 @@ subtest "Tests for advance." => sub { $schema->storage->txn_rollback; }; +subtest "Tests for investigate (singular)." => sub { + plan tests => 7; + $schema->storage->txn_begin; + + # Test brand new item's investigation ['initiation'] + my $sritem = $builder->build({ source => 'Stockrotationitem', value => { fresh => 1 } }); + my $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + is($dbitem->investigate->{reason}, 'initiation', "fresh item initiates."); + + # Test brand new item at stagebranch ['initiation'] + $sritem = $builder->build({ source => 'Stockrotationitem', value => { fresh => 1 } }); + $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id)->store; + $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id)->store; + is($dbitem->investigate->{reason}, 'initiation', "fresh item at stagebranch initiates."); + + # Test item not at stagebranch with branchtransfer history ['repatriation'] + $sritem = $builder->build({ + source => 'Stockrotationitem', + value => { 'fresh' => 0,} + }); + $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + my $dbtransfer = Koha::Item::Transfer->new({ + 'itemnumber' => $dbitem->itemnumber_id, + 'frombranch' => $dbitem->itemnumber->homebranch, + 'tobranch' => $dbitem->itemnumber->homebranch, + 'datesent' => DateTime->now, + 'datearrived' => DateTime->now, + 'comments' => "StockrotationAdvance", + })->store; + is($dbitem->investigate->{reason}, 'repatriation', "older item repatriates."); + + # Test item at stagebranch with branchtransfer history ['not-ready'] + $sritem = $builder->build({ + source => 'Stockrotationitem', + value => { 'fresh' => 0,} + }); + $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + $dbtransfer = Koha::Item::Transfer->new({ + 'itemnumber' => $dbitem->itemnumber_id, + 'frombranch' => $dbitem->itemnumber->homebranch, + 'tobranch' => $dbitem->stage->branchcode_id, + 'datesent' => DateTime->now, + 'datearrived' => DateTime->now, + 'comments' => "StockrotationAdvance", + })->store; + $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id)->store; + $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id)->store; + is($dbitem->investigate->{reason}, 'not-ready', "older item at stagebranch not-ready."); + + # Test item due for advancement ['advancement'] + $sritem = $builder->build({ source => 'Stockrotationitem', value => { fresh => 0 } }); + $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + $dbitem->indemand(0)->store; + $dbitem->stage->duration(50)->store; + my $sent_duration = DateTime::Duration->new( days => 55); + my $arrived_duration = DateTime::Duration->new( days => 52); + $dbtransfer = Koha::Item::Transfer->new({ + 'itemnumber' => $dbitem->itemnumber_id, + 'frombranch' => $dbitem->itemnumber->homebranch, + 'tobranch' => $dbitem->stage->branchcode_id, + 'datesent' => DateTime->now - $sent_duration, + 'datearrived' => DateTime->now - $arrived_duration, + 'comments' => "StockrotationAdvance", + })->store; + $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id)->store; + $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id)->store; + is($dbitem->investigate->{reason}, 'advancement', + "Item ready for advancement."); + + # Test item due for advancement but in-demand ['in-demand'] + $sritem = $builder->build({ source => 'Stockrotationitem', value => { fresh => 0 } }); + $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + $dbitem->indemand(1)->store; + $dbitem->stage->duration(50)->store; + $sent_duration = DateTime::Duration->new( days => 55); + $arrived_duration = DateTime::Duration->new( days => 52); + $dbtransfer = Koha::Item::Transfer->new({ + 'itemnumber' => $dbitem->itemnumber_id, + 'frombranch' => $dbitem->itemnumber->homebranch, + 'tobranch' => $dbitem->stage->branchcode_id, + 'datesent' => DateTime->now - $sent_duration, + 'datearrived' => DateTime->now - $arrived_duration, + 'comments' => "StockrotationAdvance", + })->store; + $dbitem->itemnumber->homebranch($dbitem->stage->branchcode_id)->store; + $dbitem->itemnumber->holdingbranch($dbitem->stage->branchcode_id)->store; + is($dbitem->investigate->{reason}, 'in-demand', + "Item advances, but in-demand."); + + # Test item ready for advancement, but at wrong library ['repatriation'] + $sritem = $builder->build({ source => 'Stockrotationitem', value => { fresh => 0 } }); + $dbitem = Koha::Stockrotationitems->find($sritem->{itemnumber_id}); + $dbitem->indemand(0)->store; + $dbitem->stage->duration(50)->store; + $sent_duration = DateTime::Duration->new( days => 55); + $arrived_duration = DateTime::Duration->new( days => 52); + $dbtransfer = Koha::Item::Transfer->new({ + 'itemnumber' => $dbitem->itemnumber_id, + 'frombranch' => $dbitem->itemnumber->homebranch, + 'tobranch' => $dbitem->stage->branchcode_id, + 'datesent' => DateTime->now - $sent_duration, + 'datearrived' => DateTime->now - $arrived_duration, + 'comments' => "StockrotationAdvance", + })->store; + is($dbitem->investigate->{reason}, 'repatriation', + "Item advances, but not at stage branch."); + + $schema->storage->txn_rollback; +}; + 1; diff --git a/t/db_dependent/Stockrotationrotas.t b/t/db_dependent/Stockrotationrotas.t index 24dcaebc06..d20c7dfec1 100644 --- a/t/db_dependent/Stockrotationrotas.t +++ b/t/db_dependent/Stockrotationrotas.t @@ -72,7 +72,7 @@ subtest 'Basic object tests' => sub { is( Koha::Stockrotationitems->find($item->{itemnumber})->stage_id, - $srrota->stockrotationstages->next->stage_id, + $srrota->first_stage->stage_id, "Adding an item results in a new sritem item being assigned to the first stage." ); diff --git a/t/db_dependent/Stockrotationstages.t b/t/db_dependent/Stockrotationstages.t index edb2e20253..145d318643 100644 --- a/t/db_dependent/Stockrotationstages.t +++ b/t/db_dependent/Stockrotationstages.t @@ -22,7 +22,7 @@ use Modern::Perl; use Koha::Database; use t::lib::TestBuilder; -use Test::More tests => 5; +use Test::More tests => 6; my $schema = Koha::Database->new->schema; @@ -193,4 +193,185 @@ subtest 'Relationship to stockrotationitems' => sub { $schema->storage->txn_rollback; }; + +subtest 'Tests for investigate (singular)' => sub { + + plan tests => 3; + + # In this subtest series we will primarily be testing whether items end up + # in the correct 'branched' section of the stage-report. We don't care + # for item reasons here, as they are tested in Stockrotationitems. + + # We will run tests on first on an empty report (the base-case) and then + # on a populated report. + + # We will need: + # - Libraries which will hold the Items + # - Rota Which containing the related stages + # + Stages on which we run investigate + # * Items on the stages + + $schema->storage->txn_begin; + + # Libraries + my $library1 = $builder->build({ source => 'Branch' }); + my $library2 = $builder->build({ source => 'Branch' }); + my $library3 = $builder->build({ source => 'Branch' }); + + my $stage1lib = $builder->build({ source => 'Branch' }); + my $stage2lib = $builder->build({ source => 'Branch' }); + my $stage3lib = $builder->build({ source => 'Branch' }); + my $stage4lib = $builder->build({ source => 'Branch' }); + + my $libraries = [ $library1, $library2, $library3, $stage1lib, $stage2lib, + $stage3lib, $stage4lib ]; + + # Rota + my $rota = $builder->build({ + source => 'Stockrotationrota', + value => { cyclical => 0 }, + }); + + # Stages + my $stage1 = $builder->build({ + source => 'Stockrotationstage', + value => { + rota_id => $rota->{rota_id}, + branchcode_id => $stage1lib->{branchcode}, + duration => 10, + position => 1, + }, + }); + my $stage2 = $builder->build({ + source => 'Stockrotationstage', + value => { + rota_id => $rota->{rota_id}, + branchcode_id => $stage2lib->{branchcode}, + duration => 20, + position => 2, + }, + }); + my $stage3 = $builder->build({ + source => 'Stockrotationstage', + value => { + rota_id => $rota->{rota_id}, + branchcode_id => $stage3lib->{branchcode}, + duration => 10, + position => 3, + }, + }); + my $stage4 = $builder->build({ + source => 'Stockrotationstage', + value => { + rota_id => $rota->{rota_id}, + branchcode_id => $stage4lib->{branchcode}, + duration => 20, + position => 4, + }, + }); + + # Test on an empty report. + my $spec = { + $library1->{branchcode} => 1, + $library2->{branchcode} => 1, + $library3->{branchcode} => 1, + $stage1lib->{branchcode} => 2, + $stage2lib->{branchcode} => 1, + $stage3lib->{branchcode} => 3, + $stage4lib->{branchcode} => 4 + }; + while ( my ( $code, $count ) = each %{$spec} ) { + my $cnt = 0; + while ( $cnt < $count ) { + my $item = $builder->build({ + source => 'Stockrotationitem', + value => { + stage_id => $stage1->{stage_id}, + indemand => 0, + fresh => 1, + } + }); + my $dbitem = Koha::Stockrotationitems->find($item); + $dbitem->itemnumber->homebranch($code) + ->holdingbranch($code)->store; + $cnt++; + } + } + my $report = Koha::Stockrotationstages + ->find($stage1->{stage_id})->investigate; + my $results = []; + foreach my $lib ( @{$libraries} ) { + my $items = $report->{branched}->{$lib->{branchcode}}->{items} || []; + push @{$results}, + scalar @{$items}; + } + + # Items assigned to stag1lib -> log, hence $results[4] = 0; + is_deeply( $results, [ 1, 1, 1, 2, 1, 3, 4 ], "Empty report test 1."); + + # Now we test by adding the next stage's items to the same report. + $spec = { + $library1->{branchcode} => 3, + $library2->{branchcode} => 2, + $library3->{branchcode} => 1, + $stage1lib->{branchcode} => 4, + $stage2lib->{branchcode} => 2, + $stage3lib->{branchcode} => 0, + $stage4lib->{branchcode} => 3 + }; + while ( my ( $code, $count ) = each %{$spec} ) { + my $cnt = 0; + while ( $cnt < $count ) { + my $item = $builder->build({ + source => 'Stockrotationitem', + value => { + stage_id => $stage2->{stage_id}, + indemand => 0, + fresh => 1, + } + }); + my $dbitem = Koha::Stockrotationitems->find($item); + $dbitem->itemnumber->homebranch($code) + ->holdingbranch($code)->store; + $cnt++; + } + } + + $report = Koha::Stockrotationstages + ->find($stage2->{stage_id})->investigate($report); + $results = []; + foreach my $lib ( @{$libraries} ) { + my $items = $report->{branched}->{$lib->{branchcode}}->{items} || []; + push @{$results}, + scalar @{$items}; + } + is_deeply( $results, [ 4, 3, 2, 6, 3, 3, 7 ], "full report test."); + + # Carry out db updates + foreach my $item (@{$report->{items}}) { + my $reason = $item->{reason}; + if ( $reason eq 'repatriation' ) { + $item->{object}->repatriate; + } elsif ( grep { $reason eq $_ } + qw/in-demand advancement initiation/ ) { + $item->{object}->advance; + } + } + + $report = Koha::Stockrotationstages + ->find($stage1->{stage_id})->investigate; + $results = []; + foreach my $lib ( @{$libraries} ) { + my $items = $report->{branched}->{$lib->{branchcode}}->{items} || []; + push @{$results}, + scalar @{$items}; + } + # All items have been 'initiated', which means they are either happily in + # transit or happily at the library they are supposed to be. Either way + # they will register as 'not-ready' in the stock rotation report. + is_deeply( $results, [ 0, 0, 0, 0, 0, 0, 0 ], "All items now in logs."); + + $schema->storage->txn_rollback; +}; + 1; -- 2.11.0