Bug 6634 - manager_id not populated when paying fines
Summary: manager_id not populated when paying fines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.8
Hardware: All All
: P3 normal (vote)
Assignee: Ian Walls
QA Contact: Bugs List
URL:
Keywords:
Depends on: 6718
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-26 18:04 UTC by Nicole C. Engard
Modified: 2013-12-05 19:59 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
sample accountlines data (116.89 KB, image/png)
2011-07-26 18:04 UTC, Nicole C. Engard
Details
another sample with account type (127.70 KB, image/png)
2011-07-27 01:13 UTC, Nicole C. Engard
Details
patch (4.70 KB, patch)
2011-07-27 11:50 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bugs 6634: manager_id not recorded for payments and rental charges (4.81 KB, patch)
2011-08-14 12:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
patch to populate manager_id and itemnumber when paying fines (2.18 KB, patch)
2012-02-07 15:42 UTC, Maxime Pelletier
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6634 manager_id and itemnumber not populated when paying fines (2.28 KB, patch)
2012-02-09 14:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Fix 3 other cases of manager_id not populated (3.72 KB, patch)
2012-03-28 21:28 UTC, Maxime Pelletier
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 2011-07-26 18:04:53 UTC
Created attachment 4743 [details]
sample accountlines data

The accountlines table has a manager_id field but this field is not populated when you pay fines (or in some other instances).  This field should be populated with the person who is logged in at the time the fine is paid or at the time any change is made to the account so you can track who's doing what with money in your library.
Comment 1 Nicole C. Engard 2011-07-27 01:13:18 UTC
Created attachment 4745 [details]
another sample with account type
Comment 2 Nicole C. Engard 2011-07-27 11:50:09 UTC Comment hidden (obsolete)
Comment 3 Ian Walls 2011-08-12 16:54:29 UTC
This patch touches two C4 modules; could I get a second signoff?  I'm not comfortable marking this as "passed QA" when only me and one of my fellow ByWaterians have checked it.
Comment 4 Katrin Fischer 2011-08-14 12:37:06 UTC
I did some additional testing on the bug and it seems there is still no manager_id saved in accountlines when you do a 'writeoff'.

To reproduce:
- create manual invoice. Example. Sundry, 0.25
- writeoff fine from pay fines page

Paying fines the way described on the bugs was working perfectly. 

I am signing off on this as the bug only mentions paying fines and I see no problems there.

The writeoff problem can is a separate bug and can be fixed in a follow up.
Comment 5 Katrin Fischer 2011-08-14 12:37:45 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2011-08-14 13:10:16 UTC
Files separate bug 6718 for writeoff problem.
Comment 7 Ian Walls 2011-08-14 13:20:32 UTC
Thank you, Katrin, for your thorough testing. The writeoff issue can be covered with a separate bug report. Marking this one as Passed QA.
Comment 8 Chris Cormack 2011-08-15 03:32:47 UTC
Pushed, please test
Comment 9 Paul Poulain 2011-10-26 16:38:48 UTC
I just tested and the manager_id is NULL (i've checked, the code is here).
I added a manual debt => manager_id=1, it's OK
I added a payement through "pay fine" > tested both pay amount and "pay" on the left of the debt line
in both cases, I got manager_ID=NULL
Comment 10 Paul Poulain 2011-10-26 16:39:19 UTC
a patch has been pushed, but it fixes only a part of the problem
Comment 11 Katrin Fischer 2011-11-02 20:28:00 UTC
I am removing the patch sent status as more work is needed on this bug.
Comment 12 Maxime Pelletier 2012-02-03 21:30:16 UTC
This was broken again with the patch for bug 5713
Comment 13 Maxime Pelletier 2012-02-07 15:42:46 UTC Comment hidden (obsolete)
Comment 14 Fred P 2012-02-08 16:05:26 UTC
Does anyone else get the behavior that if you create three manual invoices and pay one, that the other two have their outstanding balance zeroed out?
Comment 15 Fred P 2012-02-08 17:37:51 UTC
Turns out that a writeoff uses a smallint value for the account no, the account no is used to clear the amountoustanding. The account no is supposed to increment, but writeoff assigns the maximum value to the smallint, it fails to increment, and the next accountline is assigned the same (maximum) value for account no. The next time you apply a payment, all of the accountlines with that account no receive the same processing. 

It's only a writeoff that affects this, and I am using 3.5, not sure if this manifests in 3.6.
Comment 16 Fred P 2012-02-08 18:39:24 UTC
Sorry to jump in on your bug, I am going to write the "Writeoff" issue up as a separate bug. Thatcher suggested it was similar to bug#3498, but I think it is different enough that it needs separate documentation.
Comment 17 Owen Leonard 2012-02-09 14:53:12 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2012-02-09 14:54:45 UTC
Tested successfully in master following this procedure (Comment 5):

1.  Check out an item with an issuing charge
2.  Renew the item
3.  Pay one of the rental charges on the item
4.  Verify in koha.accountlines that each of the three above operations
    recorded your borrowernumber in accountlines.manager_id

manager_id was correctly populated for all transactions.
Comment 19 Paul Poulain 2012-02-13 14:38:33 UTC
QA comment

small patch, code is OK.

Will push it, but my tests shows that there is one case that is still not covered: when paying fines with the button "Pay Selected" or "Pay Amount", the columns are still NULL (manager_id and itemnumber)
When paying from "Pay" or "Pay Amount", it's OK though

I see in the sub recordpayement that the same kind of fix should be made:

    my $usth = $dbh->prepare(
        "INSERT INTO accountlines
  (borrowernumber, accountno,date,amount,description,accounttype,amountoutstanding)
  VALUES (?,?,now(),?,'Payment,thanks','Pay',?)"

The sub chargelostitem has also the same problem:
        my $sth2=$dbh->prepare("INSERT INTO accountlines
        (borrowernumber,accountno,date,amount,description,accounttype,amountoutstanding,itemnumber)
        VALUES (?,?,now(),?,?,'L',?,?)");

(itemnumber is OK, but manager_id is not)
Comment 20 Paul Poulain 2012-02-13 14:39:14 UTC
The 1st patch has been pushed, status resetted to "assigned" to address the 2 other cases identified
Comment 21 Maxime Pelletier 2012-03-28 21:28:36 UTC
Created attachment 8676 [details] [review]
Fix 3 other cases of manager_id not populated

Sorry for missing them at first and taking this long to send a new patch!
Comment 22 Owen Leonard 2012-03-30 13:46:20 UTC
If there were still instances where this was broken I must not have tested all the possible cases. Can you describe what other operations need testing?
Comment 23 Paul Poulain 2012-04-02 14:27:26 UTC
(In reply to comment #22)
> If there were still instances where this was broken I must not have tested
> all the possible cases. Can you describe what other operations need testing?

(not sure you were answering my QA comment, but thinking so)
I haven't tested, it's just that i've checked the code, and some "INSERT" clauses where lacking the manager_id & itemnumber parameters.
Maybe that's some dead/unreachable code, but that's a code we have in recordpayement and chargelostitem subs
Comment 24 Irma Birchall 2012-06-11 14:40:56 UTC
I have tested the payment of fines and Paul has verified in koha.accountlines.manager_id that each of the operations recorded the borrowernumber and the manager_id details. This patch can be signed off.
Comment 25 Ian Walls 2012-06-15 17:21:08 UTC
Adds managerid recording to recordpayment(), chargelostitem(), and recordpayment_selectaccts() subroutines.  Marking Passed QA.
Comment 26 Paul Poulain 2012-06-20 20:19:33 UTC
New follow-up pushed

(it's not an ENH, chris_c, I think those patches should to ported to 3.8)
Comment 27 Chris Cormack 2012-06-23 08:20:43 UTC
Pushed to 3.8.x, will be in 3.8.3