From: Dan Wells <dbw2@calvin.edu>
Date: Wed, 26 Feb 2014 22:44:45 +0000 (-0500)
Subject: LP 1198465: Fix and improve void/adjustment code
X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=8ea8f03896f1d97513393ef6cd2f94f9b359f31b;p=working%2FEvergreen.git

LP 1198465: Fix and improve void/adjustment code

This commit does three things:

- Replace ou_ancestor_setting() with ou_ancestor_setting_value() calls
  This also fixed a bug where we were expecting just the setting, not
  a HASH

- Reword interval checking
  This fix is two part.  First, we simplify the check to not require
  the whole payment map.  Second, we use this newfound simplicity to
  push this check up into the gatekeeper functions, further clarifying
  the code paths.

- make $note into $for_note for void_or_zero_bills_of_type()
  Because the function can both void and adjust, we can't supply a
  complete note, so let's just supply text of what the void/adjustment
  is for.

Signed-off-by: Dan Wells <dbw2@calvin.edu>
---

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/CircCommon.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/CircCommon.pm
index ed10646ac9..c201c43f44 100644
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/CircCommon.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/CircCommon.pm
@@ -65,22 +65,26 @@ sub void_or_zero_overdues {
     if ($billids && @$billids) {
         # overdue settings come from transaction org unit
         my $prohibit_neg_balance_overdues = (
-            $U->ou_ancestor_setting($circ->circ_lib(), 'bill.prohibit_negative_balance_on_overdues')
+            $U->ou_ancestor_setting_value($circ->circ_lib(), 'bill.prohibit_negative_balance_on_overdues')
             ||
-            $U->ou_ancestor_setting($circ->circ_lib(), 'bill.prohibit_negative_balance_default')
+            $U->ou_ancestor_setting_value($circ->circ_lib(), 'bill.prohibit_negative_balance_default')
         );
         my $neg_balance_interval_overdues = (
-            $U->ou_ancestor_setting($circ->circ_lib(), 'bill.negative_balance_interval_on_overdues')
+            $U->ou_ancestor_setting_value($circ->circ_lib(), 'bill.negative_balance_interval_on_overdues')
             ||
-            $U->ou_ancestor_setting($circ->circ_lib(), 'bill.negative_balance_interval_default')
+            $U->ou_ancestor_setting_value($circ->circ_lib(), 'bill.negative_balance_interval_default')
         );
         my $result;
-        # if we prohibit negative overdue balances outright, OR we have an
-        # interval setting which determines what we do, let
-        # adjust_bills_to_zero() do the heavy lifting
+        # if we prohibit negative overdue balances and all payments
+        # are outside the refund interval (if given), zero the transaction
         if ($opts->{force_zero}
-            or (!$opts->{force_void} and ($U->is_true($prohibit_neg_balance_overdues) or $neg_balance_interval_overdues))
-            ) {
+            or (!$opts->{force_void}
+                and (
+                    $U->is_true($prohibit_neg_balance_overdues)
+                    and !_has_refundable_payments($e, $circ->id, $neg_balance_interval_overdues)
+                )
+            )
+        ) {
             $result = $class->adjust_bills_to_zero($e, $billids, $opts->{note}, $neg_balance_interval_overdues);
         } else {
             # otherwise, just void the usual way
@@ -103,7 +107,7 @@ sub void_or_zero_overdues {
 # Returns undef on success or the result from void_bills.
 # ------------------------------------------------------------------
 sub void_or_zero_bills_of_type {
-    my ($class, $e, $circ, $copy, $btype, $note) = @_;
+    my ($class, $e, $circ, $copy, $btype, $for_note) = @_;
 
     # Get a bill payment map.
     my $bpmap = $class->bill_payment_map_for_xact($e, $circ);
@@ -113,16 +117,24 @@ sub void_or_zero_bills_of_type {
         if (@bills) {
             # settings for lost come from copy circlib.
             my $prohibit_neg_balance_lost = (
-                $U->ou_ancestor_setting($copy->circ_lib(), 'bill.prohibit_negative_balance_on_lost')
+                $U->ou_ancestor_setting_value($copy->circ_lib(), 'bill.prohibit_negative_balance_on_lost')
                 ||
-                $U->ou_ancestor_setting($copy->circ_lib(), 'bill.prohibit_negative_balance_default')
+                $U->ou_ancestor_setting_value($copy->circ_lib(), 'bill.prohibit_negative_balance_default')
             );
             my $neg_balance_interval_lost = (
-                $U->ou_ancestor_setting($copy->circ_lib(), 'bill.negative_balance_interval_on_lost')
+                $U->ou_ancestor_setting_value($copy->circ_lib(), 'bill.negative_balance_interval_on_lost')
                 ||
-                $U->ou_ancestor_setting($copy->circ_lib(), 'bill.negative_balance_interval_default')
+                $U->ou_ancestor_setting_value($copy->circ_lib(), 'bill.negative_balance_interval_default')
             );
-            my $result = $class->void_bills($e, \@bills, $note);
+            my $result;
+            if (
+                $U->is_true($prohibit_neg_balance_lost)
+                and !_has_refundable_payments($e, $circ->id, $neg_balance_interval_lost)
+            ) {
+                $result = $class->adjust_bills_to_zero($e, \@bills, "System: ADJUSTED $for_note");
+            } else {
+                $result = $class->void_bills($e, \@bills, "System: VOIDED $for_note");
+            }
             if (ref($result)) {
                 return $result;
             }
@@ -551,10 +563,9 @@ sub void_bills {
 
 
 # This subroutine actually handles "adjusting" bills to zero.  It takes a
-# CStoreEditor, an arrayref of bill ids or bills, an optional note, and an
-# optional interval.
+# CStoreEditor, an arrayref of bill ids or bills, and an optional note.
 sub adjust_bills_to_zero {
-    my ($class, $e, $billids, $note, $interval) = @_;
+    my ($class, $e, $billids, $note) = @_;
 
     # Get with the editor to see if we have permission to void bills.
     return $e->die_event unless $e->checkauth;
@@ -612,10 +623,6 @@ sub adjust_bills_to_zero {
 
             # Get the bill_payment_map entry for this bill:
             my ($bpentry) = grep {$_->{bill}->id() == $bill->id()} @$bpmap;
-            if (_within_refund_interval($bpentry, $interval)) {
-#TODO
-                next;
-            }
 
             # From here on out, use the bill object from the bill
             # payment map entry.
@@ -674,38 +681,34 @@ sub adjust_bills_to_zero {
     return 1;
 }
 
-# A helper function to check if the payments on a bill are within the
-# range of a given interval.  The first argument is the entry hash
-# from the bill payment map for the bill to check and the second
-# argument is the interval.  It returns true (1) if any of the bills
-# are within range of the interval, or false (0) otherwise.  It also
-# returns true if the interval argument is undefined or empty, or if
-# the bill has no payments whatsoever.  It will return false if the
-# entry has no payments other than voids.
-sub _within_refund_interval {
-    my ($entry, $interval) = @_;
-    my $result = ($interval ? 0 : 1);
-
-    # A check to see if we were given the settings hash or the value:
-    if (ref($interval) eq 'HASH') {
-        $interval = $interval->{value};
-    }
+# A helper function to check if the payments on a bill are inside the
+# range of a given interval.
+# TODO: here is one simple place we could do voids in the absence
+# of any payments
+sub _has_refundable_payments {
+    my ($e, $xactid, $interval) = @_;
 
-    if ($interval && $entry && $entry->{payments} && @{$entry->{payments}}) {
-        my $interval_secs = interval_to_seconds($interval);
-        my @pay_dates = map {$_->payment_ts()} sort {$b->payment_ts() cmp $a->payment_ts()}  grep {$_->payment_type() ne 'adjustment_payment'} @{$entry->{payments}};
-        if (@pay_dates) {
-            # Since we've sorted the payment dates from highest to
-            # lowest, we really only need to check the 0th one.
-            my $payment_date = DateTime::Format::ISO8601->parse_datetime(cleanse_ISO8601($pay_dates[0]))->epoch;
-            my $now = time;
-            $result = 1 if ($payment_date + $interval_secs >= $now);
+    # for now, just short-circuit with no interval
+    return 0 if (!$interval);
+
+    my $last_payment = $e->search_money_payment(
+        {
+            xact => $xactid,
+            payment_type => {"!=" => 'adjustment_payment'}
+        },{
+            limit => 1,
+            order_by => { mp => "payment_ts DESC" }
         }
-    } elsif ($interval && (!$entry->{payments} || !@{$entry->{payments}})) {
-        $result = 1;
+    );
+
+    if ($last_payment->[0]) {
+        my $interval_secs = interval_to_seconds($interval);
+        my $payment_ts = DateTime::Format::ISO8601->parse_datetime(cleanse_ISO8601($last_payment->[0]->payment_ts))->epoch;
+        my $now = time;
+        return 1 if ($payment_ts + $interval_secs >= $now);
     }
 
-    return $result;
+    return 0;
 }
 
 1;
diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Circulate.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Circulate.pm
index 5508927ad4..9e1d018e9f 100644
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Circulate.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Circulate.pm
@@ -3983,7 +3983,7 @@ sub checkin_handle_lost_or_lo_now_found {
     my $tag = $is_longoverdue ? "LONGOVERDUE" : "LOST";
 
     $logger->debug("voiding $tag item billings");
-    my $result = $CC->void_or_zero_bills_of_type($self->editor, $self->circ, $self->copy, $bill_type, "System: VOIDED FOR $tag ITEM RETURNED");
+    my $result = $CC->void_or_zero_bills_of_type($self->editor, $self->circ, $self->copy, $bill_type, "$tag ITEM RETURNED");
     $self->bail_on_events($self->editor->event) if ($result);
 }