Bug 23586 - Issuing rules failing in 19.05
Summary: Issuing rules failing in 19.05
Status: RESOLVED DUPLICATE of bug 23382
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-11 08:47 UTC by Andreas Jonsson
Modified: 2019-10-02 07:33 UTC (History)
4 users (show)

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


Attachments
Bug 23586: Correct call to CalcDateDue (1.03 KB, patch)
2019-09-19 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23586: (follow-up) Clarify variable names (4.53 KB, patch)
2019-09-19 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Jonsson 2019-09-11 08:47:18 UTC
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
Comment 1 Martin Renvoize 2019-09-19 13:43:09 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.
Comment 2 Martin Renvoize 2019-09-19 13:50:33 UTC
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.
Comment 3 Martin Renvoize 2019-09-19 14:13:25 UTC
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.
Comment 4 Katrin Fischer 2019-09-30 11:28:34 UTC
What does failing mean? Can we get a test plan please?
Comment 5 Katrin Fischer 2019-09-30 11:39:25 UTC
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?
Comment 6 Andreas Jonsson 2019-10-01 08:37:26 UTC
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
Comment 7 Andreas Jonsson 2019-10-01 08:47:15 UTC
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.
Comment 8 Katrin Fischer 2019-10-01 13:02:51 UTC
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!
Comment 9 Katrin Fischer 2019-10-01 13:06:16 UTC
(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 ;) )
Comment 10 Martin Renvoize 2019-10-01 13:23:38 UTC
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.
Comment 11 Katrin Fischer 2019-10-01 13:36:03 UTC
(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 :)
Comment 12 Martin Renvoize 2019-10-02 07:33:39 UTC

*** This bug has been marked as a duplicate of bug 23382 ***