We're still researching this, but Koha appears to be doubling fines after an upgrade from 3.6.x to 3.8.1.
Created attachment 10369 [details] [review] Bug 8253 - Fine doubling This issue only occurs when upgrading from pre-3.8 to post 3.8 Koha. The issue is caused by the change from dates to datetimes that was neccesary to implement hourly loans. All pre-3.8 fines have the date in the description. This date is how the fines script know which fine to update. If the fine script does not locate an existing row to update, it creates a new row. The switch from dates to datetimes means the fines script now looks for a format such as 'YYYY-MM-DD 00:00' ( for iso ), but all the previous fines are still in the format 'YYYY-MM-DD' and so it fails to find the matching row, and creates a new row instead. This commit consists of a database update that alters the dates in the accountlines description field to be datetimes instead. This eliminates future fine doubling. It is also safe to run on a system that has been previously upgraded, it will ignore any rows where the date is already of the format 'YYYY-MM-DD 00:00' ( or whichever date format you have chosen ).
Created attachment 10405 [details] [review] Bug 8253 - Fine doubling This issue only occurs when upgrading from pre-3.8 to post 3.8 Koha. The issue is caused by the change from dates to datetimes that was neccesary to implement hourly loans. All pre-3.8 fines have the date in the description. This date is how the fines script know which fine to update. If the fine script does not locate an existing row to update, it creates a new row. The switch from dates to datetimes means the fines script now looks for a format such as 'YYYY-MM-DD 00:00' ( for iso ), but all the previous fines are still in the format 'YYYY-MM-DD' and so it fails to find the matching row, and creates a new row instead. This commit consists of a database update that alters the dates in the accountlines description field to be datetimes instead. This eliminates future fine doubling. It is also safe to run on a system that has been previously upgraded, it will ignore any rows where the date is already of the format 'YYYY-MM-DD 00:00' ( or whichever date format you have chosen ).
Created attachment 10416 [details] [review] Bug 8253 - Fine doubling - Remove Previous Duplicates
Created attachment 10429 [details] [review] Bug 8253 - Fine doubling - Remove Previous Duplicates
Created attachment 10436 [details] [review] Bug 8253 - Fine doubling - Remove Previous Duplicates
There are 2 patches still 'active'. Should they be applied both? In this case, why not merging them into one single DB rev? Just a note: in the 2nd patch there are commented warns. I think they should be removed.
They were simply written at different times. I'd be more than happy to provide a squashed variation.
Created attachment 11087 [details] [review] Bug 8253 - Fine doubling This issue only occurs when upgrading from pre-3.8 to post 3.8 Koha. The issue is caused by the change from dates to datetimes that was neccesary to implement hourly loans. All pre-3.8 fines have the date in the description. This date is how the fines script know which fine to update. If the fine script does not locate an existing row to update, it creates a new row. The switch from dates to datetimes means the fines script now looks for a format such as 'YYYY-MM-DD 00:00' ( for iso ), but all the previous fines are still in the format 'YYYY-MM-DD' and so it fails to find the matching row, and creates a new row instead. This commit consists of a database update that alters the dates in the accountlines description field to be datetimes instead. This eliminates future fine doubling. It is also safe to run on a system that has been previously upgraded, it will ignore any rows where the date is already of the format 'YYYY-MM-DD HH:MM' ( or whichever date format you have chosen ). The second part of this update will remove any existing duplicate fines that have been created as a result of this error.
I think there is a problem: duplicate rows are not removed. Here's how I tested: I have 1 row in accountlines with accounttype='FU' and description='Orchidées 31/05/2012' I run misc/cronjobs/fines.pl: a duplicate row is created with description='Orchidées 31/05/2012 23:59' Now, I run the updatedatabase: 1st row description is now 'Orchidées 31/05/2012 23:59' (OK) but the 2 rows are still there (KO) Am I doing something wrong? About the patch: there are 2 '<<<<<<< HEAD' remaining in updatedatabase.pl
Created attachment 11127 [details] [review] Bug 8253 - Fine doubling This issue only occurs when upgrading from pre-3.8 to post 3.8 Koha. The issue is caused by the change from dates to datetimes that was neccesary to implement hourly loans. All pre-3.8 fines have the date in the description. This date is how the fines script know which fine to update. If the fine script does not locate an existing row to update, it creates a new row. The switch from dates to datetimes means the fines script now looks for a format such as 'YYYY-MM-DD 00:00' ( for iso ), but all the previous fines are still in the format 'YYYY-MM-DD' and so it fails to find the matching row, and creates a new row instead. This commit consists of a database update that alters the dates in the accountlines description field to be datetimes instead. This eliminates future fine doubling. It is also safe to run on a system that has been previously upgraded, it will ignore any rows where the date is already of the format 'YYYY-MM-DD HH:MM' ( or whichever date format you have chosen ). The second part of this update will remove any existing duplicate fines that have been created as a result of this error.
(In reply to comment #9) > I think there is a problem: duplicate rows are not removed. Here's how I > tested: > > I have 1 row in accountlines with accounttype='FU' and > description='Orchidées 31/05/2012' > I run misc/cronjobs/fines.pl: a duplicate row is created with > description='Orchidées 31/05/2012 23:59' > Now, I run the updatedatabase: 1st row description is now 'Orchidées > 31/05/2012 23:59' (OK) but the 2 rows are still there (KO) > > Am I doing something wrong? > Can you you post the descriptions for those other two rows. It should have deleted any other rows where the description starts with 'Orchidées 31/05/2012'.
I replayed my tests. Here's what I did: # empty the accounlines table mysql> delete from accountlines; # run fines script $ misc/cronjobs/fines.pl mysql> select * from accountlines\G *************************** 1. row *************************** borrowernumber: 58 accountno: 1 itemnumber: 2 date: 2012-08-06 amount: 112.000000 description: Orchidées 06/01/2012 23:59 dispute: NULL accounttype: FU amountoutstanding: 112.000000 lastincrement: 112.000000 timestamp: 2012-08-06 14:46:45 notify_id: 0 notify_level: 0 note: NULL manager_id: NULL 1 row in set (0.00 sec) # simulate that fines script was run pre-3.8 mysql> update accountlines set description="Orchidées 06/01/2012"; # re-run $ misc/cronjobs/fines.pl # lines are duplicated (OK) mysql> select * from accountlines\G *************************** 1. row *************************** borrowernumber: 58 accountno: 1 itemnumber: 2 date: 2012-08-06 amount: 112.000000 description: Orchidées 06/01/2012 dispute: NULL accounttype: FU amountoutstanding: 112.000000 lastincrement: 112.000000 timestamp: 2012-08-06 15:01:49 notify_id: 0 notify_level: 0 note: NULL manager_id: NULL *************************** 2. row *************************** borrowernumber: 58 accountno: 2 itemnumber: 2 date: 2012-08-06 amount: 112.000000 description: Orchidées 06/01/2012 23:59 dispute: NULL accounttype: FU amountoutstanding: 112.000000 lastincrement: 112.000000 timestamp: 2012-08-06 15:01:52 notify_id: 0 notify_level: 0 note: NULL manager_id: NULL 2 rows in set (0.00 sec) # run the update script $ perl installer/data/mysql/updatedatabase.pl # by the way, this warning is introduced by your patch: "my" variable $sth masks earlier declaration in same scope at installer/data/mysql/updatedatabase.pl line 5697. Upgrade to 3.09.00.XXX done (Fix fine descriptions and remove duplicate fines) # lines are still duplicated, but description has changed mysql> select * from accountlines\G *************************** 1. row *************************** borrowernumber: 58 accountno: 1 itemnumber: 2 date: 2012-08-06 amount: 112.000000 description: Orchidées 06/01/2012 23:59 dispute: NULL accounttype: FU amountoutstanding: 112.000000 lastincrement: 112.000000 timestamp: 2012-08-06 15:02:25 notify_id: 0 notify_level: 0 note: NULL manager_id: NULL *************************** 2. row *************************** borrowernumber: 58 accountno: 2 itemnumber: 2 date: 2012-08-06 amount: 112.000000 description: Orchidées 06/01/2012 23:59 dispute: NULL accounttype: FU amountoutstanding: 112.000000 lastincrement: 112.000000 timestamp: 2012-08-06 15:01:52 notify_id: 0 notify_level: 0 note: NULL manager_id: NULL 2 rows in set (0.00 sec) This is because, in the update process, you try to delete only lines that do not contain 23:59: $query = "SELECT * FROM accountlines WHERE description LIKE ? AND description NOT LIKE ?"; ... foreach my $keeper ( @$results ) { my ( $description_to_match ) = split( / 23:59/, $keeper->{'description'} ); $description_to_match .= '%'; $sth->execute( $description_to_match, $keeper->{'description'} ); while ( my $f = $sth->fetchrow_hashref() ) { ... my $sql = "DELETE FROM accountlines WHERE borrowernumber = ? AND accountno = ? AND itemnumber = ? AND date = ? AND description = ? LIMIT 1"; $dbh->do($sql,undef,$f->{'borrowernumber'},$f->{'accountno'},$f->{'itemnumber'}, $f->{'date'}, $f->{'description'}); } }
(In reply to comment #12) Julian, I think I understand why this isn't working for you. When I wrote this patch, Koha also had the bug where it was using '00:00' instead of '23:59' for the hour/minutes portion. In that case, this would work just fine. Perhaps we need to make this delete all lines matching 'Orchidées 06/01/2012', but instead of not matching on 'Orchidées 06/01/2012 23:59", we use the timestamp of the keeper instead. So, instead of SELECT * FROM accountlines WHERE description LIKE $truncated_description AND description NOT LIKE $full_description We would use SELECT * FROM accountlines WHERE description LIKE $truncated_description AND timestamp NOT LIKE $keepers_timestamp
Created attachment 11660 [details] [review] Bug 8253 - Fine doubling - Followup Use timestamp as the 'un-matcher' rather than the fine description.
This does not work for me. Instead of having the 2 duplicates kept, they are now both removed by updatedatabase.pl. I think this comes from here: my $datetime = "$date 23:59"; my $new_description = $old_description; $new_description =~ s/$date/$datetime/g; $dbh->do("UPDATE accountlines SET description = ? WHERE description = ?", undef, $new_description, $old_description); In the above code, all descriptions are updated to have the new date format with 23:59 And then: my $query = " SELECT * FROM accountlines WHERE ( accounttype = 'FU' OR accounttype = 'F' ) AND description like '%23:59%' ORDER BY borrowernumber, itemnumber, accountno, description, timestamp DESC "; You select all lines with a description that contains '23:59'. So both lines of my example are returned. Finally for each line, other lines that have a different timestamp are deleted. So both lines are deleted.
I see. I cannot think of a way to resolve this. Any ideas? Kyle (In reply to comment #15) > This does not work for me. Instead of having the 2 duplicates kept, they are > now both removed by updatedatabase.pl. > > I think this comes from here: > > my $datetime = "$date 23:59"; > my $new_description = $old_description; > $new_description =~ s/$date/$datetime/g; > $dbh->do("UPDATE accountlines SET description = ? WHERE description = > ?", undef, $new_description, $old_description); > > In the above code, all descriptions are updated to have the new date format > with 23:59 > And then: > > my $query = " > SELECT * FROM accountlines > WHERE ( accounttype = 'FU' OR accounttype = 'F' ) > AND description like '%23:59%' > ORDER BY borrowernumber, itemnumber, accountno, description, > timestamp DESC > "; > > You select all lines with a description that contains '23:59'. So both lines > of my example are returned. > Finally for each line, other lines that have a different timestamp are > deleted. So both lines are deleted.
Created attachment 12653 [details] [review] Bug 8253 - Fine doubling
(In reply to comment #17) > Created attachment 12653 [details] [review] > Bug 8253 - Fine doubling this patch has been tested OK for 3.6 to 3.8 upgrades ... by Catalyst-NZ, and Calyx/KohaAloha this patch provides a simpler fix to the existing 2 patches.
(In reply to comment #18) > (In reply to comment #17) > > Created attachment 12653 [details] [review] > > Bug 8253 - Fine doubling > > this patch has been tested OK for 3.6 to 3.8 upgrades > ... by Catalyst-NZ, and Calyx/KohaAloha > > this patch provides a simpler fix to the existing 2 patches. I like the simplicity of your patch, but it doesn't cover the case where libraries already have upgraded to a version with hourly loans, and already have fines that include an hour/minute.
(02:07:27 PM) wajasu: khall: on bug 8253 what if Mason's patch also has and description NOT like '%23:59' or some variation to handle hourly loans existing. (02:10:05 PM) khall: wajasu: that would cover the case I mentioned in comment 19. The only big problem is it doesn't cover libraries with already duplicated fines. However, I'd rather see his patch get in to prevent future upgraders from having problems. I can post my de-duplication script to the wiki for anyone to use who already has upgraded and has duplicated fines. (02:10:11 PM) rangi: its not for that (02:10:29 PM) rangi: yes, that patch fixes it for people upgrade to 3.8.x (02:10:37 PM) rangi: from before (02:10:51 PM) rangi: and anyone alreay upgraded, it wont affec (02:10:52 PM) rangi: t (02:11:05 PM) rangi: because it changes an updatedatabase.pl call earlier (02:11:11 PM) rangi: the sooner it can go in the better (02:11:25 PM) rangi: then a fix for those who have already upgraded and have duplicates can follow (02:11:31 PM) khall: yeah, i have to agree.
Created attachment 12665 [details] [review] Duplicate Fines Remover This is my fines de-duplication script. Just keep running it over and over until you no longer have any duplicate fines in the database.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > Created attachment 12653 [details] [review] > > > Bug 8253 - Fine doubling > > > > this patch has been tested OK for 3.6 to 3.8 upgrades > > ... by Catalyst-NZ, and Calyx/KohaAloha > > > > this patch provides a simpler fix to the existing 2 patches. > > I like the simplicity of your patch, but it doesn't cover the case where > libraries already have upgraded to a version with hourly loans, and already > have fines that include an hour/minute. correct, this patch stops fines doubling only, during a 3.6->3.8 upgrade it makes no attempt to fix already doubled fines in a 3.8 system (thats a separate problem, requiring a separate patch) can someone sign-off on this urgent fix please?
Created attachment 12666 [details] [review] SIGNED-OFF-Bug-8253-Fine-doubling.patch Start early in the day, when you are feeling hopeful. Test: --------------To demonstrate bug: a) created a new VM with 3.8.5 dependencies loaded with koha-common b) created kohaclone c) git checkout -b bug8253_36x origin/3.6.x d) perl Makefile.pl with dev install ( mysqladmin -uroot -p create koha, with user/pass = koha, grants/flush ) e) make; make install f) adjust install for web/staff access g) stepped through web installer to initialize fresh database (installer updatedatabase.pl happens) h) created a "student" patron stud1 i) cataloged a couple of books with z3950 search ( barcode 111 and 222 resprectively) j) configured circulation rules to charge $1.50 for each day overdue (Home ? Administration ? Circulation and Fine Rules ) ( All, All, 10, 10, blank, 1.50,1,0,0,0,0 click add ) k) configured overdue notice/status triggers (1st 1 day, 2nd 2 days) for "student" "Overdue Notice" ( Home ? Tools ? Notice Triggers) l) configured finesmode in syspref with "calculate & charge" m) checked out one book with duedate 1 day in past for patron stud1 barcode 111 n) checked out other book with duedate 2 day in past for patron stud1 barcode 222 o) used mysql to "select * from accountlines;" to see what fines exist (no dupes just rental charges, 2 $5.00 entries ) p) run misc/cronjobs/fines.pl q) used mysql to "select * from accountlines;" to see what fines exist (no dupes just two additional entries for our two overdue books) r) brought in new kohaclone s) git checkout -b bug8253_38x origin/3.8.x t) performed steps d (except no new db to create though) thru step g (new version of updatedatabase.pl) u) used mysql to "select * from accountlines;" to see what fines exist (no dupes) v) run misc/cronjobs/fines.pl w) used mysql to "select * from accountlines;" to see what fines exist (DUPES) - bug 8253 --------------To test bug fix: y) do the same as above (with a fresh new mysql DB) but after step s) apply the patch: git am -i -u -3 <thepatch> and proceed with steps t [BE SURE TO APPLY d-g] and then continue with steps u thru v z) used mysql to "select * from accountlines;" to see what fines exist (no dupes) - TEST PASSES Signoff patch generation: git commit --amend git format-patch -s origin/3.8.x Note: You can't reuse the database because it won't be upgraded. :) Note: Patch assumes F/FU accountlines will not already have ' 23:59' on the end, but this patch only addresses the folks who upgrade, not the ones who already upgraded. There is another bug fix script for that. Note: QA - A variation on this test, if you can, is to create a fine that has accounttype='F', since I only had 'FU' wajasu
Khall appended his emergency duplicate fine remover script, which I think we can break off into another bug, since its for those folks who already upgraded. That way someone can start QA on this one. So ignore it until it gets moved in its own bug.
Created attachment 12667 [details] wajasu's test notes showing accountline queries at points in time Just my notes while testing for reference.
Created attachment 12685 [details] [review] Bug 8253 - fix fine doubling, when upgrading from 3.6 to 3.8 - Add de-duplication script
Created attachment 12686 [details] [review] SIGNED-OFF-Bug-8253-fix-fine-doubling-when-upgrading from 3.6 to 3.8 - Add de-duplication script Forget about my wanting to split this into another bug. I was tired after testing the first one. After testing the fine-doubling patch, I had entries in accountlines without dupes. I used mysql to return the dupes by removing the ' 23:59' from the descriptions of a couple of entries. select * from accountlines; update accountlines set description='The Hobbit and philosophy : 10/01/2012' where accountno= (where i matched the accountno accordingly and set the dscription to be consistent) select * from accountlines; ./misc/cronjobs/fines.pl select * from accountlines; I see DUPS now. applied the "fix fine doubling - when upgrading..." script ./misc/maintenance/fix_accountlines_rmdupfines_bug8253.pl select * from accountlines; NO DUPES!!! TEST passed. ------- I actually did more: - i created an entry with 'F' as well in accounttype and tested that. - i created dupes, and made the original entries have a lower amountoutstanding, to see if the script preserved that use case, and it did. - observation: repeated runs of fines.pl doesn't create more dups for me. Duplicate fines would happen if you upgraded to a 3.8 version that does not have the bug8253 patch, and the misc/cronjobs/fines.pl is run. In 3.8 an upgrade to a more granular date/time was not addressed for pre-existing fine entries and this script removes the resulting duplicates. It also intelligently preserves the amount outstanding for payments already applied. If your version already had the bug8253 patch at the time of the upgrade, duplicate fines should not have been generatd. Thanks khall for this fix! wajasu Note: I tested khalls prior obsolete script itself (attachment 12665 [details] [review]), which worked as well, and this patch includes it as a formalized maintenance script.
Though I tested the functionality of attachment 12666 [details] [review] for an upgrade from 3.6.x to 3.8.x, I was also able to apply the same patch to master (circa 2012-10-04) and is inserted the update statement cleanly for installer/data/mysql/updatedatabase.pl So, someone should QA it against master as well to address folks who upgrade from <3.8.x to >3.8.x
*** Bug 8112 has been marked as a duplicate of this bug. ***
QA comment: * tiny patch in "core" Koha, only one change in updatedatabase * medium-size follow-up to remove duplicate fines. No side effect risk I can see : if it improperly does the job, it won't break anything else * no string change, so pushable now (and, anyway, it's a blocker) * in the code of the fixing script there are 2 unconditional warn, that could have been written with "say", but not a problem, as it's a maintenance script passed QA
Patch pushed to master (severity lowered to critical = even for libraries having the problem, it's always possible to manually fix it. Very annoying, but the consequence is that the bug is not a blocker)
Pushed to 3.8.x will be in 3.8.7