Attachment 46875 Details for Bug 14310 – Bug 14310 [QA Followup] - Adapt existing code to use new methods

[patch] Bug 14310 [QA Followup] - Adapt existing code to use new methods

Bug-14310-QA-Followup---Adapt-existing-code-to-use.patch (text/plain), 8.77 KB, created by Jonathan Druart on 2016-01-19 13:59:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2016-01-19 13:59:32 UTC

Size: 8.77 KB

patch

obsolete

>From 4e437f08d3607e6215bf7d91cd7202712088043b Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Tue, 19 Jan 2016 12:18:40 +0000
>Subject: [PATCH] Bug 14310 [QA Followup] - Adapt existing code to use new
> methods
>
>Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>---
> 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 2808c97..865bf5d 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.0

Actions: View | Diff | Splinter Review

Attachments on bug 14310: 39750 | 39751 | 39752 | 39753 | 39754 | 40024 | 40030 | 40031 | 40034 | 43525 | 43526 | 43830 | 43831 | 43832 | 43978 | 44203 | 44204 | 44205 | 44206 | 44207 | 46857 | 46858 | 46859 | 46860 | 46861 | 46862 | 46870 | 46871 | 46872 | 46873 | 46874 | 46875 | 46876