Bug 7272 - Fix for Bug 6328 causes user accounts to be frozen (SIP2)
Summary: Fix for Bug 6328 causes user accounts to be frozen (SIP2)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) major (vote)
Assignee: Paul Poulain
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-28 19:11 UTC by Larry Baerveldt
Modified: 2013-12-05 19:59 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 7272 setting NULL to debarred field, to avoid having 0000-00-00 (1.50 KB, patch)
2011-11-30 13:54 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Larry Baerveldt 2011-11-28 19:11:19 UTC
The fix for Bug 6328, which added dates to borrowers.debarred has apparently caused SIP2 users (and probably others) to be frozen.

After a recent update, it appears that the default value of borrowers.debarred was set to '0000-00-00', however, this causes all accounts using that date to be considered frozen by the system. This is shown by logging into the catalog as that user.

For SIP2 users, this is causing some SIP systems to fail to authenticate, Overdrive being one. Changing the value of debarred to NULL has temporarily fixed the problem, but a permanent fix is needed.

As far as SIP2 is concerned, fixing C4/SIP/ILS/Patron.pm to use the new value may solve the problem.

- Larry
Comment 1 Larry Baerveldt 2011-11-28 19:15:15 UTC
Update,

This bug is actually causing all borrower accounts to be frozen, even though they have no fines or overdues. A date of '0000-00-00' is still being interpreted as the account being frozen.

Upgrading status to major. 

- Larry
Comment 2 Larry Baerveldt 2011-11-28 19:16:35 UTC
Changing subject of ticket.

Changing status to major.

Changing component to "Authentication".
Comment 3 Larry Baerveldt 2011-11-28 19:19:29 UTC
Adding link to bug 6328.
Comment 4 Paul Poulain 2011-11-30 13:13:54 UTC
I investigated this issue, and i'm quite confused.
* adding a new patron result in borrowers.debarred being NULL (so not 0000-00-00)
* checking the updatedatabase, I think it should also be NULL

So, a question: could you check if your patrons have 0000-00-00 in the borrowers.debarred field, or NULL ?
(still investigating)
Comment 5 D Ruth Holloway 2011-11-30 13:23:33 UTC
Paul,

It works fine for newly-created borrowers, or any borrower that you block and unblock on this version, etc.  But the borrowers that were in the database at the time of the migration to this version, previously having had NULL, numeric zero, or numeric one, get broken when the updatedatabase.pl is run.  In particular, borrowers with numeric zero get turned into "0000-00-00", which the SIP authentication code is not coping with properly; an "if $debarred" will evaluate as TRUE, which is not what you want here.

The workaround, of course, is this:

UPDATE borrowers SET debarred=NULL WHERE debarred="0000-00-00"; 

...but it may be a better idea to actually fix the SIP code at the point Larry indicated to cope with the fact that borrowers.debarred is no longer a boolean.  Similar gotchas may exist elsewhere in Koha.  To do this with minimal code touching you could set $debarred based on the value (at C4/SIP/ILS/Patron.pm:51)--if borrowers.debarred is NULL or "0000-00-00" or a date IN THE PAST, set zero, otherwise, set 1.  Then the rest of the SIP checks for $debarred would work correctly.
Comment 6 Paul Poulain 2011-11-30 13:26:12 UTC
OK, I think I've found the problem: switching back to tinyint(1) as it was previously, then forcing debarred=0 for patron, then
ALTER TABLE borrowers MODIFY debarred DATE DEFAULT NULL

=> bingo = the debarred now contains 0000-00-00 !

why was debarred containing 0, I don't know for sure, but I feel the problem comes from here.

Larry, could you run
UPDATE borrowers SET debarred=NULL WHERE debarred="0000-00-00";

and see if that fixes the problem ?
If yes, i'll provide a fix to run this query automatically when upgrading.
Comment 7 Paul Poulain 2011-11-30 13:30:58 UTC
(In reply to comment #5)
> Paul,
you've beaten me by 20 seconds ;-)

> ...but it may be a better idea to actually fix the SIP code at the point Larry
> indicated to cope with the fact that borrowers.debarred is no longer a boolean.
>  Similar gotchas may exist elsewhere in Koha.  To do this with minimal code
> touching you could set $debarred based on the value (at
> C4/SIP/ILS/Patron.pm:51)--if borrowers.debarred is NULL or "0000-00-00" or a
> date IN THE PAST, set zero, otherwise, set 1.  Then the rest of the SIP checks
> for $debarred would work correctly.

I'm not sure = 0000-00-00 is a date that should never be here. You can't assign such a date manually, and it is never assigned by Koha itself (otherwise, it's a bug ;-) ). So setting NULL during updatedatabase is OK I think.
your opinion ? (available on IRC if usefull)
Comment 8 Paul Poulain 2011-11-30 13:54:04 UTC
Created attachment 6469 [details] [review]
Bug 7272 setting NULL to debarred field, to avoid having 0000-00-00

0000-00-00 can come only from a problem in the 3.06.00.001 update
Comment 9 Ian Walls 2011-11-30 14:58:47 UTC
Paul,


I think that database update line is exactly what we need; it won't interfere with any instances of valid dates, so it's safe to run.

Once we get this transitional mess cleaned up, we shouldn't need to worry about checking for debarred = 0000-00-00 anymore (though it wouldn't hurt).  It would be possible, for example, for someone to do borrower imports that have debarred column containing 0, which would break things all over again.  A check somewhere in the code would catch these potential problems before they cause anyone suffering.
Comment 10 Larry Baerveldt 2011-11-30 15:07:49 UTC
I'm just getting caught up on the comments.

Yes, I already ran the sql to change '000-00-00' to NULL, and it does fix the problem. Confirmed.
Comment 11 Paul Poulain 2011-11-30 16:07:53 UTC
Patch pushed, please test
Comment 12 Jared Camins-Esakov 2012-05-23 12:24:03 UTC
Fix will be included in 3.6.5.