[Bug 24553] New: Cancelling hold via SIP returns a failed response even when cancellation succeeds
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Bug ID: 24553 Summary: Cancelling hold via SIP returns a failed response even when cancellation succeeds Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: SIP2 Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: colin.campbell@ptfs-europe.com 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 } -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 AspenCat Team <aspencatteam@clicweb.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aspencatteam@clicweb.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- What is the point of SIP::Patron->drop_hold? Are we maintaining a list of holds to avoid fetching them several times? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 99592 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99592&action=edit Bug 24553: Unit tests -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 99593 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99593&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Those tests are 90% a copy of the subtest "cancel_hold" from t/db_dependent/SIP/Transaction.t, that is actually testing drop_hold... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99592|0 |1 is obsolete| | --- Comment #7 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 100891 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100891&action=edit Bug 24553: Unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99593|0 |1 is obsolete| | --- Comment #8 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 100892 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100892&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100891|0 |1 is obsolete| | Attachment #100892|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 101095 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101095&action=edit Bug 24553: Unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 101096 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101096&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #101095|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 101111 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101111&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #101096|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 101112 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101112&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com Version(s)|20.05.00 |20.05.00, 19.11.05 released in| | Status|Pushed to master |Pushed to stable --- Comment #14 from Joy Nelson <joy@bywatersolutions.com> --- backported to 19.11.x for 19.11.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102518 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102518&action=edit Bug 24553: Fix tests for 19.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #16 from Joy Nelson <joy@bywatersolutions.com> --- fix for 19.11.x tests pushed to 19.11.x branch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 --- Comment #17 from Joy Nelson <joy@bywatersolutions.com> --- fix for 19.11.x tests pushed to 19.11.x branch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24553 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |28464 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28464 [Bug 28464] Cancelling a wiating hold via SIP returns a failed response even when cancellation succeeds -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org