Bug 23057 - If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason
Summary: If checked_in_ok is set and item is not checked out, alert flag is supressed ...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords: RM_priority
Depends on: 15221
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-05 18:40 UTC by Kyle M Hall
Modified: 2019-07-22 17:26 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.02


Attachments
Bug 23057: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason (3.10 KB, patch)
2019-06-06 00:32 UTC, David Cook
Details | Diff | Splinter Review
Bug 23057: Unit tests (2.74 KB, patch)
2019-06-06 14:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: Unit tests (2.89 KB, patch)
2019-06-06 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: Update subroutine so tests pass (2.36 KB, patch)
2019-06-06 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: Update do_checkin (2.87 KB, patch)
2019-06-06 18:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: (QA Follow-up) [ALT PATCH] Handle new ReturnOfLostItemBlocked message in returns.pl (3.92 KB, patch)
2019-06-11 11:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: (QA Follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl (1.88 KB, patch)
2019-06-11 11:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason (3.21 KB, patch)
2019-06-11 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: Unit tests (2.99 KB, patch)
2019-06-11 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: Update do_checkin (2.97 KB, patch)
2019-06-11 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: (QA Follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl (1.98 KB, patch)
2019-06-11 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23057: If checked_in_ok is set and item is not checked out, alert flag is supressed for *any* reason (3.29 KB, patch)
2019-07-05 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23057: Unit tests (3.07 KB, patch)
2019-07-05 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23057: Update do_checkin (3.05 KB, patch)
2019-07-05 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23057: (QA follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl (2.06 KB, patch)
2019-07-05 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23057: (QA follow-up) Stay closer to old code (1.83 KB, patch)
2019-07-05 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter 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 (3.89 KB, patch)
2019-07-19 11:54 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2019-06-05 18:40:24 UTC
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.
Comment 1 David Cook 2019-06-06 00:21:48 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...
Comment 2 David Cook 2019-06-06 00:32:38 UTC
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.
Comment 3 David Cook 2019-06-06 00:33:46 UTC
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.
Comment 4 Kyle M Hall 2019-06-06 14:37:51 UTC
Created attachment 90377 [details] [review]
Bug 23057: Unit tests
Comment 5 Kyle M Hall 2019-06-06 16:43:18 UTC
Created attachment 90384 [details] [review]
Bug 23057: Unit tests
Comment 6 Kyle M Hall 2019-06-06 16:43:28 UTC
Created attachment 90385 [details] [review]
Bug 23057: Update subroutine so tests pass
Comment 7 Kyle M Hall 2019-06-06 18:04:58 UTC
Created attachment 90388 [details] [review]
Bug 23057: Update do_checkin
Comment 8 Jonathan Druart 2019-06-08 17:52:57 UTC
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?
Comment 9 Kyle M Hall 2019-06-10 16:56:47 UTC
(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.
Comment 10 Kyle M Hall 2019-06-11 11:43:17 UTC
Created attachment 90479 [details] [review]
Bug 23057: (QA Follow-up) [ALT PATCH] Handle new ReturnOfLostItemBlocked message in returns.pl
Comment 11 Kyle M Hall 2019-06-11 11:47:21 UTC
Created attachment 90480 [details] [review]
Bug 23057: (QA Follow-up) Remove new AddReturn message ReturnOfLostItemBlocked, use existing pattern from returns.pl
Comment 12 Kyle M Hall 2019-06-11 11:51:07 UTC
(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.
Comment 13 Kyle M Hall 2019-06-11 11:53:52 UTC
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!
Comment 14 Kyle M Hall 2019-06-11 16:23:01 UTC
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>
Comment 15 Kyle M Hall 2019-06-11 16:23:14 UTC
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>
Comment 16 Kyle M Hall 2019-06-11 16:23:17 UTC
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>
Comment 17 Kyle M Hall 2019-06-11 16:23:20 UTC
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>
Comment 18 Marcel de Rooy 2019-07-05 07:52:25 UTC
QA: Looking here again
Comment 19 Marcel de Rooy 2019-07-05 08:43:21 UTC
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>
Comment 20 Marcel de Rooy 2019-07-05 08:43:25 UTC
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>
Comment 21 Marcel de Rooy 2019-07-05 08:43:30 UTC
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>
Comment 22 Marcel de Rooy 2019-07-05 08:43:34 UTC
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>
Comment 23 Marcel de Rooy 2019-07-05 08:43:39 UTC
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>
Comment 24 Marcel de Rooy 2019-07-05 08:45:57 UTC
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
Comment 25 Liz Rea 2019-07-09 18:24:55 UTC
I don't understand why the last patch is necessary?
Comment 26 Kyle M Hall 2019-07-10 10:48:30 UTC
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
Comment 27 Marcel de Rooy 2019-07-10 11:05:11 UTC
(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
Comment 28 Marcel de Rooy 2019-07-10 11:07:18 UTC
(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.
Comment 29 Kyle M Hall 2019-07-10 11:22:01 UTC
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.
Comment 30 Martin Renvoize 2019-07-16 13:37:09 UTC
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 :)
Comment 31 Martin Renvoize 2019-07-16 13:40:48 UTC
Nice work!

Pushed to master for 19.11.00
Comment 32 David Cook 2019-07-19 03:50:41 UTC
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
Comment 33 David Cook 2019-07-19 03:51:23 UTC
Kyle, how are folks managing the terrors I caused with 15221?
Comment 34 Kyle M Hall 2019-07-19 11:53:50 UTC
(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.
Comment 35 Kyle M Hall 2019-07-19 11:54:51 UTC
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.
Comment 36 David Cook 2019-07-22 03:51:05 UTC
(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?
Comment 37 Kyle M Hall 2019-07-22 12:21:32 UTC
(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! :)
Comment 38 Fridolin SOMERS 2019-07-22 15:58:22 UTC
Pushed to 19.05.x for 19.05.02
Comment 39 Lucas Gass 2019-07-22 17:26:04 UTC
backported Kyle's rebase for 18.11.x for 18.11.08