|
Description
Jonathan Druart
2020-08-04 10:26:10 UTC
Created attachment 107734 [details] [review] Bug 26132: TooMany refactoring The different calls to _check_max_qty can be improved to highlight what is really changing between each of them. Created attachment 107735 [details] [review] Bug 26132: Remove raw sql query Making use of Koha::Checkouts make the code much more readable here. It fixes 2 flaws: * $type was not quote escaped * the effective itemtype was not used which could lead to wrong calculation (for instance item-level_itypes is set but the item does not have the itype defined) However there is something to note, we are going to make things a bit less effective as we are now fetching the items to get their effective itemtype (vs a SUM done at DB level) Created attachment 107736 [details] [review] Bug 26132: Remove raw SQL query for patron_maxissueqty Same as before but for patron_maxissueqty Created attachment 107737 [details] [review] Bug 26132: Don't prefetch if not needed We only need to prefetch items if CircControl is set to ItemHomeLibrary Created attachment 107738 [details] [review] Bug 26132: Tidy _check_max_qty This is only a perltidy of _check_max_qty to remove some space inconsistencies, like: if( $max_checkouts_allowed eq '' ){ return;} if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { Created attachment 107796 [details] [review] Bug 26132: TooMany refactoring The different calls to _check_max_qty can be improved to highlight what is really changing between each of them. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107797 [details] [review] Bug 26132: Remove raw sql query Making use of Koha::Checkouts make the code much more readable here. It fixes 2 flaws: * $type was not quote escaped * the effective itemtype was not used which could lead to wrong calculation (for instance item-level_itypes is set but the item does not have the itype defined) However there is something to note, we are going to make things a bit less effective as we are now fetching the items to get their effective itemtype (vs a SUM done at DB level) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107798 [details] [review] Bug 26132: Remove raw SQL query for patron_maxissueqty Same as before but for patron_maxissueqty Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107799 [details] [review] Bug 26132: Don't prefetch if not needed We only need to prefetch items if CircControl is set to ItemHomeLibrary Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107800 [details] [review] Bug 26132: Tidy _check_max_qty This is only a perltidy of _check_max_qty to remove some space inconsistencies, like: if( $max_checkouts_allowed eq '' ){ return;} if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107801 [details] [review] Bug 26132: (follow-up) Count all checkouts if no limits needed Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108147 [details] [review] Bug 26132: TooMany refactoring The different calls to _check_max_qty can be improved to highlight what is really changing between each of them. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 108148 [details] [review] Bug 26132: Remove raw sql query Making use of Koha::Checkouts make the code much more readable here. It fixes 2 flaws: * $type was not quote escaped * the effective itemtype was not used which could lead to wrong calculation (for instance item-level_itypes is set but the item does not have the itype defined) However there is something to note, we are going to make things a bit less effective as we are now fetching the items to get their effective itemtype (vs a SUM done at DB level) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 108149 [details] [review] Bug 26132: Remove raw SQL query for patron_maxissueqty Same as before but for patron_maxissueqty Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 108150 [details] [review] Bug 26132: Don't prefetch if not needed We only need to prefetch items if CircControl is set to ItemHomeLibrary Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 108151 [details] [review] Bug 26132: Tidy _check_max_qty This is only a perltidy of _check_max_qty to remove some space inconsistencies, like: if( $max_checkouts_allowed eq '' ){ return;} if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 108152 [details] [review] Bug 26132: (follow-up) Count all checkouts if no limits needed Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 108153 [details] [review] Bug 26132: (QA follow-up) Make tests more robust Tests fail is SearchEngine is set to Elasticsearch. This patch makes tests build 'real' biblios. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x Created attachment 113006 [details] [review] Bug 26132: (follow-up) Add test to show errors in max issues amount calculation If you set default branch limits for issue quantity circulation rules you will get errors when calling TooMany function. To reproduce the error: 1) Apply this patch 2) Run prove t/db_dependent/Circulation/TooMany.t and notice the following error: No method count found for Koha::Checkouts DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'item.homebranch' in 'where clause' [...] Created attachment 113007 [details] [review] Bug 26132: (follow-up) Fix broken code in TooMany function The $checkouts variable was not defined in the PatronLibrary case at all and in the else case the SQL join was totally missing. To test: 1) Without this patch notice prove t/db_dependent/Circulation/TooMany.t fails 2) With this patch notice prove t/db_dependent/Circulation/TooMany.t succeeds Last two patches pushed to master for 20.11 Thanks Joonas! Created attachment 113014 [details] [review] Bug 26132: (GOLF EXAMPLE) Created attachment 113034 [details] [review] Bug 26132: continuing code golf I am willing to continue and push follow-ups here. However, Martin, I think that with this approach we are going to join in all cases, which is only needed in 1 situation. I guess that's why I wrote the different search calls. |