What is the point of SIP::Patron->drop_hold? Are we maintaining a list of holds to avoid fetching them several times? Created attachment 99592 [details] [review] Bug 24553: Unit tests 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 (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. Those tests are 90% a copy of the subtest "cancel_hold" from t/db_dependent/SIP/Transaction.t, that is actually testing drop_hold... (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 Created attachment 100891 [details] [review] Bug 24553: Unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> 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> 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> 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> 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> 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> Nice work everyone! Pushed to master for 20.05 backported to 19.11.x for 19.11.05 Created attachment 102518 [details] [review] Bug 24553: Fix tests for 19.11.x fix for 19.11.x tests pushed to 19.11.x branch fix for 19.11.x tests pushed to 19.11.x branch |
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 }