Bug 15906 - Remove use of makepayment in paycollect.pl
Summary: Remove use of makepayment in paycollect.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15896 15905
  Show dependency treegraph
 
Reported: 2016-02-24 15:51 UTC by Kyle M Hall
Modified: 2017-12-07 22:15 UTC (History)
3 users (show)

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


Attachments
Bug 15906 - Remove use of makepayment in paycollect.pl (1.44 KB, patch)
2016-02-24 15:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15906 - Remove use of makepayment in paycollect.pl (2.61 KB, patch)
2016-06-07 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15906 - Remove use of makepayment in paycollect.pl (1.45 KB, patch)
2016-06-07 15:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15906 - Remove use of makepayment in paycollect.pl (1.54 KB, patch)
2016-08-08 14:54 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 15906 - Remove use of makepayment in paycollect.pl (1.59 KB, patch)
2016-11-04 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15906: [QA Follow-up] Correct a partial payment (1.47 KB, patch)
2016-11-04 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-02-24 15:51:52 UTC

    
Comment 1 Kyle M Hall 2016-02-24 15:53:16 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-06-07 14:55:50 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2016-06-07 15:12:18 UTC Comment hidden (obsolete)
Comment 4 Michael Kuhn 2016-07-30 22:19:57 UTC
In a user account some reasons for payments were created. Then a click on "Make payment" shows these individual fines & charges. One ist selected, then the button "Pay" is clicked. The payment does not work. Instead the following message appears:

Software error:

Can't locate object method "new" via package "Koha::Account" at /home/koha/src/members/paycollect.pl line 114.
Comment 5 Owen Leonard 2016-08-08 14:54:29 UTC
Created attachment 54172 [details] [review]
[SIGNED-OFF] Bug 15906 - Remove use of makepayment in paycollect.pl

Test plan:
1) Apply this patch
2) Make a payment in full using the "Pay" button
3) Note payment succeeds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Michael Kuhn 2016-08-12 18:36:33 UTC
In a user account some reasons for payments were created. In the leftside-tab "Details" I click on "Make payment" and the individual fines & charges are shown. One is selected, then the button "Pay" on the right is clicked. Now the system asks for confirmation but still the following error message does appear and nothing is paid:

Software error: Can't locate object method "new" via package "Koha::Account" at /home/sandbox1/src/members/paycollect.pl line 114.

The buttons "Pay amount" and "Pay selected" at the bottom work all right, though. But not the "Pay" button.
Comment 7 Marcel de Rooy 2016-10-28 06:39:23 UTC
(In reply to Michael Kuhn from comment #6)
> In a user account some reasons for payments were created. In the
> leftside-tab "Details" I click on "Make payment" and the individual fines &
> charges are shown. One is selected, then the button "Pay" on the right is
> clicked. Now the system asks for confirmation but still the following error
> message does appear and nothing is paid:
> 
> Software error: Can't locate object method "new" via package "Koha::Account"
> at /home/sandbox1/src/members/paycollect.pl line 114.
> 
> The buttons "Pay amount" and "Pay selected" at the bottom work all right,
> though. But not the "Pay" button.

Cannot reproduce this error with the Pay button.
Note various warnings like this in the log:
[Fri Oct 28 08:34:25 2016] paycollect.pl: Argument "\\x{33}\\x{30}..." isn't numeric in sprintf at /usr/lib/perl5/Template/Filters.pm line 467
Comment 8 Marcel de Rooy 2016-10-28 06:45:45 UTC
When paying partial amounts via Pay and via Pay amount, the amount is positive via Pay and negative via Pay amount:

+-------------+-----------+-------------------+---------------+---------------------+
| accounttype | amount    | amountoutstanding | lastincrement | timestamp           |
+-------------+-----------+-------------------+---------------+---------------------+
| F           | 30.000000 |         14.000000 |          NULL | 2016-10-28 08:38:42 |
| Pay         | 10.000000 |          0.000000 |          NULL | 2016-10-28 08:34:25 |
| Pay         | -1.000000 |          0.000000 |          NULL | 2016-10-28 08:37:04 |
| Pay         |  2.000000 |          0.000000 |          NULL | 2016-10-28 08:38:15 |
| Pay         | -3.000000 |          0.000000 |          NULL | 2016-10-28 08:38:42 |
+-------------+-----------+-------------------+---------------+---------------------+

Does not look good to me.
Comment 9 Michael Kuhn 2016-10-29 12:41:53 UTC
I succesfully applied patches 15896 and 15906.

In a user account some reasons for payments were created. In the leftside-tab "Details" I click on "Make payment" and the individual fines & charges are shown. One is selected, then the button "Pay" on the right is clicked. Now the system asks for confirmation and the amount is paid.

(So I can't reproduce the message "Software error: ..." from 2016-07-30 anymore and evyerything looks alright).

I made another paymont via button "Pay amount" and this works also as I expect it.

I then created a fee of 7, table "accountlines" looks as follows.

mysql> select accounttype, amount, amountoutstanding, lastincrement, timestamp from accountlines;
+-------------+------------+-------------------+---------------+---------------------+
| accounttype | amount     | amountoutstanding | lastincrement | timestamp           |
+-------------+------------+-------------------+---------------+---------------------+
| N           |   7.000000 |          7.000000 |          NULL | 2016-10-29 12:33:03 |
+-------------+------------+-------------------+---------------+---------------------+

Out of 7 I pay only 4. Now table "accountlines" looks as follows - the original line (N) changes in fields "amountoutstanding" and "timestamp" and another line (Pay) is added. This looks as I expect ist.

+-------------+------------+-------------------+---------------+---------------------+
| accounttype | amount     | amountoutstanding | lastincrement | timestamp           |
+-------------+------------+-------------------+---------------+---------------------+
| N           |   7.000000 |          3.000000 |          NULL | 2016-10-29 12:33:42 |
| Pay         |   4.000000 |          0.000000 |          NULL | 2016-10-29 12:33:42 |
+-------------+------------+-------------------+---------------+---------------------+

However, as already reported by M. de Rooy the logfile shows a lot of warnings like the following for every Pay action:

[Sat Oct 29 12:33:43.008191 2016] [cgi:error] [pid 3382] [client 10.0.2.2:44132] AH01215: [Sat Oct 29 12:33:43 2016] paycollect.pl: Argument "7.00&descr..." isn't numeric in sprintf at /usr/lib/x86_64-linux-gnu/perl5/5.20/Template/Filters.pm line 467., referer: http://localhost:8081/cgi-bin/koha/members/paycollect.pl?borrowernumber=52&pay_individual=1&accounttype=N&amount=7.00&amountoutstanding=7.00%26description%3DN%26title%3D&notify_id=1&notify_level=0&accountlines_id=13&remote_user=
Comment 10 Marcel de Rooy 2016-11-04 08:21:20 UTC
Create manual invoice: 20
Pay button: 1
Pay amount button: 2
Results in the following records in accountlines (still a negative amount with Pay amount):

|            1513 |     NULL |              1 |         1 |       NULL | 2016-11-04 | 20.000000 | F           | NULL    | F           |         17.000000 |          NULL | 2016-11-04 09:14:33 |         1 |            0 | test411 |          1 |
|            1514 |     NULL |              1 |         2 |       NULL | 2016-11-04 |  1.000000 |             | NULL    | Pay         |          0.000000 |          NULL | 2016-11-04 09:13:45 |         0 |            0 |         |          1 |
|            1515 |     NULL |              1 |         2 |       NULL | 2016-11-04 | -2.000000 |             | NULL    | Pay         |          0.000000 |          NULL | 2016-11-04 09:14:33 |         0 |            0 |         |          1 |
Comment 11 Marcel de Rooy 2016-11-04 08:48:34 UTC
OK Looking further: I must conclude that the actual error that I discover is in makepartialpayment, not included in this change.
Adding a follow-up.
Comment 12 Marcel de Rooy 2016-11-04 09:46:18 UTC
Created attachment 57168 [details] [review]
Bug 15906 - Remove use of makepayment in paycollect.pl

Test plan:
1) Apply this patch
2) Make a payment in full using the "Pay" button
3) Note payment succeeds

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2016-11-04 09:46:22 UTC
Created attachment 57169 [details] [review]
Bug 15906: [QA Follow-up] Correct a partial payment

The amount in accountlines should be saved as a negative amount, but
amount outstanding is correct.

Note: This routine is actually used only once (in paycollect.pl). And is
only cosmetically tested in Accounts.t.

Test plan:
[1] Create a manual invoice, Fine, say 20.
[2] Click on the Pay button next to this fine, and pay 1.
[3] Pay again, using 'Pay amount' button, and pay 2.
[4] Pay again, using 'Pay' on the same line, the full remaining amount.
[5] Verify that you see -1, -2 and -17 on the Accounts tab.
[6] Run t/db_dependent/Accounts.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2016-11-04 09:56:37 UTC
QA: Still working here
Comment 15 Marcel de Rooy 2016-11-04 10:17:03 UTC
The simple test scenario works.
But if we add another fine, the situation changes little bit:

If I have fine1==20 and fine2==30, and I do individual payment on the second fine, the first fine will be paid first. Not really a big problem, but it is not what you expect.

This is not caused by this patch set, but should be solved by bug 15896.
So I am labeling this patch with "push together".

Passed QA
Comment 16 Kyle M Hall 2016-12-09 17:59:25 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 17 Katrin Fischer 2016-12-12 22:24:05 UTC
This won't get backported to 16.11.x as it is an enhancement.