[Bug 8868] New: ILS-DI: CancelHold needs to take a reserve_id
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Priority: P5 - low Change sponsored?: --- Bug ID: 8868 CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Assignee: julian.maurice@biblibre.com Summary: ILS-DI: CancelHold needs to take a reserve_id Severity: enhancement Classification: Unclassified OS: All Reporter: julian.maurice@biblibre.com Hardware: All Status: ASSIGNED Version: master Component: Circulation Product: Koha CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 12657 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12657&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. Added subroutines C4::Reserves::GetReserve and C4::Reserves::CancelReserveFromId C4::ILSDI::Services::GetRecords now returns the reserve_id -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 claire.hernandez@biblibre.com <claire.hernandez@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P3 CC| |claire.hernandez@biblibre.c | |om -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Koha Team AMU <koha.aixmarseille@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |koha.aixmarseille@gmail.com --- Comment #2 from Koha Team AMU <koha.aixmarseille@gmail.com> --- Tried on a sandbox. Not possible : Something went wrong ! Applying: Bug 8868: ILS-DI: CancelHold needs to take a reserve_id Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/ILSDI/Services.pm CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm Auto-merging C4/Reserves.pm Failed to merge in the changes. Patch failed at 0001 Bug 8868: ILS-DI: CancelHold needs to take a reserve_id When you have resolved this problem run "git am --resolved". -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #12657|0 |1 is obsolete| | --- Comment #3 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 16313 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16313&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. Added subroutines C4::Reserves::GetReserve and C4::Reserves::CancelReserveFromId C4::ILSDI::Services::GetRecords now returns the reserve_id -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |paul.poulain@biblibre.com --- Comment #4 from Paul Poulain <paul.poulain@biblibre.com> --- Patch tested with a sandbox, by Leila and Sonia <koha.aixmarseille@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #16313|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #5 from Paul Poulain <paul.poulain@biblibre.com> --- Created attachment 16527 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16527&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. Added subroutines C4::Reserves::GetReserve and C4::Reserves::CancelReserveFromId C4::ILSDI::Services::GetRecords now returns the reserve_id Signed-off-by: Leila and Sonia <koha.aixmarseille@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Koha Team AMU <koha.aixmarseille@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff --- Comment #6 from Koha Team AMU <koha.aixmarseille@gmail.com> --- Test ok. Perhaps would it be better if the text of this page is changed : http://catalogue.test3.biblibre.com/cgi-bin/koha/ilsdi.pl?service=Describe&verb=CancelHold Now : Paramètres patron_id (Obligatoire) identifiant unique de l'adhérent; identifiant retourné par LookupPatron et AuthenticatePatron item_id (Obligatoire) identifiant exemplaire Exemple d'utilisation ilsdi.pl?service=CancelHold&patron_id=1&item_id=1 BUT : no more item_id but with the patch reserve_id -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Koha Team AMU <koha.aixmarseille@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |chris@bigballofwax.co.nz --- Comment #7 from Chris Cormack <chris@bigballofwax.co.nz> --- This seems to work well, 1/ but could you please do a follow up fixing the text in the ilsdi.pl?service=Describe&verb=CancelHold page 2/ Also we introduce 2 new subroutines, so could we get a unit test for each one. 3/ Finally we do this $query = qq{ INSERT INTO old_reserves SELECT * FROM reserves WHERE reserve_id = ? }; $sth = $dbh->prepare($query); $sth->execute($reserve_id); $query = qq{ DELETE FROM reserves WHERE reserve_id = ? }; $sth = $dbh->prepare($query); $sth->execute($reserve_id); It would be safer/nicer to check the result of our execute into the old_reserves before we delete it from reserves, if that insert fails we do the delete anyway at the moment Failing QA for 1 and 2, bonus points if you fix 3 also -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #8 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 18921 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18921&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReservedFromId and GetReserve - Do not delete row in reserves table if insert in old_reserves fails -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #9 from Julian Maurice <julian.maurice@biblibre.com> --- 1, 2, and 3 fixed ;) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #16527|Bug 8868: ILS-DI: |[SIGNED-OFF] Bug 8868: description|CancelHold needs to take a |ILS-DI: CancelHold needs to |reserve_id |take a reserve_id -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 I'm just a bot <gitbot@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |gitbot@bugs.koha-community. | |org --- Comment #10 from I'm just a bot <gitbot@bugs.koha-community.org> --- Applying: Bug 8868: ILS-DI: CancelHold needs to take a reserve_id Using index info to reconstruct a base tree... M C4/ILSDI/Services.pm M C4/Reserves.pm Falling back to patching base and 3-way merge... Auto-merging C4/Reserves.pm Auto-merging C4/ILSDI/Services.pm CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm Patch failed at 0001 Bug 8868: ILS-DI: CancelHold needs to take a reserve_id The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #16527|0 |1 is obsolete| | --- Comment #11 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 21337 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21337&action=edit [SIGNED-OFF] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. Added subroutines C4::Reserves::GetReserve and C4::Reserves::CancelReserveFromId C4::ILSDI::Services::GetRecords now returns the reserve_id Signed-off-by: Leila and Sonia <koha.aixmarseille@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18921|0 |1 is obsolete| | --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 21338 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21338&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReservedFromId and GetReserve - Do not delete row in reserves table if insert in old_reserves fails -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #13 from Julian Maurice <julian.maurice@biblibre.com> --- All patches rebased on master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 I'm just a bot <gitbot@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- When did the bot| |2013-09-24 last check this| | --- Comment #14 from I'm just a bot <gitbot@bugs.koha-community.org> --- Patch applied cleanly, go forth and signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 I'm just a bot <gitbot@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply When did the bot|2013-09-24 00:00:00 |2013-10-25 last check this| | --- Comment #15 from I'm just a bot <gitbot@bugs.koha-community.org> --- Applying: Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) Using index info to reconstruct a base tree... M C4/Reserves.pm M t/db_dependent/Reserves.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Reserves.t CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t Auto-merging C4/Reserves.pm Patch failed at 0001 Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21337|0 |1 is obsolete| | --- Comment #16 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 22723 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22723&action=edit [SIGNED-OFF] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id Rebased on master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21338|0 |1 is obsolete| | --- Comment #17 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 22724 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22724&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) Rebased on master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Deichman Digital Development <digitalutvikling@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22724|0 |1 is obsolete| | --- Comment #18 from Deichman Digital Development <digitalutvikling@gmail.com> --- Created attachment 24879 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=24879&action=edit Rebased against master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |benjamin.rokseth@kul.oslo.k | |ommune.no --- Comment #19 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- ilsdi.pl?service=CancelHold&patron_id=114&item_id=47 ("47" was the reserve_id of a hold) Got the following error : Can't use string ("1974687") as a HASH ref while "strict refs" in use at /usr/local/src/kohaclone/C4/Reserves.pm line 1724. points to : sub _FixPriority { my ( $params ) = @_; my $reserve_id = $params->{reserve_id}; <<< 1974687 was here the biblionumber, not the reserve_id. The hold is correctly deleted, though. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18921|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22724|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18921|0 |1 is obsolete| | Attachment #22723|0 |1 is obsolete| | Attachment #22724|0 |1 is obsolete| | Attachment #24879|0 |1 is obsolete| | --- Comment #20 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 26150 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26150&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReservedFromId and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Fixed test t/db_dependant/Reserves.t and updated return reference in CancelReserveFromId. Tested via ILS-DI : placing hold (HoldItem) checking patron info (GetPatronInfo) cancelling hold (CancelHold) Verified that CancelHold parameter item_id takes reserve_id May need additional testing and sign-off Signed-off-by: Benjamin Rokseth <bensinober@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26150|0 |1 is obsolete| | --- Comment #21 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 26271 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26271&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReservedFromId and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Benjamin Rokseth <bensinober@gmail.com> Rebased. Hopefully it applies now. Fixed test t/db_dependant/Reserves.t and updated return reference in CancelReserveFromId. Tested via ILS-DI : placing hold (HoldItem) checking patron info (GetPatronInfo) cancelling hold (CancelHold) Verified that CancelHold parameter item_id takes reserve_id May need additional testing and sign-off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #22 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: The two routines CancelReserve and CancelReserveFromId really are twins :) This seems not very necessary. Sorry but this is a blocker for me. Also a more general question: Would there be a need for some kind of check if cancelling the hold via ILS-DI is allowed? Could someone misuse this to cancel reserves of someone else? Failed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #23 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 26333 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26333&action=edit Bug 8868: Remove CancelReserveFromId CancelReserve does the same job. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #24 from Julian Maurice <julian.maurice@biblibre.com> ---
Also a more general question: Would there be a need for some kind of check if cancelling the hold via ILS-DI is allowed? Could someone misuse this to cancel reserves of someone else?
This can certainly happen as Koha only uses IP address to trust the remote user, and IP address cannot guarantee user's identity (http://en.wikipedia.org/wiki/IP_address_spoofing). But ILS-DI protocol doesn't provide any authentication mechanisms, so... what can we do? Note: RenewLoan, HoldTitle and HoldItem also allows to modify database without authentication. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26333|0 |1 is obsolete| | --- Comment #25 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 26645 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=26645&action=edit Bug 8868: Remove CancelReserveFromId CancelReserve does the same job. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #26 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Might be the two patches should be squashed into one, but I leave it for QA to decide. Regarding authentication, I personally would like to see a more united work on a RESTful API á la http://git.biblibre.com/?p=koha-restful, rather than the /svc which only addresses marc records and /ils-di which has some patron and holds calls. Perhaps it could also implement a basic session authentication, api-key, or such. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26271|0 |1 is obsolete| | --- Comment #27 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 27023 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27023&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReservedFromId and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Benjamin Rokseth <bensinober@gmail.com> Rebased. Hopefully it applies now. Fixed test t/db_dependant/Reserves.t and updated return reference in CancelReserveFromId. Tested via ILS-DI : placing hold (HoldItem) checking patron info (GetPatronInfo) cancelling hold (CancelHold) Verified that CancelHold parameter item_id takes reserve_id May need additional testing and sign-off Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #26645|0 |1 is obsolete| | --- Comment #28 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 27024 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27024&action=edit Bug 8868: Remove CancelReserveFromId CancelReserve does the same job. Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #29 from Galen Charlton <gmcharlt@gmail.com> --- The second patch is weird -- it doesn't just remove CancelReserveFromId, it has the effect of *changing* CancelReserve: - it adds a return that no callers actually check - more importantly, it removes the code to insert the old hold into old_reserves, which is a pretty clear regression. I'm marking failed QA on this basis. I strongly encourage starting the patch over from scratch; a patch that adds a routine that then gets immediately removed by its follow-up is confusing, at the very least, and risks whatever merge reconciliation failure happened here. In particular, I don't see that it is necessary to touch C4/Reserves.pm at all to implement this change; if there are concerns about CancelReserve's behavior, that should go into a separate bug. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27023|0 |1 is obsolete| | Attachment #27024|0 |1 is obsolete| | --- Comment #30 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 27771 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27771&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. - Added subroutine C4::Reserves::GetReserve - C4::ILSDI::Services::GetRecords now returns the reserve_id - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReserved and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Leila and Sonia <koha.aixmarseille@gmail.com> Signed-off-by: Benjamin Rokseth <bensinober@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #31 from Julian Maurice <julian.maurice@biblibre.com> --- Squashed all patches into one. about CancelReserve changes
- it adds a return that no callers actually check It is used in tests
more importantly, it removes the code to insert the old hold into old_reserves, which is a pretty clear regression. Are you sure ? git show -b shows me that the patch only adds an if (and the return, of course)
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #32 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Julian Maurice from comment #31)
more importantly, it removes the code to insert the old hold into old_reserves, which is a pretty clear regression. Are you sure ? git show -b shows me that the patch only adds an if (and the return, of course) It appears you are right, in the previous patch there was some code missing in CancelReserve. I don't know what happened, but the last patch I just submitted is correct.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #33 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking at this one now.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #34 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: You add a routine GetReserve to C4/Reserves.pm This is used only in C4/ILSDI/Services.pm. The returned hashref is used to check for the existence of the reserves record and to verify the borrowernumber. It seems that you could use GetReserveInfo to do exactly the same job. This would prevent changing the Reserves module, the need of a unit test, etc. So less maintenance. The only very small overhead is the join of a few tables there; imo not too high. Could you please adjust/simplify? Thanks. Failed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27771|0 |1 is obsolete| | --- Comment #35 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 27947 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27947&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. - Added subroutine C4::Reserves::GetReserve - C4::ILSDI::Services::GetRecords now returns the reserve_id - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReserved and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Leila and Sonia <koha.aixmarseille@gmail.com> Signed-off-by: Benjamin Rokseth <bensinober@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #36 from Julian Maurice <julian.maurice@biblibre.com> --- GetReserve is already in master (added by bug 9394) This patch was creating another GetReserve subroutine. I only removed the GetReserve added by this patch (but still continue to use GetReserve in C4::ILSDI::Services) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #37 from Chris Cormack <chris@bigballofwax.co.nz> --- Comment on attachment 27947 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27947 Bug 8868: ILS-DI: CancelHold needs to take a reserve_id Review of attachment 27947: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=8868&attachment=27947) ----------------------------------------------------------------- ::: C4/ILSDI/Services.pm @@ +26,4 @@
use C4::Branch; use C4::Accounts; use C4::Biblio; +use C4::Reserves qw(AddReserve GetReservesFromBiblionumber GetReservesFromBorrowernumber CanBookBeReserved CanItemBeReserved);
Why do we not add CancelReserve here @@ +744,2 @@
+ C4::Reserves::CancelReserve({reserve_id => $reserve_id});
And instead do this? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27947|0 |1 is obsolete| | --- Comment #38 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 30881 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30881&action=edit Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. - Added subroutine C4::Reserves::GetReserve - C4::ILSDI::Services::GetRecords now returns the reserve_id - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReserved and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Leila and Sonia <koha.aixmarseille@gmail.com> Signed-off-by: Benjamin Rokseth <bensinober@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signing off, while noting a style issue in the patch review -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Katrin Fischer <katrin.fischer@bsz-bw.de> 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.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #30881|0 |1 is obsolete| | --- Comment #39 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 30908 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30908&action=edit [PASSED QA] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id CancelHold takes two parameters: patron_id and item_id. If item_id is considered as an itemnumber, holds on title can't be canceled. If item_id is considered as a biblionumber, all holds on this biblionumber (for a borrower) will be canceled. So CancelHold have to consider item_id as a reserve_id. - Added subroutine C4::Reserves::GetReserve - C4::ILSDI::Services::GetRecords now returns the reserve_id - Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page - Unit tests for CancelReserved and GetReserve - Do not delete row in reserves table if insert in old_reserves fails Signed-off-by: Leila and Sonia <koha.aixmarseille@gmail.com> Signed-off-by: Benjamin Rokseth <bensinober@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signing off, while noting a style issue in the patch review Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes tests and QA script. Placed and cancelled a hold using ILS-DI successfully. Adding a follow-up to also update the ils-di documentation page in the bootstrap theme. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #40 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 30909 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30909&action=edit Bug 8868: Follow-up: Update boostrap documentation page Updates the bootstrap documentation page for CancelHold. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #41 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Note: this is a change in how the API currently works and people who are using it to cancel holds will need to change their programs. That said, I think it's still a good change - but maybe it should not go into lower versions and have a note in the release notes for 3.18? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #42 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Because it took me a bit to find it: http://old.diglib.org/architectures/ilsdi/DLF_ILS_Discovery_1.1.pdf -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 --- Comment #43 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I know it is not part of the spec - but should we maybe add the reserve_id as a new parameter to the API? This way the old behaviour could be kept (explaining that it will only work for item level holds) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8868 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #44 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patch pushed to master. Thanks Julian! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org