Bug 26132 - Improve readability of TooMany
Summary: Improve readability of TooMany
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 21946
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-04 10:26 UTC by Jonathan Druart
Modified: 2020-11-05 08:22 UTC (History)
2 users (show)

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


Attachments
Bug 26132: TooMany refactoring (4.40 KB, patch)
2020-08-04 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26132: Remove raw sql query (8.77 KB, patch)
2020-08-04 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26132: Remove raw SQL query for patron_maxissueqty (4.09 KB, patch)
2020-08-04 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26132: Don't prefetch if not needed (2.74 KB, patch)
2020-08-04 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26132: Tidy _check_max_qty (4.29 KB, patch)
2020-08-04 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26132: TooMany refactoring (4.46 KB, patch)
2020-08-04 19:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26132: Remove raw sql query (8.83 KB, patch)
2020-08-04 19:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26132: Remove raw SQL query for patron_maxissueqty (4.14 KB, patch)
2020-08-04 19:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26132: Don't prefetch if not needed (2.79 KB, patch)
2020-08-04 19:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26132: Tidy _check_max_qty (4.35 KB, patch)
2020-08-04 19:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26132: (follow-up) Count all checkouts if no limits needed (1.41 KB, patch)
2020-08-04 19:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26132: TooMany refactoring (4.51 KB, patch)
2020-08-12 18:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26132: Remove raw sql query (8.88 KB, patch)
2020-08-12 18:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26132: Remove raw SQL query for patron_maxissueqty (4.20 KB, patch)
2020-08-12 18:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26132: Don't prefetch if not needed (2.85 KB, patch)
2020-08-12 18:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26132: Tidy _check_max_qty (4.40 KB, patch)
2020-08-12 18:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26132: (follow-up) Count all checkouts if no limits needed (1.46 KB, patch)
2020-08-12 18:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26132: (QA follow-up) Make tests more robust (1.61 KB, patch)
2020-08-12 18:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26132: (follow-up) Add test to show errors in max issues amount calculation (1.56 KB, patch)
2020-11-04 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26132: (follow-up) Fix broken code in TooMany function (1.46 KB, patch)
2020-11-04 15:11 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26132: (GOLF EXAMPLE) (1.97 KB, patch)
2020-11-04 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26132: continuing code golf (1.60 KB, patch)
2020-11-05 08:21 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-08-04 10:26:10 UTC
This is a follow-up of bug 21946. We need to improve the readability of C4::Circulation::TooMany to prevent regressions.
Comment 1 Jonathan Druart 2020-08-04 10:34:17 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.
Comment 2 Jonathan Druart 2020-08-04 10:34:20 UTC
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)
Comment 3 Jonathan Druart 2020-08-04 10:34:23 UTC
Created attachment 107736 [details] [review]
Bug 26132: Remove raw SQL query for patron_maxissueqty

Same as before but for patron_maxissueqty
Comment 4 Jonathan Druart 2020-08-04 10:34:26 UTC
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
Comment 5 Jonathan Druart 2020-08-04 10:34:30 UTC
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 ) {
Comment 6 Nick Clemens 2020-08-04 19:02:13 UTC
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>
Comment 7 Nick Clemens 2020-08-04 19:02:16 UTC
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>
Comment 8 Nick Clemens 2020-08-04 19:02:18 UTC
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>
Comment 9 Nick Clemens 2020-08-04 19:02:21 UTC
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>
Comment 10 Nick Clemens 2020-08-04 19:02:24 UTC
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>
Comment 11 Nick Clemens 2020-08-04 19:02:26 UTC
Created attachment 107801 [details] [review]
Bug 26132: (follow-up) Count all checkouts if no limits needed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2020-08-12 18:16:08 UTC
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>
Comment 13 Tomás Cohen Arazi 2020-08-12 18:16:15 UTC
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>
Comment 14 Tomás Cohen Arazi 2020-08-12 18:16:24 UTC
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>
Comment 15 Tomás Cohen Arazi 2020-08-12 18:16:32 UTC
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>
Comment 16 Tomás Cohen Arazi 2020-08-12 18:16:37 UTC
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>
Comment 17 Tomás Cohen Arazi 2020-08-12 18:16:46 UTC
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>
Comment 18 Tomás Cohen Arazi 2020-08-12 18:16:56 UTC
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>
Comment 19 Jonathan Druart 2020-08-13 08:19:34 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 20 Lucas Gass 2020-08-17 15:33:47 UTC
enhancement will not be backported to 20.05.x
Comment 21 Joonas Kylmälä 2020-11-04 15:11:03 UTC
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' [...]
Comment 22 Joonas Kylmälä 2020-11-04 15:11:07 UTC
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
Comment 23 Jonathan Druart 2020-11-04 16:34:46 UTC
Last two patches pushed to master for 20.11
Thanks Joonas!
Comment 24 Martin Renvoize 2020-11-04 16:53:07 UTC
Created attachment 113014 [details] [review]
Bug 26132: (GOLF EXAMPLE)
Comment 25 Jonathan Druart 2020-11-05 08:21:14 UTC
Created attachment 113034 [details] [review]
Bug 26132: continuing code golf
Comment 26 Jonathan Druart 2020-11-05 08:22:25 UTC
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.