Bug 12225 - SIP does not respect the "no block" flag
Summary: SIP does not respect the "no block" flag
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 32516 32515 32537
  Show dependency treegraph
 
Reported: 2014-05-09 14:29 UTC by Kyle M Hall
Modified: 2023-12-28 20:43 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.06


Attachments
patch incorrect formats (2.99 KB, patch)
2015-04-02 15:36 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 12225: Fix SIP message templates (3.35 KB, patch)
2022-07-11 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12225: Re-implement no block flag for checkouts (4.48 KB, patch)
2022-07-12 11:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12225: Fix SIP message templates (3.40 KB, patch)
2022-07-12 19:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 12225: Re-implement no block flag for checkouts (4.53 KB, patch)
2022-07-12 19:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 12225: Use ProcessOfflineIssue (2.57 KB, patch)
2022-07-13 14:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12225: Use ProcessOfflineIssue (2.16 KB, patch)
2022-07-14 11:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12225: Use ProcessOfflineIssue (2.21 KB, patch)
2022-07-14 12:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 12225: Fix SIP message templates (3.46 KB, patch)
2022-08-23 15:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12225: Re-implement no block flag for checkouts (4.59 KB, patch)
2022-08-23 15:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12225: Use ProcessOfflineIssue (2.27 KB, patch)
2022-08-23 15:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12225: Add unit tests (2.03 KB, patch)
2022-08-23 16:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12225: Add unit tests (2.09 KB, patch)
2022-08-24 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12225: Fix SIP message templates (3.46 KB, patch)
2022-08-24 10:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12225: Re-implement no block flag for checkouts (4.59 KB, patch)
2022-08-24 10:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12225: Use ProcessOfflineIssue (2.27 KB, patch)
2022-08-24 10:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12225: Add unit tests (2.09 KB, patch)
2022-08-24 10:55 UTC, Martin Renvoize
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 2014-05-09 14:29:05 UTC
If a renewal via SIP would produce an error due to being on reserve, or exceeding maximum renewals, Koha's SIP2 implementation will refuse to renew the item even if the "no block" flag is set to Y.
Comment 1 Kyle M Hall 2014-05-09 14:35:54 UTC
It appears this happens for checkouts as well.
Comment 2 Colin Campbell 2015-04-02 14:55:56 UTC
There are a number of bugs meaning that Koha does not implement no_block handling although some of the ILS routines appear to handle it.

The formats parsing fixed fields are incorrect for single character flags such as no_block In the template they are defined as C which does not return a single character string but the integer value of that octet so where the code tests if ($no_block eq 'Y') that always fails because no_block == 89

If you fix the above issuing a non_block checkout calls a method checkout_no_block unfortunately no such method is present and the SIPServer instance crashes

Will try fixing these and testing if we can then get the correct behaviour
Comment 3 Colin Campbell 2015-04-02 15:36:35 UTC
Created attachment 37446 [details] [review]
patch incorrect formats

An initial patch correct the formats. But beware we dont want to deploy this until the affected handlers have been corrected, this bug has in effect been hiding a bigger one
Comment 4 Katrin Fischer 2015-05-15 21:42:27 UTC
Switching status to 'needs signoff'
Comment 5 Colin Campbell 2015-12-11 10:51:38 UTC
Reverted back to assigned as a much more extensive patch is required. For Info the no block flag is placed when the unit uploads transaction which werte recorded when the unit was offline. It flags that these Checkins, Checkouts and Renewals have occured and cannot be refused. We need to address 3 layers of bugs in the existing code.
1. Interpret the flag correctly (this was addressed by the original patch) at present the code has errors in testing the flag.
2. Supply the missing methods. The current code calls no-existent methods if ho_block is set which would cause the server to abort. The missing methods need to be supplied
3. Make the appropriate checks in those methods. Currently no checks are made that the transaction is processed correctly e.g. a no block checkin my record a return from loan at 1pm but if the book was reissued at 2pm it is an error to apply that checkin to the second issue

Working on a fuller patch to address these issues
Comment 6 Katrin Fischer 2020-01-05 14:09:01 UTC
Hi Colin, could you give an update here? Is the old patch something that could be reused or should we obsolete it?
Comment 7 Colin Campbell 2020-01-06 15:48:22 UTC
(In reply to Katrin Fischer from comment #6)
> Hi Colin, could you give an update here? Is the old patch something that
> could be reused or should we obsolete it?

think the old patch is still valid, I'll set up a test against a current system to confirm. I think some of the effects of the underlying problem are worked around by some later changes
Comment 8 Katrin Fischer 2020-01-06 15:54:09 UTC
Thx!
Comment 9 Kyle M Hall 2022-07-11 16:58:16 UTC
Created attachment 137578 [details] [review]
Bug 12225: Fix SIP message templates

If a renewal via SIP would produce an error due to being on reserve, or exceeding maximum renewals, Koha's SIP2 implementation will refuse to renew the item even if the "no block" flag is set to Y.
Comment 10 Martin Renvoize 2022-07-12 07:25:55 UTC
Do you understand the patch here Kyle.. must admit it's way over my head ;-|
Comment 11 Kyle M Hall 2022-07-12 11:51:37 UTC
Created attachment 137622 [details] [review]
Bug 12225: Re-implement no block flag for checkouts

Test Plan ( assuming Koha Testing Docker or kohadevbox ):
1) Check out master
2) Start the SIP server ( edit the SIP config koha instutution to be
   "CPL" instead of "kohalibrary" )
3) Telnet to 6001
4) Send 9300CNkoha|COkoha|CPCPL|
5) Send 11YY20220711    16350220250711    163502AOCPL|AA23529000035676|AB39999000001396|ACkoha|BON|BIN|
6) Note the due date for the checkout in Koha is not in the year 2025:
   Henry Acevedo (23529000035676) checked out Philippics. by Cicero, Marcus Tullius. 39999000001396
7) Apply this patch set
8) Restart all the things!
9) Check in the checkout
10) Repeat steps 3 through 5
11) Note the due date is now in 2025!
Comment 12 David Nind 2022-07-12 19:12:19 UTC
Created attachment 137655 [details] [review]
Bug 12225: Fix SIP message templates

If a renewal via SIP would produce an error due to being on reserve, or exceeding maximum renewals, Koha's SIP2 implementation will refuse to renew the item even if the "no block" flag is set to Y.

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2022-07-12 19:12:24 UTC
Created attachment 137656 [details] [review]
Bug 12225: Re-implement no block flag for checkouts

Test Plan ( assuming Koha Testing Docker or kohadevbox ):
1) Check out master
2) Start the SIP server ( edit the SIP config koha instutution to be
   "CPL" instead of "kohalibrary" )
3) Telnet to 6001
4) Send 9300CNkoha|COkoha|CPCPL|
5) Send 11YY20220711    16350220250711    163502AOCPL|AA23529000035676|AB39999000001396|ACkoha|BON|BIN|
6) Note the due date for the checkout in Koha is not in the year 2025:
   Henry Acevedo (23529000035676) checked out Philippics. by Cicero, Marcus Tullius. 39999000001396
7) Apply this patch set
8) Restart all the things!
9) Check in the checkout
10) Repeat steps 3 through 5
11) Note the due date is now in 2025!

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2022-07-12 19:17:01 UTC
Testing notes (using koha-testing-docker)

1. To install telnet: apt-get install telnet

2. Restart the SIP server: koha-sip --restart kohadev

3. To access SIP server using telnet: telnet 127.0.0.1 6001

4. To exit telnet: CTRL+] and then quit
Comment 15 Kyle M Hall 2022-07-13 14:31:24 UTC
Created attachment 137683 [details] [review]
Bug 12225: Use ProcessOfflineIssue
Comment 16 Kyle M Hall 2022-07-13 14:33:08 UTC
David, can you test again with the latest patch? It switches to using the offline circ checkout code. The idea behind "no block" is the patron already has it. It's basically SIP offline circ ( aka store and forward ) where the patron already has it so we'd better check it out to them! It makes sense to use the same code for both.
Comment 17 David Nind 2022-07-13 21:22:44 UTC
(In reply to Kyle M Hall from comment #16)
> David, can you test again with the latest patch? It switches to using the
> offline circ checkout code. The idea behind "no block" is the patron already
> has it. It's basically SIP offline circ ( aka store and forward ) where the
> patron already has it so we'd better check it out to them! It makes sense to
> use the same code for both.

Hi Kyle.

I have retested

When I post the command in step 5 with the patches applied and after restarting everything:
- Telnet exits saying: Connection closed by foreign host.
- The item is not checked out.

Commands entered after applying patches, flush_memcached, restart_all, and koha-sip --restart kohadev:

telnet 127.0.0.1 6001
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
9300CNkoha|COkoha|CPCPL|
941
11YY20220711    16350220250711    163502AOCPL|AA23529000035676|AB39999000001396|ACkoha|BON|BIN|
Connection closed by foreign host.

From the sip.log:

 87 [2022/07/13 21:16:28] [4660] [DEBUG] [undef]@[undef]: Configuration::find_service: Trying 6001/tcp C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 88 [2022/07/13 21:16:28] [4660] [DEBUG] [undef]@[undef]: Configuration::find_service: Trying *:6001/tcp C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 89 [2022/07/13 21:16:28] [4660] [DEBUG] [undef]@[undef]: Configuration::find_service: Trying 127.0.0.1:6001/tcp C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 90 [2022/07/13 21:16:28] [4660] [DEBUG] [undef]@[undef]: raw_transport: timeout is 60 C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 91 [2022/07/13 21:16:43] [4660] [DEBUG] [undef]@[undef]: read_request trimmed 1 character(s)  C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 92 [2022/07/13 21:16:43] [4660] [INFO] [undef]@[undef]: INPUT MSG: '9300CNkoha|COkoha|CPCPL' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 93 [2022/07/13 21:16:43] [4660] [DEBUG] [undef]@[undef]: Sip::MsgType::new('C4::SIP::Sip::MsgType', '9300CNkoha...', '93'): seq.no '0', protocol 2 C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 94 [2022/07/13 21:16:43] [4660] [DEBUG] [undef]@[undef]: Sip::MsgType::_initialize('Login', '00CNkoha|COkoha|CPCPL', 'A1A1', '2', ...) C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 95 [2022/07/13 21:16:43] [4660] [INFO] [undef]@[undef]: Successful login/auth for 'koha' of 'CPL' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 96 [2022/07/13 21:16:43] [4660] [DEBUG] [undef]@[undef]: login_core: $VAR1 = 'ILS';
 97  C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 98 [2022/07/13 21:16:43] [4660] [DEBUG] [undef]@[undef]: new ILS 'CPL' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
 99 [2022/07/13 21:16:43] [4660] [INFO] [undef]@[undef]: OUTPUT MSG: '941' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
100 [2022/07/13 21:16:43] [4660] [DEBUG] koha@127.0.0.1: raw_transport: uname/inst: 'koha/CPL' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
101 [2022/07/13 21:16:56] [4660] [DEBUG] koha@127.0.0.1: read_request trimmed 2 character(s)  C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
102 [2022/07/13 21:16:56] [4660] [INFO] koha@127.0.0.1: INPUT MSG: '11YY20220711    16350220250711    163502AOCPL|AA23529000035676|AB39999000001396|ACkoha|BON|BIN' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
103 [2022/07/13 21:16:56] [4660] [DEBUG] koha@127.0.0.1: Sip::MsgType::new('C4::SIP::Sip::MsgType', '11YY202207...', '11'): seq.no '0', protocol 2 C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
104 [2022/07/13 21:16:56] [4660] [DEBUG] koha@127.0.0.1: Sip::MsgType::_initialize('Checkout', 'YY20220711    16350220250711    163502AOCPL|AA2    3529000035676|AB39999000001396|ACkoha|BON|BIN', 'A1A1A18A18', '38', ...) C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
105 [2022/07/13 21:16:56] [4660] [WARN] koha@127.0.0.1: received no-block checkout from terminal 'koha' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
106 [2022/07/13 21:16:56] [4660] [DEBUG] koha@127.0.0.1: new ILS::Patron(23529000035676): found patron '23529000035676' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
107 [2022/07/13 21:16:56] [4660] [DEBUG] koha@127.0.0.1: new ILS::Item('39999000001396'): found with title 'Philippics.' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)
108 [2022/07/13 21:16:56] [4660] [DEBUG] koha@127.0.0.1: ILS::Transaction::Checkout performing checkout... C4::SIP::Sip::siplog /kohadevbox/koh a/C4/SIP/Sip.pm (220)
109 [2022/07/13 21:16:56] [4660] [INFO] koha@127.0.0.1: raw_transport: shutting down C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (220)

I'm assuming all three patches should be applied.
Comment 18 Kyle M Hall 2022-07-14 11:47:11 UTC
Created attachment 137712 [details] [review]
Bug 12225: Use ProcessOfflineIssue
Comment 19 Kyle M Hall 2022-07-14 11:48:30 UTC
(In reply to David Nind from comment #17)
> I have retested
> When I post the command in step 5 with the patches applied and after
> restarting everything:
> - Telnet exits saying: Connection closed by foreign host.
> - The item is not checked out.

This new version of the patch should fix that! Would you mind giving it another test David?

Thanks!
Comment 20 David Nind 2022-07-14 12:12:40 UTC
Created attachment 137713 [details] [review]
Bug 12225: Use ProcessOfflineIssue

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 Martin Renvoize 2022-08-23 15:03:11 UTC
Created attachment 139684 [details] [review]
Bug 12225: Fix SIP message templates

If a renewal via SIP would produce an error due to being on reserve, or
exceeding maximum renewals, Koha's SIP2 implementation will refuse to
renew the item even if the "no block" flag is set to Y.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-08-23 15:03:16 UTC
Created attachment 139685 [details] [review]
Bug 12225: Re-implement no block flag for checkouts

Test Plan ( assuming Koha Testing Docker or kohadevbox ):
1) Check out master
2) Start the SIP server ( edit the SIP config koha instutution to be
   "CPL" instead of "kohalibrary" )
3) Telnet to 6001
4) Send 9300CNkoha|COkoha|CPCPL|
5) Send 11YY20220711    16350220250711    163502AOCPL|AA23529000035676|AB39999000001396|ACkoha|BON|BIN|
6) Note the due date for the checkout in Koha is not in the year 2025:
   Henry Acevedo (23529000035676) checked out Philippics. by Cicero, Marcus Tullius. 39999000001396
7) Apply this patch set
8) Restart all the things!
9) Check in the checkout
10) Repeat steps 3 through 5
11) Note the due date is now in 2025!

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-08-23 15:03:20 UTC
Created attachment 139686 [details] [review]
Bug 12225: Use ProcessOfflineIssue

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2022-08-23 15:04:51 UTC
OK, this took a while to wrap my head around fully but I do agree with the changes now I have.

QA scripts are happy and existing tests are still passing.. however.. I think we should be covering this change with new tests, please.

Failing QA for lack of tests.
Comment 25 Kyle M Hall 2022-08-23 16:15:43 UTC
Created attachment 139691 [details] [review]
Bug 12225: Add unit tests
Comment 26 Martin Renvoize 2022-08-24 10:34:56 UTC
Created attachment 139706 [details] [review]
Bug 12225: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2022-08-24 10:48:08 UTC
Nice one.. PQA
Comment 28 Martin Renvoize 2022-08-24 10:55:26 UTC
Created attachment 139707 [details] [review]
Bug 12225: Fix SIP message templates

If a renewal via SIP would produce an error due to being on reserve, or
exceeding maximum renewals, Koha's SIP2 implementation will refuse to
renew the item even if the "no block" flag is set to Y.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2022-08-24 10:55:30 UTC
Created attachment 139708 [details] [review]
Bug 12225: Re-implement no block flag for checkouts

Test Plan ( assuming Koha Testing Docker or kohadevbox ):
1) Check out master
2) Start the SIP server ( edit the SIP config koha instutution to be
   "CPL" instead of "kohalibrary" )
3) Telnet to 6001
4) Send 9300CNkoha|COkoha|CPCPL|
5) Send 11YY20220711    16350220250711    163502AOCPL|AA23529000035676|AB39999000001396|ACkoha|BON|BIN|
6) Note the due date for the checkout in Koha is not in the year 2025:
   Henry Acevedo (23529000035676) checked out Philippics. by Cicero, Marcus Tullius. 39999000001396
7) Apply this patch set
8) Restart all the things!
9) Check in the checkout
10) Repeat steps 3 through 5
11) Note the due date is now in 2025!

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2022-08-24 10:55:34 UTC
Created attachment 139709 [details] [review]
Bug 12225: Use ProcessOfflineIssue

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2022-08-24 10:55:39 UTC
Created attachment 139710 [details] [review]
Bug 12225: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Tomás Cohen Arazi 2022-08-25 11:54:56 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 33 Lucas Gass 2022-10-14 22:12:43 UTC
Backported to 22.05.x for upcoming 22.05.06 release
Comment 34 Arthur Suzuki 2022-10-18 23:28:31 UTC
That one brings in conflicts plus the added tests don't pass on 21.11.x if I fix the conflicts.
Could you add backport patches for 21.11.x if that is needed?
If that is needed I could spend more time on it but I won't unless you ask.
Arthur
Comment 35 Blou 2023-01-16 21:31:03 UTC
funny.  checkout_no_block doesn't exist since forever, and this patch fixes it after all these years... but no fix for checkin_no_block.  Any 09Y call crashes and die.
Comment 36 Katrin Fischer 2023-01-16 21:51:10 UTC
(In reply to Blou from comment #35)
> funny.  checkout_no_block doesn't exist since forever, and this patch fixes
> it after all these years... but no fix for checkin_no_block.  Any 09Y call
> crashes and die.

Bug 32515 might help.