Bug 20912: (follow-up) Improve test coverage

Increase test coverage for CanBookBeIssued and fix a introduced during
the refactoring to Koha::Fees.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

C4/Circulation.pm

@@ -1004,7 +1004,7 @@ sub CanBookBeIssued {
     if ( $rentalConfirmation ){
         my ($rentalCharge) = GetIssuingCharges( $item->itemnumber, $patron->borrowernumber );
         my $itemtype = Koha::ItemTypes->find( $item->itype ); # GetItem sets effective itemtype
-        $rentalCharge += $itemtype->calc_rental_charge_daily( { from => dt_from_string(), to => $duedate } );
+        $rentalCharge += $fees->accumulate_rentalcharge({ from => dt_from_string(), to => $duedate });
         if ( $rentalCharge > 0 ){
             $needsconfirmation{RENTALCHARGE} = $rentalCharge;
         }
@@ -1464,7 +1464,7 @@ sub AddIssue {
             );
             ModDateLastSeen( $item->itemnumber );
 
-           # If it costs to borrow this book, charge it to the patron's account.
+            # If it costs to borrow this book, charge it to the patron's account.
             my ( $charge, $itemtype ) = GetIssuingCharges( $item->itemnumber, $borrower->{'borrowernumber'} );
             if ( $charge > 0 ) {
                 my $description = "Rental";
@@ -1473,10 +1473,10 @@ sub AddIssue {
 
             my $itemtype = Koha::ItemTypes->find( $item_object->effective_itemtype );
             if ( $itemtype ) {
-                my $daily_charge = $fees->rental_charge_daily();
-                if ( $daily_charge > 0 ) {
-                    AddIssuingCharge( $issue, $daily_charge, 'Daily rental' ) if $daily_charge > 0;
-                    $charge += $daily_charge;
+                my $accumulate_charge = $fees->accumulate_rentalcharge();
+                if ( $accumulate_charge > 0 ) {
+                    AddIssuingCharge( $issue, $accumulate_charge, 'Daily rental' ) if $accumulate_charge > 0;
+                    $charge += $accumulate_charge;
                     $item->{charge} = $charge;
                 }
             }
@@ -2923,15 +2923,15 @@ sub AddRenewal {
         AddIssuingCharge($issue, $charge, $description);
     }
 
-    # Charge a new daily rental fee, if applicable
+    # Charge a new accumulate rental fee, if applicable
     my $itemtype = Koha::ItemTypes->find( $item_object->effective_itemtype );
     if ( $itemtype ) {
-        my $daily_charge = $fees->rental_charge_daily();
-        if ( $daily_charge > 0 ) {
+        my $accumulate_charge = $fees->accumulate_rentalcharge();
+        if ( $accumulate_charge > 0 ) {
             my $type_desc = "Renewal of Daily Rental Item " . $biblio->title . " $item->{'barcode'}";
-            AddIssuingCharge( $issue, $daily_charge, $type_desc )
+            AddIssuingCharge( $issue, $accumulate_charge, $type_desc )
         }
-        $charge += $daily_charge;
+        $charge += $accumulate_charge;
     }
 
     # Send a renewal slip according to checkout alert preferencei

Koha/Charges/Fees.pm

@@ -75,22 +75,22 @@ sub new {
     return bless( $params, $class );
 }
 
-=head3 rental_charge_daily
+=head3 accumulate_rentalcharge
 
-    my $fee = $self->rental_charge_daily();
+    my $fee = $self->accumulate_rentalcharge();
 
     This method calculates the daily rental fee for a given itemtype for a given
     period of time passed in as a pair of DateTime objects.
 
 =cut
 
-sub rental_charge_daily {
+sub accumulate_rentalcharge {
     my ( $self, $params ) = @_;
 
     my $itemtype = Koha::ItemTypes->find( $self->item->effective_itemtype );
-    my $rental_charge_daily = $itemtype->rental_charge_daily;
+    my $rentalcharge_daily = $itemtype->rentalcharge_daily;
 
-    return undef unless $rental_charge_daily && $rental_charge_daily > 0;
+    return undef unless $rentalcharge_daily && $rentalcharge_daily > 0;
 
     my $duration;
     if ( C4::Context->preference('finesCalendar') eq 'noFinesWhenClosed' ) {
@@ -102,7 +102,7 @@ sub rental_charge_daily {
     }
     my $days = $duration->in_units('days');
 
-    my $charge = $rental_charge_daily * $days;
+    my $charge = $rentalcharge_daily * $days;
 
     return $charge;
 }

admin/itemtypes.pl

@@ -72,7 +72,7 @@ if ( $op eq 'add_form' ) {
     my $itemtype     = Koha::ItemTypes->find($itemtype_code);
     my $description  = $input->param('description');
     my $rentalcharge = $input->param('rentalcharge');
-    my $rental_charge_daily = $input->param('rental_charge_daily');
+    my $rentalcharge_daily = $input->param('rentalcharge_daily');
     my $defaultreplacecost = $input->param('defaultreplacecost');
     my $processfee = $input->param('processfee');
     my $image = $input->param('image') || q||;
@@ -93,7 +93,7 @@ if ( $op eq 'add_form' ) {
     if ( $itemtype and $is_a_modif ) {    # it's a modification
         $itemtype->description($description);
         $itemtype->rentalcharge($rentalcharge);
-        $itemtype->rental_charge_daily($rental_charge_daily);
+        $itemtype->rentalcharge_daily($rentalcharge_daily);
         $itemtype->defaultreplacecost($defaultreplacecost);
         $itemtype->processfee($processfee);
         $itemtype->notforloan($notforloan);
@@ -118,7 +118,7 @@ if ( $op eq 'add_form' ) {
                 itemtype            => $itemtype_code,
                 description         => $description,
                 rentalcharge        => $rentalcharge,
-                rental_charge_daily => $rental_charge_daily,
+                rentalcharge_daily => $rentalcharge_daily,
                 defaultreplacecost  => $defaultreplacecost,
                 processfee          => $processfee,
                 notforloan          => $notforloan,

installer/data/mysql/kohastructure.sql

@@ -951,7 +951,7 @@ CREATE TABLE `itemtypes` ( -- defines the item types
   itemtype varchar(10) NOT NULL default '', -- unique key, a code associated with the item type
   description LONGTEXT, -- a plain text explanation of the item type
   rentalcharge decimal(28,6) default NULL, -- the amount charged when this item is checked out/issued
-  rental_charge_daily decimal(28,6) default NULL, -- the amount charged for each day between checkout date and due date
+  rentalcharge_daily decimal(28,6) default NULL, -- the amount charged for each increment (day/hour) between checkout date and due date
   defaultreplacecost decimal(28,6) default NULL, -- default replacement cost
   processfee decimal(28,6) default NULL, -- default text be recorded in the column note when the processing fee is applied
   notforloan smallint(6) default NULL, -- 1 if the item is not for loan, 0 if the item is available for loan

koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt

@@ -234,8 +234,8 @@ Item types administration
                     <span class="hint">This fee is charged once per checkout/renewal per item</span>
                 </li>
                 <li>
-                    <label for="rental_charge_daily">Daily rental charge: </label>
-                    <input type="text" id="rental_charge_daily" name="rental_charge_daily" size="10" value="[% itemtype.rental_charge_daily | $Price %]" />
+                    <label for="rentalcharge_daily">Daily rental charge: </label>
+                    <input type="text" id="rentalcharge_daily" name="rentalcharge_daily" size="10" value="[% itemtype.rentalcharge_daily | $Price %]" />
                     <span class="hint">This fee is charged a checkout/renewal time for each day between the checkout/renewal date and due date.</span>
                 </li>
                 <li>
@@ -380,7 +380,7 @@ Item types administration
             </td>
             <td>
             [% UNLESS ( itemtype.notforloan ) %]
-              [% itemtype.rental_charge_daily | $Price %]
+              [% itemtype.rentalcharge_daily | $Price %]
             [% END %]
             </td>
             <td>[% itemtype.defaultreplacecost | $Price %]</td>

koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt

@@ -35,7 +35,7 @@
         <li><span class="label">Item type:</span> [% itemtypename | html %]&nbsp;</li>
         [% END %]
         [% IF ( rentalcharge ) %]<li><span class="label">Rental charge:</span>[% rentalcharge | $Price %]&nbsp;</li>[% END %]
-        [% IF ( rental_charge_daily ) %]<li><span class="label">Daily rental charge:</span>[% rental_charge_daily | $Price %]&nbsp;</li>[% END %]
+        [% IF ( rentalcharge_daily ) %]<li><span class="label">Daily rental charge:</span>[% rentalcharge_daily | $Price %]&nbsp;</li>[% END %]
         <li><span class="label">ISBN:</span> [% isbn | html %]&nbsp;</li>
         <li><span class="label">Publisher:</span>[% place | html %] [% publishercode | html %] [% publicationyear | html %]&nbsp;</li>
         [% IF ( volumeddesc ) %]<li><span class="label">Volume:</span> [% volumeddesc | html %]</li>[% END %]

t/db_dependent/Circulation.t

@@ -18,7 +18,7 @@
 use Modern::Perl;
 use utf8;
 
-use Test::More tests => 125;
+use Test::More tests => 126;
 use Test::MockModule;
 
 use Data::Dumper;
@@ -75,7 +75,7 @@ my $itemtype = $builder->build(
         value  => {
             notforloan          => undef,
             rentalcharge        => 0,
-            rental_charge_daily => 0,
+            rentalcharge_daily => 0,
             defaultreplacecost  => undef,
             processfee          => undef
         }
@@ -1962,7 +1962,7 @@ subtest '_FixAccountForLostAndReturned' => sub {
                 rentalcharge       => 0,
                 defaultreplacecost => undef,
                 processfee         => $processfee_amount,
-                rental_charge_daily => 0,
+                rentalcharge_daily => 0,
             }
         }
     );
@@ -2260,7 +2260,7 @@ subtest '_FixAccountForLostAndReturned' => sub {
                     rentalcharge       => 0,
                     defaultreplacecost => undef,
                     processfee         => 0,
-                    rental_charge_daily => 0,
+                    rentalcharge_daily => 0,
                 }
             }
         );
@@ -2864,7 +2864,7 @@ subtest 'AddRenewal and AddIssuingCharge tests' => sub {
     );
 
     my $library  = $builder->build_object({ class => 'Koha::Libraries' });
-    my $itemtype = $builder->build_object({ class => 'Koha::ItemTypes', value => { rental_charge_daily => 0.00 }});
+    my $itemtype = $builder->build_object({ class => 'Koha::ItemTypes', value => { rentalcharge_daily => 0.00 }});
     my $patron   = $builder->build_object({
         class => 'Koha::Patrons',
         value => { branchcode => $library->id }
@@ -3029,7 +3029,7 @@ subtest 'Incremented fee tests' => sub {
             value  => {
                 notforloan          => undef,
                 rentalcharge        => 0,
-                rental_charge_daily => 1.000000
+                rentalcharge_daily => 1.000000
             }
         }
     )->store;
@@ -3051,7 +3051,7 @@ subtest 'Incremented fee tests' => sub {
         }
     )->store;
 
-    is( $itemtype->rental_charge_daily, '1.000000', 'Daily rental charge stored and retreived correctly' );
+    is( $itemtype->rentalcharge_daily, '1.000000', 'Daily rental charge stored and retreived correctly' );
     is( $item->effective_itemtype, $itemtype->id, "Itemtype set correctly for item");
 
     my $dt_from = dt_from_string();
@@ -3108,3 +3108,60 @@ subtest 'Incremented fee tests' => sub {
     $accountlines->delete();
     $issue->delete();
 };
+
+subtest 'CanBookBeIssued & RentalFeesCheckoutConfirmation' => sub {
+    plan tests => 2;
+
+    t::lib::Mocks::mock_preference('RentalFeesCheckoutConfirmation', 1);
+    t::lib::Mocks::mock_preference('item-level_itypes', 1);
+
+    my $library =
+      $builder->build_object( { class => 'Koha::Libraries' } )->store;
+    my $patron = $builder->build_object(
+        {
+            class => 'Koha::Patrons',
+            value => { categorycode => $patron_category->{categorycode} }
+        }
+    )->store;
+
+    my $itemtype = $builder->build_object(
+        {
+            class => 'Koha::ItemTypes',
+            value => {
+                notforloan             => 0,
+                rentalcharge           => 0,
+                rentalcharge_daily => 0
+            }
+        }
+    );
+
+    my $biblioitem = $builder->build( { source => 'Biblioitem' } );
+    my $item = $builder->build_object(
+        {
+            class => 'Koha::Items',
+            value  => {
+                homebranch    => $library->id,
+                holdingbranch => $library->id,
+                notforloan    => 0,
+                itemlost      => 0,
+                withdrawn     => 0,
+                itype         => $itemtype->id,
+                biblionumber  => $biblioitem->{biblionumber},
+                biblioitemnumber => $biblioitem->{biblioitemnumber},
+            }
+        }
+    )->store;
+
+    my ( $issuingimpossible, $needsconfirmation );
+    my $dt_from = dt_from_string();
+    my $dt_due = dt_from_string()->add( days => 3 );
+
+    $itemtype->rentalcharge('1.000000')->store;
+    ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, $dt_due, undef, undef, undef );
+    is_deeply( $needsconfirmation, { RENTALCHARGE => '1' }, 'Item needs rentalcharge confirmation to be issued' );
+    $itemtype->rentalcharge('0')->store;
+    $itemtype->rentalcharge_daily('1.000000')->store;
+    ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $patron, $item->barcode, $dt_due, undef, undef, undef );
+    is_deeply( $needsconfirmation, { RENTALCHARGE => '3' }, 'Item needs rentalcharge confirmation to be issued, increment' );
+    $itemtype->rentalcharge_daily('0')->store;
+};

t/db_dependent/Koha/Charges/Fees.t

@@ -58,7 +58,7 @@ my $itemtype = $builder->build_object(
     {
         class => 'Koha::ItemTypes',
         value => {
-            rental_charge_daily => '0.00',
+            rentalcharge_daily => '0.00',
             rentalcharge        => '0.00',
             processfee          => '0.00',
             defaultreplacecost  => '0.00',
@@ -99,20 +99,20 @@ my $fees = Koha::Charges::Fees->new(
     }
 );
 
-subtest 'Koha::ItemType::rental_charge_daily tests' => sub {
+subtest 'accumulate_rentalcharge tests' => sub {
     plan tests => 4;
 
-    $itemtype->rental_charge_daily(1.00);
+    $itemtype->rentalcharge_daily(1.00);
     $itemtype->store();
-    is( $itemtype->rental_charge_daily,
+    is( $itemtype->rentalcharge_daily,
         1.00, 'Daily return charge stored correctly' );
 
     t::lib::Mocks::mock_preference( 'finesCalendar', 'ignoreCalendar' );
-    my $charge = $fees->rental_charge_daily();
+    my $charge = $fees->accumulate_rentalcharge();
     is( $charge, 6.00, 'Daily rental charge calculated correctly with finesCalendar = ignoreCalendar' );
 
     t::lib::Mocks::mock_preference( 'finesCalendar', 'noFinesWhenClosed' );
-    $charge = $fees->rental_charge_daily();
+    $charge = $fees->accumulate_rentalcharge();
     is( $charge, 6.00, 'Daily rental charge calculated correctly with finesCalendar = noFinesWhenClosed' );
 
     my $calendar = C4::Calendar->new( branchcode => $library->id );
@@ -121,6 +121,6 @@ subtest 'Koha::ItemType::rental_charge_daily tests' => sub {
         title       => 'Test holiday',
         description => 'Test holiday'
     );
-    $charge = $fees->rental_charge_daily();
+    $charge = $fees->accumulate_rentalcharge();
     is( $charge, 5.00, 'Daily rental charge calculated correctly with finesCalendar = noFinesWhenClosed and closed Wednesdays' );
 };