[Bug 23710] New: Holds broken, displays a JSON page with an error
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Bug ID: 23710 Summary: Holds broken, displays a JSON page with an error Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: blocker Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: victor.grousset@biblibre.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com == Test plan == 1. Try to place a hold on a record 2. You get an error 😱 {"errors":[{"message":"Expected object - got null.","path":"\/body"}],"status":400} It happened - on a DevBox of a colleague - a my koha-testing-docker - an instance of another colleague The sandbox throws a 404 not found and the URL is the following: http://pro.user01-koha.sandbox.biblibre.eu/api/v1/holds So definitely related. -- 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=23710 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 93360 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93360&action=edit Bug 23710: Revert to usng placerequest.pl -- 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=23710 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|NEW |Needs Signoff -- 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=23710 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This is a quick fix.. but it's made me wonder about the club holds feature more generally. Do we still need placerequest.pl if we're aiming to move to the API entirely as that patch attempted. -- 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=23710 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- To add detail to the failure, I could replicate it on Version 77.0.3865.90 (Official Build) Arch Linux (64-bit) I used a patron which should have blocked my hold but chose to hold anyway as you can. -- 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=23710 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |agustinmoyano@theke.io |ity.org | -- 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=23710 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch 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=23710 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93360|0 |1 is obsolete| | --- Comment #4 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 93362 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93362&action=edit Bug 23710: Revert to usng placerequest.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #5 from Agustín Moyano <agustinmoyano@theke.io> --- (In reply to Martin Renvoize from comment #2)
This is a quick fix.. but it's made me wonder about the club holds feature more generally.
Do we still need placerequest.pl if we're aiming to move to the API entirely as that patch attempted.
Sorry about this bug, at first I tried to move everything to the API, but then I thought it would be better to move common holds (holds for patron, not for clubs) in a following bug.. It seems I didn't Ctrl-z enough.. I'll now attach a patch that effectively uses API to place holds for patrons. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #6 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 93386 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93386&action=edit Bug 23710: Use API to place holds for patrons This patch effectively uses API to place holds for patrons. It adds a listener on submit event of the form in javascript, where it calls holds API. To test: 1. Place a hold on any biblio for a patron SUCCESS => hold is placed or rejected, but no blank page with JSON error is shown. 2. Place a multi hold for any patron SUCCESS => holds are placed or rejected, but no blank page with JSON error is shown. 3. Sign off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |agustinmoyano@theke.io -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93362|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93386|0 |1 is obsolete| | --- Comment #7 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 93387 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93387&action=edit Bug 23710: Use API to place holds for patrons This patch effectively uses API to place holds for patrons. It adds a listener on submit event of the form in javascript, where it calls holds API. To test: 1. Place a hold on any biblio for a patron SUCCESS => hold is placed or rejected, but no blank page with JSON error is shown. 2. Place a multi hold for any patron SUCCESS => holds are placed or rejected, but no blank page with JSON error is shown. 3. Sign off Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Lisette Scheer <lisetteslatah@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |lisetteslatah@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Excellent work, thankyou for the updated patchset Agustín -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Signed Off |Failed QA --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Hi Augustin, This works very well, but there are a few issues I spotted: 1 - The error messages are not translated from the return values to human readable. i.e. cannotReserveFromOtherBranches = Patron is from different library 2 - If 'AllowHoldsPolicyOverride' is enabled behvaiour can be strange To test: in Circ rules set Default Holds Policy by Item Type to 'From home library' find a patron from one library and a book with items from another attempt to place a single hold - for a single hold you get the error and cannot confirm/override do a search and attempt multiple holds, with some allowed, some not - the error message appear to pop up in yellow, but you are sent to next page and no error is indicated though the hold is not placed where it violates policy I think that is all I caught -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #10 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 93491 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93491&action=edit Bug 23710: (follow-up) Human readable error messages in request.tt, check AllowHoldPolicyOverride and AllowHoldDateInFuture in Koha::REST::V1::Holds.pm -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- We need tests for the behavior. Also, we shouldn't fallback to undef. Just return bad request explaining what went wrong -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |19618 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19618 [Bug 19618] Add 'Club Holds' feature -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17229 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17229 [Bug 17229] ILS-DI HoldTitle and HoldItem should check if patron is expired -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Agustín Moyano <agustinmoyano@theke.io> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #12 from Agustín Moyano <agustinmoyano@theke.io> --- Created attachment 93661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93661&action=edit Bug 23710: (follow-up) Add tests for new features in Koha::REST::V!::Holds::add and return error when hold date in future is not allowed and it is passed as parameter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED CC| |m.de.rooy@rijksmuseum.nl --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Victor Grousset/tuxayo from comment #0)
== Test plan == 1. Try to place a hold on a record
Specify doing via intranet. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Holds broken, displays a |Holds broken on intranet, |JSON page with an error |displays a JSON page with | |an error -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #15 from Victor Grousset/tuxayo <victor.grousset@biblibre.com> --- Indeed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Minor detail + <input type="hidden" class="datepickerto_hidden" /> hidden input without name or id ? would not recommend doing that Similarly: + $dbh->do('DELETE FROM reserves'); + + Koha::CirculationRules->set_rules( Here you are mixing the old dbi school stuff with Koha objects even next to each other. We have Koha objects for holds. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93387|0 |1 is obsolete| | --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 93734 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93734&action=edit Bug 23710: Use API to place holds for patrons This patch effectively uses API to place holds for patrons. It adds a listener on submit event of the form in javascript, where it calls holds API. To test: 1. Place a hold on any biblio for a patron SUCCESS => hold is placed or rejected, but no blank page with JSON error is shown. 2. Place a multi hold for any patron SUCCESS => holds are placed or rejected, but no blank page with JSON error is shown. 3. Sign off Signed-off-by: Lisette Scheer <lisetteslatah@gmail.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=23710 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93491|0 |1 is obsolete| | --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 93735 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93735&action=edit Bug 23710: (follow-up) Human readable error messages in request.tt, check AllowHoldPolicyOverride and AllowHoldDateInFuture in Koha::REST::V1::Holds.pm 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=23710 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #93661|0 |1 is obsolete| | --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 93737 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=93737&action=edit Bug 23710: (follow-up) Add tests for new features in Koha::REST::V!::Holds::add and return error when hold date in future is not allowed and it is passed as parameter 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=23710 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|Trivial patch |Small patch QA Contact|nick@bywatersolutions.com |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=23710 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: I will pass QA on this patch set since we want to resolve the issue introduced by the clubs feature. Need to say that we should also test stuff that makes a feature fail in various ways so that a thing like this should have been noticed in earlier testing. I am happy that we reached a point that we start using the API in code now, but I think that the communication about doing so was a bit "poor" (no offense). I would have expected more info on the mailing list or so.. Maybe it was discussed in the dev meeting, but I missed it ;) I have my doubts on the code quality in this particular case though. Which is probably a direct result of incorporating the API in old templates/scripts. I understand completely the complexity of doing this in a minimalistic way.. [1] The current reserve template on OPAC does not use API calls. The intranet template uses a mix of API calls and the old script. Solving bugs here will even be harder while holds already is a difficult area. [2] The Holds module of the REST API and especially here the add subroutine should not contain lots of "business logic" such as e.g. testing AllowHoldDateInFuture here. There is really too much code there, and again a mix of AddReserve and Koha::Holds. Again I understand that the refactoring of C4/Reserves is not complete. Performance issues etc. But in the meantime we now also "polluted" our new REST API code? Please do not consider this as hard criticism on the REST API, but I am just being concerned about the growing complexity of the codebase while we all are still working on parallel tracks.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #21 from Agustín Moyano <agustinmoyano@theke.io> --- (In reply to Marcel de Rooy from comment #20)
QA Comment: I will pass QA on this patch set since we want to resolve the issue introduced by the clubs feature. Need to say that we should also test stuff that makes a feature fail in various ways so that a thing like this should have been noticed in earlier testing.
I am happy that we reached a point that we start using the API in code now, but I think that the communication about doing so was a bit "poor" (no offense). I would have expected more info on the mailing list or so.. Maybe it was discussed in the dev meeting, but I missed it ;)
I have my doubts on the code quality in this particular case though. Which is probably a direct result of incorporating the API in old templates/scripts. I understand completely the complexity of doing this in a minimalistic way..
[1] The current reserve template on OPAC does not use API calls. The intranet template uses a mix of API calls and the old script. Solving bugs here will even be harder while holds already is a difficult area.
[2] The Holds module of the REST API and especially here the add subroutine should not contain lots of "business logic" such as e.g. testing AllowHoldDateInFuture here. There is really too much code there, and again a mix of AddReserve and Koha::Holds. Again I understand that the refactoring of C4/Reserves is not complete. Performance issues etc. But in the meantime we now also "polluted" our new REST API code?
Please do not consider this as hard criticism on the REST API, but I am just being concerned about the growing complexity of the codebase while we all are still working on parallel tracks..
Hi Marcel, first of all, thanks for testing, and don't sorry, no offence taken. On point [1] I agree completly. OPAC should use the same method as intranet, so I'll add a bug as soon as I can. On point [2], I agree that code in API modules should be as simple and clean as possible, but business logic must be checked on the "API side".. we can not rely that it will be implemented on the cliente side. Maybe this kind of logic could be implemented with helpers of mojolicious. What are your thoughts? Thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #22 from Agustín Moyano <agustinmoyano@theke.io> --- This is what happens when you write with your pone.. Please s/don't sorry/don't worry/g :P -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Agustín Moyano from comment #21)
(In reply to Marcel de Rooy from comment #20)
On point [2], I agree that code in API modules should be as simple and clean as possible, but business logic must be checked on the "API side".. we can not rely that it will be implemented on the cliente side. Maybe this kind of logic could be implemented with helpers of mojolicious. What are your thoughts?
Yes it must be checked on the API side. But iirc the idea was that the business logic should (mostly) be in the Koha objects, not duplicated on the REST API side. Because code redundancy == inconsistency && bugs. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #24 from Agustín Moyano <agustinmoyano@theke.io> --- (In reply to Marcel de Rooy from comment #23)
(In reply to Agustín Moyano from comment #21)
(In reply to Marcel de Rooy from comment #20)
On point [2], I agree that code in API modules should be as simple and clean as possible, but business logic must be checked on the "API side".. we can not rely that it will be implemented on the cliente side. Maybe this kind of logic could be implemented with helpers of mojolicious. What are your thoughts?
Yes it must be checked on the API side. But iirc the idea was that the business logic should (mostly) be in the Koha objects, not duplicated on the REST API side. Because code redundancy == inconsistency && bugs.
Yes, of course.. I thoughts it after I commented before. I'll make a new bug to move that code. Thanks again Thanks again -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Did you add a new bug to record the intention of moving some code from the API to the Koha::Objects... if so could you link to it from here please? Otherwise, many thanks for the quick fix here. A thorough treatment with quick followups.. great work! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |19.11.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=23710 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work! Pushed to master for 19.11.00 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23710 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED CC| |fridolin.somers@biblibre.co | |m --- Comment #27 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Depends on Bug 19618 not in 19.05.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org