@@ -, +, @@ --- Koha/Hold.pm | 170 +++++++++++++++--------------------- Koha/Holds.pm | 22 +++-- t/db_dependent/Koha/Hold.t | 34 ++++---- t/db_dependent/Koha/Holds.t | 17 ++-- 4 files changed, 112 insertions(+), 131 deletions(-) --- a/Koha/Hold.pm +++ a/Koha/Hold.pm @@ -67,11 +67,11 @@ sub age { my $age; if ( $use_calendar ) { - my $calendar = Koha::Calendar->new( branchcode => $self->branchcode ); - $age = $calendar->days_between( dt_from_string( $self->reservedate ), $today ); + my $calendar = Koha::Calendar->new( branchcode => $self->pickup_library_id ); + $age = $calendar->days_between( dt_from_string( $self->created_date ), $today ); } else { - $age = $today->delta_days( dt_from_string( $self->reservedate ) ); + $age = $today->delta_days( dt_from_string( $self->created_date ) ); } $age = $age->in_units( 'days' ); @@ -91,11 +91,9 @@ sub suspend_hold { my $date = $dt ? $dt->clone()->truncate( to => 'day' )->datetime : undef; if ( $self->is_found ) { # We can't suspend found holds - if ( $self->is_waiting ) { - Koha::Exceptions::Hold::CannotSuspendFound->throw( status => 'W' ); - } - elsif ( $self->is_in_transit ) { - Koha::Exceptions::Hold::CannotSuspendFound->throw( status => 'T' ); + if ( $self->is_waiting or + $self->is_in_transit ) { + Koha::Exceptions::Hold::CannotSuspendFound->throw( status => $self->status ); } elsif ( $self->is_in_processing ) { Koha::Exceptions::Hold::CannotSuspendFound->throw( status => 'P' ); @@ -104,17 +102,17 @@ sub suspend_hold { Koha::Exceptions::Hold::CannotSuspendFound->throw( 'Unhandled data exception on found hold (id=' . $self->id - . ', found=' - . $self->found + . ', status=' + . $self->status . ')' ); } } - $self->suspend(1); - $self->suspend_until($date); + $self->suspended(1); + $self->suspended_until_date($date); $self->store(); - logaction( 'HOLDS', 'SUSPEND', $self->reserve_id, Dumper( $self->unblessed ) ) + logaction( 'HOLDS', 'SUSPEND', $self->id, Dumper( $self->unblessed ) ) if C4::Context->preference('HoldsLog'); return $self; @@ -129,12 +127,12 @@ my $hold = $hold->resume(); sub resume { my ( $self ) = @_; - $self->suspend(0); - $self->suspend_until( undef ); + $self->suspended(0); + $self->suspended_until_date( undef ); $self->store(); - logaction( 'HOLDS', 'RESUME', $self->reserve_id, Dumper($self->unblessed) ) + logaction( 'HOLDS', 'RESUME', $self->id, Dumper($self->unblessed) ) if C4::Context->preference('HoldsLog'); return $self; @@ -151,7 +149,7 @@ sub delete { my $deleted = $self->SUPER::delete($self); - logaction( 'HOLDS', 'DELETE', $self->reserve_id, Dumper($self->unblessed) ) + logaction( 'HOLDS', 'DELETE', $self->id, Dumper($self->unblessed) ) if C4::Context->preference('HoldsLog'); return $deleted; @@ -165,7 +163,7 @@ sub set_transfer { my ( $self ) = @_; $self->priority(0); - $self->found('T'); + $self->status('in_transit'); $self->store(); return $self; @@ -182,14 +180,14 @@ sub set_waiting { my $today = dt_from_string(); my $values = { - found => 'W', - waitingdate => $today->ymd, - desk_id => $desk_id, + status => 'waiting', + waiting_date => $today->ymd, + desk_id => $desk_id, }; my $requested_expiration; - if ($self->expirationdate) { - $requested_expiration = dt_from_string($self->expirationdate); + if ($self->expiration_date) { + $requested_expiration = dt_from_string($self->expiration_date); } my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); @@ -202,12 +200,12 @@ sub set_waiting { my $itemtype = $self->item ? $self->item->effective_itemtype : $self->biblio->itemtype; my $daysmode = Koha::CirculationRules->get_effective_daysmode( { - categorycode => $self->borrower->categorycode, + categorycode => $self->patron->categorycode, itemtype => $itemtype, - branchcode => $self->branchcode, + branchcode => $self->pickup_library_id, } ); - my $calendar = Koha::Calendar->new( branchcode => $self->branchcode, days_mode => $daysmode ); + my $calendar = Koha::Calendar->new( branchcode => $self->pickup_library_id, days_mode => $daysmode ); $expirationdate = $calendar->days_forward( dt_from_string(), $max_pickup_delay ); } @@ -215,7 +213,7 @@ sub set_waiting { # If patron's requested expiration date is prior to the # calculated one, we keep the patron's one. my $cmp = $requested_expiration ? DateTime->compare($requested_expiration, $expirationdate) : 0; - $values->{expirationdate} = $cmp == -1 ? $requested_expiration->ymd : $expirationdate->ymd; + $values->{expiration_date} = $cmp == -1 ? $requested_expiration->ymd : $expirationdate->ymd; $self->set($values)->store(); @@ -242,7 +240,7 @@ sub is_pickup_location_valid { my @pickup_locations; - if ( $self->itemnumber ) { # item-level + if ( $self->item_id ) { # item-level @pickup_locations = $self->item->pickup_locations({ patron => $self->patron }); } else { # biblio-level @@ -269,7 +267,7 @@ sub set_pickup_location { if ( $self->is_pickup_location_valid({ library_id => $params->{library_id} }) ) { # all good, set the new pickup location - $self->branchcode( $params->{library_id} )->store; + $self->pickup_library_id( $params->{library_id} )->store; } else { Koha::Exceptions::Hold::InvalidPickupLocation->throw; @@ -290,7 +288,7 @@ sub set_processing { my ( $self ) = @_; $self->priority(0); - $self->found('P'); + $self->status('processing'); $self->store(); return $self; @@ -305,10 +303,16 @@ Returns true if hold is waiting, in transit or in processing sub is_found { my ($self) = @_; - return 0 unless $self->found(); - return 1 if $self->found() eq 'W'; - return 1 if $self->found() eq 'T'; - return 1 if $self->found() eq 'P'; + if ( + $self->status eq 'waiting' + or $self->status eq 'in_transit' + or $self->status eq 'processing' + ) + { + return 1; + } + + return 0; } =head3 is_waiting @@ -320,8 +324,7 @@ Returns true if hold is a waiting hold sub is_waiting { my ($self) = @_; - my $found = $self->found; - return $found && $found eq 'W'; + return $self->status eq 'waiting'; } =head3 is_in_transit @@ -333,8 +336,7 @@ Returns true if hold is a in_transit hold sub is_in_transit { my ($self) = @_; - return 0 unless $self->found(); - return $self->found() eq 'T'; + return $self->status eq 'in_transit'; } =head3 is_in_processing @@ -346,8 +348,7 @@ Returns true if hold is a in_processing hold sub is_in_processing { my ($self) = @_; - return 0 unless $self->found(); - return $self->found() eq 'P'; + return $self->status eq 'processing'; } =head3 is_cancelable_from_opac @@ -378,7 +379,7 @@ the hold item's holding branch sub is_at_destination { my ($self) = @_; - return $self->is_waiting() && ( $self->branchcode() eq $self->item()->holdingbranch() ); + return $self->is_waiting() && ( $self->pickup_library_id() eq $self->item()->holdingbranch() ); } =head3 biblio @@ -390,7 +391,7 @@ Returns the related Koha::Biblio object for this hold sub biblio { my ($self) = @_; - $self->{_biblio} ||= Koha::Biblios->find( $self->biblionumber() ); + $self->{_biblio} ||= Koha::Biblios->find( $self->biblio_id ); return $self->{_biblio}; } @@ -417,7 +418,7 @@ Returns the related Koha::Item object for this Hold sub item { my ($self) = @_; - $self->{_item} ||= Koha::Items->find( $self->itemnumber() ); + $self->{_item} ||= Koha::Items->find( $self->item_id ); return $self->{_item}; } @@ -431,7 +432,7 @@ Returns the related Koha::Library object for this Hold sub branch { my ($self) = @_; - $self->{_branch} ||= Koha::Libraries->find( $self->branchcode() ); + $self->{_branch} ||= Koha::Libraries->find( $self->pickup_library_id ); return $self->{_branch}; } @@ -459,7 +460,7 @@ Returns the related Koha::Patron object for this Hold sub borrower { my ($self) = @_; - $self->{_borrower} ||= Koha::Patrons->find( $self->borrowernumber() ); + $self->{_borrower} ||= Koha::Patrons->find( $self->patron_id ); return $self->{_borrower}; } @@ -473,10 +474,9 @@ my $bool = $hold->is_suspended(); sub is_suspended { my ( $self ) = @_; - return $self->suspend(); + return $self->suspended(); } - =head3 cancel my $cancel_hold = $hold->cancel( @@ -487,10 +487,10 @@ my $cancel_hold = $hold->cancel( ); Cancel a hold: -- The hold will be moved to the old_reserves table with a priority=0 +- The hold will be marked as completed, status 'cancelled' and a priority=0 - The priority of other holds will be updated - The patron will be charge (see ExpireReservesMaxPickUpDelayCharge) if the charge_cancel_fee parameter is set -- The canceled hold will have the cancellation reason added to old_reserves.cancellation_reason if one is passed in +- The canceled hold will have the cancellation reason added to cancellation_reason if one is passed in - a CANCEL HOLDS log will be done if the pref HoldsLog is on =cut @@ -499,10 +499,15 @@ sub cancel { my ( $self, $params ) = @_; $self->_result->result_source->schema->txn_do( sub { - $self->cancellationdate( dt_from_string->strftime( '%Y-%m-%d %H:%M:%S' ) ); - $self->priority(0); - $self->cancellation_reason( $params->{cancellation_reason} ); - $self->store(); + $self->set( + { + completed_date => dt_from_string->strftime('%Y-%m-%d %H:%M:%S'), + completed => 1, + priority => 0, + status => 'cancelled', + cancellation_reason => $params->{cancellation_reason}, + } + )->store(); if ( $params->{cancellation_reason} ) { my $letter = C4::Letters::GetPreparedLetter( @@ -513,10 +518,10 @@ sub cancel { lang => $self->borrower->lang, tables => { branches => $self->borrower->branchcode, - borrowers => $self->borrowernumber, - items => $self->itemnumber, - biblio => $self->biblionumber, - biblioitems => $self->biblionumber, + borrowers => $self->patron_id, + items => $self->item_id, + biblio => $self->biblio_id, + biblioitems => $self->biblio_id, reserves => $self->unblessed, } ); @@ -524,25 +529,22 @@ sub cancel { if ($letter) { C4::Letters::EnqueueLetter( { - letter => $letter, - borrowernumber => $self->borrowernumber, + letter => $letter, + borrowernumber => $self->patron_id, message_transport_type => 'email', } ); } } - $self->_move_to_old; - $self->SUPER::delete(); # Do not add a DELETE log - # now fix the priority on the others.... - C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber }); + C4::Reserves::_FixPriority({ biblionumber => $self->biblio_id }); # and, if desired, charge a cancel fee my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); if ( $charge && $params->{'charge_cancel_fee'} ) { my $account = - Koha::Account->new( { patron_id => $self->borrowernumber } ); + Koha::Account->new( { patron_id => $self->patron_id } ); $account->add_debit( { amount => $charge, @@ -550,32 +552,18 @@ sub cancel { interface => C4::Context->interface, library_id => C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef, type => 'RESERVE_EXPIRED', - item_id => $self->itemnumber + item_id => $self->item_id } ); } - C4::Log::logaction( 'HOLDS', 'CANCEL', $self->reserve_id, Dumper($self->unblessed) ) + C4::Log::logaction( 'HOLDS', 'CANCEL', $self->id, Dumper($self->unblessed) ) if C4::Context->preference('HoldsLog'); } ); return $self; } -=head3 _move_to_old - -my $is_moved = $hold->_move_to_old; - -Move a hold to the old_reserve table following the same pattern as Koha::Patron->move_to_deleted - -=cut - -sub _move_to_old { - my ($self) = @_; - my $hold_infos = $self->unblessed; - return Koha::Old::Hold->new( $hold_infos )->store; -} - =head3 to_api_mapping This method returns the mapping for representing a Koha::Hold object @@ -585,24 +573,7 @@ on the API. sub to_api_mapping { return { - reserve_id => 'hold_id', - borrowernumber => 'patron_id', - reservedate => 'hold_date', - biblionumber => 'biblio_id', - branchcode => 'pickup_library_id', - notificationdate => undef, - reminderdate => undef, - cancellationdate => 'cancellation_date', - reservenotes => 'notes', - found => 'status', - itemnumber => 'item_id', - waitingdate => 'waiting_date', - expirationdate => 'expiration_date', - lowestPriority => 'lowest_priority', - suspend => 'suspended', - suspend_until => 'suspended_until', - itemtype => 'item_type', - item_level_hold => 'item_level', + id => 'hold_id' }; } @@ -613,7 +584,7 @@ sub to_api_mapping { =cut sub _type { - return 'Reserve'; + return 'Hold'; } =head1 AUTHORS @@ -621,6 +592,7 @@ sub _type { Kyle M Hall Jonathan Druart Martin Renvoize +Tomás Cohen Arazu =cut --- a/Koha/Holds.pm +++ a/Koha/Holds.pm @@ -33,7 +33,7 @@ Koha::Holds - Koha Hold object set class =head1 API -=head2 Class Methods +=head2 Class methods =cut @@ -46,7 +46,12 @@ returns a set of holds that are waiting from an existing set sub waiting { my ( $self ) = @_; - return $self->search( { found => 'W' } ); + return $self->search( + { + status => 'waiting', + completed => 0 + } + ); } =head3 unfilled @@ -58,7 +63,12 @@ returns a set of holds that are unfilled from an existing set sub unfilled { my ( $self ) = @_; - return $self->search( { found => undef } ); + return $self->search( + { + status => 'placed', + completed => 0 + } + ); } =head3 forced_hold_level @@ -83,10 +93,10 @@ that would prevent one or the other. sub forced_hold_level { my ($self) = @_; - my $item_level_count = $self->search( { itemnumber => { '!=' => undef } } )->count(); + my $item_level_count = $self->search( { item_id => { '!=' => undef } } )->count(); return 'item' if $item_level_count > 0; - my $record_level_count = $self->search( { itemnumber => undef } )->count(); + my $record_level_count = $self->search( { item_id => undef } )->count(); return 'record' if $record_level_count > 0; return; @@ -158,7 +168,7 @@ sub get_items_that_can_fill { =cut sub _type { - return 'Reserve'; + return 'Hold'; } =head3 object_class --- a/t/db_dependent/Koha/Hold.t +++ a/t/db_dependent/Koha/Hold.t @@ -42,7 +42,7 @@ subtest 'patron() tests' => sub { { class => 'Koha::Holds', value => { - borrowernumber => $patron->borrowernumber + patron_id => $patron->borrowernumber } } ); @@ -84,9 +84,9 @@ subtest 'set_pickup_location() tests' => sub { { class => "Koha::Holds", value => { - biblionumber => $biblio->biblionumber, - branchcode => $library_3->branchcode, - itemnumber => undef, + biblio_id => $biblio->biblionumber, + pickup_library_id => $library_3->branchcode, + item_id => undef, } } ); @@ -97,22 +97,22 @@ subtest 'set_pickup_location() tests' => sub { 'Exception thrown on invalid pickup location'; $biblio_hold->discard_changes; - is( $biblio_hold->branchcode, $library_3->branchcode, 'branchcode remains untouched' ); + is( $biblio_hold->pickup_library_id, $library_3->branchcode, 'branchcode remains untouched' ); my $ret = $biblio_hold->set_pickup_location({ library_id => $library_2->id }); is( ref($ret), 'Koha::Hold', 'self is returned' ); $biblio_hold->discard_changes; - is( $biblio_hold->branchcode, $library_2->id, 'Pickup location changed correctly' ); + is( $biblio_hold->pickup_library_id, $library_2->id, 'Pickup location changed correctly' ); # Test item-level holds my $item_hold = $builder->build_object( { class => "Koha::Holds", value => { - biblionumber => $biblio->biblionumber, - branchcode => $library_3->branchcode, - itemnumber => $item->itemnumber, + biblio_id => $biblio->biblionumber, + pickup_library_id => $library_3->branchcode, + item_id => $item->itemnumber, } } ); @@ -123,13 +123,13 @@ subtest 'set_pickup_location() tests' => sub { 'Exception thrown on invalid pickup location'; $item_hold->discard_changes; - is( $item_hold->branchcode, $library_3->branchcode, 'branchcode remains untouched' ); + is( $item_hold->pickup_library_id, $library_3->branchcode, 'branchcode remains untouched' ); $ret = $item_hold->set_pickup_location({ library_id => $library_2->id }); is( ref($ret), 'Koha::Hold', 'self is returned' ); $item_hold->discard_changes; - is( $item_hold->branchcode, $library_2->id, 'Pickup location changed correctly' ); + is( $item_hold->pickup_library_id, $library_2->id, 'Pickup location changed correctly' ); throws_ok { $item_hold->set_pickup_location({ library_id => undef }); } @@ -171,9 +171,9 @@ subtest 'is_pickup_location_valid() tests' => sub { { class => "Koha::Holds", value => { - biblionumber => $biblio->biblionumber, - branchcode => $library_3->branchcode, - itemnumber => undef, + biblio_id => $biblio->biblionumber, + pickup_library_id => $library_3->branchcode, + item_it => undef, } } ); @@ -186,9 +186,9 @@ subtest 'is_pickup_location_valid() tests' => sub { { class => "Koha::Holds", value => { - biblionumber => $biblio->biblionumber, - branchcode => $library_3->branchcode, - itemnumber => $item->itemnumber, + biblio_id => $biblio->biblionumber, + pickup_library_id => $library_3->branchcode, + item_id => $item->itemnumber, } } ); --- a/t/db_dependent/Koha/Holds.t +++ a/t/db_dependent/Koha/Holds.t @@ -298,9 +298,8 @@ subtest 'cancel with reason' => sub { $hold->cancel({cancellation_reason => 'TEST_REASON'}); $hold = Koha::Holds->find($reserve_id); - is( $hold, undef, 'Hold is not in the reserves table'); - $hold = Koha::Old::Holds->find($reserve_id); - ok( $hold, 'Hold was found in the old reserves table'); + is( ref($hold), 'Koha::Hold', 'Hold still exists'); + ok( $hold->completed, 'Hold marked as complete'); my $message = Koha::Notice::Messages->find({ borrowernumber => $patron->id, letter_code => 'HOLD_CANCELLATION'}); ok( $message, 'Found hold cancellation message'); @@ -313,7 +312,9 @@ subtest 'cancel with reason' => sub { }; subtest 'cancel all with reason' => sub { - plan tests => 7; + + plan tests => 6; + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); my $item = $builder->build_sample_item({ library => $library->branchcode }); my $manager = $builder->build_object( { class => "Koha::Patrons" } ); @@ -355,10 +356,8 @@ subtest 'cancel all with reason' => sub { ModReserveCancelAll($item->id, $patron->id, 'TEST_REASON'); - $hold = Koha::Holds->find($reserve_id); - is( $hold, undef, 'Hold is not in the reserves table'); - $hold = Koha::Old::Holds->find($reserve_id); - ok( $hold, 'Hold was found in the old reserves table'); + $hold->discard_changes; + ok( $hold->completed, 'Hold is marked completed'); my $message = Koha::Notice::Messages->find({ borrowernumber => $patron->id, letter_code => 'HOLD_CANCELLATION'}); ok( $message, 'Found hold cancellation message'); @@ -404,7 +403,7 @@ subtest 'Desks' => sub { ok($reserve_id, "Hold created"); ok($hold, "Hold found"); $hold->set_waiting($desk->desk_id); - is($hold->found, 'W', 'Hold is waiting with correct status set'); + is($hold->status, 'waiting', 'Hold is waiting with correct status set'); is($hold->desk_id, $desk->desk_id, 'Hold is attach to its desk'); }; --