From Nicole: It was my understanding that checking the dropbox mode option on the check in screen made the due date effectively the last day the library was open and as such wiped out fines for the extra day. It appears that is not the case. Fines for the item being late remain on the patron's account. The only way around this is to 'forgive fines' when using dropbox mode, but that forgives all fines and really we only want to forgive one day of fines. So if the fines script has already run, then fines are not wiped.
changing from enhancement to bug - since the understanding is that dropbox mode is used to push the 'returned day' back and as such shouldn't charge fines for the days past the 'checkin day'
Nope the dropbox mode exists only to change the day of return to the previous day. Adding a new feature to remove fines is certainly an enhancement.
What's the point of changing the day ... except to get rid of the fines? That at least is the point here in the US.
I tend to agree with Nicole, I had always assumed that dropbox mode was backdating the return date to yesterday so that fines would be generated as if it had been returned the day before.
There does seem to be a common expectation amongst users that dropbox mode clears fines in some way. And subsequently they find it to be behaving inconsistently. The behaviour does need documenting as it seems to be adding confusion.
Probably doesn't matter whether it's an enhancement or bug unless a patch to change it actually exists. But it is working as designed, adding the ability for it to wipe fines is therefore an enhancement. But yeah without a patch its moot anyway.
As an end-user, I've got to agree with Melia, Nicole, and Kyle. This is totally unexpected behavior, and I can't quite wrap my head around the business rule that would have resulted in this program logic. Maybe that's just me, but I don't think so. As Chris says, whether you call this a bug or enhancement is really moot; the functionality is not in there and we *really* need this functionality. Erasing all fines on an item when we're just trying to compensate for the fact that we were closed yesterday is not the way we do business. If there is a use case where all fines should be wiped because a branch was closed yesterday, then perhaps we should have radio buttons to select either "Forgive all fines" vs. "Forgive fines while closed", but we at least need the option at our library.
(In reply to Aaron Sakovich from comment #7) > As an end-user, I've got to agree with Melia, Nicole, and Kyle. This is > totally unexpected behavior, and I can't quite wrap my head around the > business rule that would have resulted in this program logic. Maybe that's > just me, but I don't think so. One thing you learn with working with libraries, is that assuming that the way your library does it is the same way others do it is the single biggest mistake you can make. > > As Chris says, whether you call this a bug or enhancement is really moot; > the functionality is not in there and we *really* need this functionality. > Erasing all fines on an item when we're just trying to compensate for the > fact that we were closed yesterday is not the way we do business. > What I actually said is talking about if it's a bug or an enhancement is moot, until someone either pays someone to, or writes code themselves to change it. IE without a patch, nothing will change, no matter how we label it. > If there is a use case where all fines should be wiped because a branch was > closed yesterday, then perhaps we should have radio buttons to select either > "Forgive all fines" vs. "Forgive fines while closed", but we at least need > the option at our library. Awesome, I assume you will be working on a patch for this? Or paying someone to do this for you?
(In reply to Chris Cormack from comment #8) > One thing you learn with working with libraries, is that assuming that the > way your library does it is the same way others do it is the single biggest > mistake you can make. Totally agree, which is why I tried to ensure I personalized my comment. But then, that also goes the other way -- just because (it appears) some libraries may want to wipe all their fines doesn't mean everyone follows that business rule. > What I actually said is talking about if it's a bug or an enhancement is > moot, until someone either pays someone to, or writes code themselves to > change it. If I misled you to thinking I did not understand your point, I apologize, as I absolutely do -- again, I tried to express my perspective by saying "we", as in HMCPL. I tempered that with questioning if the existing business logic was indeed a valid use case, which if no one uses it this way, why is it coded thus? If no one uses a feature because the business logic does not reflect the real world, is it really a feature, or a bug? And yes, I would be okay if numLibraries ge 1. I'd even be okay with anecdotal evidence. Like I said, I (personally, and as a representative of HMCPL) was just having trouble wrapping my head around this business logic. > Awesome, I assume you will be working on a patch for this? Or paying someone > to do this for you? Indeed, I've been working on this angle for a few weeks since discovering this "feature". Thanks for your time and consideration.
It looks like bug 10694 will fix this: "Sometimes libraries need to backdate returns further back in time than Koha's dropbox mode will allow. The returns backdating will check in an item as if it had been returned on the specified date, and will reduce any fine accordingly." *** This bug has been marked as a duplicate of bug 10694 ***
Hm, on second thought might not be quite the same, please check if the new feature on bug 10694 is a fix for you.
(In reply to Katrin Fischer from comment #10) > It looks like bug 10694 will fix this: > > "Sometimes libraries need to backdate returns further back in time than > Koha's dropbox mode will allow. The returns backdating will check in an item > as if it had been returned on the specified date, and will reduce any fine > accordingly." > > *** This bug has been marked as a duplicate of bug 10694 *** Yes, as the author of bug 10694 I believe that it will effectively resolve this bug. The only difference is that feature will require you to select the return date rather than having it decided for you automatically.
I think at this point this comes down to the question I asked in comment 11 bug 22359 > Question: This patch uses the existing UpdateFine (and as such > Koha::Account->adjust) functionality. It will result in a fine line > with an amount of '0'. Is this actually what we want? > > We have a few alternatives we could consider: > > 1) We could leave it as is. > 1) We could delete the fine entirely so it's as if it never existed > 2) We could record a 'Writeoff' or something line against it to show it was > removed by dropbox/backdated returns mode.
bug 22539 even.. oops.
(In reply to Martin Renvoize from comment #13) > I think at this point this comes down to the question I asked in comment 11 > bug 22359 > > > Question: This patch uses the existing UpdateFine (and as such > > Koha::Account->adjust) functionality. It will result in a fine line > > with an amount of '0'. Is this actually what we want? > > > > We have a few alternatives we could consider: > > > > 1) We could leave it as is. > > 1) We could delete the fine entirely so it's as if it never existed > > 2) We could record a 'Writeoff' or something line against it to show it was > > removed by dropbox/backdated returns mode. My personal preference is to leave the 0 fine as part of the 'paper trail' for Koha's accounting system. I would not be opposed to option 2. We could add new 'states' for 'Dropboxed', 'Backdated return' and 'Fine forgiven' if we build it on top of bug 22512.
If the user returned the book on time and left it in a bookdrop due to the library being closed, then that user should not have a fine imposed. I realise that the fines job will have run and populated the fine before the library removes the book from the bookdrop. However when they then set the return date to reflect the 'real' date (e.g. yesterday), then Koha will know that the book was returned on time and therefore no fine should have been generated. Personally I think an audit trail is not required for this purpose. The fine (whilst it was applied) would not have been if the library had been open and the user could return the item to the counter. From a user's perspective they are not going to want evidence of a fine on their account when it was not needed nor a proper fine. I would opt for it to be deleted completely.
Created attachment 104841 [details] [review] Bug 8338: Unit Tests
Created attachment 104842 [details] [review] Bug 8338: Remove zero amount overdues on backdated returns where appropriate This patch removes any overdues which would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine.
Test Plan.. play with accounts and backdated returns.. report any oddities. More seriously. Test One 1 - Checkout an item to a patron and make it overdue (can backdate the checkout) 2 - Make sure the itemtype has fines that will be charged 3 - Charge the fines: Set finesMode = production perl misc/cronjobs/fines.pl -v 4 - Check the fine appears on the patrons account 5 - Browse to Circulation->Check in 6 - In 'Checkin settings' backdate to before the due date 7 - Check the fine no longer appears on the patrons account Test Two 1 - Checkout an item to a patron and make it overdue (can backdate the checkout) 2 - Make sure the itemtype has fines that will be charged 3 - Charge the fines: Set finesMode = production perl misc/cronjobs/fines.pl -v 4 - Check the fine appears on the patrons account 5 - Pay off the fine 6 - Browse to Circulation->Check in 7 - In 'Checkin settings' backdate to before the due date 8 - Check the fine still appears on the patrons account (but is now 'zero' and fixed) 9 - Check that there is a credit on the patrons account for the amount they originally paid
Bonus points.. check that the offsets (account details) make sense from each perspective.. looking at them from the payment, refund credit and overdue fine perspectives.
This looks good except for one extremely minor quibble -- the acount_offsets entry for the credit created in test 2 shows type "Manual credit," which is misleading on a credit generated automatically by the system. Signing off anyway, but would love to see that changed to "system-generated credit" or something more accurate.
Created attachment 104900 [details] [review] Bug 8338: Unit Tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 104901 [details] [review] Bug 8338: Remove zero amount overdues on backdated returns where appropriate This patch removes any overdues which would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Oh, I should mention that in the course of your test plan I ran into an unrelated error. Confirmed it happens without your patch, filed bug 25508 for it. At step 5 of test 2, when you pay off the fine, Koha gets weird about whether or not it should renew the overdue item.
I wonder if you have seen bug 13044 - we discussed how to merge the book drop with the specify return date feature. Test case 1 makes perfect sense to me: | 3 | NULL | 2 | OVERDUE_DECREASE | -0.100000 | 2020-05-23 13:09:43 Test case 2 is a bit confusing: What we see: 1 Manual debit (something I added for testing) 2 Overdue fine from first test 3 Decrease from returning it with book drop mode 4 Me paying off 4.00 to get over the fine limit (be able to checkout out again) 5 Creating another overdue (same item!) 6 Paying the overdue off 7 Now I see what Andrew means: Manual Credit and OVERDUE_DECREASE seem to be both the result of me using returning the item with book drop. I now have a credit of .10 on the account, which matches the amount 'overpaid'. I think it makes sense to me as it is linked to the accountlines entry 'Overpayment refund'. +----+-----------+----------+------------------+-----------+---------------------+ | id | credit_id | debit_id | type | amount | created_on | +----+-----------+----------+------------------+-----------+---------------------+ | 1 | NULL | 1 | Manual Debit | 4.000000 | 2020-05-22 11:20:08 | | 2 | NULL | 2 | OVERDUE | 2.300000 | 2020-05-23 13:06:48 | | 3 | NULL | 2 | OVERDUE_DECREASE | -0.100000 | 2020-05-23 13:09:43 | | 4 | 3 | 1 | Payment | -4.000000 | 2020-05-23 13:13:21 | | 5 | NULL | 4 | OVERDUE | 0.500000 | 2020-05-23 13:13:52 | | 6 | 5 | 4 | Payment | -0.500000 | 2020-05-23 13:14:02 | | 7 | 6 | NULL | Manual Credit | -0.100000 | 2020-05-23 13:14:24 | | 8 | NULL | 4 | OVERDUE_DECREASE | -0.100000 | 2020-05-23 13:14:24 | +----+-----------+----------+------------------+-----------+---------------------+ NOTE: Am I the only one getting confused by our mix of descriptions (with false capitalization) and codes in these tables? Also "Overpayment refund" is not translatable :(
I was ready to pass QA, but tests fail (pass with master): kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 29/48 _FixOverduesOnReturn(233, Koha::Item=HASH(0x55c8b6955450)->itemnumber...) failed! at /home/vagrant/kohaclone/C4/Circulation.pm line 2056. t/db_dependent/Circulation.t .. 45/48 # No tests run! # Failed test 'No tests run for subtest "Test Backdating of Returns"' # at t/db_dependent/Circulation.t line 3930. Can't call method "amountoutstanding" on an undefined value at t/db_dependent/Circulation.t line 3928. # Looks like your test exited with 255 just after 46. t/db_dependent/Circulation.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 3/48 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 65280 Tests: 46 Failed: 1) Failed test: 46 Non-zero exit status: 255 Parse errors: Bad plan. You planned 48 tests but ran 46.
Created attachment 105496 [details] [review] Bug 8338: Unit Tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105497 [details] [review] Bug 8338: Remove zero amount overdues on backdated returns where appropriate This patch removes any overdues which would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 105498 [details] [review] Bug 8338: (follow-up) Fix test This patch moves the previous test introduced with bug 24075 into the same block as the rest of the AddReturn tests and updates it to test for the new 'remove accountline' behaviour as well as the reduce and refund behaviour.
The failing test is fixed and enhanced to catch more cases in the follow :)
Requires another little fix: FAIL t/db_dependent/Circulation.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid "my" variable $accountline masks earlier declaration in same scope
I've filed a new bug for the translation issue noted in comment#25: Bug 25833 - "Overpayment refund" in charge descriptions is not translatable
Created attachment 106152 [details] [review] Bug 8338: Unit Tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 106153 [details] [review] Bug 8338: Remove zero amount overdues on backdated returns where appropriate This patch removes any overdues which would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 106154 [details] [review] Bug 8338: (follow-up) Fix test This patch moves the previous test introduced with bug 24075 into the same block as the rest of the AddReturn tests and updates it to test for the new 'remove accountline' behaviour as well as the reduce and refund behaviour.
Created attachment 106155 [details] [review] Bug 8338: (QA follow-up) Fix perlcritic error
The tests pass, but I am irritated by the output, can you explain? t/db_dependent/Circulation.t .. 30/47 _FixOverduesOnReturn(137, Koha::Item=HASH(0x560ff2799448)->itemnumber...) failed! at /home/vagrant/kohaclone/C4/Circulation.pm line 2056. It doesn't show without this patch.
The nasty looking warn is actually introduced with Bug 21206: Replace C4::Items::GetItem where `->{itemnumber}` is replaced with `->itemnumber`. You say the warning doesn't appear at all prior to the patch.. that feels odd.. I'll investigate that.
Aha.. it's because with this patch the return of _FixAccountOnReturn is '0' as opposed to a result object.. but as the note says in the routine that doesn't signify an error. I'll add a little followup to clean up the warning.. not really even sure we need it at all now.
Hmm... not it's not.. we already check for defined in the relevant line.. bit confused
Created attachment 106209 [details] [review] Bug 8338: (QA follow-up) Clean up warning This patch moves the accountline->store call below the FinesLog code such that we return the same 'thing' from _FixOverduesOnReturn as the other clauses of the routine. We also take the oportunity to clean up the warning thrown by an errant call to the routine such that we output the actual itemnumber rather than a HASH reference marker.
Created attachment 106523 [details] [review] Bug 8338: Unit Tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 106524 [details] [review] Bug 8338: Remove zero amount overdues on backdated returns where appropriate This patch removes any overdues which would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 106525 [details] [review] Bug 8338: (follow-up) Fix test This patch moves the previous test introduced with bug 24075 into the same block as the rest of the AddReturn tests and updates it to test for the new 'remove accountline' behaviour as well as the reduce and refund behaviour. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 106526 [details] [review] Bug 8338: (QA follow-up) Fix perlcritic error Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 106527 [details] [review] Bug 8338: (QA follow-up) Clean up warning This patch moves the accountline->store call below the FinesLog code such that we return the same 'thing' from _FixOverduesOnReturn as the other clauses of the routine. We also take the oportunity to clean up the warning thrown by an errant call to the routine such that we output the actual itemnumber rather than a HASH reference marker. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 20.11, thanks to everybody involved!
Created attachment 107140 [details] [review] Bug 8338: (QA follow-up) Fix test for backdated return A test for CalculateFinesOnBackdate was introduced since this bug was written and needed updateing.
Follow-up pushed to master for 20.11
backported to 20.05.x for 20.05.03
(In reply to Lucas Gass from comment #50) > backported to 20.05.x for 20.05.03 Lucas, you missed the last patch "(QA follow-up) Fix test for backdated return", it's not in 20.05.x
Thanks Jonathan, I have backported the last patch: "(QA follow-up) Fix test for backdated return"
Patches do not apply cleanly on 19.11.x. Please rebase if needed.
I changed the title a bit for the release notes.
Martin, can you please rebase this for 19.11? Thanks!
Created attachment 110640 [details] [review] Bug 8338: [19.11] Remove zero amount overdues on backdated returns [19.11.x] This is a squashed patchset ready for application to 19.11 * Remove zero amount overdues on backdated returns where appropriate This patch removes any overdues which would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine. * Fix test This patch moves the previous test introduced with bug 24075 into the same block as the rest of the AddReturn tests and updates it to test for the new 'remove accountline' behaviour as well as the reduce and refund behaviour.
(In reply to Andrew Fuerste-Henry from comment #55) > Martin, can you please rebase this for 19.11? > Thanks! As requested, I have added a rebased and squashed patch for application to the 19.11 branch.. to apply it you will also need the rebased 19.11 patch for bug 24252 which introduces a relation accessor we use here.
(In reply to Martin Renvoize from comment #57) > (In reply to Andrew Fuerste-Henry from comment #55) > > Martin, can you please rebase this for 19.11? > > Thanks! > > As requested, I have added a rebased and squashed patch for application to > the 19.11 branch.. to apply it you will also need the rebased 19.11 patch > for bug 24252 which introduces a relation accessor we use here. Hi Martin Bug 24252 was purposefully not backported to 19.11.x as it is an enhancement. Can you please rewrite the 19.11 patch for this bug so that it doesn't use a relation accessor that doesn't already exist in 19.11?
(In reply to Aleisha Amohia from comment #58) > (In reply to Martin Renvoize from comment #57) > > (In reply to Andrew Fuerste-Henry from comment #55) > > > Martin, can you please rebase this for 19.11? > > > Thanks! > > > > As requested, I have added a rebased and squashed patch for application to > > the 19.11 branch.. to apply it you will also need the rebased 19.11 patch > > for bug 24252 which introduces a relation accessor we use here. > > Hi Martin > > Bug 24252 was purposefully not backported to 19.11.x as it is an enhancement. > > Can you please rewrite the 19.11 patch for this bug so that it doesn't use a > relation accessor that doesn't already exist in 19.11? I don't really have any more time for working on this.. bug 24252 is an under the hood enhancement that will not be seen by end users, has unit tests and should apply without issue.. I'm not sure there's a real reason to require a re-write of this bug to accommodate skipping that patch.
It was not picked as being an enhancement by the previous release maintainer. If this bug really can only be done using that enhancement (under the hood, over the hood, beside the hood it's still an enhancement) Then it will just have to miss out on being in 19.11.x
(In reply to Aleisha Amohia from comment #53) > Patches do not apply cleanly on 19.11.x. Please rebase if needed. I just wish it had been made clear it wouldn't be backported even if rebased rather than suggested it would.. I've wasted time working on it and cannot afford to waste any more time now. All good.. dropping it and putting it down as experience.