[Koha-patches] [PATCH] Prevent fines failure on NULL borrowernumber.

[Joe Atzberger]

Michael --

Thanks for the reply.  I think NULL borrowernumber in issues is in fact a
mistake, broadly speaking.  You are correct that the design currently allows
it, but it also falls apart after it is allowed.  I understand why we
compromised at 3.0 release time, mainly to allow offline circ (and legacy
code, perhaps dealing w/ lost items?) to work without failing, but I still
regard it as inadvisable.

Offline circ is a cool feature, but the checkout transaction in any ILS has
as its most fundamental role to make a borrower responsible for an item.
This mapping gets broken at a conceptual level if there is nobody
responsible for the issue.  At a technical level, Koha's checkin doesn't
work on NULL borrwernumber transactions.  What's worse, it allows additional
transactions for the same item to be added to the issues table.  Upon
checkin, none of these subsequent transactions are cleared either.  This
means potentially many people get fined for the SAME item, that they already
turned in, and they'll keep getting it no matter how many times the item is
turned in!  This is very bad if you are running fines, hence the severity of
my error message in the fines script.

If I wanted to totally code around the problem, I could have fines look for
NULL borrowernumber lines in issues, then see if there are subsequent
transactions on the same item for VALID borrowernumbers, and then delete all
except the most recent (since if it was checked out to somebody else later,
by definition it isn't checked out to previous borrowers anymore).  Then I
could insert the old lines to old_issues.  But this is clearly more of a
"cover_up_bad_circ_data.pl" cronjob than a logical component of fines and
*still* wouldn't ensure valid data.

I'm in the camp that says, break all the other features but keep your circ
intact.  Nobody is willing to run an ILS with unreliable circulation.

--Joe

On Fri, Jan 23, 2009 at 6:02 PM, Michael Hafen <mdhafen at tech.washk12.org>wrote:

> I don't think it is a mistake that the borrowernumber in the issues
> table can have NULL rows.  I do think that is is a bad thing though.
> Looking at kohastructure.sql there is a Foreign Key Constraint that
> states 'On Delete Set Null On Update Set Null'.
>
> I agree that this should not be. I think it would be better to state 'On
> Delete Delete On Update Cascade' if possible.  That would be much
> preferable in my opinion from a data integrity point of view.
>
> My point is that I think your error message below is a little misleading
> given the foreign key constraint.
>
> Thanks for working on this problem though.  Have a nice day.
>
> On Fri, 2009-01-23 at 16:22 -0600, Joe Atzberger wrote:
> > The problem is that we do not ensure that the issues table has valid
> > borrowernumber in each line.  This is exacerbated by Getoverdues()
> > returning data sorted BY borrowernumber.  So one NULL borrowernumber
> > in issues prevented ALL fines from being assessed.  The actual error
> > from fines.pl cron log is:
> >   No branchcode argument to new.  Should be C4::Calendar->new(branchcode
> => $branchcode)
> >   at /home/user/kohaclone/misc/cronjobs/fines.pl line 98
> >
> > This patch deals only with getting fines to avoid crashing.  It does
> > not fix the underlying data integrity problem.
> [snip]
> > +        print STDERR "ERROR in Getoverdues line $i:
> > issues.borrowernumber IS NULL.  Repair 'issues' table now!  Skipping
> > record.\n";
> > +        next;   # Note: this doesn't solve everything.  After NULL
> > borrowernumber, multiple issues w/ real borrowernumbers can pile up.
> > +    }
> [snip]
> --
> Michael Hafen
> Systems Analyst and Programmer
> Washington County School District
> Utah, USA
>
> for Koha checkout
> http://koha-dev.washk12.org
> or
> git://koha-dev.washk12.org/koha
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20090123/91ea2994/attachment-0001.htm>