|
Description
Kyle M Hall (khall)
2019-06-05 18:40:24 UTC
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... Created attachment 90344 [details] [review] 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. 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. Created attachment 90377 [details] [review] Bug 23057: Unit tests Created attachment 90384 [details] [review] Bug 23057: Unit tests Created attachment 90385 [details] [review] Bug 23057: Update subroutine so tests pass Created attachment 90388 [details] [review] Bug 23057: Update do_checkin 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? (In reply to Jonathan Druart from comment #8) > Comment on attachment 90388 [details] [review] [review] > Bug 23057: Update do_checkin > > Review of attachment 90388 [details] [review] [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. Created attachment 90479 [details] [review] Bug 23057: (QA Follow-up) [ALT PATCH] Handle new ReturnOfLostItemBlocked message in returns.pl Created attachment 90480 [details] [review] Bug 23057: (QA Follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl (In reply to Jonathan Druart from comment #8) > Comment on attachment 90388 [details] [review] [review] > Bug 23057: Update do_checkin > > Review of attachment 90388 [details] [review] [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. 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! Created attachment 90505 [details] [review] 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> Created attachment 90506 [details] [review] Bug 23057: Unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Created attachment 90507 [details] [review] Bug 23057: Update do_checkin Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Created attachment 90508 [details] [review] 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> QA: Looking here again Created attachment 91322 [details] [review] 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> Created attachment 91323 [details] [review] 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> Created attachment 91324 [details] [review] 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> Created attachment 91325 [details] [review] 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> Created attachment 91326 [details] [review] 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> 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 I don't understand why the last patch is necessary? 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. (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 (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 (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. 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] [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. 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 :) Nice work! Pushed to master for 19.11.00 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 Kyle, how are folks managing the terrors I caused with 15221? (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. Created attachment 91644 [details] [review] [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. (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? (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! :) Pushed to 19.05.x for 19.05.02 backported Kyle's rebase for 18.11.x for 18.11.08 |