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.
Can you confirm we are talking about RefundLostOnReturnControl here?
(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.
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?
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
Created attachment 108004 [details] [review] Bug 18501: Prepare the ground This code was duplicated and we are going to need it.
Created attachment 108005 [details] [review] Bug 18501: Remove the borrowernumber parameter It was not used
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
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
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.
Created attachment 108121 [details] [review] Bug 18501: Prepare the ground This code was duplicated and we are going to need it.
Created attachment 108122 [details] [review] Bug 18501: Remove the borrowernumber parameter It was not used
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
(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!
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>
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>
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>
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>
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.
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.
Created attachment 108193 [details] [review] Bug 18501: but.. what was the meaning of holdingbranch here?
Created attachment 108194 [details] [review] Bug 18501: Add _set_found_trigger
Created attachment 108195 [details] [review] Bug 18501: but.. what was the meaning of holdingbranch here?
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 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!
(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?
Created attachment 108243 [details] [review] Bug 18501: wip - need to know it has been refunded
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
Created attachment 108249 [details] [review] Bug 18501: Adjust condition for flagging an item found
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>
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>
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>
Created attachment 108294 [details] [review] Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
Created attachment 108298 [details] [review] Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
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 :)
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?
(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 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 `&& !`
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.
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.
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>
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>
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>
Created attachment 108486 [details] [review] Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
Created attachment 108490 [details] [review] Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
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.
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.
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
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.
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>
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>
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>
Created attachment 108579 [details] [review] Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
Created attachment 108583 [details] [review] Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
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
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.
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>
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>
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>
Created attachment 108600 [details] [review] Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
Created attachment 108604 [details] [review] Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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
Pushed to master for 20.11, thanks to everybody involved!
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.
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>
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 ;)
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>
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
Created attachment 108740 [details] [review] Bug 18501: (follow-up) Do not rely on userenv->{branchcode}
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.
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
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)
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>
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
Created attachment 108786 [details] [review] Bug 18501: (follow-up) Do not rely on userenv->{branchcode}
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.
Created attachment 108788 [details] [review] Bug 18501: (follow-up) Use t::lib::Dates::compare in tests
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.
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
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
this patchset does not apply cleanly to 20.05.x, if needed please rebase
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