Created attachment 14817 [details]
fines for those titles
I am not seeing a way to endorse this bug, but I'll endorse it by commenting. Created attachment 26193 [details] [review] Bug 9481 - charge not showing fines Added new subroutine to sum up all outstanding charges by borrower for this item including all historical issues and rent charges. Previous routine gets charge amount from system preferences instead of amount outstanding. 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. Created 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 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: Holger Meißner <h.meissner.82@web.de> Could you please provide unit tests for the new routine please? Created attachment 26674 [details] [review] Bug 9481 (follow up) change parameter order in routine Small change in parameters order. Because other routines in C4::Overudes gets first itemnumber then borrowernumber, changed this to stay consistent. Created attachment 26675 [details] [review] Bug 9481 QA follow-up Add unit test Add unit test for GetItemCharges in C4::Overdues. Created attachment 26684 [details] [review] Bug 9481 (follow-up) - change parameter order in routine Only small change in parameters order. Because other routines in C4::Overudes gets first itemnumber then borrowernumber, changed this to stay consistent. Signed-off-by: Holger Meißner <h.meissner.82@web.de> Created attachment 26685 [details] [review] Bug 9481 (QA follow-up) - Add unit test for GetItemCharges in C4::Overdues. Signed-off-by: Holger Meißner <h.meissner.82@web.de> 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 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. Marked as Failed QA. See also bug 13732. *** Bug 13732 has been marked as a duplicate of this bug. *** Is there any prospect for this bug getting solved? Rafal, do you have time to make the fixes pointed out by Jonathan? 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. Created attachment 36739 [details] [review] [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. 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. Created attachment 36740 [details] [review] [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. 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. 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? 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. Created attachment 36742 [details] [review] [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. 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. (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. Created attachment 36787 [details] [review] 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> 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> Patch pushed to master. Thanks Rafal! Pushed to 3.18.x will be in 3.18.6 |
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.