Bug 21683 - Remove accountlines.accountno
Summary: Remove accountlines.accountno
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Josef Moravec
QA Contact: Tomás Cohen Arazi
URL:
Whiteboard:
Keywords:
Depends on: 21681 21756 21804
Blocks: 13795 17702 22516
  Show dependency treegraph
 
Reported: 2018-10-26 10:59 UTC by Josef Moravec
Modified: 2019-03-21 18:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21683: Database update (1.72 KB, patch)
2018-10-26 11:25 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (23.91 KB, patch)
2018-10-26 11:25 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: Update schema file (1.43 KB, patch)
2018-10-26 11:25 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: Database update (1.74 KB, patch)
2018-11-09 17:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (24.97 KB, patch)
2018-11-09 17:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: Update schema file (1.44 KB, patch)
2018-11-09 17:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: (follow-up) Polish the change - based on feedback (2.77 KB, patch)
2018-11-09 17:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: Database update (1.71 KB, patch)
2019-02-22 14:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (20.90 KB, patch)
2019-02-22 14:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) Polish the change - based on feedback (2.86 KB, patch)
2019-02-22 14:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Koha::Account::Line rebase fix (912 bytes, patch)
2019-02-22 14:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Database update (1.78 KB, patch)
2019-02-22 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (20.90 KB, patch)
2019-02-22 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) Polish the change - based on feedback (2.86 KB, patch)
2019-02-22 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Koha::Account::Line rebase fix (977 bytes, patch)
2019-02-22 14:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Database update (1.78 KB, patch)
2019-03-07 07:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (20.96 KB, patch)
2019-03-07 07:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) Polish the change - based on feedback (2.86 KB, patch)
2019-03-07 07:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Koha::Account::Line rebase fix (977 bytes, patch)
2019-03-07 07:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Database update (1.78 KB, patch)
2019-03-07 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (20.96 KB, patch)
2019-03-07 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) Polish the change - based on feedback (2.86 KB, patch)
2019-03-07 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Koha::Account::Line rebase fix (977 bytes, patch)
2019-03-07 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Drop statistics.proccode (1.29 KB, patch)
2019-03-07 08:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Database update (1.78 KB, patch)
2019-03-07 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (20.96 KB, patch)
2019-03-07 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) Polish the change - based on feedback (2.86 KB, patch)
2019-03-07 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Koha::Account::Line rebase fix (977 bytes, patch)
2019-03-07 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Drop statistics.proccode (2.65 KB, patch)
2019-03-07 08:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) Remove last occurences of statistics.proccode (2.08 KB, patch)
2019-03-08 06:25 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21683: Database update (1.83 KB, patch)
2019-03-11 17:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21683: Remove accountlines.accountno (21.01 KB, patch)
2019-03-11 17:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21683: (follow-up) Polish the change - based on feedback (2.91 KB, patch)
2019-03-11 17:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Koha::Account::Line rebase fix (1.01 KB, patch)
2019-03-11 17:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21683: (QA follow-up) Drop statistics.proccode (2.70 KB, patch)
2019-03-11 17:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21683: (follow-up) Remove last occurences of statistics.proccode (2.14 KB, patch)
2019-03-11 17:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2018-10-26 10:59:53 UTC
This database column is obsolete.
Comment 1 Josef Moravec 2018-10-26 11:25:05 UTC
Created attachment 81281 [details] [review]
Bug 21683: Database update
Comment 2 Josef Moravec 2018-10-26 11:25:09 UTC
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
Comment 3 Josef Moravec 2018-10-26 11:25:13 UTC
Created attachment 81283 [details] [review]
Bug 21683: Update schema file
Comment 4 M. Tompsett 2018-10-28 14:13:24 UTC
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.
Comment 5 Josef Moravec 2018-11-06 09:42:58 UTC
I will wait to the patches on ->add_debit related bug reports
Comment 6 Josef Moravec 2018-11-09 17:06:28 UTC
Created attachment 82183 [details] [review]
Bug 21683: Database update
Comment 7 Josef Moravec 2018-11-09 17:06:42 UTC
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
Comment 8 Josef Moravec 2018-11-09 17:06:46 UTC
Created attachment 82185 [details] [review]
Bug 21683: Update schema file
Comment 9 Josef Moravec 2018-11-09 17:20:15 UTC
Created attachment 82188 [details] [review]
Bug 21683: (follow-up) Polish the change - based on feedback
Comment 10 Josef Moravec 2018-11-09 17:21:21 UTC
(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
Comment 11 Josef Moravec 2019-01-22 07:24:14 UTC
Marking as BLOCKED, I'll wait for dependency patches
Comment 12 Martin Renvoize 2019-02-22 14:34:59 UTC
Created attachment 85531 [details] [review]
Bug 21683: Database update
Comment 13 Martin Renvoize 2019-02-22 14:35:02 UTC
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>
Comment 14 Martin Renvoize 2019-02-22 14:35:06 UTC
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>
Comment 15 Martin Renvoize 2019-02-22 14:35:09 UTC
Created attachment 85534 [details] [review]
Bug 21683: (QA follow-up) Koha::Account::Line rebase fix
Comment 16 Martin Renvoize 2019-02-22 14:36:43 UTC
Created attachment 85535 [details] [review]
Bug 21683: Database update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2019-02-22 14:36:46 UTC
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>
Comment 18 Martin Renvoize 2019-02-22 14:36:50 UTC
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>
Comment 19 Martin Renvoize 2019-02-22 14:36:54 UTC
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>
Comment 20 Martin Renvoize 2019-02-22 14:38:09 UTC
Brought back up to date and signed off.
Comment 21 Tomás Cohen Arazi 2019-03-06 12:05:32 UTC
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.
Comment 22 Tomás Cohen Arazi 2019-03-06 12:31:04 UTC
(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.
Comment 23 Martin Renvoize 2019-03-06 12:39:44 UTC
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...
Comment 24 Martin Renvoize 2019-03-07 07:59:00 UTC
Created attachment 86221 [details] [review]
Bug 21683: Database update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2019-03-07 07:59:04 UTC
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>
Comment 26 Martin Renvoize 2019-03-07 07:59:07 UTC
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>
Comment 27 Martin Renvoize 2019-03-07 07:59:10 UTC
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>
Comment 28 Martin Renvoize 2019-03-07 08:09:11 UTC
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.
Comment 29 Martin Renvoize 2019-03-07 08:14:07 UTC
Created attachment 86225 [details] [review]
Bug 21683: Database update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2019-03-07 08:14:10 UTC
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>
Comment 31 Martin Renvoize 2019-03-07 08:14:14 UTC
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>
Comment 32 Martin Renvoize 2019-03-07 08:14:17 UTC
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>
Comment 33 Martin Renvoize 2019-03-07 08:14:21 UTC
Created attachment 86229 [details] [review]
Bug 21683: (QA follow-up) Drop statistics.proccode

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2019-03-07 08:16:19 UTC
Created attachment 86230 [details] [review]
Bug 21683: Database update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2019-03-07 08:16:23 UTC
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>
Comment 36 Martin Renvoize 2019-03-07 08:16:26 UTC
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>
Comment 37 Martin Renvoize 2019-03-07 08:16:30 UTC
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>
Comment 38 Martin Renvoize 2019-03-07 08:16:33 UTC
Created attachment 86234 [details] [review]
Bug 21683: (QA follow-up) Drop statistics.proccode

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2019-03-07 08:17:50 UTC
(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 :)
Comment 40 Josef Moravec 2019-03-08 06:17:47 UTC
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
Comment 41 Josef Moravec 2019-03-08 06:25:32 UTC
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
Comment 42 Martin Renvoize 2019-03-08 13:26:28 UTC
Agreed.
Comment 43 Tomás Cohen Arazi 2019-03-11 17:20:02 UTC
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>
Comment 44 Tomás Cohen Arazi 2019-03-11 17:20:07 UTC
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>
Comment 45 Tomás Cohen Arazi 2019-03-11 17:20:12 UTC
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>
Comment 46 Tomás Cohen Arazi 2019-03-11 17:20:16 UTC
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>
Comment 47 Tomás Cohen Arazi 2019-03-11 17:20:20 UTC
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>
Comment 48 Tomás Cohen Arazi 2019-03-11 17:20:24 UTC
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>
Comment 49 Nick Clemens 2019-03-21 18:32:52 UTC
Awesome work all!

Pushed to master for 19.05