Bug 5789 - Fines don't work when items have null homebranch
Summary: Fines don't work when items have null homebranch
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Barton Chittenden
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-21 20:50 UTC by Jared Camins-Esakov
Modified: 2018-10-25 17:20 UTC (History)
8 users (show)

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


Attachments
Add warning if homebranch is undefined. (2.34 KB, patch)
2018-05-06 19:57 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 5789 - Fines don't work when items have null homebranch (2.40 KB, patch)
2018-06-15 20:08 UTC, Pierre-Luc Lapointe
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2011-02-21 20:50:56 UTC
It seems that the fines cron job cannot handle databases that include items where homebranch, and possibly holdingbranch, are NULL.
Comment 1 Katrin Fischer 2013-04-13 20:32:14 UTC
This sounds like a data problem - would the right fix be to generate a warning?
Or should we print a warning at checkout already?
Comment 2 John.Seymour 2013-08-16 05:17:05 UTC
I am having the same problem with the fines cron job. I also have a problem with checking in items which have a NULL homebranch. I get the following error.
"No branchcode argument passed to Koha::Calendar->new at /usr/share/koha/lib/C4/Overdues.pm line 310". This is probably due to the fact that my database contains items which where created using a much earlier version of KOHA. A possible solution would be a script to add the missing data to the database for these old items.
Comment 3 Barton Chittenden 2014-05-28 20:10:57 UTC
Fines.pl crashes with the following error messages when processing an item that has a NULL homebranch:

Use of uninitialized value $branchcode in exists at /home/koha/kohaclone/misc/cronjobs/fines.pl line 110.
No branchcode argument passed to Koha::Calendar->new at /home/koha/kohaclone/misc/cronjobs/fines.pl line 169.

Here is the offending code at line 110:

109 # In final case, CircControl must be PickupLibrary. (branchcode comes from issues table here).
110     if ( !exists $is_holiday{$branchcode} ) {
111         $is_holiday{$branchcode} = set_holiday( $branchcode, $today );
112     }

line 169:

166 sub set_holiday {
167     my ( $branch, $dt ) = @_;
168 
169     my $calendar = Koha::Calendar->new( branchcode => $branch );
170     return $calendar->is_holiday($dt);
171 }

$branchcode is set here:

104     my $branchcode =
105         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch}
106       : ( $control eq 'PatronLibrary' )   ? $borrower->{branchcode}
107       :                                     $overdue->{branchcode};
108 

There should be error checking at the following points: 

after $branchcode is set at line 104 -- if $branchcode is not set, the current item is not valid. A warning should be emitted, and further processing for this record should be skipped.

in Koha::Calendar->new() -- If the value of 'branchcode' is not set, a warning should be emitted, and the return value should be undef.

after $is_holiday{$branchcode} is set at line 111, it should be tested for 'defined'. If it is undefined, The value should be set to "''", so that !exists $is_holiday{$branchcode} will test correctly on future iterations.

When testing, make sure that the 'CircControl' system preference is set to 
Use the checkout and fines rules of the library the item is from, otherwise the borrower's branchcode may mask the bug.
Comment 4 Saiful Amin 2018-05-06 13:45:53 UTC
This is not a data issue but a bug in the code. Here are the lines:

109     my $branchcode =
110         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch}
111       : ( $control eq 'PatronLibrary' )   ? $borrower->{branchcode}
112       :                                     $overdue->{branchcode};
113

The variable $overdue->{homebranch} is bad because the 'issues' table has no column called 'homebranch'. It should be 'branchcode' instead. I'm able to fix the error by changing the line 110 to this:

110         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{branchcode}
Comment 5 Barton Chittenden 2018-05-06 15:39:59 UTC
(In reply to Saiful Amin from comment #4)
> This is not a data issue but a bug in the code. Here are the lines:
> 
> 109     my $branchcode =
> 110         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch}
> 111       : ( $control eq 'PatronLibrary' )   ? $borrower->{branchcode}
> 112       :                                     $overdue->{branchcode};
> 113
> 
> The variable $overdue->{homebranch} is bad because the 'issues' table has no
> column called 'homebranch'. It should be 'branchcode' instead. I'm able to
> fix the error by changing the line 110 to this:
> 
> 110         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{branchcode}

Saiful,

This bug was marked 'RESOLVED, FIXED'. Have you tested this in the most recent version of Koha? If it's still an issue, please let us know; this does cause support problems.
Comment 6 Saiful Amin 2018-05-06 16:26:22 UTC
After posting that I thought of changing the status from RESOLVED to something else, but didn't see any option to modify.

Coming back to testing, it was done on a fresh installation of 17.11.05.000 installed from Debian repo on Ubuntu 17.04, using MariaDB.

- We migrated data from another ILS for all bibs, items, patrons and circulation transactions.
- There is only one branch for all items and borrowers.
- After migration we checked there are no NULL values in items.homebranch, issues.branchcode, borrowers.branchcode columns.
- In preferences,
  - Checkout policy 'CircControl' is set to 'ItemHomeLibrary' ("the library the item is from.")
  - 'item-level_itypes' is set to 'specific item' (1)
  - 'finesMode' set to 'Calculate and charge'

We run this command (copied from cron file):
koha-foreach --chdir --enabled /usr/share/koha/bin/cronjobs/fines.pl

The fines don't get generated and we see this error:

Use of uninitialized value $branchcode in exists at /usr/share/koha/bin/cronjobs/fines.pl line 115.
No branchcode argument passed to Koha::Calendar->new at /usr/share/koha/bin/cronjobs/fines.pl line 179.

With the following change:

-         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch}
+         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{branchcode}

the error goes away and fines are getting calculated.

My comment about 'issues' not containing 'homebranch' was not relevant since I realize now that the Getoverdues() function left joins the 'item' table (which contains the 'homebranch').

Should I add a pull request for this so that someone can run other tests?
Comment 7 Saiful Amin 2018-05-06 18:10:41 UTC
I've done some more digging into this and found that it was indeed a data issue, the item itself was missing. Single record caused the failure.

I feel that the error message does not do justice to the error. The script should not crash on faulty data. The final fix that I would suggest is this (as suggested in Comment #3):

  109     my $branchcode =
  110         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch}
  111       : ( $control eq 'PatronLibrary' )   ? $borrower->{branchcode}
  112       :                                     $overdue->{branchcode};
+ 113     if (!defined $branchcode ) {
+ 114         carp "ERROR in getting branchcode. Skipping record.\n";
+ 115         next;
+ 116     }
  117
  118 # In final case, CircControl must be PickupLibrary. (branchcode comes from issues table here).
  119     if ( !exists $is_holiday{$branchcode} ) {
  120         $is_holiday{$branchcode} = set_holiday( $branchcode, $today );
Comment 8 Barton Chittenden 2018-05-06 19:57:17 UTC
Created attachment 75103 [details] [review]
Add warning if homebranch is undefined.
Comment 9 Barton Chittenden 2018-05-06 20:07:28 UTC
(In reply to Saiful Amin from comment #7)
> I've done some more digging into this and found that it was indeed a data
> issue, the item itself was missing. Single record caused the failure.
> 
> I feel that the error message does not do justice to the error. The script
> should not crash on faulty data. The final fix that I would suggest is this
> (as suggested in Comment #3):
> 
>   109     my $branchcode =
>   110         ( $control eq 'ItemHomeLibrary' ) ? $overdue->{homebranch}
>   111       : ( $control eq 'PatronLibrary' )   ? $borrower->{branchcode}
>   112       :                                     $overdue->{branchcode};
> + 113     if (!defined $branchcode ) {
> + 114         carp "ERROR in getting branchcode. Skipping record.\n";
> + 115         next;
> + 116     }
>   117
>   118 # In final case, CircControl must be PickupLibrary. (branchcode comes
> from issues table here).
>   119     if ( !exists $is_holiday{$branchcode} ) {
>   120         $is_holiday{$branchcode} = set_holiday( $branchcode, $today );

As luck would have it, I was writing my patch at the same time that you were writing this; mine has the same logic but gives a bit more detail about which item failed. Here's my test plan:

    When the CircControl system preference is set to 'the library the item is from' and items.homebranch is not set,
    fines.pl will stop processing witha fatal error that looks something like this:
    
        Use of uninitialized value $branchcode in exists at /home/koha/kohaclone/misc/cronjobs/fines.pl line 110
        No branchcode argument passed to Koha::Calendar->new at /home/koha/kohaclone/misc/cronjobs/fines.pl line 169.
    
    This patch will check the value of $branchcode before it is used. If $branchcode is undefined, a warning will
    be issued and, processing will be skipped for that item, and continue for the next item.
    
    To test:
    
    1) Set CircControl to  'the library the item is from'
    2) Check out items and back-date them to be overdue
    3) Set homebranch of one of the items to NULL
    4) Run misc/cronjobs/fines.pl, note that processing stops at that item.
    5) Apply patch
    6) Repeat step 4. not that processing completes.
    7) Check logs, you should see a warning message for the item missing its homebranch.
    8) Sign off.
Comment 10 Pierre-Luc Lapointe 2018-06-15 20:08:04 UTC
Created attachment 76109 [details] [review]
Bug 5789 - Fines don't work when items have null homebranch

When the CircControl system preference is set to 'the library the item is from' and items.homebranch is not set,
fines.pl will stop processing witha fatal error that looks something like this:

    Use of uninitialized value $branchcode in exists at /home/koha/kohaclone/misc/cronjobs/fines.pl line 110
    No branchcode argument passed to Koha::Calendar->new at /home/koha/kohaclone/misc/cronjobs/fines.pl line 169.

This patch will check the value of $branchcode before it is used. If $branchcode is undefined, a warning will
be issued and, processing will be skipped for that item, and continue for the next item.

To test:

1) Set CircControl to  'the library the item is from'
2) Check out items and back-date them to be overdue
3) Set homebranch of one of the items to NULL
4) Run misc/cronjobs/fines.pl, note that processing stops at that item.
5) Apply patch
6) Repeat step 4. not that processing completes.
7) Check logs, you should see a warning message for the item missing its homebranch.
8) Sign off.

Signed-off-by: Pierre-Luc Lapointe <pierreluc.lapointe@inLibro.com>
Comment 11 Jonathan Druart 2018-06-22 14:02:35 UTC
If we consider that it is a data migration issue I think we should start writing a script to catch them.

See also bug 20509.

We cannot deal everywhere with such inconsistency, and if we do it in this script, we need to do it everywhere else.

I would prefer to see a misc script searching for any kinds of data inconsistency in DB. We will then improve it incrementally.
Comment 12 Saiful Amin 2018-06-25 17:07:25 UTC
(In reply to Jonathan Druart from comment #11)
> If we consider that it is a data migration issue I think we should start
> writing a script to catch them.
> 
> See also bug 20509.
> 
> We cannot deal everywhere with such inconsistency, and if we do it in this
> script, we need to do it everywhere else.
> 
> I would prefer to see a misc script searching for any kinds of data
> inconsistency in DB. We will then improve it incrementally.

Separate script idea to check data inconsistency is good one. It will save lot of time in fixing issues, such as missing foreign keys.

I still don't think scripts should break on missing/bad data. For example, I've observed that bulkmarcimport.pl breaks if 245$c contains Arabic date. Ideally, it should warn and skip the record if it can't process the data. I'm not sure if this is by design.
Comment 13 Saiful Amin 2018-06-25 17:16:26 UTC
(In reply to Saiful Amin from comment #12)
> I still don't think scripts should break on missing/bad data. For example,
> I've observed that bulkmarcimport.pl breaks if 245$c contains Arabic date.
> Ideally, it should warn and skip the record if it can't process the data.
> I'm not sure if this is by design.

My bad. That's tag 260$c (not 245$c) that breaks batch import. I'll probably add a separate bug for that if it doesn't exist.
Comment 14 Katrin Fischer 2018-06-25 20:19:30 UTC
(In reply to Saiful Amin from comment #13)
> (In reply to Saiful Amin from comment #12)
> > I still don't think scripts should break on missing/bad data. For example,
> > I've observed that bulkmarcimport.pl breaks if 245$c contains Arabic date.
> > Ideally, it should warn and skip the record if it can't process the data.
> > I'm not sure if this is by design.
> 
> My bad. That's tag 260$c (not 245$c) that breaks batch import. I'll probably
> add a separate bug for that if it doesn't exist.

Please do!
Comment 15 Jonathan Druart 2018-06-27 18:29:17 UTC
(In reply to Jonathan Druart from comment #11)
> I would prefer to see a misc script searching for any kinds of data
> inconsistency in DB. We will then improve it incrementally.

I am going to provide a patch on bug 21010.
Comment 16 Marcel de Rooy 2018-07-06 06:25:01 UTC
Changing status to In Discussion. Leaving this in Signed Off is confusing.
I do not object to an additional check of the branchcode in the fines script btw.
Comment 17 Jonathan Druart 2018-10-25 17:20:16 UTC
(In reply to Marcel de Rooy from comment #16)
> I do not object to an additional check of the branchcode in the fines script
> btw.

So PQA?