[Bug 18501] New: Automatic refunds need protection from failure
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Bug ID: 18501 Summary: Automatic refunds need protection from failure Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: cbrannon@cdalibrary.org QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |alexbuckley@catalyst.net.nz Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |alexbuckley@catalyst.net.nz |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Can you confirm we are talking about RefundLostOnReturnControl here? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #2 from Christopher Brannon <cbrannon@cdalibrary.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|alexbuckley@catalyst.net.nz |koha-bugs@lists.koha-commun | |ity.org Status|ASSIGNED |NEW --- Comment #4 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108004 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108004&action=edit Bug 18501: Prepare the ground This code was duplicated and we are going to need it. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108005 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108005&action=edit Bug 18501: Remove the borrowernumber parameter It was not used -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108006 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108006&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |jonathan.druart@bugs.koha-c |ity.org |ommunity.org --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25663 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23091 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108004|0 |1 is obsolete| | Attachment #108005|0 |1 is obsolete| | Attachment #108006|0 |1 is obsolete| | --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108121 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108121&action=edit Bug 18501: Prepare the ground This code was duplicated and we are going to need it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108122 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108122&action=edit Bug 18501: Remove the borrowernumber parameter It was not used -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108123 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108123&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |25663 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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! Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25663 [Bug 25663] Koha::RefundLostItemFeeRules should be merged into Koha::CirculationRules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108121|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108167 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108167&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108122|0 |1 is obsolete| | --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108168 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108168&action=edit Bug 18501: Remove the borrowernumber parameter It was not used Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108123|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108169 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108169&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108170 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108170&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #18 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108193 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108193&action=edit Bug 18501: but.. what was the meaning of holdingbranch here? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108193|0 |1 is obsolete| | --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108194 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108194&action=edit Bug 18501: Add _set_found_trigger -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108195 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108195&action=edit Bug 18501: but.. what was the meaning of holdingbranch here? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108170|0 |1 is obsolete| | --- Comment #24 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 108170 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108170 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |In Discussion --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #22)
Created attachment 108195 [details] [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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #26 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108243 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108243&action=edit Bug 18501: wip - need to know it has been refunded -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108248 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108248&action=edit Bug 18501: set the is_refunded flag as internal var After discussion with Martin we decided that it could be the correct way -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108249 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108249&action=edit Bug 18501: Adjust condition for flagging an item found -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |26210 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26210 [Bug 26210] issue_id should be consistently set for Lost item handling -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108167|0 |1 is obsolete| | --- Comment #29 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108291 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108291&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108168|0 |1 is obsolete| | --- Comment #30 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108292 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108292&action=edit Bug 18501: Remove the borrowernumber parameter It was not used Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108169|0 |1 is obsolete| | --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108293 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108293&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108194|0 |1 is obsolete| | --- Comment #32 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108294 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108294&action=edit Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108195|0 |1 is obsolete| | --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108295 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108295&action=edit Bug 18501: but.. what was the meaning of holdingbranch here? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108243|0 |1 is obsolete| | --- Comment #34 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108296 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108296&action=edit Bug 18501: wip - need to know it has been refunded Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108248|0 |1 is obsolete| | --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108297 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108297&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108249|0 |1 is obsolete| | --- Comment #36 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108298 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108298&action=edit Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108299 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108299&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108300 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108300&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108301 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108301&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #40 from 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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25663 | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #41 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #42 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #43 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 108301 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108301 Bug 18501: (QA follow-up) Fix regressions highlighted by unit tests Review of attachment 108301: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=18501&attachment=108301) ----------------------------------------------------------------- ::: 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 `&& !` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #44 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #45 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108482 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108482&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108291|0 |1 is obsolete| | --- Comment #46 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108483 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108483&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108292|0 |1 is obsolete| | --- Comment #47 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108484 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108484&action=edit Bug 18501: Remove the borrowernumber parameter It was not used Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108293|0 |1 is obsolete| | --- Comment #48 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108485 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108485&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108294|0 |1 is obsolete| | --- Comment #49 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108486 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108486&action=edit Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108295|0 |1 is obsolete| | --- Comment #50 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108487 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108487&action=edit Bug 18501: but.. what was the meaning of holdingbranch here? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108296|0 |1 is obsolete| | --- Comment #51 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108488 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108488&action=edit Bug 18501: wip - need to know it has been refunded Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108297|0 |1 is obsolete| | --- Comment #52 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108489 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108489&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108298|0 |1 is obsolete| | --- Comment #53 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108490 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108490&action=edit Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108299|0 |1 is obsolete| | --- Comment #54 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108491 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108491&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108300|0 |1 is obsolete| | --- Comment #55 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108492 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108492&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108301|0 |1 is obsolete| | --- Comment #56 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108493 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108493&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #57 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108494 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108494&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108494|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #58 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #59 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108482|0 |1 is obsolete| | --- Comment #60 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108575 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108575&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108483|0 |1 is obsolete| | --- Comment #61 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108576 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108576&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108484|0 |1 is obsolete| | --- Comment #62 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108577 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108577&action=edit Bug 18501: Remove the borrowernumber parameter It was not used Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108485|0 |1 is obsolete| | --- Comment #63 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108578 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108578&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108486|0 |1 is obsolete| | --- Comment #64 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108579 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108579&action=edit Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108487|0 |1 is obsolete| | --- Comment #65 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108580 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108580&action=edit Bug 18501: but.. what was the meaning of holdingbranch here? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108488|0 |1 is obsolete| | --- Comment #66 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108581 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108581&action=edit Bug 18501: wip - need to know it has been refunded Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108489|0 |1 is obsolete| | --- Comment #67 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108582 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108582&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108490|0 |1 is obsolete| | --- Comment #68 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108583 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108583&action=edit Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108491|0 |1 is obsolete| | --- Comment #69 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108584 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108584&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108492|0 |1 is obsolete| | --- Comment #70 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108585 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108585&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108493|0 |1 is obsolete| | --- Comment #71 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108586 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108586&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off --- Comment #72 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108575|0 |1 is obsolete| | --- Comment #73 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108596 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108596&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108576|0 |1 is obsolete| | --- Comment #74 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108597 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108597&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108577|0 |1 is obsolete| | --- Comment #75 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108598 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108598&action=edit Bug 18501: Remove the borrowernumber parameter It was not used Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108578|0 |1 is obsolete| | --- Comment #76 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108599 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108599&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108579|0 |1 is obsolete| | --- Comment #77 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108600 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108600&action=edit Bug 18501: Add _set_found_trigger Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108580|0 |1 is obsolete| | --- Comment #78 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108601 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108601&action=edit Bug 18501: but.. what was the meaning of holdingbranch here? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108581|0 |1 is obsolete| | --- Comment #79 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108602 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108602&action=edit Bug 18501: wip - need to know it has been refunded Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108582|0 |1 is obsolete| | --- Comment #80 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108603 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108603&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108583|0 |1 is obsolete| | --- Comment #81 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108604 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108604&action=edit Bug 18501: Adjust condition for flagging an item found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108584|0 |1 is obsolete| | --- Comment #82 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108605 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108605&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108585|0 |1 is obsolete| | --- Comment #83 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108606 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108606&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108586|0 |1 is obsolete| | --- Comment #84 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108607 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108607&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #85 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108608 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108608&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108608|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108596|0 |1 is obsolete| | Attachment #108597|0 |1 is obsolete| | Attachment #108598|0 |1 is obsolete| | Attachment #108599|0 |1 is obsolete| | Attachment #108600|0 |1 is obsolete| | Attachment #108601|0 |1 is obsolete| | Attachment #108602|0 |1 is obsolete| | Attachment #108603|0 |1 is obsolete| | Attachment #108604|0 |1 is obsolete| | Attachment #108605|0 |1 is obsolete| | Attachment #108606|0 |1 is obsolete| | Attachment #108607|0 |1 is obsolete| | --- Comment #86 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108610&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #87 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108611 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108611&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #88 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108612 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108612&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #89 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108613 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108613&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #90 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108614&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #91 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108615 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108615&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #92 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108616 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108616&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #93 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108617 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108617&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #94 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108618 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108618&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #95 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108619 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108619&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #96 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108620 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108620&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #97 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108621 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108621&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23091 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23091 [Bug 23091] Add options to charge new or restore forgiven overdues when a lost item is returned -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #98 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108717 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108717&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #99 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 20.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #100 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #101 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108732 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108732&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #102 from 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 ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108732|0 |1 is obsolete| | --- Comment #103 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108738 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108738&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #104 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108739 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108739&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #105 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108740 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108740&action=edit Bug 18501: (follow-up) Do not rely on userenv->{branchcode} -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #106 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108741 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108741&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #107 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #108 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108738|0 |1 is obsolete| | --- Comment #109 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108784 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108784&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108739|0 |1 is obsolete| | --- Comment #110 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108785 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108785&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108740|0 |1 is obsolete| | --- Comment #111 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108786 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108786&action=edit Bug 18501: (follow-up) Do not rely on userenv->{branchcode} -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #108741|0 |1 is obsolete| | --- Comment #112 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108787 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108787&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #113 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 108788 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108788&action=edit Bug 18501: (follow-up) Use t::lib::Dates::compare in tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #114 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 108614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108614 Bug 18501: Add _set_found_trigger Review of attachment 108614: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=18501&attachment=108614) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #115 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 108795 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108795&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |26268 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26268 [Bug 26268] Remove items.paidfor once and for all -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #116 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #117 from Lucas Gass <lucas@bywatersolutions.com> --- this patchset does not apply cleanly to 20.05.x, if needed please rebase -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 --- Comment #118 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 111295 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111295&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27963 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27963 [Bug 27963] touch_all_items.pl script is not working at all -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|27963 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27963 [Bug 27963] touch_all_items.pl script is not working at all -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18501 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29221 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29221 [Bug 29221] On returns.tt modal displays wrong message when lost items are returned -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org