[Bug 23057] New: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Bug ID: 23057 Summary: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: SIP2 Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: colin.campbell@ptfs-europe.com Target Milestone: --- Because of the way the alert suppression was implemented on bug 15221, the alert flag is suppressed for any and all reasons if checked_in_ok is set and the item is not checked out. This is problematic because the alert flag may be set because the item has been trapped for a hold or another reasons not related to its checked in status. Because of this, SIP AMHs ( automated material handlers iirc ) will send the items to be reshelved instead of sending them to the hold shelf. This code needs to be made aware of the reason the alert flag was triggered or not ( and probably a list of the reasons it was triggered ). That way if the only reason the flag was triggered is due to the item not being checked out, the SIP server can suppress the flag correctly, and allow it to be flagged if there are other reason to raise the alert flag. -- 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=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15221 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15221 [Bug 15221] SIP server always sets the alert flag when item not returned -- 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=23057 Martha Fuerst <mfuerst@hmcpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mfuerst@hmcpl.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #1 from David Cook <dcook@prosentient.com.au> --- Uh oh, that's no good. When I was writing the patch for 15221, I was looking at implementing the alert suppression a different way...looking at sub do_checkin in C4/SIP/ILS/Transaction/Checkin.pm, I see the following lines: my ($return, $messages, $issue, $borrower) = AddReturn($barcode, $branch, undef, dt_from_string($return_date)); $self->alert(!$return); So we should revert the patch for 15221, and add $checked_in_ok to the params for sub do_checkin. And wrap that $self->alert(!$return); within a condition looking for $checked_in_ok. Not sure why I didn't do that originally. I was going to... but then I figured I'd try to centralize the changes with the existing use of $checked_in_ok. Bad call on my part. I'm super pressed for time at the moment, but I'll do up a quick patch with what I'm thinking and post it here for review... -- 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=23057 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Created attachment 90344 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90344&action=edit Bug 23057: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason This patch fixes a bug from Bug 15221 and hopefully also makes it so that the alert flag is suppressed when checked_in_ok is set and an item is not checked out. To test: 0) Create patron in web interface with a cardnumber and userid of "staff" with a password that matches the account in SIPconfig.xml. Also set their branch to CPL (also matching SIPconfig.xml). 1) Create an item with a barcode of 'test' 2) Choose a patron to check out to and record their borrowernumber In one terminal: 3) cd to your git directory (e.g. /home/koha/koha) 4) perl ./C4/SIP/SIPServer.pm ~/koha-dev/etc/SIPconfig.xml In another terminal: 5) cd to your git directory (e.g. /home/koha/koha) 6) perl ./misc/sip_cli_emulator.pl -l CPL -su staff -sp <password> --port=6001 --address=localhost --item test -m checkin --patron <borrowernumber> NOTE: You need to replace <borrowernumber> with the borrowernumber from Step 2, and <password> with the password from Step 0. -- 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=23057 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- I haven't actually tested my patch or run the unit tests, but hopefully this is the solution. I have more urgent matters to attend to but I'll look back on this one as soon as I can. -- 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=23057 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90377 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90377&action=edit Bug 23057: Unit tests -- 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=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- 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=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90377|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90384 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90384&action=edit Bug 23057: Unit tests -- 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=23057 --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90385 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90385&action=edit Bug 23057: Update subroutine so tests pass -- 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=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90385|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90388 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90388&action=edit Bug 23057: Update do_checkin -- 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=23057 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 90388 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90388 Bug 23057: Update do_checkin Review of attachment 90388: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=23057&attachment=90388) ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +1945,4 @@
}
if ( $item->itemlost and C4::Context->preference("BlockReturnOfLostItems") ) { + $messages->{'ReturnOfLostItemBlocked'};
Should we handle it from returns.pl as well? -- 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=23057 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #8)
Comment on attachment 90388 [details] [review] Bug 23057: Update do_checkin
Review of attachment 90388 [details] [review]: -----------------------------------------------------------------
::: C4/Circulation.pm @@ +1945,4 @@
}
if ( $item->itemlost and C4::Context->preference("BlockReturnOfLostItems") ) { + $messages->{'ReturnOfLostItemBlocked'};
Should we handle it from returns.pl as well?
It would make sense. I find it odd that that is the *only* doreturn block without a message. -- 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=23057 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90479 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90479&action=edit Bug 23057: (QA Follow-up) [ALT PATCH] Handle new ReturnOfLostItemBlocked message in returns.pl -- 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=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90479|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90480 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90480&action=edit Bug 23057: (QA Follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl -- 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=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |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=23057 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #8)
Comment on attachment 90388 [details] [review] Bug 23057: Update do_checkin
Review of attachment 90388 [details] [review]: -----------------------------------------------------------------
::: C4/Circulation.pm @@ +1945,4 @@
}
if ( $item->itemlost and C4::Context->preference("BlockReturnOfLostItems") ) { + $messages->{'ReturnOfLostItemBlocked'};
Should we handle it from returns.pl as well?
So, implementing this turned out to be way more complicated than I expected. The logic in AddReturn for lost items is "messy" to put it kindly. You can find this patch in the obsolete patches on this bug. As an alternative solution with far fewer changes to the code, I've added a different followup that removes the new ReturnOfLostItemBlocked and follows the existing pattern implemented by returns.pl where the logic is implemented outside AddReturn. I find this to be technically inferior, but re-implementing the ReturnOfLostItemBlocked message properly is far outside the scope of a SIP bug report. The now obsoleted patch could definitely be re-used on a bug dedicated to this issue. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- To follow up on my last comment, I did want to say that if QA feels that my ALT patch is ok as part of this bug, please by all means use it instead of my more terse followup! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90344|0 |1 is obsolete| | Attachment #90384|0 |1 is obsolete| | Attachment #90388|0 |1 is obsolete| | Attachment #90480|0 |1 is obsolete| | --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90505 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90505&action=edit Bug 23057: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason This patch fixes a bug from Bug 15221 and hopefully also makes it so that the alert flag is suppressed when checked_in_ok is set and an item is not checked out. To test: 0) Create patron in web interface with a cardnumber and userid of "staff" with a password that matches the account in SIPconfig.xml. Also set their branch to CPL (also matching SIPconfig.xml). 1) Create an item with a barcode of 'test' 2) Choose a patron to check out to and record their borrowernumber In one terminal: 3) cd to your git directory (e.g. /home/koha/koha) 4) perl ./C4/SIP/SIPServer.pm ~/koha-dev/etc/SIPconfig.xml In another terminal: 5) cd to your git directory (e.g. /home/koha/koha) 6) perl ./misc/sip_cli_emulator.pl -l CPL -su staff -sp <password> --port=6001 --address=localhost --item test -m checkin --patron <borrowernumber> NOTE: You need to replace <borrowernumber> with the borrowernumber from Step 2, and <password> with the password from Step 0. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90506 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90506&action=edit Bug 23057: Unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90507 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90507&action=edit Bug 23057: Update do_checkin Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90508 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90508&action=edit Bug 23057: (QA Follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |RM_priority CC| |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=23057 Bug 23057 depends on bug 15221, which changed state. Bug 15221 Summary: SIP server always sets the alert flag when item not returned https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15221 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Sally Healey <sally.healey@cheshiresharedservices.gov.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sally.healey@cheshireshared | |services.gov.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here again -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90505|0 |1 is obsolete| | Attachment #90506|0 |1 is obsolete| | Attachment #90507|0 |1 is obsolete| | Attachment #90508|0 |1 is obsolete| | --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 91322 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91322&action=edit Bug 23057: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason This patch fixes a bug from Bug 15221 and hopefully also makes it so that the alert flag is suppressed when checked_in_ok is set and an item is not checked out. To test: 0) Create patron in web interface with a cardnumber and userid of "staff" with a password that matches the account in SIPconfig.xml. Also set their branch to CPL (also matching SIPconfig.xml). 1) Create an item with a barcode of 'test' 2) Choose a patron to check out to and record their borrowernumber In one terminal: 3) cd to your git directory (e.g. /home/koha/koha) 4) perl ./C4/SIP/SIPServer.pm ~/koha-dev/etc/SIPconfig.xml In another terminal: 5) cd to your git directory (e.g. /home/koha/koha) 6) perl ./misc/sip_cli_emulator.pl -l CPL -su staff -sp <password> --port=6001 --address=localhost --item test -m checkin --patron <borrowernumber> NOTE: You need to replace <borrowernumber> with the borrowernumber from Step 2, and <password> with the password from Step 0. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 91323 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91323&action=edit Bug 23057: Unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 91324 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91324&action=edit Bug 23057: Update do_checkin Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 91325 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91325&action=edit Bug 23057: (QA follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 91326 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91326&action=edit Bug 23057: (QA follow-up) Stay closer to old code Somehow it feels safer to me to alert for a !$return directly after calling AddReturn. And restore the old code for alert_type at the end. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Kyle, Please have a look at the last follow-up. And let me know if you are comfortable with it. I am at least, since we stay closer to the old code while resolving the bug. If you are too, I will pass QA. Marcel -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wizzyrea@gmail.com --- Comment #25 from Liz Rea <wizzyrea@gmail.com> --- I don't understand why the last patch is necessary? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91326|0 |1 is obsolete| | --- Comment #26 from Kyle M Hall <kyle@bywatersolutions.com> --- Comment on attachment 91326 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91326 Bug 23057: (QA follow-up) Stay closer to old code I agree with Liz of course ;) The old code is messy and confusing. I put a lot of thought into the changes I made to make the code clean, terse and understandable. I don't think we should put back bad code just for the sake of leaving it unchanged in this case. There is very real benefit to the new code consolidating logic for the benefit of future developers who have to deal with this code. (In reply to Marcel de Rooy from comment #24)
Kyle, Please have a look at the last follow-up. And let me know if you are comfortable with it. I am at least, since we stay closer to the old code while resolving the bug. If you are too, I will pass QA.
Marcel
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Liz Rea from comment #25)
I don't understand why the last patch is necessary?
Explained in the commit message. Not much more to say -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Kyle M Hall from comment #26)
Comment on attachment 91326 [details] [review] Bug 23057: (QA follow-up) Stay closer to old code
I agree with Liz of course ;)
The old code is messy and confusing. I put a lot of thought into the changes I made to make the code clean, terse and understandable. I don't think we should put back bad code just for the sake of leaving it unchanged in this case. There is very real benefit to the new code consolidating logic for the benefit of future developers who have to deal with this code.
No problem. But in that case it may not harm to have another QAer look at it too. Thanks for your feedback. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- No problem! A second pair of eyes never hurts :) (In reply to Marcel de Rooy from comment #28)
(In reply to Kyle M Hall from comment #26)
Comment on attachment 91326 [details] [review] [review] Bug 23057: (QA follow-up) Stay closer to old code
I agree with Liz of course ;)
The old code is messy and confusing. I put a lot of thought into the changes I made to make the code clean, terse and understandable. I don't think we should put back bad code just for the sake of leaving it unchanged in this case. There is very real benefit to the new code consolidating logic for the benefit of future developers who have to deal with this code.
No problem. But in that case it may not harm to have another QAer look at it too. Thanks for your feedback.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #30 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I'm happy without the final followup, the code is nice and terse and I don't feel the followup clarifies it greatly personally.. lets move forward :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |19.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=23057 --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work! Pushed to master for 19.11.00 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #32 from David Cook <dcook@prosentient.com.au> --- Hoping folks managing 19.05.x and 18.11.x see this one, as they must be bitten by https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15221 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #33 from David Cook <dcook@prosentient.com.au> --- Kyle, how are folks managing the terrors I caused with 15221? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #34 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to David Cook from comment #33)
Kyle, how are folks managing the terrors I caused with 15221?
lol, at ByWater we backported this patch to out build of Koha 18.11. I'd be happy to provide that patch for community backporting once we are sure it is stable. In fact, I'll post it right now for anyone to use. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #35 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 91644 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91644&action=edit [Koha v18.11.x] Bug 23057: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason Acknowledgements: Patch also contains original code by David Cook. Patches were squashed for ease of use. This patch fixes a bug from Bug 15221 and hopefully also makes it so that the alert flag is suppressed when checked_in_ok is set and an item is not checked out. To test: 0) Create patron in web interface with a cardnumber and userid of "staff" with a password that matches the account in SIPconfig.xml. Also set their branch to CPL (also matching SIPconfig.xml). 1) Create an item with a barcode of 'test' 2) Choose a patron to check out to and record their borrowernumber In one terminal: 3) cd to your git directory (e.g. /home/koha/koha) 4) perl ./C4/SIP/SIPServer.pm ~/koha-dev/etc/SIPconfig.xml In another terminal: 5) cd to your git directory (e.g. /home/koha/koha) 6) perl ./misc/sip_cli_emulator.pl -l CPL -su staff -sp <password> --port=6001 --address=localhost --item test -m checkin --patron <borrowernumber> NOTE: You need to replace <borrowernumber> with the borrowernumber from Step 2, and <password> with the password from Step 0. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #36 from David Cook <dcook@prosentient.com.au> --- (In reply to Kyle M Hall from comment #34)
(In reply to David Cook from comment #33)
Kyle, how are folks managing the terrors I caused with 15221?
lol, at ByWater we backported this patch to out build of Koha 18.11. I'd be happy to provide that patch for community backporting once we are sure it is stable. In fact, I'll post it right now for anyone to use.
That's good to hear. So it's already running in the wild and not causing chaos? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 --- Comment #37 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to David Cook from comment #36)
(In reply to Kyle M Hall from comment #34)
(In reply to David Cook from comment #33)
Kyle, how are folks managing the terrors I caused with 15221?
lol, at ByWater we backported this patch to out build of Koha 18.11. I'd be happy to provide that patch for community backporting once we are sure it is stable. In fact, I'll post it right now for anyone to use.
That's good to hear. So it's already running in the wild and not causing chaos?
As far as I know, that is correct! :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|19.11.00 |19.11.00,19.05.02 released in| | CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable --- Comment #38 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 19.05.x for 19.05.02 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #39 from Lucas Gass <lucas@bywatersolutions.com> --- backported Kyle's rebase for 18.11.x for 18.11.08 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23057 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|RM_priority | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org