Bug 24553 - Cancelling hold via SIP returns a failed response even when cancellation succeeds
Summary: Cancelling hold via SIP returns a failed response even when cancellation succ...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-31 11:49 UTC by Nick Clemens
Modified: 2020-04-17 19:00 UTC (History)
7 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:
20.05.00, 19.11.05


Attachments
Bug 24553: Unit tests (3.01 KB, patch)
2020-02-25 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron (1.42 KB, patch)
2020-02-25 14:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24553: Unit tests (3.08 KB, patch)
2020-03-17 13:28 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron (1.48 KB, patch)
2020-03-17 13:28 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24553: Unit tests (3.14 KB, patch)
2020-03-20 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron (1.55 KB, patch)
2020-03-20 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24553: Unit tests (3.23 KB, patch)
2020-03-20 13:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24553: Use 'barcode' not 'item_id' when removing hold from SIP patron (1.63 KB, patch)
2020-03-20 13:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24553: Fix tests for 19.11.x (1.13 KB, patch)
2020-04-07 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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