|
Description
Josef Moravec
2018-10-26 10:59:53 UTC
Created attachment 81281 [details] [review] Bug 21683: Database update Created attachment 81282 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Created attachment 81283 [details] [review] Bug 21683: Update schema file Comment on attachment 81282 [details] [review] Bug 21683: Remove accountlines.accountno Review of attachment 81282 [details] [review]: ----------------------------------------------------------------- Some of these are issues. Some of them are reminders, at least for me, to look more closely in places that might have side-effects. ::: C4/Circulation.pm @@ +2400,4 @@ > accounttype => { -in => [ 'L', 'Rep', 'W' ] }, > }, > { > + order_by => { -desc => [ 'date' ] } accountno was being used to keep them in the same order they were added. I would have substituted accountlines_id for the same effect. I noticed the other patched files were. ::: C4/Stats.pm @@ +130,5 @@ > (datetime, > branch, type, value, > other, itemnumber, itemtype, location, > + borrowernumber, ccode) > + VALUES (now(),?,?,?,?,?,?,?,?,?)" Strangely enough, I think the defaulting to null is better for the statistics for fine/charges. accountno was NOT really a "type of procedure used when making payments". But I'm going to have to read the code as a whole after application to make sure this didn't mess things up for other statistics. ::: Koha/Account.pm @@ -93,5 @@ > - { > - order_by => 'accountno' > - } > - )->next(); > - my $accountno = $last ? $last->accountno + 1 : 1; This was what I was thinking should be it's only function in the Koha namespace, before notions of removing it came out. @@ -351,5 @@ > sub { > - # We should remove accountno, it is no longer needed > - my $last = Koha::Account::Lines->search( { borrowernumber => $self->{patron_id} }, > - { order_by => 'accountno' } )->next(); > - my $accountno = $last ? $last->accountno + 1 : 1; See how it was put in multiple places? That's why. ::: members/printfeercpt.pl @@ -56,5 @@ > > -if ( $action eq 'print' ) { > -# ReversePayment( $borrowernumber, $input->param('accountno') ); > -} > - Scope creep! ;) -- but totally fine. ::: misc/maintenance/fix_accountlines_date.pl @@ -127,4 @@ > our $dbh = C4::Context->dbh; > $dbh->{AutoCommit} = 0; > my $sth = $dbh->prepare(" > -SELECT borrowernumber, itemnumber, accountno, description Will need to read applied patch to see if removing itemnumber from query has impact. ::: misc/maintenance/fix_accountlines_rmdupfines_bug8253.pl @@ +98,4 @@ > } > > my $sql = > + "DELETE FROM accountlines WHERE accountlines_id = ? LIMIT 1"; accountlines_id is auto increment and thus unique. There is no need for LIMIT 1. @@ +103,5 @@ > } > > if ($has_changed) { > my $sql = > + "UPDATE accountlines SET amountoutstanding = ? WHERE accountlines_id = ? LIMIT 1"; accountlines_id is auto increment and thus unique. There is no need for LIMIT 1. ::: t/db_dependent/Stats.t @@ -120,4 @@ > is ($params->{other}, $line->{other}, "UpdateStats save other param in other field of statistics table"); > is ($params->{itemtype}, $line->{itemtype}, "UpdateStats save itemtype param in itemtype field of statistics table"); > is ($params->{location}, $line->{location}, "UpdateStats save location param in location field of statistics table"); > -is ($params->{accountno}, $line->{proccode}, "UpdateStats save accountno param in proccode field of statistics table"); Removed test, could have changed test to confirm it is null/undef. I will wait to the patches on ->add_debit related bug reports Created attachment 82183 [details] [review] Bug 21683: Database update Created attachment 82184 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Created attachment 82185 [details] [review] Bug 21683: Update schema file Created attachment 82188 [details] [review] Bug 21683: (follow-up) Polish the change - based on feedback (In reply to M. Tompsett from comment #4) > Comment on attachment 81282 [details] [review] [review] > Bug 21683: Remove accountlines.accountno > > Review of attachment 81282 [details] [review] [review]: > ----------------------------------------------------------------- Thanks, I added a follow-up based on your comments Marking as BLOCKED, I'll wait for dependency patches Created attachment 85531 [details] [review] Bug 21683: Database update Created attachment 85532 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 85533 [details] [review] Bug 21683: (follow-up) Polish the change - based on feedback Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 85534 [details] [review] Bug 21683: (QA follow-up) Koha::Account::Line rebase fix Created attachment 85535 [details] [review] Bug 21683: Database update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 85536 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 85537 [details] [review] Bug 21683: (follow-up) Polish the change - based on feedback Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 85538 [details] [review] Bug 21683: (QA follow-up) Koha::Account::Line rebase fix Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Brought back up to date and signed off. Shouldn't we use accountline_id for the proccode parameter in UpdateStats? I'm getting failures in all tests that set statistics.
To reproduce:
$ dbic
$ updatedatabase
$ kshell
k$ prove t/db_dependent/Accounts.t \
t/db_dependent/Circ* \
t/db_dependent/Koha/Account*
=> FAIL: Lots of errors like this:
DBD::mysql::st execute failed: Column count doesn't match value count at row 1 [for Statement "INSERT INTO statistics
(datetime,
branch, type, value,
other, itemnumber, itemtype, location,
borrowernumber, proccode, ccode)
VALUES (now(),?,?,?,?,?,?,?,?,?)" with ParamValues: 0='VBVsbXZxI', 1='payment', 2=150, 3='', 4=undef, 5='', 6=undef, 7='205', 8=''] at /home/vagrant/kohaclone/C4/Stats.pm line 136.
(In reply to Tomás Cohen Arazi from comment #21) > Shouldn't we use accountline_id for the proccode parameter in UpdateStats? > I'm getting failures in all tests that set statistics. > > To reproduce: > $ dbic > $ updatedatabase > $ kshell > k$ prove t/db_dependent/Accounts.t \ > t/db_dependent/Circ* \ > t/db_dependent/Koha/Account* > => FAIL: Lots of errors like this: > DBD::mysql::st execute failed: Column count doesn't match value count at row > 1 [for Statement "INSERT INTO statistics > (datetime, > branch, type, value, > other, itemnumber, itemtype, location, > borrowernumber, proccode, ccode) > VALUES (now(),?,?,?,?,?,?,?,?,?)" with ParamValues: 0='VBVsbXZxI', > 1='payment', 2=150, 3='', 4=undef, 5='', 6=undef, 7='205', 8=''] at > /home/vagrant/kohaclone/C4/Stats.pm line 136. Reverting the changes in t/db_dependent/Stats.t and C4/Stats.pm fixes the failures. Still not sure what's the best solution. I think the failures must have been introduced by my rebase.. simple mistake on number of `?` chars.. but.. I agree with you.. I 'think' rather that outright removing the link to the accountline here we should probably be recording accountline_id as an alternative... Created attachment 86221 [details] [review] Bug 21683: Database update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86222 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86223 [details] [review] Bug 21683: (follow-up) Polish the change - based on feedback Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86224 [details] [review] Bug 21683: (QA follow-up) Koha::Account::Line rebase fix Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I've now uploaded a correctly rebased patchset so the tests should all now be passing. Having discussed this with Josef in IRC we are both in agreement that statistucs.proccode should be removed as is. I've looked through the SQL Reports library on the wiki and can't find a single mention of it's use and neither of us could work out a consistent and correct way to report against it which remained accurate. We also couldn't work out any form of migration path from accountno to accountline_id. So.. I believe this patchset is now sound, although I would like one minor followup which actually removed the field from the statistics table too. I believe if we do decide we want a link to accountlines in this table we should treat that as a separate issue. I'll add the followup to remove the field from the database. Created attachment 86225 [details] [review] Bug 21683: Database update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86226 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86227 [details] [review] Bug 21683: (follow-up) Polish the change - based on feedback Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86228 [details] [review] Bug 21683: (QA follow-up) Koha::Account::Line rebase fix Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86229 [details] [review] Bug 21683: (QA follow-up) Drop statistics.proccode Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86230 [details] [review] Bug 21683: Database update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86231 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86232 [details] [review] Bug 21683: (follow-up) Polish the change - based on feedback Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86233 [details] [review] Bug 21683: (QA follow-up) Koha::Account::Line rebase fix Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86234 [details] [review] Bug 21683: (QA follow-up) Drop statistics.proccode Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> (In reply to Martin Renvoize from comment #28) > I've now uploaded a correctly rebased patchset so the tests should all now > be passing. > > Having discussed this with Josef in IRC we are both in agreement that > statistucs.proccode should be removed as is. I've looked through the SQL > Reports library on the wiki and can't find a single mention of it's use and > neither of us could work out a consistent and correct way to report against > it which remained accurate. We also couldn't work out any form of migration > path from accountno to accountline_id. > > So.. I believe this patchset is now sound, although I would like one minor > followup which actually removed the field from the statistics table too. > > I believe if we do decide we want a link to accountlines in this table we > should treat that as a separate issue. > > I'll add the followup to remove the field from the database. Back to you Tomas :) It is not finished yet, the last follow-up is not enough, there is occurence of proccode in: koha-tmpl/intranet-tmpl/prog/en/columns.def t/db_dependent/Stats.t - test if proccode is undef could be removed i think Created attachment 86336 [details] [review] Bug 21683: (follow-up) Remove last occurences of statistics.proccode columns.def is used to define descriptions of db columns in guided reports test is not useful anymore IMO Agreed. Created attachment 86470 [details] [review] Bug 21683: Database update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 86471 [details] [review] Bug 21683: Remove accountlines.accountno Test plan: 1) Play with fines, should work OK 2) Try to print receipts on fines - prinfeercpt.pl, printinvoice.pl 3) git grep getnextacctno -> no occurences 4) git grep accountno should return only: installer/data/mysql/atomicupdate/bug_21683_remove_column_accountno.perl installer/data/mysql/update22to30.pl misc/release_notes/release_notes_3_10_0.txt misc/release_notes/release_notes_3_22_0.txt 5) prove t/db_dependent/Accounts.t t/db_dependent/ILSDI_Services.t t/db_dependent/Stats.t t/db_dependent/Koha/Account.t Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 86472 [details] [review] Bug 21683: (follow-up) Polish the change - based on feedback Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 86473 [details] [review] Bug 21683: (QA follow-up) Koha::Account::Line rebase fix Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 86474 [details] [review] Bug 21683: (QA follow-up) Drop statistics.proccode Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 86475 [details] [review] Bug 21683: (follow-up) Remove last occurences of statistics.proccode columns.def is used to define descriptions of db columns in guided reports test is not useful anymore IMO Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Awesome work all! Pushed to master for 19.05 Enhancement will not be backported to 18.11.x series. *** Bug 15732 has been marked as a duplicate of this bug. *** |