Bugzilla – Attachment 65618 Details for
Bug 19057
Move C4::Reserves::GetReserve to the Koha namespace
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 19057: Move C4::Reserve::GetReserve to Koha::Holds
Bug-19057-Move-C4ReserveGetReserve-to-KohaHolds.patch (text/plain), 17.59 KB, created by
Marc Véron
on 2017-08-08 08:40:43 UTC
(
hide
)
Description:
Bug 19057: Move C4::Reserve::GetReserve to Koha::Holds
Filename:
MIME Type:
Creator:
Marc Véron
Created:
2017-08-08 08:40:43 UTC
Size:
17.59 KB
patch
obsolete
>From 9b1a68a9b981c335b5b04913826839173a006110 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Wed, 2 Aug 2017 12:21:20 -0300 >Subject: [PATCH] Bug 19057: Move C4::Reserve::GetReserve to Koha::Holds >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >This GetReserve subroutine can be replaced with Koha::Holds->find > >Test plan: >- git grep GetReserve >must not return results where GetReserve is called >- Cancel a reserve > >Signed-off-by: Marc Véron <veron@veron.ch> >--- > C4/ILSDI/Services.pm | 6 +-- > C4/Reserves.pm | 86 +++++++++++++++---------------- > Koha/REST/V1/Hold.pm | 18 +++---- > t/db_dependent/Circulation/issue.t | 5 +- > t/db_dependent/Holds.t | 68 ++++++++++++------------ > t/db_dependent/Items/MoveItemFromBiblio.t | 13 ++--- > t/db_dependent/Reserves.t | 10 ++-- > 7 files changed, 101 insertions(+), 105 deletions(-) > >diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm >index 5d35b2b..dd52619 100644 >--- a/C4/ILSDI/Services.pm >+++ b/C4/ILSDI/Services.pm >@@ -769,9 +769,9 @@ sub CancelHold { > > # Get the reserve or return an error code > my $reserve_id = $cgi->param('item_id'); >- my $reserve = C4::Reserves::GetReserve($reserve_id); >- return { code => 'RecordNotFound' } unless $reserve; >- return { code => 'RecordNotFound' } unless ($reserve->{borrowernumber} == $borrowernumber); >+ my $hold = Koha::Holds->find( $reserve_id ); >+ return { code => 'RecordNotFound' } unless $hold; >+ return { code => 'RecordNotFound' } unless ($hold->borrowernumber == $borrowernumber); > > C4::Reserves::CancelReserve({reserve_id => $reserve_id}); > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 924ce5f..e1b3227 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -465,10 +465,10 @@ sub CanReserveBeCanceledFromOpac { > my ($reserve_id, $borrowernumber) = @_; > > return unless $reserve_id and $borrowernumber; >- my $reserve = GetReserve($reserve_id); >+ my $reserve = Koha::Holds->find($reserve_id); > >- return 0 unless $reserve->{borrowernumber} == $borrowernumber; >- return 0 if ( $reserve->{found} eq 'W' ) or ( $reserve->{found} eq 'T' ); >+ return 0 unless $reserve->borrowernumber == $borrowernumber; >+ return 0 if ( $reserve->found eq 'W' ) or ( $reserve->found eq 'T' ); > > return 1; > >@@ -874,48 +874,46 @@ sub CancelReserve { > > my $dbh = C4::Context->dbh; > >- my $reserve = GetReserve( $reserve_id ); >- if ($reserve) { >+ my $hold = Koha::Holds->find( $reserve_id ); >+ return unless $hold; > >- my $hold = Koha::Holds->find( $reserve_id ); >- logaction( 'HOLDS', 'CANCEL', $hold->reserve_id, Dumper($hold->unblessed) ) >- if C4::Context->preference('HoldsLog'); >+ logaction( 'HOLDS', 'CANCEL', $hold->reserve_id, Dumper($hold->unblessed) ) >+ if C4::Context->preference('HoldsLog'); > >- my $query = " >- UPDATE reserves >- SET cancellationdate = now(), >- priority = 0 >- WHERE reserve_id = ? >- "; >- my $sth = $dbh->prepare($query); >- $sth->execute( $reserve_id ); >+ my $query = " >+ UPDATE reserves >+ SET cancellationdate = now(), >+ priority = 0 >+ WHERE reserve_id = ? >+ "; >+ my $sth = $dbh->prepare($query); >+ $sth->execute( $reserve_id ); > >- $query = " >- INSERT INTO old_reserves >- SELECT * FROM reserves >- WHERE reserve_id = ? >- "; >- $sth = $dbh->prepare($query); >- $sth->execute( $reserve_id ); >+ $query = " >+ INSERT INTO old_reserves >+ SELECT * FROM reserves >+ WHERE reserve_id = ? >+ "; >+ $sth = $dbh->prepare($query); >+ $sth->execute( $reserve_id ); > >- $query = " >- DELETE FROM reserves >- WHERE reserve_id = ? >- "; >- $sth = $dbh->prepare($query); >- $sth->execute( $reserve_id ); >+ $query = " >+ DELETE FROM reserves >+ WHERE reserve_id = ? >+ "; >+ $sth = $dbh->prepare($query); >+ $sth->execute( $reserve_id ); > >- # now fix the priority on the others.... >- _FixPriority({ biblionumber => $reserve->{biblionumber} }); >+ # now fix the priority on the others.... >+ _FixPriority({ biblionumber => $hold->biblionumber }); > >- # and, if desired, charge a cancel fee >- my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); >- if ( $charge && $params->{'charge_cancel_fee'} ) { >- manualinvoice($reserve->{'borrowernumber'}, $reserve->{'itemnumber'}, '', 'HE', $charge); >- } >+ # and, if desired, charge a cancel fee >+ my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); >+ if ( $charge && $params->{'charge_cancel_fee'} ) { >+ manualinvoice($hold->borrowernumber, $hold->itemnumber, '', 'HE', $charge); > } > >- return $reserve; >+ return $hold->unblessed; > } > > =head2 ModReserve >@@ -1311,16 +1309,17 @@ Input: $where is 'up', 'down', 'top' or 'bottom'. Biblionumber, Date reserve was > sub AlterPriority { > my ( $where, $reserve_id ) = @_; > >- my $reserve = GetReserve( $reserve_id ); >+ my $hold = Koha::Holds->find( $reserve_id ); >+ return unless $hold; > >- if ( $reserve->{cancellationdate} ) { >- warn "I cannot alter the priority for reserve_id $reserve_id, the reserve has been cancelled (".$reserve->{cancellationdate}.')'; >+ if ( $hold->cancellationdate ) { >+ warn "I cannot alter the priority for reserve_id $reserve_id, the reserve has been cancelled (" . $hold->cancellationdate . ')'; > return; > } > > if ( $where eq 'up' || $where eq 'down' ) { > >- my $priority = $reserve->{'priority'}; >+ my $priority = $hold->priority; > $priority = $where eq 'up' ? $priority - 1 : $priority + 1; > _FixPriority({ reserve_id => $reserve_id, rank => $priority }) > >@@ -1333,6 +1332,7 @@ sub AlterPriority { > _FixPriority({ reserve_id => $reserve_id, rank => '999999' }); > > } >+ # FIXME Should return the new priority > } > > =head2 ToggleLowestPriority >@@ -1468,8 +1468,8 @@ sub _FixPriority { > my $dbh = C4::Context->dbh; > > unless ( $biblionumber ) { >- my $res = GetReserve( $reserve_id ); >- $biblionumber = $res->{biblionumber}; >+ my $hold = Koha::Holds->find( $reserve_id ); >+ $biblionumber = $hold->biblionumber; > } > > if ( $rank eq "del" ) { >diff --git a/Koha/REST/V1/Hold.pm b/Koha/REST/V1/Hold.pm >index 9128c24..a31a9e6 100644 >--- a/Koha/REST/V1/Hold.pm >+++ b/Koha/REST/V1/Hold.pm >@@ -118,11 +118,10 @@ sub edit { > my ($c, $args, $cb) = @_; > > my $reserve_id = $args->{reserve_id}; >- my $reserve = C4::Reserves::GetReserve($reserve_id); >+ my $hold = Koha::Holds->find( $reserve_id ); > >- unless ($reserve) { >- return $c->$cb({error => "Reserve not found"}, 404); >- } >+ return $c->$cb({error => "Reserve not found"}, 404) >+ unless $hold; > > my $body = $c->req->json; > >@@ -142,20 +141,19 @@ sub edit { > }; > > C4::Reserves::ModReserve($params); >- $reserve = Koha::Holds->find($reserve_id); >+ $hold = Koha::Holds->find($reserve_id); > >- return $c->$cb($reserve, 200); >+ return $c->$cb($hold, 200); > } > > sub delete { > my ($c, $args, $cb) = @_; > > my $reserve_id = $args->{reserve_id}; >- my $reserve = C4::Reserves::GetReserve($reserve_id); >+ my $hold = Koha::Holds->find( $reserve_id ); > >- unless ($reserve) { >- return $c->$cb({error => "Reserve not found"}, 404); >- } >+ return $c->$cb({error => "Reserve not found"}, 404) >+ unless $hold; > > C4::Reserves::CancelReserve({ reserve_id => $reserve_id }); > >diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t >index 5adb123..ed8b3c8 100644 >--- a/t/db_dependent/Circulation/issue.t >+++ b/t/db_dependent/Circulation/issue.t >@@ -31,6 +31,7 @@ use C4::Members; > use C4::Reserves; > use Koha::Database; > use Koha::DateUtils; >+use Koha::Holds; > use Koha::Library; > use Koha::Patrons; > >@@ -373,8 +374,8 @@ my $reserve_id = AddReserve($branchcode_1, $borrower_id1, $biblionumber, > undef, 1, undef, undef, "a note", "a title", undef, ''); > ok( $reserve_id, 'The reserve should have been inserted' ); > AddIssue( $borrower_2, $barcode_1, dt_from_string, 'cancel' ); >-my $reserve = GetReserve( $reserve_id ); >-is( $reserve, undef, 'The reserve should have been correctly cancelled' ); >+my $hold = Koha::Holds->find( $reserve_id ); >+is( $hold, undef, 'The reserve should have been correctly cancelled' ); > > #End transaction > $schema->storage->txn_rollback; >diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t >index 871d687..3d41670 100755 >--- a/t/db_dependent/Holds.t >+++ b/t/db_dependent/Holds.t >@@ -145,22 +145,22 @@ ModReserve({ > suspend_until => output_pref( { dt => dt_from_string( "2013-01-01", "iso" ), dateonly => 1 } ), > }); > >-my $reserve = GetReserve( $reserve_id ); >-ok( $reserve->{'priority'} eq '4', "Test GetReserve(), priority changed correctly" ); >-ok( $reserve->{'suspend'}, "Test GetReserve(), suspend hold" ); >-is( $reserve->{'suspend_until'}, '2013-01-01 00:00:00', "Test GetReserve(), suspend until date" ); >+$hold = Koha::Holds->find( $reserve_id ); >+ok( $hold->priority eq '4', "Test ModReserve, priority changed correctly" ); >+ok( $hold->suspend, "Test ModReserve, suspend hold" ); >+is( $hold->suspend_until, '2013-01-01 00:00:00', "Test ModReserve, suspend until date" ); > > ToggleSuspend( $reserve_id ); >-$reserve = GetReserve( $reserve_id ); >-ok( !$reserve->{'suspend'}, "Test ToggleSuspend(), no date" ); >+$hold = Koha::Holds->find( $reserve_id ); >+ok( ! $hold->suspend, "Test ToggleSuspend(), no date" ); > > ToggleSuspend( $reserve_id, '2012-01-01' ); >-$reserve = GetReserve( $reserve_id ); >-is( $reserve->{'suspend_until'}, '2012-01-01 00:00:00', "Test ToggleSuspend(), with date" ); >+$hold = Koha::Holds->find( $reserve_id ); >+is( $hold->suspend_until, '2012-01-01 00:00:00', "Test ToggleSuspend(), with date" ); > > AutoUnsuspendReserves(); >-$reserve = GetReserve( $reserve_id ); >-ok( !$reserve->{'suspend'}, "Test AutoUnsuspendReserves()" ); >+$hold = Koha::Holds->find( $reserve_id ); >+ok( ! $hold->suspend, "Test AutoUnsuspendReserves()" ); > > SuspendAll( > borrowernumber => $borrowernumber, >@@ -168,18 +168,18 @@ SuspendAll( > 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" ); >+$hold = Koha::Holds->find( $reserve_id ); >+is( $hold->suspend, 1, "Test SuspendAll()" ); >+is( $hold->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" ); >+$hold = Koha::Holds->find( $reserve_id ); >+is( $hold->suspend, 0, "Test resuming with SuspendAll()" ); >+is( $hold->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( >@@ -204,27 +204,27 @@ my $reserveid = C4::Reserves::GetReserveId( > } > ); > is( $reserveid, $holds->next->reserve_id, "Test GetReserveId" ); >-ModReserveMinusPriority( $itemnumber, $reserve->{'reserve_id'} ); >+ModReserveMinusPriority( $itemnumber, $reserveid ); > $holds = $patron->holds; > is( $holds->next->itemnumber, $itemnumber, "Test ModReserveMinusPriority()" ); > > $holds = $biblio->holds; > $hold = $holds->next; > AlterPriority( 'top', $hold->reserve_id ); >-$reserve = GetReserve( $reserve->{'reserve_id'} ); >-is( $reserve->{'priority'}, '1', "Test AlterPriority(), move to top" ); >+$hold = Koha::Holds->find( $reserveid ); >+is( $hold->priority, '1', "Test AlterPriority(), move to top" ); > >-AlterPriority( 'down', $reserve->{'reserve_id'} ); >-$reserve = GetReserve( $reserve->{'reserve_id'} ); >-is( $reserve->{'priority'}, '2', "Test AlterPriority(), move down" ); >+AlterPriority( 'down', $hold->reserve_id ); >+$hold = Koha::Holds->find( $reserveid ); >+is( $hold->priority, '2', "Test AlterPriority(), move down" ); > >-AlterPriority( 'up', $reserve->{'reserve_id'} ); >-$reserve = GetReserve( $reserve->{'reserve_id'} ); >-is( $reserve->{'priority'}, '1', "Test AlterPriority(), move up" ); >+AlterPriority( 'up', $hold->reserve_id ); >+$hold = Koha::Holds->find( $reserveid ); >+is( $hold->priority, '1', "Test AlterPriority(), move up" ); > >-AlterPriority( 'bottom', $reserve->{'reserve_id'} ); >-$reserve = GetReserve( $reserve->{'reserve_id'} ); >-is( $reserve->{'priority'}, '5', "Test AlterPriority(), move to bottom" ); >+AlterPriority( 'bottom', $hold->reserve_id ); >+$hold = Koha::Holds->find( $reserveid ); >+is( $hold->priority, '5', "Test AlterPriority(), move to bottom" ); > > # Regression test for bug 2394 > # >@@ -315,8 +315,8 @@ my $reserveid2 = C4::Reserves::GetReserveId( > > CancelReserve({ reserve_id => $reserveid1 }); > >-my $reserve2 = GetReserve( $reserveid2 ); >-is( $reserve2->{priority}, 1, "After cancelreserve, the 2nd reserve becomes the first on the waiting list" ); >+my $hold2 = Koha::Holds->find( $reserveid2 ); >+is( $hold2->priority, 1, "After cancelreserve, the 2nd reserve becomes the first on the waiting list" ); > > ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $branch_1, holdingbranch => $branch_1 } , $bibnum); > AddReserve( >@@ -333,12 +333,12 @@ my $reserveid3 = C4::Reserves::GetReserveId( > } > ); > >-my $reserve3 = GetReserve( $reserveid3 ); >-is( $reserve3->{priority}, 2, "New reserve for patron 0, the reserve has a priority = 2" ); >+my $hold3 = Koha::Holds->find( $reserveid3 ); >+is( $hold3->priority, 2, "New reserve for patron 0, the reserve has a priority = 2" ); > > ModReserve({ reserve_id => $reserveid2, rank => 'del' }); >-$reserve3 = GetReserve( $reserveid3 ); >-is( $reserve3->{priority}, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" ); >+$hold3 = Koha::Holds->find( $reserveid3 ); >+is( $hold3->priority, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" ); > > ModItem({ damaged => 1 }, $item_bibnum, $itemnumber); > t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 1 ); >diff --git a/t/db_dependent/Items/MoveItemFromBiblio.t b/t/db_dependent/Items/MoveItemFromBiblio.t >index 29078ca..44bb318 100644 >--- a/t/db_dependent/Items/MoveItemFromBiblio.t >+++ b/t/db_dependent/Items/MoveItemFromBiblio.t >@@ -21,6 +21,7 @@ use Test::More tests => 8; > use C4::Items; > use C4::Reserves; > use Koha::Database; >+use Koha::Holds; > > use t::lib::TestBuilder; > use Data::Dumper qw|Dumper|; >@@ -85,13 +86,13 @@ is( $get_item2->{biblionumber}, $to_biblio->{biblionumber}, 'The item2 should ha > my $get_item3 = C4::Items::GetItem( $item3->{itemnumber} ); > is( $get_item3->{biblionumber}, $to_biblio->{biblionumber}, 'The item3 should not have been moved' ); > >-my $get_bib_level_hold = C4::Reserves::GetReserve( $bib_level_hold_not_to_move->{reserve_id} ); >-my $get_item_level_hold_1 = C4::Reserves::GetReserve( $item_level_hold_not_to_move->{reserve_id} ); >-my $get_item_level_hold_2 = C4::Reserves::GetReserve( $item_level_hold_to_move->{reserve_id} ); >+my $get_bib_level_hold = Koha::Holds->find( $bib_level_hold_not_to_move->{reserve_id} ); >+my $get_item_level_hold_1 = Koha::Holds->find( $item_level_hold_not_to_move->{reserve_id} ); >+my $get_item_level_hold_2 = Koha::Holds->find( $item_level_hold_to_move->{reserve_id} ); > >-is( $get_bib_level_hold->{biblionumber}, $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the biblio-level hold' ); >-is( $get_item_level_hold_1->{biblionumber}, $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the item-level hold placed on item 1' ); >-is( $get_item_level_hold_2->{biblionumber}, $to_biblio->{biblionumber}, 'MoveItemFromBiblio should have moved the item-level hold placed on item 2' ); >+is( $get_bib_level_hold->biblionumber, $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the biblio-level hold' ); >+is( $get_item_level_hold_1->biblionumber, $from_biblio->{biblionumber}, 'MoveItemFromBiblio should not have moved the item-level hold placed on item 1' ); >+is( $get_item_level_hold_2->biblionumber, $to_biblio->{biblionumber}, 'MoveItemFromBiblio should have moved the item-level hold placed on item 2' ); > > $schema->storage->txn_rollback; > >diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t >index 810050e..94c3c94 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -17,7 +17,7 @@ > > use Modern::Perl; > >-use Test::More tests => 72; >+use Test::More tests => 70; > use Test::MockModule; > use Test::Warn; > >@@ -321,16 +321,12 @@ $holds = $biblio->holds; > is($holds->count, 1, "Only one reserves for this biblio"); > my $reserve_id = $holds->next->reserve_id; > >-$reserve = GetReserve($reserve_id); >-isa_ok($reserve, 'HASH', "GetReserve return"); >-is($reserve->{biblionumber}, $biblionumber); >- > $reserve = CancelReserve({reserve_id => $reserve_id}); > isa_ok($reserve, 'HASH', "CancelReserve return"); > is($reserve->{biblionumber}, $biblionumber); > >-$reserve = GetReserve($reserve_id); >-is($reserve, undef, "GetReserve returns undef after deletion"); >+my $hold = Koha::Holds->find( $reserve_id ); >+is($hold, undef, "CancelReserve should have cancel the reserve"); > > $reserve = CancelReserve({reserve_id => $reserve_id}); > is($reserve, undef, "CancelReserve return undef if reserve does not exist"); >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 19057
:
65612
|
65613
|
65618
|
65619
|
66775
|
66776