Summary: | Issuing rules failing in 19.05 | ||
---|---|---|---|
Product: | Koha | Reporter: | Andreas Jonsson <andreas.jonsson> |
Component: | Circulation | Assignee: | Martin Renvoize (ashimema) <martin.renvoize> |
Status: | RESOLVED DUPLICATE | QA Contact: | Testopia <testopia> |
Severity: | critical | ||
Priority: | P5 - low | CC: | gmcharlt, kyle.m.hall, martin.renvoize |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Bug 23586: Correct call to CalcDateDue
Bug 23586: (follow-up) Clarify variable names |
Description
Andreas Jonsson
2019-09-11 08:47:18 UTC
Created attachment 92966 [details] [review] Bug 23586: Correct call to CalcDateDue This patch corrects the call to CalcDateDue in C4::Circulation::CanBookBeIssued correcting a regression caused by bug 20912. Created attachment 92967 [details] [review] 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. 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. What does failing mean? Can we get a test plan please? 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? 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 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. 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! (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 ;) ) 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. (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 :) *** This bug has been marked as a duplicate of bug 23382 *** |