Summary: | Fines don't work when items have null homebranch | ||
---|---|---|---|
Product: | Koha | Reporter: | Jared Camins-Esakov <jcamins> |
Component: | Circulation | Assignee: | Barton Chittenden <barton> |
Status: | In Discussion --- | QA Contact: | Bugs List <koha-bugs> |
Severity: | normal | ||
Priority: | P5 - low | CC: | barton, gmcharlt, John.Seymour, jonathan.druart, katrin.fischer, m.de.rooy, pierre-luc.lapointe, saiful |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20509 | ||
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.
Bug 5789 - Fines don't work when items have null homebranch |
Description
Jared Camins-Esakov
2011-02-21 20:50:56 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? 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. 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. 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} (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. 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? 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 ); Created attachment 75103 [details] [review] Add warning if homebranch is undefined. (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. 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> 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. (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. (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. (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! (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. 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. (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? Should this be closed now? We have started to treat this as a data issue. |