[Bug 23586] New: Issuing rules failing in 19.05
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Bug ID: 23586 Summary: Issuing rules failing in 19.05 Change sponsored?: --- Product: Koha Version: 19.05 Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: andreas.jonsson@kreablo.se QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com A Koha::Library object rather than a branchcode is passed to a database query causing the rules to never match. It looks like the bug was introduced in commit 7d71d9507202ff667776bf9cb6de0bf2603ad0dd of bug 20912: @@ -709,7 +709,7 @@ sub CanBookBeIssued { unless ( $duedate ) { my $issuedate = $now->clone(); - my $branch = _GetCircControlBranch($item_unblessed, $patron_unblessed); + my $branch = $circ_library; $duedate = CalcDateDue( $issuedate, $effective_itemtype, $branch, $patron_unblessed ); # Offline circ calls AddIssue directly, doesn't run through here -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 92966 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92966&action=edit Bug 23586: Correct call to CalcDateDue This patch corrects the call to CalcDateDue in C4::Circulation::CanBookBeIssued correcting a regression caused by bug 20912. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 92967 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92967&action=edit Bug 23586: (follow-up) Clarify variable names This patch updates the variable name '$branch' to '$branchcode' in a number of places to make it clearer what the variable contains. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- It's a little scary that this wasn't caught by any tests :(. What sort of circ rule did you have set that triggered you to find this? It may help me come up with a new test. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- What does failing mean? Can we get a test plan please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I am not sure about the second patch - If this is fixing a bug in 19.05 (not being pushed to master) - does the renaming of variables not cause some problem? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Andreas Jonsson <andreas.jonsson@kreablo.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #6 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- I think that any library that have branch specific issuing rules will be affected by this, but the problem will be visible on any Koha-installation. I propose two test plans: 1. By code inspection: * Review commit 7d71d9507202ff667776bf9cb6de0bf2603ad0dd and note that it changes the parameter "$branch" passed to CalcDateDue from the return value of _GetCircControlBranch, which is a branchcode, to $circ_library, which is a library object. * Note that CalcDateDue will eventually call CalcHardDueDate with the library object as parameter, and that CalcHardDueDate expects a branchcode which it will pass as parameter to the database query. 2. By inspecting the query log in MySQL: - Enable query-logging in MySQL by editing /etc/mysql/my.cnf and restarting the mysql server: general_log_file = /var/log/mysql/mysql.log general_log = 1 - Perform an issue and grep for 'Koha::Library=HASH(' in /var/log/mysql/mysql.log - Apply patch - sudo truncate --size=0 /var/log/mysql/mysql.log - Perform an issue and grep for 'Koha::Library=HASH(' in /var/log/mysql/mysql.log -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #7 from Andreas Jonsson <andreas.jonsson@kreablo.se> --- Since the problem is likely caused by the confusion caused by the fact that the variable name $branch is sometimes used for branchnumbers and sometimes for library objects I think that it is a good idea to change the variable names. Also, I think it would be a good idea to add to the coding standards how to name variables containing objects and object identities. These types of errors should really be caught by a type system at compile time. But in the absense of a type system, we can at least improve the situation by having a consistent variable naming scheme. The QA-team could also consider performing tests with the mysql query log enabled and querying for patterns like '=HASH(' which could catch some problems. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Thx Andreas :) Usually I'd expect something like: - Do this in Koha - Go there... click that - Verify it is broken - Apply patch - Repeat steps above - Verify it's working now! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Andreas Jonsson from comment #7)
Since the problem is likely caused by the confusion caused by the fact that the variable name $branch is sometimes used for branchnumbers and sometimes for library objects I think that it is a good idea to change the variable names.
Also, I think it would be a good idea to add to the coding standards how to name variables containing objects and object identities.
These types of errors should really be caught by a type system at compile time. But in the absense of a type system, we can at least improve the situation by having a consistent variable naming scheme.
The QA-team could also consider performing tests with the mysql query log enabled and querying for patterns like '=HASH(' which could catch some problems.
The issue is not the idea, but that this patch is marked for 19.05 - so the variable renaming would never make it into master. If we want to do that, we need to make sure there is a patch for master and a backport - it should probably be done on a separate bug report (but leaving to RM ;) ) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|19.05 |master --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This bug also applies at master to so I'm correcting the version to 'master' and will bring it to the attention of RMaints that it's required for a quick backport. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Martin Renvoize from comment #10)
This bug also applies at master to so I'm correcting the version to 'master' and will bring it to the attention of RMaints that it's required for a quick backport.
Withdrawing my complaint about variable renaming in this case :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23586 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |RESOLVED Resolution|--- |DUPLICATE --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- *** This bug has been marked as a duplicate of bug 23382 *** -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org