[Bug 26132] New: Improve readability of TooMany
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Bug ID: 26132 Summary: Improve readability of TooMany Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org Depends on: 21946 This is a follow-up of bug 21946. We need to improve the readability of C4::Circulation::TooMany to prevent regressions. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21946 [Bug 21946] Group circulation by item type -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 107734 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107734&action=edit Bug 26132: TooMany refactoring The different calls to _check_max_qty can be improved to highlight what is really changing between each of them. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 107735 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107735&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 107736 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107736&action=edit Bug 26132: Remove raw SQL query for patron_maxissueqty Same as before but for patron_maxissueqty -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 107737 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107737&action=edit Bug 26132: Don't prefetch if not needed We only need to prefetch items if CircControl is set to ItemHomeLibrary -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 107738 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107738&action=edit 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 ) { -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107734|0 |1 is obsolete| | Attachment #107735|0 |1 is obsolete| | Attachment #107736|0 |1 is obsolete| | Attachment #107737|0 |1 is obsolete| | Attachment #107738|0 |1 is obsolete| | --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 107796 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107796&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 107797 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107797&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 107798 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107798&action=edit Bug 26132: Remove raw SQL query for patron_maxissueqty Same as before but for patron_maxissueqty Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 107799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107799&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #10 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 107800 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107800&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 107801 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=107801&action=edit Bug 26132: (follow-up) Count all checkouts if no limits needed Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |RM_priority -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Version|unspecified |master QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107796|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107797|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107798|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107799|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107800|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #107801|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 108147 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108147&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 108148 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108148&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 108149 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108149&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 108150 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108150&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 108151 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108151&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 108152 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108152&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 108153 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=108153&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 20.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|RM_priority | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #20 from Lucas Gass <lucas@bywatersolutions.com> --- enhancement will not be backported to 20.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #21 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113006 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113006&action=edit 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' [...] -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #22 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113007 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113007&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Last two patches pushed to master for 20.11 Thanks Joonas! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113014 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113014&action=edit Bug 26132: (GOLF EXAMPLE) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 113034 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113034&action=edit Bug 26132: continuing code golf -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 --- Comment #26 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26132 Bug 26132 depends on bug 21946, which changed state. Bug 21946 Summary: Group circulation by item type https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21946 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org