Bug 18501 - Automatic refunds need protection from failure
Summary: Automatic refunds need protection from failure
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 25663
Blocks: 23091 26210 26268
  Show dependency treegraph
 
Reported: 2017-04-27 14:35 UTC by Christopher Brannon
Modified: 2020-10-06 11:43 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 18501: Prepare the ground (5.44 KB, patch)
2020-08-10 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.56 KB, patch)
2020-08-10 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (1.95 KB, patch)
2020-08-10 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Prepare the ground (4.74 KB, patch)
2020-08-12 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.56 KB, patch)
2020-08-12 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (1.95 KB, patch)
2020-08-12 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Prepare the ground (4.80 KB, patch)
2020-08-13 09:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.63 KB, patch)
2020-08-13 09:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (2.01 KB, patch)
2020-08-13 09:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Add POD for Koha::Item->set_found (1.47 KB, patch)
2020-08-13 09:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: but.. what was the meaning of holdingbranch here? (1.21 KB, patch)
2020-08-13 16:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Add _set_found_trigger (10.40 KB, patch)
2020-08-13 16:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: but.. what was the meaning of holdingbranch here? (1.21 KB, patch)
2020-08-13 16:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: wip - need to know it has been refunded (1.88 KB, patch)
2020-08-14 07:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: set the is_refunded flag as internal var (1.71 KB, patch)
2020-08-14 08:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Adjust condition for flagging an item found (939 bytes, patch)
2020-08-14 08:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Prepare the ground (4.80 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.63 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (2.01 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Add _set_found_trigger (10.46 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: but.. what was the meaning of holdingbranch here? (1.27 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: wip - need to know it has been refunded (1.94 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: set the is_refunded flag as internal var (1.77 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Adjust condition for flagging an item found (1004 bytes, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id (1.77 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Migrate and update unit tests (47.60 KB, patch)
2020-08-14 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests (5.38 KB, patch)
2020-08-14 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Add test of *_on functionality (3.32 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Prepare the ground (4.80 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.63 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (2.01 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Add _set_found_trigger (10.46 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: but.. what was the meaning of holdingbranch here? (1.27 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: wip - need to know it has been refunded (1.94 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: set the is_refunded flag as internal var (1.77 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Adjust condition for flagging an item found (1004 bytes, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id (1.77 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Migrate and update unit tests (47.70 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests (5.40 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26210: Allow issue_id to be passed to add_credit (1.60 KB, patch)
2020-08-18 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Add test of *_on functionality (3.31 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Prepare the ground (4.80 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.63 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (2.01 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Add _set_found_trigger (10.46 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: but.. what was the meaning of holdingbranch here? (1.27 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: wip - need to know it has been refunded (1.92 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: set the is_refunded flag as internal var (1.77 KB, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Adjust condition for flagging an item found (1004 bytes, patch)
2020-08-19 10:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id (1.77 KB, patch)
2020-08-19 10:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Migrate and update unit tests (47.72 KB, patch)
2020-08-19 10:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests (5.40 KB, patch)
2020-08-19 10:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Add test of *_on functionality (4.53 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Prepare the ground (4.80 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.63 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (2.01 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Add _set_found_trigger (10.46 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: but.. what was the meaning of holdingbranch here? (1.27 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: wip - need to know it has been refunded (1.92 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: set the is_refunded flag as internal var (1.77 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Adjust condition for flagging an item found (1004 bytes, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id (1.77 KB, patch)
2020-08-19 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Migrate and update unit tests (46.50 KB, patch)
2020-08-19 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests (5.40 KB, patch)
2020-08-19 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26210: Allow issue_id to be passed to add_credit (1.60 KB, patch)
2020-08-19 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Add test of *_on functionality (4.61 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Prepare the ground (4.88 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Remove the borrowernumber parameter (5.70 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Auto refund if mark as found from cataloguing (2.08 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Add _set_found_trigger (10.53 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: but.. what was the meaning of holdingbranch here? (1.34 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: wip - need to know it has been refunded (1.99 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: set the is_refunded flag as internal var (1.84 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Adjust condition for flagging an item found (1.05 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id (1.85 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Migrate and update unit tests (46.57 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests (5.47 KB, patch)
2020-08-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: Fix QA issues (2.24 KB, patch)
2020-08-20 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: (follow-up) Remove duplicate test (1.94 KB, patch)
2020-08-20 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Remove duplicate test (1.94 KB, patch)
2020-08-20 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Test undefined userenv behaviour (3.18 KB, patch)
2020-08-20 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Do not rely on userenv->{branchcode} (1.51 KB, patch)
2020-08-20 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Add fallback to ItemHomeBranch (2.60 KB, patch)
2020-08-20 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Remove duplicate test (1.94 KB, patch)
2020-08-21 09:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Test undefined userenv behaviour (3.19 KB, patch)
2020-08-21 09:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Do not rely on userenv->{branchcode} (1.51 KB, patch)
2020-08-21 09:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Add fallback to ItemHomeBranch (2.60 KB, patch)
2020-08-21 09:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: (follow-up) Use t::lib::Dates::compare in tests (1.52 KB, patch)
2020-08-21 09:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18501: Don't use paidfor for DB fields not mapped with a MARC field (1.71 KB, patch)
2020-08-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18501: [20.05.x] Automatic refunds need protection from failure Bug 18501: (QA follow-up) Add test of *_on functionality (75.98 KB, patch)
2020-10-06 11:43 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2017-04-27 14:35:07 UTC
I am told that the auto refund feature can fail if the status is edited in the item edit screen.  There should be some protection in place to keep this from failing so easily and quietly.

Possible solutions might be:
1) Prevent status from being edited if a fee has been charged for that item.
2) A warning and confirmation of action when the status is being edited and there is a fee involved.
3) An authorized override for such a change.
4) A note in the patron account lines indicating status was change and an auto refund will not be possible.
5) The ability to auto refund, even after the status has been altered.

I am marking this as a bug, because I cannot see how the current behavior could be a desired function.  Failure is not an option.
Comment 1 Jonathan Druart 2020-01-27 10:57:04 UTC
Can you confirm we are talking about RefundLostOnReturnControl here?
Comment 2 Christopher Brannon 2020-01-27 15:25:57 UTC
(In reply to Jonathan Druart from comment #1)
> Can you confirm we are talking about RefundLostOnReturnControl here?

I believe so.  Honestly, this was 2.5 years ago.  I don't remember much about this, but that sounds right.
Comment 3 Martin Renvoize 2020-02-06 08:58:40 UTC
I'm wondering if Alex ever actually worked on this?  If so, it would be good to see some patches, if not please can you unassign yourself so it may be re-assigned?
Comment 4 Alex Buckley 2020-03-23 01:36:17 UTC
Hi Martin, 

So sorry! I completely forgot about this patch. I'll un-assign myself now, as I currently don't have enough time to work on it. 

Thanks,
Alex
Comment 5 Jonathan Druart 2020-08-10 14:56:41 UTC
Created attachment 108004 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.
Comment 6 Jonathan Druart 2020-08-10 14:56:45 UTC
Created attachment 108005 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used
Comment 7 Jonathan Druart 2020-08-10 14:56:48 UTC
Created attachment 108006 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded
Comment 8 Jonathan Druart 2020-08-10 14:58:26 UTC
Can you test this patch and confirm that it works as you expect?

Two remarks for QA:
- unit tests can be required
- I don't understand the "ge" test, why no >=? items.itemlost is a tinyint
Comment 9 Martin Renvoize 2020-08-12 13:24:56 UTC
I would really like to have seen this written atop bug 25663, especially as it's PQA... though I suppose this makes it simpler to backport.

Bug 23091 overlaps allot too.. though I like the refactoring going on here.. I suppose I best go rebase that to match this.
Comment 10 Jonathan Druart 2020-08-12 13:48:26 UTC
Created attachment 108121 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.
Comment 11 Jonathan Druart 2020-08-12 13:48:35 UTC
Created attachment 108122 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used
Comment 12 Jonathan Druart 2020-08-12 13:48:39 UTC
Created attachment 108123 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded
Comment 13 Jonathan Druart 2020-08-12 13:49:24 UTC
(In reply to Martin Renvoize from comment #9)
> I would really like to have seen this written atop bug 25663, especially as
> it's PQA... though I suppose this makes it simpler to backport.

Done!
Comment 14 Martin Renvoize 2020-08-13 09:46:07 UTC
Created attachment 108167 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2020-08-13 09:46:10 UTC
Created attachment 108168 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2020-08-13 09:46:14 UTC
Created attachment 108169 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2020-08-13 09:46:17 UTC
Created attachment 108170 [details] [review]
Bug 18501: (QA follow-up) Add POD for Koha::Item->set_found

This patch adds a simple POD entry for the new 'set_found' method
introduced in this patchset.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2020-08-13 09:50:42 UTC
Works as expected, and with the QA follow-up, passes the QA scripts..

Signing off.

Minor niggle; It would have been nice to move C4::Circulation::_FixAccountForLostAndFound into Koha::Item at the same time - But I can do that in a followup bug.

Second minor niggle; I'm not sure about the return value of Koha::Item->set_found.. as it stands we are returning a boolean that denotes whether the set triggered a refund or not. I could see this being easily mistaken for a boolean denoting success vs failure.  This could be a good opportunity to introduce a more 'fluent interfaces' inspired design.
Comment 19 Martin Renvoize 2020-08-13 10:15:34 UTC
OK.. the more I think about this, the more little things I find.

'set_found' is a bad name, as we don't actually trigger any update of the itemlost field and so we're not actually setting the item as found.. we're simply doing a refund if required and leaving the actual update of the itemlost (and itemlost_on) field to the caller.

I'm actually starting to think this should be a trigger inside the store routine to ensure we consistently catch it any time 'itemlost' is updated.
Comment 20 Jonathan Druart 2020-08-13 16:45:39 UTC
Created attachment 108193 [details] [review]
Bug 18501: but.. what was the meaning of holdingbranch here?
Comment 21 Jonathan Druart 2020-08-13 16:50:00 UTC
Created attachment 108194 [details] [review]
Bug 18501: Add _set_found_trigger
Comment 22 Jonathan Druart 2020-08-13 16:50:04 UTC
Created attachment 108195 [details] [review]
Bug 18501: but.. what was the meaning of holdingbranch here?
Comment 23 Jonathan Druart 2020-08-13 16:51:38 UTC
Last 2 patches attached for discussion/share, not ready for testing. There are 2 FIXMEs in the first patch and a "holdingbranch" coming from... where actually?
Comment 24 Jonathan Druart 2020-08-13 16:53:23 UTC
Comment on attachment 108170 [details] [review]
Bug 18501: (QA follow-up) Add POD for Koha::Item->set_found

Obsoleting as I did not apply it and will conflict with the 2 last patches, but we should keep the POD from this patch!
Comment 25 Jonathan Druart 2020-08-14 07:55:34 UTC
(In reply to Jonathan Druart from comment #22)
> Created attachment 108195 [details] [review] [review]
> Bug 18501: but.. what was the meaning of holdingbranch here?

And also, how do we know there was a refund? We should search for the accountline, but it's not linked with the issue. Maybe we should set the issue_id for the LOST_FOUND credit?
Comment 26 Jonathan Druart 2020-08-14 07:57:01 UTC
Created attachment 108243 [details] [review]
Bug 18501: wip - need to know it has been refunded
Comment 27 Jonathan Druart 2020-08-14 08:49:28 UTC
Created attachment 108248 [details] [review]
Bug 18501: set the is_refunded flag as internal var

After discussion with Martin we decided that it could be the correct way
Comment 28 Jonathan Druart 2020-08-14 08:52:15 UTC
Created attachment 108249 [details] [review]
Bug 18501: Adjust condition for flagging an item found
Comment 29 Martin Renvoize 2020-08-14 16:22:21 UTC
Created attachment 108291 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2020-08-14 16:22:25 UTC
Created attachment 108292 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2020-08-14 16:22:29 UTC
Created attachment 108293 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2020-08-14 16:22:33 UTC
Created attachment 108294 [details] [review]
Bug 18501: Add _set_found_trigger

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2020-08-14 16:22:37 UTC
Created attachment 108295 [details] [review]
Bug 18501: but.. what was the meaning of holdingbranch here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2020-08-14 16:22:42 UTC
Created attachment 108296 [details] [review]
Bug 18501: wip - need to know it has been refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2020-08-14 16:22:47 UTC
Created attachment 108297 [details] [review]
Bug 18501: set the is_refunded flag as internal var

After discussion with Martin we decided that it could be the correct way

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2020-08-14 16:22:50 UTC
Created attachment 108298 [details] [review]
Bug 18501: Adjust condition for flagging an item found

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2020-08-14 16:22:54 UTC
Created attachment 108299 [details] [review]
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id

We correct some compilation errors here by updating the transfered
_FixAccountForLostAndFound method to use $self references.  We add
issue_id tracking so that future reports can clearly see how a refund
relates to the original charge and finally we remove a FIXME as I agree
that the 'discard_changes' call is not required.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2020-08-14 16:22:59 UTC
Created attachment 108300 [details] [review]
Bug 18501: (QA follow-up) Migrate and update unit tests

This patch moves the _FixAccountForLostAndFound tests from Circulation.t
to Koha/Items.t and updates then to call an item store as aposed to the
now removed _FixAccountForLostAndFound method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2020-08-14 16:23:03 UTC
Created attachment 108301 [details] [review]
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests

The migration of unit tests in the preceeding patch highlighted a few
regressions which are dealt with here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2020-08-14 16:24:08 UTC
Right.. we probably need to squash some of the middle patches.. but I think we're now well covered by the tests and they highlighted some issue with the approach which I have since corrected in the final follow-up.

This feels like a pretty elegant solution at this point :)
Comment 41 Jonathan Druart 2020-08-17 15:37:37 UTC
Martin, I have the feeling that the last patch embeds changes that is not directly related to the original changes. I'd like to avoid side-effects as much as possible.

I'd say there is at least one unrelated change, if we call ->store and ->itemlost is set but unchanged, then itemlost_on was updated. Now it won't.

Can you rebuild the last patch to have the minimum changes to get the tests pass?
Comment 42 Martin Renvoize 2020-08-18 11:28:14 UTC
(In reply to Jonathan Druart from comment #41)
> Martin, I have the feeling that the last patch embeds changes that is not
> directly related to the original changes. I'd like to avoid side-effects as
> much as possible.
> 
> I'd say there is at least one unrelated change, if we call ->store and
> ->itemlost is set but unchanged, then itemlost_on was updated. Now it won't.

Please explain.. I don't see this occurring and can't see how it could be in the code before or after the changeset?

> 
> Can you rebuild the last patch to have the minimum changes to get the tests
> pass?

I think it's fairly minimal as is.. I'm not sure I can reduce it any further.
Comment 43 Martin Renvoize 2020-08-18 11:35:02 UTC
Comment on attachment 108301 [details] [review]
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests

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

::: Koha/Item.pm
@@ -124,5 @@
> -          # Why not for AddItem as well?
> -            my @fields = qw( itemlost withdrawn damaged );
> -
> -            # Only retrieve the item if we need to set an "on" date field
> -            if ( $self->itemlost || $self->withdrawn || $self->damaged ) {

SO.. if any of these fields is 'true' (already in the DB or having been updated to true)...

@@ -128,5 @@
> -            if ( $self->itemlost || $self->withdrawn || $self->damaged ) {
> -                my $pre_mod_item = $self->get_from_storage;
> -                for my $field (@fields) {
> -                    if (    $self->$field
> -                        and not $pre_mod_item->$field )

Check if it is true now but wasn't true before.. and update '*_on' field..

If it was true before and is still true (changed or otherwise) then this will not trigger..

@@ +146,5 @@
> +            }
> +            # If the field has changed otherwise, we much update
> +            # the 'on' field
> +            elsif ( exists $updated_columns{$field}
> +                && $updated_columns{$field} ne $pre_mod_item->$field )

SO.. I need to change 'ne' to `&& !`
Comment 44 Martin Renvoize 2020-08-18 11:36:28 UTC
OK.. I think I see what you mean.. but I think it's the opposite of how you worded it.. 

I'm going to write a test for the behaviour anyway so we can clearly define it before I update the actual code.
Comment 45 Martin Renvoize 2020-08-18 12:34:06 UTC
Created attachment 108482 [details] [review]
Bug 18501: (QA follow-up) Add test of *_on functionality

This patch adds a test for the *_on trigger functionality to ensure
matching behaviour before and after the triggers change.
Comment 46 Martin Renvoize 2020-08-18 12:34:09 UTC
Created attachment 108483 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2020-08-18 12:34:13 UTC
Created attachment 108484 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2020-08-18 12:34:17 UTC
Created attachment 108485 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2020-08-18 12:34:21 UTC
Created attachment 108486 [details] [review]
Bug 18501: Add _set_found_trigger

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2020-08-18 12:34:25 UTC
Created attachment 108487 [details] [review]
Bug 18501: but.. what was the meaning of holdingbranch here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2020-08-18 12:34:28 UTC
Created attachment 108488 [details] [review]
Bug 18501: wip - need to know it has been refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2020-08-18 12:34:32 UTC
Created attachment 108489 [details] [review]
Bug 18501: set the is_refunded flag as internal var

After discussion with Martin we decided that it could be the correct way

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize 2020-08-18 12:34:35 UTC
Created attachment 108490 [details] [review]
Bug 18501: Adjust condition for flagging an item found

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2020-08-18 12:34:39 UTC
Created attachment 108491 [details] [review]
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id

We correct some compilation errors here by updating the transfered
_FixAccountForLostAndFound method to use $self references.  We add
issue_id tracking so that future reports can clearly see how a refund
relates to the original charge and finally we remove a FIXME as I agree
that the 'discard_changes' call is not required.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2020-08-18 12:34:43 UTC
Created attachment 108492 [details] [review]
Bug 18501: (QA follow-up) Migrate and update unit tests

This patch moves the _FixAccountForLostAndFound tests from Circulation.t
to Koha/Items.t and updates then to call an item store as aposed to the
now removed _FixAccountForLostAndFound method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2020-08-18 12:34:46 UTC
Created attachment 108493 [details] [review]
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests

The migration of unit tests in the preceeding patch highlighted a few
regressions which are dealt with here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2020-08-18 12:34:50 UTC
Created attachment 108494 [details] [review]
Bug 26210: Allow issue_id to be passed to add_credit

This patch add the ability for Koha::Account->add_credit to accept an
issue_id parameter and record it to the accountlines.
Comment 58 Martin Renvoize 2020-08-18 12:36:16 UTC
Added a unit test for the *_on trigger functionality and re-ordered the patches so you can apply that patch first and test that the test passes prior to the work in the rest of the patchset (and after too of course).

Updated the errant patch to pass the newly introduced test.
Comment 59 Jonathan Druart 2020-08-19 06:01:25 UTC
I am sorry Martin but I think there is something wrong in the order of the patches, can you fix it please?

error: sha1 information is lacking or useless (t/db_dependent/Koha/Items.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 18501: (QA follow-up) Migrate and update unit tests
Comment 60 Martin Renvoize 2020-08-19 10:41:29 UTC
Created attachment 108575 [details] [review]
Bug 18501: (QA follow-up) Add test of *_on functionality

This patch adds a test for the *_on trigger functionality to ensure
matching behaviour before and after the triggers change.
Comment 61 Martin Renvoize 2020-08-19 10:41:32 UTC
Created attachment 108576 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2020-08-19 10:41:36 UTC
Created attachment 108577 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2020-08-19 10:41:40 UTC
Created attachment 108578 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Martin Renvoize 2020-08-19 10:41:43 UTC
Created attachment 108579 [details] [review]
Bug 18501: Add _set_found_trigger

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 65 Martin Renvoize 2020-08-19 10:41:48 UTC
Created attachment 108580 [details] [review]
Bug 18501: but.. what was the meaning of holdingbranch here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 66 Martin Renvoize 2020-08-19 10:41:51 UTC
Created attachment 108581 [details] [review]
Bug 18501: wip - need to know it has been refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 67 Martin Renvoize 2020-08-19 10:41:55 UTC
Created attachment 108582 [details] [review]
Bug 18501: set the is_refunded flag as internal var

After discussion with Martin we decided that it could be the correct way

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 68 Martin Renvoize 2020-08-19 10:41:59 UTC
Created attachment 108583 [details] [review]
Bug 18501: Adjust condition for flagging an item found

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 69 Martin Renvoize 2020-08-19 10:42:03 UTC
Created attachment 108584 [details] [review]
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id

We correct some compilation errors here by updating the transfered
_FixAccountForLostAndFound method to use $self references.  We add
issue_id tracking so that future reports can clearly see how a refund
relates to the original charge and finally we remove a FIXME as I agree
that the 'discard_changes' call is not required.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 70 Martin Renvoize 2020-08-19 10:42:07 UTC
Created attachment 108585 [details] [review]
Bug 18501: (QA follow-up) Migrate and update unit tests

This patch moves the _FixAccountForLostAndFound tests from Circulation.t
to Koha/Items.t and updates then to call an item store as aposed to the
now removed _FixAccountForLostAndFound method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2020-08-19 10:42:11 UTC
Created attachment 108586 [details] [review]
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests

The migration of unit tests in the preceeding patch highlighted a few
regressions which are dealt with here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 72 Martin Renvoize 2020-08-19 10:44:47 UTC
I've rebased on master again now..

I re-ordered the patches so we could apply the final QA followup first to allow testing prior to and after applying the rest of the patchset.

Hmm.. I hate SHA1 issues.. they're always suspiciously hard to fix..

So.. as an alternative in case these still don't apply nicely I've pushed a branch up: https://gitlab.com/mrenvoize/Koha/-/tree/bug_18501
Comment 73 Martin Renvoize 2020-08-19 12:28:22 UTC
Created attachment 108596 [details] [review]
Bug 18501: (QA follow-up) Add test of *_on functionality

This patch adds a test for the *_on trigger functionality to ensure
matching behaviour before and after the triggers change.
Comment 74 Martin Renvoize 2020-08-19 12:28:26 UTC
Created attachment 108597 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Martin Renvoize 2020-08-19 12:28:30 UTC
Created attachment 108598 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2020-08-19 12:28:34 UTC
Created attachment 108599 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2020-08-19 12:28:38 UTC
Created attachment 108600 [details] [review]
Bug 18501: Add _set_found_trigger

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Martin Renvoize 2020-08-19 12:28:42 UTC
Created attachment 108601 [details] [review]
Bug 18501: but.. what was the meaning of holdingbranch here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 79 Martin Renvoize 2020-08-19 12:28:46 UTC
Created attachment 108602 [details] [review]
Bug 18501: wip - need to know it has been refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 80 Martin Renvoize 2020-08-19 12:28:50 UTC
Created attachment 108603 [details] [review]
Bug 18501: set the is_refunded flag as internal var

After discussion with Martin we decided that it could be the correct way

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2020-08-19 12:28:54 UTC
Created attachment 108604 [details] [review]
Bug 18501: Adjust condition for flagging an item found

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2020-08-19 12:28:58 UTC
Created attachment 108605 [details] [review]
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id

We correct some compilation errors here by updating the transfered
_FixAccountForLostAndFound method to use $self references.  We add
issue_id tracking so that future reports can clearly see how a refund
relates to the original charge and finally we remove a FIXME as I agree
that the 'discard_changes' call is not required.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2020-08-19 12:29:02 UTC
Created attachment 108606 [details] [review]
Bug 18501: (QA follow-up) Migrate and update unit tests

This patch moves the _FixAccountForLostAndFound tests from Circulation.t
to Koha/Items.t and updates then to call an item store as aposed to the
now removed _FixAccountForLostAndFound method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 84 Martin Renvoize 2020-08-19 12:29:06 UTC
Created attachment 108607 [details] [review]
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests

The migration of unit tests in the preceeding patch highlighted a few
regressions which are dealt with here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2020-08-19 12:29:09 UTC
Created attachment 108608 [details] [review]
Bug 26210: Allow issue_id to be passed to add_credit

This patch add the ability for Koha::Account->add_credit to accept an
issue_id parameter and record it to the accountlines.
Comment 86 Jonathan Druart 2020-08-19 12:42:04 UTC
Created attachment 108610 [details] [review]
Bug 18501: (QA follow-up) Add test of *_on functionality

This patch adds a test for the *_on trigger functionality to ensure
matching behaviour before and after the triggers change.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 87 Jonathan Druart 2020-08-19 12:42:08 UTC
Created attachment 108611 [details] [review]
Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 88 Jonathan Druart 2020-08-19 12:42:12 UTC
Created attachment 108612 [details] [review]
Bug 18501: Remove the borrowernumber parameter

It was not used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 89 Jonathan Druart 2020-08-19 12:42:17 UTC
Created attachment 108613 [details] [review]
Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 90 Jonathan Druart 2020-08-19 12:42:22 UTC
Created attachment 108614 [details] [review]
Bug 18501: Add _set_found_trigger

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 91 Jonathan Druart 2020-08-19 12:42:26 UTC
Created attachment 108615 [details] [review]
Bug 18501: but.. what was the meaning of holdingbranch here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 92 Jonathan Druart 2020-08-19 12:42:31 UTC
Created attachment 108616 [details] [review]
Bug 18501: wip - need to know it has been refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 93 Jonathan Druart 2020-08-19 12:42:36 UTC
Created attachment 108617 [details] [review]
Bug 18501: set the is_refunded flag as internal var

After discussion with Martin we decided that it could be the correct way

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 94 Jonathan Druart 2020-08-19 12:42:40 UTC
Created attachment 108618 [details] [review]
Bug 18501: Adjust condition for flagging an item found

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 95 Jonathan Druart 2020-08-19 12:42:45 UTC
Created attachment 108619 [details] [review]
Bug 18501: (QA follow-up) Remove FIXME + Track issue_id

We correct some compilation errors here by updating the transfered
_FixAccountForLostAndFound method to use $self references.  We add
issue_id tracking so that future reports can clearly see how a refund
relates to the original charge and finally we remove a FIXME as I agree
that the 'discard_changes' call is not required.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 96 Jonathan Druart 2020-08-19 12:42:50 UTC
Created attachment 108620 [details] [review]
Bug 18501: (QA follow-up) Migrate and update unit tests

This patch moves the _FixAccountForLostAndFound tests from Circulation.t
to Koha/Items.t and updates then to call an item store as aposed to the
now removed _FixAccountForLostAndFound method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 97 Jonathan Druart 2020-08-19 12:42:56 UTC
Created attachment 108621 [details] [review]
Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests

The migration of unit tests in the preceeding patch highlighted a few
regressions which are dealt with here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 98 Jonathan Druart 2020-08-20 09:49:46 UTC
Created attachment 108717 [details] [review]
Bug 18501: Fix QA issues

FAIL   Koha/Item.pm
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
   FAIL   spelling
                 Retreive ==> Retrieve
                 appropriatly ==> appropriately
                 somehwere ==> somewhere
Comment 99 Jonathan Druart 2020-08-20 10:34:18 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 100 Jonathan Druart 2020-08-20 11:24:39 UTC
We have 2 failures:

1.
#   Failed test 'ModItem tests'
#   at t/db_dependent/Items.t line 152.
Can't use an undefined value as a HASH reference at /kohadevbox/koha/Koha/Item.pm line 815.

Coming from:
 814             current_branch => C4::Context->userenv->{branch},

2.
#   Failed test 'total_outstanding() tests'
#   at t/db_dependent/Koha/Account/Lines.t line 112.
Can't call method "credit_number_enabled" on an undefined value at /kohadevbox/koha/Koha/Account/Line.pm line 890.

If it's a credit, do we require a credit_type? If so tests need adjustments.
Comment 101 Martin Renvoize 2020-08-20 11:40:45 UTC
Created attachment 108732 [details] [review]
Bug 18501: (follow-up) Remove duplicate test

The ModItem test found in t/db_dependent/Items.t was a duplciate (but
slightly less functional) for the test we introduced for the *_on
trigger functionality in t/db_dependent/Koha/Items.t::store

The failure was a missing C4::Context mock, but the logical fix is to
simply remove the duplicate test.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 102 Martin Renvoize 2020-08-20 11:46:03 UTC
The above follow-up fixes the first failure.. I think the second failure is actually introduced by 19036 so I intend on fixing it there ;)
Comment 103 Martin Renvoize 2020-08-20 14:33:39 UTC
Created attachment 108738 [details] [review]
Bug 18501: (follow-up) Remove duplicate test

The ModItem test found in t/db_dependent/Items.t was a duplciate (but
slightly less functional) for the test we introduced for the *_on
trigger functionality in t/db_dependent/Koha/Items.t::store

The failure was a missing C4::Context mock, but the logical fix is to
simply remove the duplicate test.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 104 Martin Renvoize 2020-08-20 14:33:44 UTC
Created attachment 108739 [details] [review]
Bug 18501: (follow-up) Test undefined userenv behaviour

This patch adds a test to ensure that the 'Koha::Item->store' method
will function when the userenv is not set
Comment 105 Martin Renvoize 2020-08-20 14:33:48 UTC
Created attachment 108740 [details] [review]
Bug 18501: (follow-up) Do not rely on userenv->{branchcode}
Comment 106 Martin Renvoize 2020-08-20 14:33:52 UTC
Created attachment 108741 [details] [review]
Bug 18501: (follow-up) Add fallback to ItemHomeBranch

When 'RefundLostOnReturnControl' is set to 'CheckinLibrary' but
return_branch is passed as undefined we should fallback to
'ItemHomeBranch' before falling back to the default rule.
Comment 107 Martin Renvoize 2020-08-20 14:37:04 UTC
As discussed on IRC.. we should be testing for the behaviour of an unset userenv and correctly handling it.

The first followup removes the duplicate test
The second followup adds a test for an undefined userenv
The third followup handles the undefined userenv
The fourth followup adds a test for the high fallback for when get_lostreturn_policy is called with return_branch => undef
Comment 108 Jonathan Druart 2020-08-21 08:59:37 UTC
Martin, I am getting 2 failures with the last patches

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Koha/Items.t t/db_dependent/Items.t
t/db_dependent/Koha/Items.t .. 1/11 
        #   Failed test 'damaged_on was set upon first truthy setting'
        #   at t/db_dependent/Koha/Items.t line 121.
        #          got: '2020-08-20 08:58:17'
        #     expected: '2020-08-20 08:58:16'

        #   Failed test 'damaged_on was not updated upon second truthy setting'
        #   at t/db_dependent/Koha/Items.t line 127.
        #          got: '2020-08-20 08:58:17'
        #     expected: '2020-08-20 08:58:16'
        # Looks like you failed 2 tests of 9.

    #   Failed test '*_on updates'
    #   at t/db_dependent/Koha/Items.t line 136.
    # Looks like you failed 1 test of 6.
t/db_dependent/Koha/Items.t .. 4/11 
#   Failed test 'store'
#   at t/db_dependent/Koha/Items.t line 809.
t/db_dependent/Koha/Items.t .. 9/11 # Looks like you failed 1 test of 11.
t/db_dependent/Koha/Items.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/11 subtests 
t/db_dependent/Items.t ....... 13/14 
    #   Failed test 'Non mapped field has not been reset'
    #   at t/db_dependent/Items.t line 999.
    #          got: ''
    #     expected: 'this is something'

    #   Failed test at t/db_dependent/Items.t line 1000.
    #          got: ''
    #     expected: 'this is something'
    # Looks like you failed 2 tests of 4.

#   Failed test 'ModItemFromMarc'
#   at t/db_dependent/Items.t line 1003.
# Looks like you failed 1 test of 14.
t/db_dependent/Items.t ....... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/14 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Items.t (Wstat: 256 Tests: 11 Failed: 1)
  Failed test:  4
  Non-zero exit status: 1
t/db_dependent/Items.t     (Wstat: 256 Tests: 14 Failed: 1)
  Failed test:  14
  Non-zero exit status: 1
Files=2, Tests=25, 18 wallclock secs ( 0.07 usr  0.01 sys + 12.08 cusr  3.77 csys = 15.93 CPU)
Result: FAIL


Also please use build_sample_item to create a new item (vs my $item = Koha::Item->new)
Comment 109 Martin Renvoize 2020-08-21 09:26:15 UTC
Created attachment 108784 [details] [review]
Bug 18501: (follow-up) Remove duplicate test

The ModItem test found in t/db_dependent/Items.t was a duplciate (but
slightly less functional) for the test we introduced for the *_on
trigger functionality in t/db_dependent/Koha/Items.t::store

The failure was a missing C4::Context mock, but the logical fix is to
simply remove the duplicate test.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 110 Martin Renvoize 2020-08-21 09:26:19 UTC
Created attachment 108785 [details] [review]
Bug 18501: (follow-up) Test undefined userenv behaviour

This patch adds a test to ensure that the 'Koha::Item->store' method
will function when the userenv is not set
Comment 111 Martin Renvoize 2020-08-21 09:26:23 UTC
Created attachment 108786 [details] [review]
Bug 18501: (follow-up) Do not rely on userenv->{branchcode}
Comment 112 Martin Renvoize 2020-08-21 09:26:28 UTC
Created attachment 108787 [details] [review]
Bug 18501: (follow-up) Add fallback to ItemHomeBranch

When 'RefundLostOnReturnControl' is set to 'CheckinLibrary' but
return_branch is passed as undefined we should fallback to
'ItemHomeBranch' before falling back to the default rule.
Comment 113 Martin Renvoize 2020-08-21 09:26:32 UTC
Created attachment 108788 [details] [review]
Bug 18501: (follow-up) Use t::lib::Dates::compare in tests
Comment 114 Martin Renvoize 2020-08-21 09:46:39 UTC
Comment on attachment 108614 [details] [review]
Bug 18501: Add _set_found_trigger

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

::: Koha/Item.pm
@@ +173,5 @@
> +        if ( exists $updated_columns{itemlost}
> +                and $self->itemlost != $updated_columns{itemlost}
> +                and $updated_columns{itemlost} >= 1 ) {
> +            $self->_set_found_trigger;
> +            $self->paidfor('');

This line causes the ModItemFromMarc test to fail.. I think the failure is a test issue as opposed to highlighting a problem here... we should switch the test to check for another unmapped (and untriggered) field.
Comment 115 Jonathan Druart 2020-08-21 10:04:47 UTC
Created attachment 108795 [details] [review]
Bug 18501: Don't use paidfor for DB fields not mapped with a MARC field

On "Bug 25964: Prevent data loss when editing items from a MARC record"
we picked paidfor as an example, but it has a specific behaviour and is
now handled in Koha::Item->store.
Let's pick another one, new_status
Comment 116 Jonathan Druart 2020-08-21 10:33:53 UTC
Bug 18501: Don't use paidfor for DB fields not mapped with a MARC field
Bug 18501: (follow-up) Use t::lib::Dates::compare in tests
Bug 18501: (follow-up) Add fallback to ItemHomeBranch
Bug 18501: (follow-up) Do not rely on userenv->{branchcode}
Bug 18501: (follow-up) Test undefined userenv behaviour
Bug 18501: (follow-up) Remove duplicate test

Pushed to master for 20.11
Comment 117 Lucas Gass 2020-09-04 17:19:27 UTC
this patchset does not apply cleanly to 20.05.x, if needed please rebase
Comment 118 Nick Clemens 2020-10-06 11:43:18 UTC
Created attachment 111295 [details] [review]
Bug 18501: [20.05.x] Automatic refunds need protection from failure Bug 18501: (QA follow-up) Add test of *_on functionality

This patch adds a test for the *_on trigger functionality to ensure
matching behaviour before and after the triggers change.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: Prepare the ground

This code was duplicated and we are going to need it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: Remove the borrowernumber parameter

It was not used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: Auto refund if mark as found from cataloguing

If an item is marked as found (ie. not itemlost) we need to refund the
patron.

Test plan:
Mark an item as lost to create a fee for the patron
Mark it as found from the cataloguing module and confirm that the patron
is refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: Add _set_found_trigger

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: but.. what was the meaning of holdingbranch here?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: wip - need to know it has been refunded

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: set the is_refunded flag as internal var

After discussion with Martin we decided that it could be the correct way

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: Adjust condition for flagging an item found

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: (QA follow-up) Remove FIXME + Track issue_id

We correct some compilation errors here by updating the transfered
_FixAccountForLostAndFound method to use $self references.  We add
issue_id tracking so that future reports can clearly see how a refund
relates to the original charge and finally we remove a FIXME as I agree
that the 'discard_changes' call is not required.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: (QA follow-up) Migrate and update unit tests

This patch moves the _FixAccountForLostAndFound tests from Circulation.t
to Koha/Items.t and updates then to call an item store as aposed to the
now removed _FixAccountForLostAndFound method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests

The migration of unit tests in the preceeding patch highlighted a few
regressions which are dealt with here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 18501: Fix QA issues

 FAIL   Koha/Item.pm
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
   FAIL   spelling
                 Retreive ==> Retrieve
                 appropriatly ==> appropriately
                 somehwere ==> somewhere

Bug 18501: (follow-up) Remove duplicate test

The ModItem test found in t/db_dependent/Items.t was a duplciate (but
slightly less functional) for the test we introduced for the *_on
trigger functionality in t/db_dependent/Koha/Items.t::store

The failure was a missing C4::Context mock, but the logical fix is to
simply remove the duplicate test.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 18501: (follow-up) Test undefined userenv behaviour

This patch adds a test to ensure that the 'Koha::Item->store' method
will function when the userenv is not set

Bug 18501: (follow-up) Do not rely on userenv->{branchcode}

Bug 18501: (follow-up) Add fallback to ItemHomeBranch

When 'RefundLostOnReturnControl' is set to 'CheckinLibrary' but
return_branch is passed as undefined we should fallback to
'ItemHomeBranch' before falling back to the default rule.

Bug 18501: (follow-up) Use t::lib::Dates::compare in tests

Bug 18501: Don't use paidfor for DB fields not mapped with a MARC field

On "Bug 25964: Prevent data loss when editing items from a MARC record"
we picked paidfor as an example, but it has a specific behaviour and is
now handled in Koha::Item->store.
Let's pick another one, new_status