Attachment 143598 Details for Bug 32142 – Bug 32142: Implement issued_or_reserved HoldFeeMode option

[patch] Bug 32142: Implement issued_or_reserved HoldFeeMode option

Bug-32142-Implement-issuedorreserved-HoldFeeMode-o.patch (text/plain), 10.88 KB, created by Aleisha Amohia on 2022-11-09 22:37:52 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Aleisha Amohia

Created: 2022-11-09 22:37:52 UTC

Size: 10.88 KB

patch

obsolete

>From 90a43d30b8e49d629607d89fe8ce330f7c39bc87 Mon Sep 17 00:00:00 2001
>From: Aleisha Amohia <aleishaamohia@hotmail.com>
>Date: Wed, 9 Nov 2022 01:30:55 +0000
>Subject: [PATCH] Bug 32142: Implement issued_or_reserved HoldFeeMode option
>
>This enhancement adds an issued_or_reserved option to the HoldFeeMode
>system preference, so that a hold fee may be charged if either all items
>are check out, or the record has at least one hold already.
>
>To test:
>1) Apply patch, update database, restart services
>2) Go to Administration -> Patron categories
>3) Edit Category A and add a hold fee of $1.00
>
>4) Go to Administration -> System preferences and search for
>HoldFeeMode
>5) Set HoldFeeMode to not_always - "only if all items are checked out
>and the record has at least one hold already"
>
>6) In another tab, open the staff interface. Search for a record with
>one item attached to it (Biblio A)
>7) Place a hold on this record for Patron B (any category).
>
>not_always, reserved but not issued
>
>8) Place a hold on Biblio A for Patron A. (Make sure Patron A is of
>Category A which the hold fee applies to)
>9) Check Patron A's accounting page. They should NOT have a hold fee
>
>issued_or_reserved, reserved but not issued
>
>10) In your System preferences tab, change HoldFeeMode to
>issued_or_reserved - "either if all items are checked out, or the record
>has at least one hold already"
>11) Delete Patron A's hold on Biblio A
>12) Place a hold on Biblio A for Patron A
>13) Check Patron A's accounting page. They SHOULD have a hold fee
>
>not_always, issued but not reserved
>
>14) In your System preferences tab, change HoldFeeMode to
>not_always - "only if all items are checked out and the record
>has at least one hold already"
>15) Delete all holds on Biblio A
>16) Check out Biblio A's item (Item A) to Patron B
>17) Place a hold on Biblio A for Patron A
>18) Check Patron A's accounting page. They should NOT have a hold fee
>
>issued_or_reserved, issued but not reserved
>
>19) In your System preferences tab, change HoldFeeMode to
>issued_or_reserved - "either if all items are checked out, or the record
>has at least one hold already"
>20) Delete Patron A's hold on Biblio A
>21) Place a hold on Biblio A for Patron A
>22) Check Patron A's accounting page. They SHOULD have a hold fee
>
>not_always, issued and reserved
>
>23) In your System preferences tab, change HoldFeeMode to
>not_always - "only if all items are checked out and the record
>has at least one hold already"
>24) Delete all holds on Biblio A
>25) Place a hold on Biblio A for Patron C (any category).
>26) Place a hold on Biblio A for Patron A
>27) Check Patron A's accounting page. They SHOULD have a hold fee
>
>issued_or_reserved, issued and reserved
>
>28) In your System preferences tab, change HoldFeeMode to
>issued_or_reserved - "either if all items are checked out, or the record
>has at least one hold already"
>29) Delete Patron A's hold on Biblio A. There should still be a hold for
>Patron C.
>30) Place a hold on Biblio A for Patron A
>31) Check Patron A's accounting page. They SHOULD have a hold fee
>
>32) Ensure tests pass t/db_dependent/Reserves/GetReserveFee.t
>
>Sponsored-by: Horowhenua Libraries Trust
>---
> C4/Reserves.pm                                | 12 ++++---
> .../admin/preferences/circulation.pref        |  1 +
> t/db_dependent/Reserves/GetReserveFee.t       | 36 ++++++++++++++++---
> 3 files changed, 41 insertions(+), 8 deletions(-)
>
>diff --git a/C4/Reserves.pm b/C4/Reserves.pm
>index 6ff7149edcf..eec608d607d 100644
>--- a/C4/Reserves.pm
>+++ b/C4/Reserves.pm
>@@ -764,19 +764,23 @@ SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>?
>     my $dbh = C4::Context->dbh;
>     my ( $fee ) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) );
>     my $hold_fee_mode = C4::Context->preference('HoldFeeMode') || 'not_always';
>-    if( $fee and $fee > 0 and $hold_fee_mode eq 'not_always' ) {
>+    if( $fee and $fee > 0 and ( $hold_fee_mode eq 'not_always' or $hold_fee_mode eq 'issued_or_reserved' ) ) {
>         # This is a reconstruction of the old code:
>         # Compare number of items with items issued, and optionally check holds
>-        # If not all items are issued and there are no holds: charge no fee
>+        # not_always = If not all items are issued and there are no holds: charge no fee
>+        # issued_or_reserved = If all items are issued, OR there are holds: charge fee
>         # NOTE: Lost, damaged, not-for-loan, etc. are just ignored here
>         my ( $notissued, $reserved );
>         ( $notissued ) = $dbh->selectrow_array( $issue_qry, undef,
>             ( $biblionumber ) );
>-        if( $notissued == 0 ) {
>+        if( $notissued == 0 or $hold_fee_mode eq 'issued_or_reserved' ) {
>             # all items are issued
>             ( $reserved ) = $dbh->selectrow_array( $holds_qry, undef,
>                 ( $biblionumber, $borrowernumber ) );
>-            $fee = 0 if $reserved == 0;
>+            if ( $reserved == 0 and $hold_fee_mode eq 'not_always' ) {
>+                # no reserves are placed
>+                $fee = 0;
>+            }
>         } else {
>             $fee = 0;
>         }
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>index e025fc6e9d0..7e6ad30f799 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
>@@ -1124,6 +1124,7 @@ Circulation:
>               choices:
>                   any_time_is_placed: "any time a hold is placed."
>                   not_always: "only if all items are checked out and the record has at least one hold already."
>+                  issued_or_reserved: "either if all items are checked out, or the record has at least one hold already."
>                   any_time_is_collected: "any time a hold is collected."
>         -
>             - pref: useDefaultReplacementCost
>diff --git a/t/db_dependent/Reserves/GetReserveFee.t b/t/db_dependent/Reserves/GetReserveFee.t
>index 958e6c35271..1354a31d91f 100755
>--- a/t/db_dependent/Reserves/GetReserveFee.t
>+++ b/t/db_dependent/Reserves/GetReserveFee.t
>@@ -84,15 +84,22 @@ my $item2 = $builder->build_sample_item(
> );
> 
> subtest 'GetReserveFee' => sub {
>-    plan tests => 5;
>+    plan tests => 7;
> 
>     C4::Circulation::AddIssue( $patron1, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); # the date does not really matter
>-    C4::Circulation::AddIssue( $patron3, $item2->barcode, '2015-12-31', 0, undef, 0, {} ); # the date does not really matter
>     my $acc2 = acctlines( $patron2->{borrowernumber} );
>     my $res1 = addreserve( $patron1->{borrowernumber} );
> 
>-    t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always');
>+    t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved');
>     my $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->biblionumber );
>+    is( int($fee), 2, 'HoldFeeMode=issued_or_reserved, Patron 2 should be charged' );
>+
>+    t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always');
>+    $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->biblionumber );
>+    is( $fee, 0, 'HoldFeeMode=issued_or_reserved, Patron 2 should not be charged' );
>+
>+    C4::Circulation::AddIssue( $patron3, $item2->barcode, '2015-12-31', 0, undef, 0, {} ); # the date does not really matter
>+    $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->biblionumber );
>     is( $fee > 0, 1, 'Patron 2 should be charged cf GetReserveFee' );
>     C4::Reserves::ChargeReserveFee( $patron2->{borrowernumber}, $fee, $biblio->title );
>     is( acctlines( $patron2->{borrowernumber} ), $acc2 + 1, 'Patron 2 has been charged by ChargeReserveFee' );
>@@ -139,7 +146,7 @@ subtest 'Integration with AddReserve' => sub {
>     };
> 
>     subtest 'Items are issued' => sub {
>-        plan tests => 4;
>+        plan tests => 7;
> 
>         $dbh->do( "DELETE FROM issues       WHERE itemnumber=?", undef, $item1->itemnumber);
>         $dbh->do( "DELETE FROM issues       WHERE itemnumber=?", undef, $item2->itemnumber);
>@@ -151,6 +158,13 @@ subtest 'Integration with AddReserve' => sub {
>         addreserve( $patron1->{borrowernumber} );
>         is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not be charged if items are not all checked out' );
> 
>+        t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved');
>+        $dbh->do( "DELETE FROM reserves     WHERE biblionumber=?", undef, $biblio->biblionumber );
>+        $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} );
>+        addreserve( $patron1->{borrowernumber} );
>+        is( acctlines( $patron1->{borrowernumber} ), 1, 'issued_or_reserved - Patron should be charged if all the items are not checked out because 1 hold is placed' );
>+
>+        t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always');
>         $dbh->do( "DELETE FROM reserves     WHERE biblionumber=?", undef, $biblio->biblionumber );
>         $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} );
>         addreserve( $patron3->{borrowernumber} );
>@@ -163,11 +177,25 @@ subtest 'Integration with AddReserve' => sub {
>         addreserve( $patron1->{borrowernumber} );
>         is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not be charged if all items are checked out but no holds are placed' );
> 
>+        t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved');
>+        $dbh->do( "DELETE FROM reserves     WHERE biblionumber=?", undef, $biblio->biblionumber );
>+        $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} );
>+        addreserve( $patron1->{borrowernumber} );
>+        is( acctlines( $patron1->{borrowernumber} ), 1, 'issued_or_reserved - Patron should be charged if all items are checked out but no holds are placed' );
>+
>+        t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always');
>         $dbh->do( "DELETE FROM reserves     WHERE biblionumber=?", undef, $biblio->biblionumber );
>         $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} );
>         addreserve( $patron3->{borrowernumber} );
>         addreserve( $patron1->{borrowernumber} );
>         is( acctlines( $patron1->{borrowernumber} ), 1, 'not_always - Patron should only be charged if all items are checked out and at least 1 hold is already placed' );
>+
>+        t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved');
>+        $dbh->do( "DELETE FROM reserves     WHERE biblionumber=?", undef, $biblio->biblionumber );
>+        $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} );
>+        addreserve( $patron3->{borrowernumber} );
>+        addreserve( $patron1->{borrowernumber} );
>+        is( acctlines( $patron1->{borrowernumber} ), 1, 'issued_or_reserved - Patron should be charged if all items are checked out and at least 1 hold is already placed' );
>     };
> };
> 
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 32142: 143452 | 143453 | 143598 | 143817 | 143818 | 153251 | 153286 | 153287 | 159070 | 159071 | 162626 | 162627 | 172117