Bug 29562 - Pass objects to CanItemBeReserved and checkHighHolds
Summary: Pass objects to CanItemBeReserved and checkHighHolds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 29058
Blocks: 29660
  Show dependency treegraph
 
Reported: 2021-11-23 19:59 UTC by Nick Clemens
Modified: 2022-06-06 20:31 UTC (History)
4 users (show)

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


Attachments
Bug 29562: Adjust CanItemBeReserved to take objects (58.62 KB, patch)
2021-11-23 20:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29562: Adjust CanItemBeReserved to take objects (58.68 KB, patch)
2022-01-10 11:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29562: (follow-up) Fix API controller (1.81 KB, patch)
2022-01-10 11:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29562: Fix DecreaseLoanHighHolds.t (5.78 KB, patch)
2022-01-10 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: Adjust CanItemBeReserved and checkHighHolds to take objects (58.70 KB, patch)
2022-01-10 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29562: (follow-up) Fix API controller (1.81 KB, patch)
2022-01-10 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29562: Fix DecreaseLoanHighHolds.t (5.83 KB, patch)
2022-01-10 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29562: Test $item instead of $itemNum (1.69 KB, patch)
2022-01-10 13:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: Adjust CanItemBeReserved and checkHighHolds to take objects (58.79 KB, patch)
2022-01-10 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: (follow-up) Fix API controller (1.88 KB, patch)
2022-01-10 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: Fix DecreaseLoanHighHolds.t (5.91 KB, patch)
2022-01-10 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: Test $item instead of $itemNum (1.76 KB, patch)
2022-01-10 13:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: Adjust CanItemBeReserved and checkHighHolds to take objects (58.83 KB, patch)
2022-01-28 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: (follow-up) Fix API controller (1.88 KB, patch)
2022-01-28 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: Fix DecreaseLoanHighHolds.t (5.91 KB, patch)
2022-01-28 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: Test $item instead of $itemNum (1.76 KB, patch)
2022-01-28 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29562: (follow-up) typo $tem in opac-reserve.pl (969 bytes, patch)
2022-02-01 01:06 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-11-23 19:59:04 UTC
CanItemBeReserved takes a borrowernumber and an itemnumber and then fetches the objects to do calculations.

Most calls to the routine already have the object, so we should be able to pass them 

CanBookBeReserved loops on every item, fetching the patron anew each time.

Changing the parameters will increase performance.
Comment 1 Nick Clemens 2021-11-23 20:01:03 UTC
Created attachment 127966 [details] [review]
Bug 29562: Adjust CanItemBeReserved to take objects

Most of the changes here are simple, this can be read to view the changes

Testing that holds can be placed via staff client, and opac, and are disallowed
when expected is the best test plan, beyond running the unit tests
Comment 2 Tomás Cohen Arazi 2022-01-07 20:14:40 UTC
I love this, but breaks the API and I couldn't easily find why.

To reproduce the failure:
1. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail with a 500.

What I found using Postman:
- $biblio_id is not always defined in the call to AddReserve, should be using $biblio->id instead
- This were the first tests we wrote for the API, and are super weird, including the DELETE FROM reserves. Maybe is a problem with the tests themselves.
Comment 3 Tomás Cohen Arazi 2022-01-10 11:10:42 UTC
Created attachment 129245 [details] [review]
Bug 29562: Adjust CanItemBeReserved to take objects

Most of the changes here are simple, this can be read to view the changes

Testing that holds can be placed via staff client, and opac, and are disallowed
when expected is the best test plan, beyond running the unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2022-01-10 11:10:47 UTC
Created attachment 129246 [details] [review]
Bug 29562: (follow-up) Fix API controller

This patch fixes tests failures due to bad checks in the controller.
The tests deserve to be rewritten.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Jonathan Druart 2022-01-10 12:39:57 UTC
Created attachment 129252 [details] [review]
Bug 29562: Fix DecreaseLoanHighHolds.t
Comment 6 Jonathan Druart 2022-01-10 12:40:26 UTC
DecreaseLoanHighHolds.t was failing, with this patch it's now passing but I am not sure the changes are correct.
Comment 7 Tomás Cohen Arazi 2022-01-10 12:45:15 UTC
(In reply to Jonathan Druart from comment #6)
> DecreaseLoanHighHolds.t was failing, with this patch it's now passing but I
> am not sure the changes are correct.

Ha! I missed that tests file! The patch looks correct.
Comment 8 Tomás Cohen Arazi 2022-01-10 12:55:21 UTC
Created attachment 129253 [details] [review]
Bug 29562: Adjust CanItemBeReserved and checkHighHolds to take objects

Most of the changes here are simple, this can be read to view the changes

Testing that holds can be placed via staff client, and opac, and are disallowed
when expected is the best test plan, beyond running the unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-01-10 12:55:26 UTC
Created attachment 129254 [details] [review]
Bug 29562: (follow-up) Fix API controller

This patch fixes tests failures due to bad checks in the controller.
The tests deserve to be rewritten.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2022-01-10 12:55:30 UTC
Created attachment 129255 [details] [review]
Bug 29562: Fix DecreaseLoanHighHolds.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Jonathan Druart 2022-01-10 13:07:11 UTC
Created attachment 129257 [details] [review]
Bug 29562: Test $item instead of $itemNum
Comment 12 Jonathan Druart 2022-01-10 13:09:41 UTC
I focused on:
1. Making sure the $item or $patron object was not modified in DB between the fetch and the CanItemBeReserved call (which could lead to wrong calculation). And haven't found any.
2. Call to CanItemBeReserved with undefined $item or $patron, that could explode. But I am not sure it would be a regression anyway.
Comment 13 Jonathan Druart 2022-01-10 13:12:52 UTC
To be consistent we should do the same change for CanBookBeReserved IMO.
Comment 14 Jonathan Druart 2022-01-10 13:20:39 UTC
Created attachment 129261 [details] [review]
Bug 29562: Adjust CanItemBeReserved and checkHighHolds to take objects

Most of the changes here are simple, this can be read to view the changes

Testing that holds can be placed via staff client, and opac, and are disallowed
when expected is the best test plan, beyond running the unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2022-01-10 13:20:43 UTC
Created attachment 129262 [details] [review]
Bug 29562: (follow-up) Fix API controller

This patch fixes tests failures due to bad checks in the controller.
The tests deserve to be rewritten.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2022-01-10 13:20:49 UTC
Created attachment 129263 [details] [review]
Bug 29562: Fix DecreaseLoanHighHolds.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2022-01-10 13:20:53 UTC
Created attachment 129264 [details] [review]
Bug 29562: Test $item instead of $itemNum

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2022-01-28 12:21:21 UTC
Created attachment 129926 [details] [review]
Bug 29562: Adjust CanItemBeReserved and checkHighHolds to take objects

Most of the changes here are simple, this can be read to view the changes

Testing that holds can be placed via staff client, and opac, and are disallowed
when expected is the best test plan, beyond running the unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2022-01-28 12:21:28 UTC
Created attachment 129927 [details] [review]
Bug 29562: (follow-up) Fix API controller

This patch fixes tests failures due to bad checks in the controller.
The tests deserve to be rewritten.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2022-01-28 12:21:37 UTC
Created attachment 129928 [details] [review]
Bug 29562: Fix DecreaseLoanHighHolds.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2022-01-28 12:21:46 UTC
Created attachment 129929 [details] [review]
Bug 29562: Test $item instead of $itemNum

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Fridolin Somers 2022-02-01 01:06:32 UTC
Awesome work !
I add a minor typo fix $tem => $item
Comment 23 Fridolin Somers 2022-02-01 01:06:49 UTC
Created attachment 130033 [details] [review]
Bug 29562: (follow-up) typo $tem in opac-reserve.pl
Comment 24 Fridolin Somers 2022-02-01 07:57:01 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄