From f1265cdb454128fd4be960557b4c27858d8addfc Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Tue, 19 Jan 2016 12:18:40 +0000 Subject: [PATCH] Bug 14310 [QA Followup] - Adapt existing code to use new methods --- C4/Reserves.pm | 99 ++++++++++++++++++++------------------------------ Koha/Hold.pm | 16 +++++++- t/db_dependent/Hold.t | 3 +- t/db_dependent/Holds.t | 23 +++++++++++- 4 files changed, 76 insertions(+), 65 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 7bb5d69..8794ea8 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1034,13 +1034,11 @@ Unsuspends all suspended reserves with a suspend_until date from before today. =cut sub AutoUnsuspendReserves { + my $today = dt_from_string(); - my $dbh = C4::Context->dbh; - - my $query = "UPDATE reserves SET suspend = 0, suspend_until = NULL WHERE DATE( suspend_until ) < DATE( CURDATE() )"; - my $sth = $dbh->prepare( $query ); - $sth->execute(); + my @holds = Koha::Holds->search( { suspend_until => { '<' => $today->ymd() } } ); + map { $_->suspend(0)->suspend_until(undef)->store() } @holds; } =head2 CancelReserve @@ -1161,19 +1159,26 @@ sub ModReserve { CancelReserve({ reserve_id => $reserve_id }); } elsif ($rank =~ /^\d+/ and $rank > 0) { - my $query = " - UPDATE reserves SET priority = ? ,branchcode = ?, itemnumber = ?, found = NULL, waitingdate = NULL - WHERE reserve_id = ? - "; - my $sth = $dbh->prepare($query); - $sth->execute( $rank, $branchcode, $itemnumber, $reserve_id ); + my $hold = Koha::Holds->find($reserve_id); + + $hold->set( + { + priority => $rank, + branchcode => $branchcode, + itemnumber => $itemnumber, + found => undef, + waitingdate => undef + } + )->store(); if ( defined( $suspend_until ) ) { if ( $suspend_until ) { - $suspend_until = eval { output_pref( { dt => dt_from_string( $suspend_until ), dateonly => 1, dateformat => 'iso' }); }; - $dbh->do("UPDATE reserves SET suspend = 1, suspend_until = ? WHERE reserve_id = ?", undef, ( $suspend_until, $reserve_id ) ); + $suspend_until = eval { dt_from_string( $suspend_until ) }; + $hold->suspend_hold( $suspend_until ); } else { - $dbh->do("UPDATE reserves SET suspend_until = NULL WHERE reserve_id = ?", undef, ( $reserve_id ) ); + # FIXME: Why are we doing this? Should this be resuming the hold, + # or maybe suspending it indefinitely? + $hold->set( { suspend_until => undef } )->store(); } } @@ -1614,29 +1619,15 @@ be cleared when it is unsuspended. sub ToggleSuspend { my ( $reserve_id, $suspend_until ) = @_; - $suspend_until = output_pref( - { - dt => dt_from_string($suspend_until), - dateformat => 'iso', - dateonly => 1 - } - ) if ($suspend_until); - - my $do_until = ( $suspend_until ) ? '?' : 'NULL'; - - my $dbh = C4::Context->dbh; + $suspend_until = dt_from_string($suspend_until) if ($suspend_until); - my $sth = $dbh->prepare( - "UPDATE reserves SET suspend = NOT suspend, - suspend_until = CASE WHEN suspend = 0 THEN NULL ELSE $do_until END - WHERE reserve_id = ? - "); + my $hold = Koha::Holds->find( $reserve_id ); - my @params; - push( @params, $suspend_until ) if ( $suspend_until ); - push( @params, $reserve_id ); - - $sth->execute( @params ); + if ( $hold->is_suspended ) { + $hold->resume() + } else { + $hold->suspend_hold( $suspend_until ); + } } =head2 SuspendAll @@ -1661,38 +1652,26 @@ sub SuspendAll { my $borrowernumber = $params{'borrowernumber'} || undef; my $biblionumber = $params{'biblionumber'} || undef; my $suspend_until = $params{'suspend_until'} || undef; - my $suspend = defined( $params{'suspend'} ) ? $params{'suspend'} : 1; + my $suspend = defined( $params{'suspend'} ) ? $params{'suspend'} : 1; - $suspend_until = eval { output_pref( { dt => dt_from_string( $suspend_until), dateonly => 1, dateformat => 'iso' } ); } - if ( defined( $suspend_until ) ); + $suspend_until = eval { dt_from_string($suspend_until) } + if ( defined($suspend_until) ); return unless ( $borrowernumber || $biblionumber ); - my ( $query, $sth, $dbh, @query_params ); + my $params; + $params->{found} = undef; + $params->{borrowernumber} = $borrowernumber if $borrowernumber; + $params->{biblionumber} = $biblionumber if $biblionumber; - $query = "UPDATE reserves SET suspend = ? "; - push( @query_params, $suspend ); - if ( !$suspend ) { - $query .= ", suspend_until = NULL "; - } elsif ( $suspend_until ) { - $query .= ", suspend_until = ? "; - push( @query_params, $suspend_until ); - } - $query .= " WHERE "; - if ( $borrowernumber ) { - $query .= " borrowernumber = ? "; - push( @query_params, $borrowernumber ); + my @holds = Koha::Holds->search($params); + + if ($suspend) { + map { $_->suspend_hold($suspend_until) } @holds; } - $query .= " AND " if ( $borrowernumber && $biblionumber ); - if ( $biblionumber ) { - $query .= " biblionumber = ? "; - push( @query_params, $biblionumber ); + else { + map { $_->resume() } @holds; } - $query .= " AND found IS NULL "; - - $dbh = C4::Context->dbh; - $sth = $dbh->prepare( $query ); - $sth->execute( @query_params ); } diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 707622d..0ecded9 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -50,13 +50,13 @@ my $hold = $hold->suspend_hold( $suspend_until_dt ); sub suspend_hold { my ( $self, $dt ) = @_; + $dt = $dt ? $dt->clone()->truncate( to => 'day' ) : undef; + if ( $self->is_waiting ) { # We can't suspend waiting holds carp "Unable to suspend waiting hold!"; return $self; } - $dt ||= undef; - $self->suspend(1); $self->suspend_until( $dt ); @@ -235,6 +235,18 @@ sub borrower { return $self->{_borrower}; } +=head3 is_suspended + +my $bool = $hold->is_suspended(); + +=cut + +sub is_suspended { + my ( $self ) = @_; + + return $self->suspend(); +} + =head3 type =cut diff --git a/t/db_dependent/Hold.t b/t/db_dependent/Hold.t index 48f3831..6307e5d 100755 --- a/t/db_dependent/Hold.t +++ b/t/db_dependent/Hold.t @@ -75,8 +75,9 @@ $hold->resume(); is( $hold->suspend, 0, "Hold is not suspended" ); my $dt = dt_from_string(); $hold->suspend_hold( $dt ); +$dt->truncate( to => 'day' ); is( $hold->suspend, 1, "Hold is suspended" ); -is( $hold->suspend_until, "$dt", "Hold is suspended with a date" ); +is( $hold->suspend_until, "$dt", "Hold is suspended with a date, truncation takes place automatically" ); $hold->resume(); is( $hold->suspend, 0, "Hold is not suspended" ); is( $hold->suspend_until, undef, "Hold no longer has suspend_until date" ); diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 8767223..29095d0 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -8,7 +8,7 @@ use t::lib::TestBuilder; use C4::Context; use C4::Branch; -use Test::More tests => 56; +use Test::More tests => 60; use MARC::Record; use C4::Biblio; use C4::Items; @@ -145,12 +145,31 @@ ok( !$reserve->{'suspend'}, "Test ToggleSuspend(), no date" ); ToggleSuspend( $reserve_id, '2012-01-01' ); $reserve = GetReserve( $reserve_id ); -ok( $reserve->{'suspend_until'} eq '2012-01-01 00:00:00', "Test ToggleSuspend(), with date" ); +is( $reserve->{'suspend_until'}, '2012-01-01 00:00:00', "Test ToggleSuspend(), with date" ); AutoUnsuspendReserves(); $reserve = GetReserve( $reserve_id ); ok( !$reserve->{'suspend'}, "Test AutoUnsuspendReserves()" ); +SuspendAll( + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + suspend => 1, + suspend_until => '2012-01-01', +); +$reserve = GetReserve( $reserve_id ); +is( $reserve->{'suspend'}, 1, "Test SuspendAll()" ); +is( $reserve->{'suspend_until'}, '2012-01-01 00:00:00', "Test SuspendAll(), with date" ); + +SuspendAll( + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + suspend => 0, +); +$reserve = GetReserve( $reserve_id ); +is( $reserve->{'suspend'}, 0, "Test resuming with SuspendAll()" ); +is( $reserve->{'suspend_until'}, undef, "Test resuming with SuspendAll(), should have no suspend until date" ); + # Add a new hold for the borrower whose hold we canceled earlier, this time at the bib level AddReserve( $branch_1, -- 2.1.4