Browse Source

Bug 19057: Move C4::Reserve::GetReserve to Koha::Holds

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>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
17.11.x
Jonathan Druart 6 years ago
parent
commit
a6de2b66d1
  1. 6
      C4/ILSDI/Services.pm
  2. 86
      C4/Reserves.pm
  3. 18
      Koha/REST/V1/Hold.pm
  4. 5
      t/db_dependent/Circulation/issue.t
  5. 68
      t/db_dependent/Holds.t
  6. 13
      t/db_dependent/Items/MoveItemFromBiblio.t
  7. 10
      t/db_dependent/Reserves.t

6
C4/ILSDI/Services.pm

@ -771,9 +771,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});

86
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" ) {

18
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 });

5
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;
@ -372,8 +373,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;

68
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 );

13
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;

10
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;
@ -320,16 +320,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");

Loading…
Cancel
Save