Created attachment 130525 [details] [review] Bug 30085: Reduce scope of holds count / today holds count We retrieve two counts that are only needed if rules for hold limits are defined. The DB counts should only be fetched once the rules are confirmed to exist Further improvement would be possiblke by allowing them to be passed in (or cached?) from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information
Created attachment 130526 [details] [review] Bug 30085: Don't fetch ReservesControlBranch twice We essentially copy the code from GetReservesControlBranch here, because we also calculate 'branchfield' Setting it to "" vs undef makes no difference in this code, so lets not fetch this again later. Rename the variable to make it clearer where the branchcode came from
Created attachment 130527 [details] [review] Bug 30085: Consolidate querycoutn code and only check if needed Similar to first patch, move a count only conditionally used into the conditional This could be updated to use DBIC, but the itemtype conditionals add complexity
Created attachment 130528 [details] [review] Bug 30085: Move IndependentBranches check sooner This doesn't rely on the other statuses, and requires only cached preference check, so lets allow the possibiliy of an early exit
Created attachment 130829 [details] [review] Bug 30085: Reduce scope of holds count / today holds count We retrieve two counts that are only needed if rules for hold limits are defined. The DB counts should only be fetched once the rules are confirmed to exist Further improvement would be possiblke by allowing them to be passed in (or cached?) from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 130830 [details] [review] Bug 30085: Don't fetch ReservesControlBranch twice We essentially copy the code from GetReservesControlBranch here, because we also calculate 'branchfield' Setting it to "" vs undef makes no difference in this code, so lets not fetch this again later. Rename the variable to make it clearer where the branchcode came from Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 130831 [details] [review] Bug 30085: Consolidate querycoutn code and only check if needed Similar to first patch, move a count only conditionally used into the conditional This could be updated to use DBIC, but the itemtype conditionals add complexity Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 130832 [details] [review] Bug 30085: Move IndependentBranches check sooner This doesn't rely on the other statuses, and requires only cached preference check, so lets allow the possibiliy of an early exit Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Code is clearer to read and all the unit tests that call this function are still passing. I've not actually tested performance directly, but the changes all make sense to me as improvements. Signing off.
Created attachment 130856 [details] [review] Bug 30085: Reduce scope of holds count / today holds count We retrieve two counts that are only needed if rules for hold limits are defined. The DB counts should only be fetched once the rules are confirmed to exist Further improvement would be possiblke by allowing them to be passed in (or cached?) from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 130857 [details] [review] Bug 30085: Don't fetch ReservesControlBranch twice We essentially copy the code from GetReservesControlBranch here, because we also calculate 'branchfield' Setting it to "" vs undef makes no difference in this code, so lets not fetch this again later. Rename the variable to make it clearer where the branchcode came from Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 130858 [details] [review] Bug 30085: Consolidate querycoutn code and only check if needed Similar to first patch, move a count only conditionally used into the conditional This could be updated to use DBIC, but the itemtype conditionals add complexity Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 130859 [details] [review] Bug 30085: Move IndependentBranches check sooner This doesn't rely on the other statuses, and requires only cached preference check, so lets allow the possibiliy of an early exit Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Consider for backport?
(In reply to Nick Clemens from comment #15) > Consider for backport? Sure! Can you rebase against 21.11.x?
Created attachment 138058 [details] [review] Bug 30085: [21.11.x] Reduce scope of holds count / today holds count We retrieve two counts that are only needed if rules for hold limits are defined. The DB counts should only be fetched once the rules are confirmed to exist Further improvement would be possiblke by allowing them to be passed in (or cached?) from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Bug 30085: Don't fetch ReservesControlBranch twice We essentially copy the code from GetReservesControlBranch here, because we also calculate 'branchfield' Setting it to "" vs undef makes no difference in this code, so lets not fetch this again later. Rename the variable to make it clearer where the branchcode came from Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Bug 30085: Consolidate querycoutn code and only check if needed Similar to first patch, move a count only conditionally used into the conditional This could be updated to use DBIC, but the itemtype conditionals add complexity Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Bug 30085: Move IndependentBranches check sooner This doesn't rely on the other statuses, and requires only cached preference check, so lets allow the possibiliy of an early exit Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>