Summary: | Fix for Bug 6328 causes user accounts to be frozen (SIP2) | ||
---|---|---|---|
Product: | Koha | Reporter: | Larry Baerveldt <larry> |
Component: | Patrons | Assignee: | Paul Poulain <paul.poulain> |
Status: | CLOSED FIXED | QA Contact: | Ian Walls <koha.sekjal> |
Severity: | major | ||
Priority: | PATCH-Sent (DO NOT USE) | CC: | dpavlin, gmcharlt, paul.poulain, ruth |
Version: | 3.6 | ||
Hardware: | All | ||
OS: | All | ||
See Also: | http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6328 | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: | Bug 7272 setting NULL to debarred field, to avoid having 0000-00-00 |
Description
Larry Baerveldt
2011-11-28 19:11:19 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 Changing subject of ticket. Changing status to major. Changing component to "Authentication". 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) 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. 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. (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) 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 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. 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. Patch pushed, please test Fix will be included in 3.6.5. |