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.
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
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.
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>
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>
Created attachment 129252 [details] [review] Bug 29562: Fix DecreaseLoanHighHolds.t
DecreaseLoanHighHolds.t was failing, with this patch it's now passing but I am not sure the changes are correct.
(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.
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>
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>
Created attachment 129255 [details] [review] Bug 29562: Fix DecreaseLoanHighHolds.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129257 [details] [review] Bug 29562: Test $item instead of $itemNum
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.
To be consistent we should do the same change for CanBookBeReserved IMO.
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>
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>
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>
Created attachment 129264 [details] [review] Bug 29562: Test $item instead of $itemNum Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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>
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>
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>
Created attachment 129929 [details] [review] Bug 29562: Test $item instead of $itemNum Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Awesome work ! I add a minor typo fix $tem => $item
Created attachment 130033 [details] [review] Bug 29562: (follow-up) typo $tem in opac-reserve.pl
Pushed to master for 22.05, thanks to everybody involved [U+1F984]