Bug 23710 - Holds broken on intranet, displays a JSON page with an error
Summary: Holds broken on intranet, displays a JSON page with an error
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Agustín Moyano
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 19618
Blocks: 17229
  Show dependency treegraph
 
Reported: 2019-10-01 09:59 UTC by Victor Grousset/tuxayo
Modified: 2020-11-30 21:45 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 23710: Revert to usng placerequest.pl (1.13 KB, patch)
2019-10-01 12:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23710: Revert to usng placerequest.pl (1.18 KB, patch)
2019-10-01 12:43 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23710: Use API to place holds for patrons (6.86 KB, patch)
2019-10-01 18:37 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23710: Use API to place holds for patrons (6.92 KB, patch)
2019-10-01 19:21 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23710: (follow-up) Human readable error messages in request.tt, check AllowHoldPolicyOverride and AllowHoldDateInFuture in Koha::REST::V1::Holds.pm (4.16 KB, patch)
2019-10-02 14:34 UTC, Agustín Moyano
Details | Diff | Splinter Review
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 (4.15 KB, patch)
2019-10-03 17:19 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 23710: Use API to place holds for patrons (7.02 KB, patch)
2019-10-04 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23710: (follow-up) Human readable error messages in request.tt, check AllowHoldPolicyOverride and AllowHoldDateInFuture in Koha::REST::V1::Holds.pm (4.27 KB, patch)
2019-10-04 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
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 (4.25 KB, patch)
2019-10-04 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Grousset/tuxayo 2019-10-01 09:59:46 UTC
== 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.
Comment 1 Martin Renvoize 2019-10-01 12:28:35 UTC
Created attachment 93360 [details] [review]
Bug 23710: Revert to usng placerequest.pl
Comment 2 Martin Renvoize 2019-10-01 12:33:10 UTC
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.
Comment 3 Martin Renvoize 2019-10-01 12:38:03 UTC
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.
Comment 4 Owen Leonard 2019-10-01 12:43:06 UTC
Created attachment 93362 [details] [review]
Bug 23710: Revert to usng placerequest.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Agustín Moyano 2019-10-01 18:26:18 UTC
(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.
Comment 6 Agustín Moyano 2019-10-01 18:37:54 UTC
Created attachment 93386 [details] [review]
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
Comment 7 ByWater Sandboxes 2019-10-01 19:21:18 UTC
Created attachment 93387 [details] [review]
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>
Comment 8 Martin Renvoize 2019-10-02 10:27:18 UTC
Excellent work, thankyou for the updated patchset Agustín
Comment 9 Nick Clemens 2019-10-02 12:04:35 UTC
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
Comment 10 Agustín Moyano 2019-10-02 14:34:32 UTC
Created attachment 93491 [details] [review]
Bug 23710: (follow-up) Human readable error messages in request.tt, check AllowHoldPolicyOverride and AllowHoldDateInFuture in Koha::REST::V1::Holds.pm
Comment 11 Tomás Cohen Arazi 2019-10-02 21:52:18 UTC
We need tests for the behavior.
Also, we shouldn't fallback to undef. Just return bad request explaining what went wrong
Comment 12 Agustín Moyano 2019-10-03 17:19:26 UTC
Created attachment 93661 [details] [review]
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
Comment 13 Marcel de Rooy 2019-10-04 08:51:01 UTC
QA: Looking here
Comment 14 Marcel de Rooy 2019-10-04 10:00:13 UTC
(In reply to Victor Grousset/tuxayo from comment #0)
> == Test plan ==
> 1. Try to place a hold on a record

Specify doing via intranet.
Comment 15 Victor Grousset/tuxayo 2019-10-04 10:03:45 UTC
Indeed
Comment 16 Marcel de Rooy 2019-10-04 10:04:54 UTC
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.
Comment 17 Marcel de Rooy 2019-10-04 10:06:10 UTC
Created attachment 93734 [details] [review]
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>
Comment 18 Marcel de Rooy 2019-10-04 10:06:15 UTC
Created attachment 93735 [details] [review]
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>
Comment 19 Marcel de Rooy 2019-10-04 10:06:19 UTC
Created attachment 93737 [details] [review]
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>
Comment 20 Marcel de Rooy 2019-10-04 10:28:31 UTC
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..
Comment 21 Agustín Moyano 2019-10-04 14:28:08 UTC
(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!
Comment 22 Agustín Moyano 2019-10-04 14:32:19 UTC
This is what happens when you write with your pone..

Please s/don't sorry/don't worry/g

:P
Comment 23 Marcel de Rooy 2019-10-04 15:31:06 UTC
(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.
Comment 24 Agustín Moyano 2019-10-04 16:18:00 UTC
(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
Comment 25 Martin Renvoize 2019-10-07 11:54:52 UTC
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!
Comment 26 Martin Renvoize 2019-10-07 11:58:34 UTC
Nice work!

Pushed to master for 19.11.00
Comment 27 Fridolin Somers 2019-11-08 14:01:39 UTC
Depends on Bug 19618 not in 19.05.x