Bug 9481

Summary: charge not showing fines
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: CirculationAssignee: Rafal Kopaczka <rkopaczka>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: abl, chris, david.roberts, gmcharlt, h.meissner.82, jonathan.druart, katrin.fischer, kyle.m.hall, martin.renvoize, olli-antti.kivilahti, rkopaczka, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7985
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14841    
Attachments: $0 charges
fines for those titles
Bug 9481 - charge not showing fines
Bug 9481 - charge not showing fines On 'Check out' and 'Details' screens in patron record fines for item aren't showing. Added new soubroutine to sum up all outstanding charges by borrower for this item including all historical issues and rent charges. Pr
Bug 9481 (follow up) change parameter order in routine
Bug 9481 QA follow-up Add unit test
Bug 9481 (follow-up) - change parameter order in routine
Bug 9481 (QA follow-up) - Add unit test for GetItemCharges in C4::Overdues.
[PATCH] Bug 9481 - charge not showing fines On 'Check out' and 'Details'. When looking at the patron record or the checkout screen the checkout summary is now showing 0 for all the Charges even if the item was overdue and has accrued fines.
[PATCH] Bug 9481 - charge not showing fines On 'Check out' and 'Details'.
[PATCH] Bug 9481 - charge not showing fines On 'Check out' and 'Details'.
Bug 9481 - charge not showing fines On 'Check out' and 'Details'.
[PASSED QA] Bug 9481 - charge not showing fines On 'Check out' and 'Details'.

Description Nicole C. Engard 2013-01-24 12:53:52 UTC
Created attachment 14816 [details]
$0 charges

When looking at the patron record or the checkout screen the checkout summary is now showing 0 for all the Charges even if the item was overdue and has accrued fines.

This column I think used to show the fine amount.  

I have confirmed in 3.8.5 and master.
Comment 1 Nicole C. Engard 2013-01-24 12:54:12 UTC
Created attachment 14817 [details]
fines for those titles
Comment 2 Olli-Antti Kivilahti 2014-01-15 11:45:47 UTC
I am not seeing a way to endorse this bug, but I'll endorse it by commenting.
Comment 3 Rafal Kopaczka 2014-03-12 15:38:29 UTC Comment hidden (obsolete)
Comment 4 Holger Meißner 2014-03-19 15:43:07 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-03-20 08:26:42 UTC
Could you please provide unit tests for the new routine please?
Comment 6 Rafal Kopaczka 2014-03-28 09:12:50 UTC Comment hidden (obsolete)
Comment 7 Rafal Kopaczka 2014-03-28 09:14:15 UTC Comment hidden (obsolete)
Comment 8 Holger Meißner 2014-03-28 14:16:56 UTC Comment hidden (obsolete)
Comment 9 Holger Meißner 2014-03-28 14:17:00 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-04-16 09:58:26 UTC
Comment on attachment 26457 [details] [review]
Bug 9481 - charge not showing fines On 'Check out' and 'Details' screens in patron record fines for item aren't showing. Added new soubroutine to sum up all outstanding charges by borrower for this item including all historical issues and rent charges. Pr

Review of attachment 26457 [details] [review]:
-----------------------------------------------------------------

::: C4/Overdues.pm
@@ +659,5 @@
>      return 0;
>  }
>  
> +=head2 GetItemCharges
> +

Why don't you put this subroutine into C4::Account?

@@ +668,5 @@
> +=cut
> +
> +sub GetItemCharges {
> +    my ( $borrowernumber, $itemnumber ) = @_;
> +

I would prefer you pass an hashref to this new subroutine.

@@ +674,5 @@
> +
> +    my $query = "SELECT SUM(amountoutstanding) FROM accountlines WHERE borrowernumber = ? AND itemnumber = ? ";
> +    my $sth = $dbh->prepare( $query );
> +    $sth->execute( $borrowernumber, $itemnumber );
> +    return $sth->fetchrow_arrayref()->[0];

Could be done using $dbh->selectcol_arrayref
Comment 11 Jonathan Druart 2014-04-16 09:58:37 UTC
Comment on attachment 26685 [details] [review]
Bug 9481 (QA follow-up) - Add unit test for GetItemCharges in C4::Overdues.

Review of attachment 26685 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Overdues.t
@@ +69,5 @@
> +    $sth->execute(@$loop);
> +}
> +
> +ok(GetItemCharges($itemnumber, $borrowernumber) == 348.25, "GetItemCharges returns all outstanding charges for item");
> +

What is 348.25? Please make this more readable.

@@ +71,5 @@
> +
> +ok(GetItemCharges($itemnumber, $borrowernumber) == 348.25, "GetItemCharges returns all outstanding charges for item");
> +
> +my $anotheritemnumber = $itemnumber + 999999;
> +my $anotherborrowernumber = $borrowernumber + 999999;

These numbers could exist in a big DB and the test will fail.
Comment 12 Jonathan Druart 2014-04-16 09:59:26 UTC
Marked as Failed QA.
Comment 13 Katrin Fischer 2015-02-22 15:21:16 UTC
See also bug 13732.
Comment 14 Katrin Fischer 2015-02-22 15:21:59 UTC
*** Bug 13732 has been marked as a duplicate of this bug. ***
Comment 15 Martin Renvoize 2015-02-23 14:14:10 UTC
Is there any prospect for this bug getting solved?
Comment 16 Katrin Fischer 2015-02-23 14:30:59 UTC
Rafal, do you have time to make the fixes pointed out by Jonathan?
Comment 17 Rafal Kopaczka 2015-02-23 14:40:14 UTC
Sorry I've forgot about this bug... But it seems that there isn't much work to do (beside rebase), but currently I'm working on something else. So if anyone want to work on this feel free to do so. If not I'll have time to look at this patch for about 1-2 weeks.
Comment 18 Rafal Kopaczka 2015-03-09 13:39:06 UTC Comment hidden (obsolete)
Comment 19 Rafal Kopaczka 2015-03-09 13:39:59 UTC Comment hidden (obsolete)
Comment 20 Rafal Kopaczka 2015-03-09 13:41:24 UTC
This patch, turns out is trivial after datatables patch has been pushed. But now I'm wondering if showing fines in charges column is correct behaviour? Maybe there should be some syspref for showing fines/charges or separate column in table?
Comment 21 David Roberts 2015-03-09 13:45:03 UTC
Showing fines in the charges column is definitely not correct (see my bug 13732 which was marked as a duplicate of this one). Charges refer to e.g. rental fees which are different to fines. It would be better perhaps if there was a separate column for fines. If more screen real-estate is needed, some of the columns that display on this screen are probably not that useful (replacement price, for example) so could be removed.
Comment 22 Rafal Kopaczka 2015-03-09 14:25:53 UTC Comment hidden (obsolete)
Comment 23 Rafal Kopaczka 2015-03-09 14:30:12 UTC
(In reply to David Roberts from comment #21)
> Showing fines in the charges column is definitely not correct (see my bug
> 13732 which was marked as a duplicate of this one). Charges refer to e.g.
> rental fees which are different to fines. It would be better perhaps if
> there was a separate column for fines. If more screen real-estate is needed,
> some of the columns that display on this screen are probably not that useful
> (replacement price, for example) so could be removed.

That's what I've think. New patch with new column for fines. As for removing columns, there is some feature for defining visibility of column. Maybe that could be used to hide columns? But it's another bug I think.
Comment 24 Chris Cormack 2015-03-11 06:57:42 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-03-13 12:28:50 UTC
Created attachment 36848 [details] [review]
[PASSED QA] Bug 9481 - charge not showing fines On 'Check out' and 'Details'.

When looking at the patron record or the checkout screen the checkout
summary is now showing 0 for all the Charges even if the item was
overdue and has accrued fines.
Removed unused(?) footer values in checkouts-table-footer.inc

To test:
1/ Check out items with past due date
2/ Run fines.pl script (ensure finesMode is set to Calculate and Charge)
3/ Verify on Fines->Pay Fines screen that fines where calculated
correct.
4/ Go to Patron record, charge column on Details and Check out
screen
should be 0 or rental charge amount only. But total amount row
display right
number, same as in pay fines screen.
4/ Apply patch.
5/ Now charges on display and check out screen shows all outstanding
fees for each item.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Tomás Cohen Arazi 2015-03-31 17:18:42 UTC
Patch pushed to master.

Thanks Rafal!
Comment 27 Chris Cormack 2015-04-06 08:47:39 UTC
Pushed to 3.18.x will be in 3.18.6