[Koha-bugs] [Bug 21683] Remove accountlines.accountno

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21683

--- Comment #4 from M. Tompsett <mtompset at hotmail.com> ---
Comment on attachment 81282
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81282
Bug 21683: Remove accountlines.accountno

Review of attachment 81282:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=21683&attachment=81282)
-----------------------------------------------------------------

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.

-- 
You are receiving this mail because:
You are watching all bug changes.