Bug 24553

Summary: Cancelling hold via SIP returns a failed response even when cancellation succeeds
Product: Koha Reporter: Nick Clemens <nick>
Component: SIP2Assignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: 1joynelson, aspencatteam, colin.campbell, jonathan.druart, kyle, m.de.rooy, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05
Bug Depends on:    
Bug Blocks: 28464    
Attachments: Bug 24553: Unit tests
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron
Bug 24553: Unit tests
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron
Bug 24553: Unit tests
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron
Bug 24553: Unit tests
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron
Bug 24553: Fix tests for 19.11.x

Description Nick Clemens 2020-01-31 11:49:45 UTC
It looks like we have some odd code here. We delete the hold (C4::SIP::ILS::Transaction::Hold->drop_hold), then we check the patron to see if they have a hold on the item (C4::SIP::ILS::Patron->drop_hold) - of course we don't find the hold because we already cancelled it.

348     $trans->patron($patron);
349     $trans->item($item);
350     $trans->drop_hold;
351     unless ($trans->ok) {
352         $trans->screen_msg("Error with transaction drop_hold: " . $trans->screen_msg);
353         return $trans;
354     }
355     # Remove the hold from the patron's record first
356     $trans->ok($patron->drop_hold($item_id));   # different than the transaction drop!
357 
358     unless ($trans->ok) {
359         # We didn't find it on the patron record
360         $trans->screen_msg("No such hold on patron record.");
361         return $trans;
362     }
Comment 1 Jonathan Druart 2020-02-03 15:39:35 UTC
What is the point of SIP::Patron->drop_hold? Are we maintaining a list of holds to avoid fetching them several times?
Comment 2 Nick Clemens 2020-02-25 14:56:52 UTC
Created attachment 99592 [details] [review]
Bug 24553: Unit tests
Comment 3 Nick Clemens 2020-02-25 14:56:54 UTC
Created attachment 99593 [details] [review]
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron

The drop_hold routine was using the wrong parameter for item info in the patron's
array of holds. We store it as 'barcode' not 'item_id'. This is true for both waiting and unavailable
holds

To test:
1 - Apply unit tests patch
2 - prove -v t/db_dependent/SIP/ILS.t
3 - It fails
4 - Apply second patch
5 - prove -v t/db_dependent/SIP/ILS.t
6 - It passes!

You can also see bug 24175 - you cna hack sip_cli_emulator to allow cancelling a hold
and check the messages, test coverage is improved by this patch and should be sufficient
for testing
Comment 4 Nick Clemens 2020-02-25 14:57:49 UTC
(In reply to Jonathan Druart from comment #1)
> What is the point of SIP::Patron->drop_hold? Are we maintaining a list of
> holds to avoid fetching them several times?

Indeed, we are.
Comment 5 Jonathan Druart 2020-02-25 16:22:23 UTC
Those tests are 90% a copy of the subtest "cancel_hold" from t/db_dependent/SIP/Transaction.t, that is actually testing drop_hold...
Comment 6 Nick Clemens 2020-02-25 16:39:51 UTC
(In reply to Jonathan Druart from comment #5)
> Those tests are 90% a copy of the subtest "cancel_hold" from
> t/db_dependent/SIP/Transaction.t, that is actually testing drop_hold...

Yes, but cancel_hold calls drop_hold - and does other things. These tests test that all the extra stuff happens - the other tests test that function specifically.

We can move those tests here, but they cover two different modules - it seemed reasonable to cover the functions indivudally in their own files
Comment 7 Andrew Fuerste-Henry 2020-03-17 13:28:21 UTC
Created attachment 100891 [details] [review]
Bug 24553: Unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 8 Andrew Fuerste-Henry 2020-03-17 13:28:23 UTC
Created attachment 100892 [details] [review]
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron

The drop_hold routine was using the wrong parameter for item info in the patron's
array of holds. We store it as 'barcode' not 'item_id'. This is true for both waiting and unavailable
holds

To test:
1 - Apply unit tests patch
2 - prove -v t/db_dependent/SIP/ILS.t
3 - It fails
4 - Apply second patch
5 - prove -v t/db_dependent/SIP/ILS.t
6 - It passes!

You can also see bug 24175 - you cna hack sip_cli_emulator to allow cancelling a hold
and check the messages, test coverage is improved by this patch and should be sufficient
for testing

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 9 Kyle M Hall 2020-03-20 12:45:17 UTC
Created attachment 101095 [details] [review]
Bug 24553: Unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2020-03-20 12:45:27 UTC
Created attachment 101096 [details] [review]
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron

The drop_hold routine was using the wrong parameter for item info in the patron's
array of holds. We store it as 'barcode' not 'item_id'. This is true for both waiting and unavailable
holds

To test:
1 - Apply unit tests patch
2 - prove -v t/db_dependent/SIP/ILS.t
3 - It fails
4 - Apply second patch
5 - prove -v t/db_dependent/SIP/ILS.t
6 - It passes!

You can also see bug 24175 - you cna hack sip_cli_emulator to allow cancelling a hold
and check the messages, test coverage is improved by this patch and should be sufficient
for testing

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Marcel de Rooy 2020-03-20 13:42:07 UTC
Created attachment 101111 [details] [review]
Bug 24553: Unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2020-03-20 13:42:12 UTC
Created attachment 101112 [details] [review]
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron

The drop_hold routine was using the wrong parameter for item info in the patron's
array of holds. We store it as 'barcode' not 'item_id'. This is true for both waiting and unavailable
holds

To test:
1 - Apply unit tests patch
2 - prove -v t/db_dependent/SIP/ILS.t
3 - It fails
4 - Apply second patch
5 - prove -v t/db_dependent/SIP/ILS.t
6 - It passes!

You can also see bug 24175 - you cna hack sip_cli_emulator to allow cancelling a hold
and check the messages, test coverage is improved by this patch and should be sufficient
for testing

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Martin Renvoize 2020-03-20 15:43:38 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-04-03 21:22:05 UTC
backported to 19.11.x for 19.11.05
Comment 15 Jonathan Druart 2020-04-07 13:32:26 UTC
Created attachment 102518 [details] [review]
Bug 24553: Fix tests for 19.11.x
Comment 16 Joy Nelson 2020-04-15 18:41:38 UTC
fix for 19.11.x tests pushed to 19.11.x branch
Comment 17 Joy Nelson 2020-04-17 19:00:47 UTC
fix for 19.11.x tests pushed to 19.11.x branch