Bug 8338 - Add ability to remove fines with dropbox mode
Summary: Add ability to remove fines with dropbox mode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact:
URL:
Keywords:
Depends on: 5644 22512 24252
Blocks: 14825 28097
  Show dependency treegraph
 
Reported: 2012-06-29 20:46 UTC by Melia Meggs
Modified: 2021-12-13 21:11 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement will remove any overdue fines that would be reversed on a backdated return if CalcFineOnBackdate is enabled and the user has not already attempted to pay off the accruing fine.
Version(s) released in:
20.11.00, 20.05.03


Attachments
Bug 8338: Unit Tests (6.77 KB, patch)
2020-05-13 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: Remove zero amount overdues on backdated returns where appropriate (1.95 KB, patch)
2020-05-13 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: Unit Tests (6.83 KB, patch)
2020-05-14 21:29 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 8338: Remove zero amount overdues on backdated returns where appropriate (2.02 KB, patch)
2020-05-14 21:29 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 8338: Unit Tests (6.82 KB, patch)
2020-06-02 16:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: Remove zero amount overdues on backdated returns where appropriate (2.02 KB, patch)
2020-06-02 16:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: (follow-up) Fix test (6.43 KB, patch)
2020-06-02 16:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: Unit Tests (6.82 KB, patch)
2020-06-22 07:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: Remove zero amount overdues on backdated returns where appropriate (2.02 KB, patch)
2020-06-22 07:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: (follow-up) Fix test (6.42 KB, patch)
2020-06-22 07:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: (QA follow-up) Fix perlcritic error (888 bytes, patch)
2020-06-22 07:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: (QA follow-up) Clean up warning (1.97 KB, patch)
2020-06-23 14:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: Unit Tests (6.89 KB, patch)
2020-07-04 20:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8338: Remove zero amount overdues on backdated returns where appropriate (2.07 KB, patch)
2020-07-04 20:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8338: (follow-up) Fix test (6.48 KB, patch)
2020-07-04 20:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8338: (QA follow-up) Fix perlcritic error (945 bytes, patch)
2020-07-04 20:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8338: (QA follow-up) Clean up warning (2.03 KB, patch)
2020-07-04 20:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8338: (QA follow-up) Fix test for backdated return (1.69 KB, patch)
2020-07-21 10:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 8338: [19.11] Remove zero amount overdues on backdated returns (16.51 KB, patch)
2020-09-24 09:13 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Melia Meggs 2012-06-29 20:46:25 UTC
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.
Comment 1 Nicole C. Engard 2013-02-04 16:18:21 UTC
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'
Comment 2 Chris Cormack 2013-02-05 02:39:22 UTC
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.
Comment 3 Nicole C. Engard 2013-02-05 14:08:18 UTC
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.
Comment 4 Kyle M Hall 2013-02-05 14:10:41 UTC
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.
Comment 5 Colin Campbell 2013-02-05 16:57:51 UTC
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.
Comment 6 Chris Cormack 2013-02-05 17:37:34 UTC
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.
Comment 7 Aaron Sakovich 2014-04-10 18:16:25 UTC
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.
Comment 8 Chris Cormack 2014-04-10 18:27:01 UTC
(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?
Comment 9 Aaron Sakovich 2014-04-10 18:48:44 UTC
(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.
Comment 10 Katrin Fischer 2014-04-15 08:43:29 UTC
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 ***
Comment 11 Katrin Fischer 2014-04-15 08:45:11 UTC
Hm, on second thought might not be quite the same, please check if the new feature on bug 10694 is a fix for you.
Comment 12 Kyle M Hall 2014-04-25 11:07:41 UTC
(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.
Comment 13 Martin Renvoize 2019-04-03 10:08:07 UTC
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.
Comment 14 Martin Renvoize 2019-04-03 10:09:18 UTC
bug 22539 even.. oops.
Comment 15 Kyle M Hall 2019-04-03 10:49:27 UTC
(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.
Comment 16 Fiona Borthwick 2020-05-12 13:20:27 UTC
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.
Comment 17 Martin Renvoize 2020-05-13 13:15:00 UTC
Created attachment 104841 [details] [review]
Bug 8338: Unit Tests
Comment 18 Martin Renvoize 2020-05-13 13:15:03 UTC
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.
Comment 19 Martin Renvoize 2020-05-13 13:25:36 UTC
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
Comment 20 Martin Renvoize 2020-05-13 13:27:16 UTC
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.
Comment 21 Andrew Fuerste-Henry 2020-05-14 21:28:23 UTC
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.
Comment 22 Andrew Fuerste-Henry 2020-05-14 21:29:22 UTC
Created attachment 104900 [details] [review]
Bug 8338: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 23 Andrew Fuerste-Henry 2020-05-14 21:29:26 UTC
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>
Comment 24 Andrew Fuerste-Henry 2020-05-14 23:15:37 UTC
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.
Comment 25 Katrin Fischer 2020-05-23 21:51:33 UTC
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 :(
Comment 26 Katrin Fischer 2020-05-23 22:04:23 UTC
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.
Comment 27 Martin Renvoize 2020-06-02 16:40:56 UTC
Created attachment 105496 [details] [review]
Bug 8338: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 28 Martin Renvoize 2020-06-02 16:41:00 UTC
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>
Comment 29 Martin Renvoize 2020-06-02 16:41:04 UTC
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.
Comment 30 Martin Renvoize 2020-06-02 16:41:31 UTC
The failing test is fixed and enhanced to catch more cases in the follow :)
Comment 31 Katrin Fischer 2020-06-21 14:59:31 UTC
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
Comment 32 Katrin Fischer 2020-06-21 15:03:11 UTC
I've filed a new bug for the translation issue noted in comment#25:
Bug 25833 - "Overpayment refund" in charge descriptions is not translatable
Comment 33 Martin Renvoize 2020-06-22 07:50:21 UTC
Created attachment 106152 [details] [review]
Bug 8338: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 34 Martin Renvoize 2020-06-22 07:50:25 UTC
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>
Comment 35 Martin Renvoize 2020-06-22 07:50:28 UTC
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.
Comment 36 Martin Renvoize 2020-06-22 07:50:32 UTC
Created attachment 106155 [details] [review]
Bug 8338: (QA follow-up) Fix perlcritic error
Comment 37 Katrin Fischer 2020-06-22 21:18:28 UTC
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.
Comment 38 Martin Renvoize 2020-06-23 13:39:03 UTC
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.
Comment 39 Martin Renvoize 2020-06-23 13:50:41 UTC
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.
Comment 40 Martin Renvoize 2020-06-23 13:53:49 UTC
Hmm... not it's not.. we already check for defined in the relevant line.. bit confused
Comment 41 Martin Renvoize 2020-06-23 14:13:05 UTC
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.
Comment 42 Katrin Fischer 2020-07-04 20:28:49 UTC
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>
Comment 43 Katrin Fischer 2020-07-04 20:28:54 UTC
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>
Comment 44 Katrin Fischer 2020-07-04 20:28:59 UTC
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>
Comment 45 Katrin Fischer 2020-07-04 20:29:04 UTC
Created attachment 106526 [details] [review]
Bug 8338: (QA follow-up) Fix perlcritic error

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 46 Katrin Fischer 2020-07-04 20:29:09 UTC
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>
Comment 47 Jonathan Druart 2020-07-20 15:47:50 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 48 Martin Renvoize 2020-07-21 10:19:54 UTC
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.
Comment 49 Jonathan Druart 2020-07-21 10:23:59 UTC
Follow-up pushed to master for 20.11
Comment 50 Lucas Gass 2020-07-25 18:23:32 UTC
backported to 20.05.x for 20.05.03
Comment 51 Jonathan Druart 2020-07-27 10:13:18 UTC
(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
Comment 52 Lucas Gass 2020-07-27 15:19:15 UTC
Thanks Jonathan,

I have backported the last patch: "(QA follow-up) Fix test for backdated return"
Comment 53 Aleisha Amohia 2020-08-03 22:52:51 UTC
Patches do not apply cleanly on 19.11.x. Please rebase if needed.
Comment 54 Caroline Cyr La Rose 2020-08-20 21:39:46 UTC
I changed the title a bit for the release notes.
Comment 55 Andrew Fuerste-Henry 2020-09-23 19:21:39 UTC
Martin, can you please rebase this for 19.11?
Thanks!
Comment 56 Martin Renvoize 2020-09-24 09:13:11 UTC
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.
Comment 57 Martin Renvoize 2020-09-24 09:14:26 UTC
(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.
Comment 58 Aleisha Amohia 2020-09-24 20:52:28 UTC
(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?
Comment 59 Martin Renvoize 2020-09-25 11:59:51 UTC
(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.
Comment 60 Chris Cormack 2020-09-28 20:36:57 UTC
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
Comment 61 Martin Renvoize 2020-09-29 07:09:32 UTC
(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.