From f3e5dc9fb0e78308a9d99e7e5565eb29ae83f92f Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 23 Dec 2020 07:35:19 -0300 Subject: [PATCH] Bug 25260: Adapt Koha::Hold(s) The new table structure aligns the 'holds' table with the terminology that's been agreed, and also makes some design changes, like having an ENUM status field, that cover all the current options for the hold lifecycle. This requires changing the Koha::Hold(s) classes to this new situation. Signed-off-by: Martin Renvoize --- 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(-) diff --git a/Koha/Hold.pm b/Koha/Hold.pm index bd5458f494..e4e37a3932 100644 --- a/Koha/Hold.pm +++ b/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 @@ -283,7 +281,7 @@ sub set_pickup_location { ) { # 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; @@ -304,7 +302,7 @@ sub set_processing { my ( $self ) = @_; $self->priority(0); - $self->found('P'); + $self->status('processing'); $self->store(); return $self; @@ -319,10 +317,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 @@ -334,8 +338,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 @@ -347,8 +350,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 @@ -360,8 +362,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 @@ -392,7 +393,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 @@ -404,7 +405,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}; } @@ -431,7 +432,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}; } @@ -445,7 +446,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}; } @@ -473,7 +474,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}; } @@ -487,10 +488,9 @@ my $bool = $hold->is_suspended(); sub is_suspended { my ( $self ) = @_; - return $self->suspend(); + return $self->suspended(); } - =head3 cancel my $cancel_hold = $hold->cancel( @@ -501,10 +501,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 @@ -513,10 +513,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( @@ -527,10 +532,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, } ); @@ -538,25 +543,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, @@ -564,12 +566,12 @@ 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'); } ); @@ -628,20 +630,6 @@ sub _set_default_expirationdate { dt_from_string( $self->reservedate )->add( $timeunit => $period ) ); } -=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 @@ -651,24 +639,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' }; } @@ -679,7 +650,7 @@ sub to_api_mapping { =cut sub _type { - return 'Reserve'; + return 'Hold'; } =head1 AUTHORS @@ -687,6 +658,7 @@ sub _type { Kyle M Hall Jonathan Druart Martin Renvoize +Tomás Cohen Arazu =cut diff --git a/Koha/Holds.pm b/Koha/Holds.pm index 296f99cb0a..2dc56460f9 100644 --- a/Koha/Holds.pm +++ b/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 diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index 5f6a9016c8..336905d877 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/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,7 +123,7 @@ 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' ); $item_hold->set_pickup_location({ library_id => $library_1->branchcode, force => 1 }); $item_hold->discard_changes; @@ -133,7 +133,7 @@ subtest 'set_pickup_location() tests' => sub { 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 }); } @@ -175,9 +175,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, } } ); @@ -190,9 +190,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, } } ); diff --git a/t/db_dependent/Koha/Holds.t b/t/db_dependent/Koha/Holds.t index 7d19a132f1..da12024063 100755 --- a/t/db_dependent/Koha/Holds.t +++ b/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'); }; -- 2.32.0