Bug 8253 - fix fine doubling, when upgrading from 3.6 to 3.8 (or later)
Summary: fix fine doubling, when upgrading from 3.6 to 3.8 (or later)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high critical (vote)
Assignee: Kyle M Hall
QA Contact: Paul Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-15 14:14 UTC by Nicole C. Engard
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8253 - Fine doubling (2.39 KB, patch)
2012-06-19 19:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling (3.60 KB, patch)
2012-06-21 11:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling - Remove Previous Duplicates (2.76 KB, patch)
2012-06-21 16:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling - Remove Previous Duplicates (2.78 KB, patch)
2012-06-22 12:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling - Remove Previous Duplicates (2.87 KB, patch)
2012-06-22 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling (6.20 KB, patch)
2012-07-23 20:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling (5.65 KB, patch)
2012-07-25 12:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling - Followup (1.71 KB, patch)
2012-08-17 12:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8253 - Fine doubling (1.26 KB, patch)
2012-10-02 22:11 UTC, Mason James
Details | Diff | Splinter Review
Duplicate Fines Remover (2.09 KB, patch)
2012-10-03 18:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
SIGNED-OFF-Bug-8253-Fine-doubling.patch (1.44 KB, patch)
2012-10-03 21:50 UTC, wajasu
Details | Diff | Splinter Review
wajasu's test notes showing accountline queries at points in time (11.97 KB, text/plain)
2012-10-03 22:00 UTC, wajasu
Details
Bug 8253 - fix fine doubling, when upgrading from 3.6 to 3.8 - Add de-duplication script (4.40 KB, patch)
2012-10-04 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
SIGNED-OFF-Bug-8253-fix-fine-doubling-when-upgrading from 3.6 to 3.8 - Add de-duplication script (5.06 KB, patch)
2012-10-04 16:06 UTC, wajasu
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-06-15 14:14:17 UTC
We're still researching this, but Koha appears to be doubling fines after an upgrade from 3.6.x to 3.8.1.
Comment 1 Kyle M Hall 2012-06-19 19:50:59 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-06-21 11:55:02 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-06-21 16:15:56 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-06-22 12:08:52 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-06-22 15:30:23 UTC Comment hidden (obsolete)
Comment 6 Julian Maurice 2012-07-20 10:34:07 UTC
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.
Comment 7 Kyle M Hall 2012-07-20 10:38:33 UTC
They were simply written at different times. I'd be more than happy to provide a squashed variation.
Comment 8 Kyle M Hall 2012-07-23 20:20:41 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2012-07-24 09:03:24 UTC
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
Comment 10 Kyle M Hall 2012-07-25 12:34:37 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2012-07-25 12:37:11 UTC
(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'.
Comment 12 Julian Maurice 2012-08-06 13:23:10 UTC
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'});
	}
}
Comment 13 Kyle M Hall 2012-08-17 12:40:46 UTC
(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
Comment 14 Kyle M Hall 2012-08-17 12:48:53 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2012-08-23 10:18:17 UTC
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.
Comment 16 Kyle M Hall 2012-09-26 12:57:52 UTC
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.
Comment 17 Mason James 2012-10-02 22:11:12 UTC Comment hidden (obsolete)
Comment 18 Mason James 2012-10-02 22:21:53 UTC
(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.
Comment 19 Kyle M Hall 2012-10-03 16:17:18 UTC
(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.
Comment 20 Kyle M Hall 2012-10-03 18:13:40 UTC
(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.
Comment 21 Kyle M Hall 2012-10-03 18:25:39 UTC Comment hidden (obsolete)
Comment 22 Mason James 2012-10-03 19:24:13 UTC
(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?
Comment 23 wajasu 2012-10-03 21:50:29 UTC
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
Comment 24 wajasu 2012-10-03 21:54:58 UTC
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.
Comment 25 wajasu 2012-10-03 22:00:44 UTC
Created attachment 12667 [details]
wajasu's test notes showing accountline queries at points in time

Just my notes while testing for reference.
Comment 26 Kyle M Hall 2012-10-04 14:34:58 UTC Comment hidden (obsolete)
Comment 27 wajasu 2012-10-04 16:06:00 UTC
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.
Comment 28 wajasu 2012-10-04 16:59:01 UTC
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
Comment 29 wajasu 2012-10-04 18:18:49 UTC
*** Bug 8112 has been marked as a duplicate of this bug. ***
Comment 30 Paul Poulain 2012-10-28 18:23:48 UTC
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
Comment 31 Paul Poulain 2012-10-28 18:25:24 UTC
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)
Comment 32 Chris Cormack 2012-10-28 20:23:56 UTC
Pushed to 3.8.x will be in 3.8.7