Bug 15896 - Use Koha::Account::pay internally for makepayment
Summary: Use Koha::Account::pay internally for makepayment
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: 15895 15906
Blocks: 15894 15897 20631
  Show dependency treegraph
 
Reported: 2016-02-24 14:18 UTC by Kyle M Hall
Modified: 2018-04-24 18:02 UTC (History)
7 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 15896 - Use Koha::Account::pay internally for makepayment (8.31 KB, patch)
2016-02-24 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug 15896 - A few tests for Koha::Account (3.38 KB, patch)
2016-05-31 04:34 UTC, Florent Mara
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.32 KB, patch)
2016-06-07 15:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.60 KB, patch)
2016-07-12 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (13.64 KB, patch)
2016-07-28 15:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.66 KB, patch)
2016-07-28 15:58 UTC, Josef Moravec
Details | Diff | Splinter Review
bug 15896 - A few tests for Koha::Account (3.38 KB, patch)
2016-07-28 15:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (13.64 KB, patch)
2016-07-28 15:59 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.66 KB, patch)
2016-07-28 15:59 UTC, Josef Moravec
Details | Diff | Splinter Review
bug 15896 - A few tests for Koha::Account (3.51 KB, patch)
2016-09-29 13:29 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.83 KB, patch)
2016-09-29 13:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15896 - Unit Tests (2.61 KB, patch)
2016-11-04 10:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15896 - Unit Tests (2.41 KB, patch)
2016-11-04 10:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect (1.93 KB, patch)
2016-11-04 10:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896 - Unit Tests (2.51 KB, patch)
2016-11-04 10:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.92 KB, patch)
2016-11-04 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect (1.93 KB, patch)
2016-11-04 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896 - Unit Tests (2.51 KB, patch)
2016-11-04 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896: [QA Follow-up] Fix parameter name in POD (1023 bytes, patch)
2016-11-04 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.78 KB, patch)
2016-11-04 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect (1.93 KB, patch)
2016-11-04 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896 - Unit Tests (2.51 KB, patch)
2016-11-04 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896: [QA Follow-up] Fix parameter name in POD (1023 bytes, patch)
2016-11-04 12:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896 - Use Koha::Account::pay internally for makepayment (8.84 KB, patch)
2016-11-04 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect (1.93 KB, patch)
2016-11-04 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896 - Unit Tests (2.51 KB, patch)
2016-11-04 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15896: [QA Follow-up] Fix parameter name in POD (1023 bytes, patch)
2016-11-04 12:40 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 14:18:25 UTC

    
Comment 1 Kyle M Hall 2016-02-24 14:23:23 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2016-03-03 03:37:04 UTC
Oops... forgot to apply 15895. Sorry.
Comment 3 Owen Leonard 2016-03-23 16:36:38 UTC
This isn't working for me. Here are the steps that are showing a problem:

1. Start with a patron who has a $16.00 fine on their account.
2. Go to Fines, click the "Pay" button next to the fine.
3. Enter a partial amount in the "Collect from patron" field: 8.00.
4. Confirm the payment. The correct payment is registered.
5. Click the "Pay" button again.
6. Click the "Confirm" button without changing the "Collect from
   patron" amount (now 8.00).

The "Pay fines" tab now shows 

Amount: -8.00
Amount outstanding: -8.00
Sub total: -8.00
Total due: -8.00

Testing the process in master and it worked correctly. The last step results in a 0.00 balance.
Comment 4 Kyle M Hall 2016-04-18 12:31:36 UTC
(In reply to Owen Leonard from comment #3)

Can you please re-test? I ran your scenario on master and with this patch and the end result for me was that both were identical. If you are still getting a different result a screencast would be very appreciated!

Thanks!
Comment 5 Florent Mara 2016-05-31 04:34:05 UTC Comment hidden (obsolete)
Comment 6 Florent Mara 2016-05-31 04:38:07 UTC
I have not had time to fully validate this patch but started to write some tests for Koha::Account module. (I hope these tests won't be causing trouble) 

There would preferably be more tests but the method is ~150+ lines which makes it difficult to get started.
Comment 7 Kyle M Hall 2016-06-07 15:10:46 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2016-06-07 16:27:24 UTC
(In reply to Florent Mara from comment #6)
> I have not had time to fully validate this patch but started to write some
> tests for Koha::Account module. (I hope these tests won't be causing
> trouble) 
> 
> There would preferably be more tests but the method is ~150+ lines which
> makes it difficult to get started.

Not a problem at all! Unit tests are always welcome! Thanks!
Comment 9 Josef Moravec 2016-06-16 11:56:12 UTC
I have same issue as Owen, here is screencast:
https://www.youtube.com/watch?v=yLbFgxl_g50

I feel it could be somehow related to bug 15397
Comment 10 Kyle M Hall 2016-06-27 15:42:57 UTC
(In reply to Josef Moravec from comment #9)
> I have same issue as Owen, here is screencast:
> https://www.youtube.com/watch?v=yLbFgxl_g50
> 
> I feel it could be somehow related to bug 15397

Indeed, the issue is that makepartialpayment has a bug. This bug is not resolved by this patch because it does not alter makepartialpayment. Bug 15898 *does* do that however. The test plan for this bug needs to be clarified:

3) Test fine payment *in full* via the "Pay" button
Comment 11 Kyle M Hall 2016-07-12 16:09:05 UTC Comment hidden (obsolete)
Comment 12 Josef Moravec 2016-07-28 15:58:29 UTC Comment hidden (obsolete)
Comment 13 Josef Moravec 2016-07-28 15:58:38 UTC Comment hidden (obsolete)
Comment 14 Josef Moravec 2016-07-28 15:59:36 UTC
Created attachment 53784 [details] [review]
bug 15896 - A few tests for Koha::Account
Comment 15 Josef Moravec 2016-07-28 15:59:40 UTC Comment hidden (obsolete)
Comment 16 Josef Moravec 2016-07-28 15:59:45 UTC
Created attachment 53786 [details] [review]
Bug 15896 - Use Koha::Account::pay internally for makepayment

This is the second patch in a series to unify all payment functions into
a single mathod

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
3) Test fine payment via the "Pay" button

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Marc Véron 2016-09-29 13:29:30 UTC
Created attachment 55924 [details] [review]
bug 15896 - A few tests for Koha::Account

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 18 Marc Véron 2016-09-29 13:30:44 UTC
Created attachment 55925 [details] [review]
Bug 15896 - Use Koha::Account::pay internally for makepayment

This is the second patch in a series to unify all payment functions into
a single mathod

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
3) Test fine payment via the "Pay" button

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 19 Jonathan Druart 2016-10-03 14:32:00 UTC
Kyle, what are you testing on t/db_dependent/Koha/Account.t?
it seems that it is already covered by the tests on t/db_dependent/Accounts.t
Comment 20 Kyle M Hall 2016-10-11 13:23:42 UTC
(In reply to Jonathan Druart from comment #19)
> Kyle, what are you testing on t/db_dependent/Koha/Account.t?
> it seems that it is already covered by the tests on t/db_dependent/Accounts.t

I did not add that unit test file. I'll just deprecate it since it is not necessary.
Comment 21 Jonathan Druart 2016-10-18 08:31:25 UTC
(In reply to Kyle M Hall from comment #20)
> (In reply to Jonathan Druart from comment #19)
> > Kyle, what are you testing on t/db_dependent/Koha/Account.t?
> > it seems that it is already covered by the tests on t/db_dependent/Accounts.t
> 
> I did not add that unit test file. I'll just deprecate it since it is not
> necessary.

Then now you need to provide tests for these changes :)
Comment 22 Kyle M Hall 2016-11-03 16:28:26 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Kyle M Hall from comment #20)
> > (In reply to Jonathan Druart from comment #19)
> > > Kyle, what are you testing on t/db_dependent/Koha/Account.t?
> > > it seems that it is already covered by the tests on t/db_dependent/Accounts.t
> > 
> > I did not add that unit test file. I'll just deprecate it since it is not
> > necessary.
> 
> Then now you need to provide tests for these changes :)

Jonathan, I'm not understanding, the additional behaviors are already being unit tested by the unit tests for makepayment.
Comment 23 Jonathan Druart 2016-11-04 09:31:31 UTC
(In reply to Kyle M Hall from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > (In reply to Kyle M Hall from comment #20)
> > > (In reply to Jonathan Druart from comment #19)
> > > > Kyle, what are you testing on t/db_dependent/Koha/Account.t?
> > > > it seems that it is already covered by the tests on t/db_dependent/Accounts.t
> > > 
> > > I did not add that unit test file. I'll just deprecate it since it is not
> > > necessary.
> > 
> > Then now you need to provide tests for these changes :)
> 
> Jonathan, I'm not understanding, the additional behaviors are already being
> unit tested by the unit tests for makepayment.

Yes but now it's not makepayment that makes the job, but Koha::Account->pay (which is starting to be too big btw).
Moreover the makepayment subroutine is not enough covered by tests.
Feel free to ask someone else opinion if you disagree.
Comment 24 Marcel de Rooy 2016-11-04 10:40:13 UTC
Will come back to the testing argument of Jonathan.
First add a followup to resolve the two fines-problem of bug 15906.
Comment 25 Kyle M Hall 2016-11-04 10:41:23 UTC
Created attachment 57175 [details] [review]
Bug 15896 - Unit Tests
Comment 26 Kyle M Hall 2016-11-04 10:42:22 UTC
Created attachment 57176 [details] [review]
Bug 15896 - Unit Tests
Comment 27 Marcel de Rooy 2016-11-04 10:49:28 UTC
Created attachment 57177 [details] [review]
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect

We can solve the minor problem reported on 15906 now by using the
accountlines_id parameter of this report.

Test plan:
[1] Add two manual fines (say 20 and 30).
[2] Pay the second one in full, and check that the first one is not paid
    first. So the 20 should remain and not the 30 with 20 outstanding.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2016-11-04 10:51:54 UTC
Created attachment 57178 [details] [review]
Bug 15896 - Unit Tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2016-11-04 12:35:57 UTC
Created attachment 57182 [details] [review]
Bug 15896 - Use Koha::Account::pay internally for makepayment

This is the second patch in a series to unify all payment functions into
a single mathod

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
3) Test fine payment via the "Pay" button

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2016-11-04 12:36:02 UTC
Created attachment 57183 [details] [review]
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect

We can solve the minor problem reported on 15906 now by using the
accountlines_id parameter of this report.

Test plan:
[1] Add two manual fines (say 20 and 30).
[2] Pay the second one in full, and check that the first one is not paid
    first. So the 20 should remain and not the 30 with 20 outstanding.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2016-11-04 12:36:07 UTC
Created attachment 57184 [details] [review]
Bug 15896 - Unit Tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2016-11-04 12:36:11 UTC
Created attachment 57185 [details] [review]
Bug 15896: [QA Follow-up] Fix parameter name in POD

Also adjust typo feees.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2016-11-04 12:38:01 UTC
Created attachment 57186 [details] [review]
Bug 15896 - Use Koha::Account::pay internally for makepayment

This is the second patch in a series to unify all payment functions into
a single mathod

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
3) Test fine payment via the "Pay" button

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Followed test plan, works as expected.
Signed-off-by: Marc Veron <veron@veron.ch>
Comment 34 Marcel de Rooy 2016-11-04 12:38:06 UTC
Created attachment 57187 [details] [review]
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect

We can solve the minor problem reported on 15906 now by using the
accountlines_id parameter of this report.

Test plan:
[1] Add two manual fines (say 20 and 30).
[2] Pay the second one in full, and check that the first one is not paid
    first. So the 20 should remain and not the 30 with 20 outstanding.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2016-11-04 12:38:11 UTC
Created attachment 57188 [details] [review]
Bug 15896 - Unit Tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2016-11-04 12:38:15 UTC
Created attachment 57189 [details] [review]
Bug 15896: [QA Follow-up] Fix parameter name in POD

Also adjust typo feees.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2016-11-04 12:38:59 UTC
Aarg Corrected Veron now and lost my own signoff line again..
Comment 38 Marcel de Rooy 2016-11-04 12:39:44 UTC
Created attachment 57190 [details] [review]
Bug 15896 - Use Koha::Account::pay internally for makepayment

This is the second patch in a series to unify all payment functions into
a single mathod

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Accounts.t
3) Test fine payment via the "Pay" button

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Followed test plan, works as expected.
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2016-11-04 12:40:29 UTC
Created attachment 57191 [details] [review]
Bug 15896: [QA Follow-up] Add accountlines_id parameter in paycollect

We can solve the minor problem reported on 15906 now by using the
accountlines_id parameter of this report.

Test plan:
[1] Add two manual fines (say 20 and 30).
[2] Pay the second one in full, and check that the first one is not paid
    first. So the 20 should remain and not the 30 with 20 outstanding.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2016-11-04 12:40:34 UTC
Created attachment 57192 [details] [review]
Bug 15896 - Unit Tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2016-11-04 12:40:38 UTC
Created attachment 57193 [details] [review]
Bug 15896: [QA Follow-up] Fix parameter name in POD

Also adjust typo feees.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2016-11-04 12:51:58 UTC
QA Comment:
Since we are in a migration/consolidation process for Accounts routines, I will not block this patch set, although Jonathan had some concerns about incomplete test coverage and code quality.

I agree with Jonathan that we should have some more tests here, but note that you are not doing complete new things here (just move code).
You added two tests btw. We now have calls to ->pay with and without accountlines_id.
The statistics test in Accounts.t is quite rudimentary. Could be extended.
A test related to the call of ReturnLostItem could be added. And what about sip type?

About the code quality. The code in sub pay is getting longer. That is true. If we first consolidate everything in one spot, we can still improve. But maybe this is just of matter of adding one or two private subroutines to have a more readable sub pay?

Another note in the larger scope of making payments: The call to ReturnLostItem was and now is only when you pay on one line. But what about paying multiple lines including such a lost fee? Seems to be a point of attention.

On the paycollect interface: If you pay more, the interface responds with a warn Pay less or equal. The interface should also protest against amount zero or 0.00 btw.

And in connection to makepayment: There is still a call to makepayment in opac/opac-account-pay-paypal-return.pl. Glancing through that code, I am wondering if you really want to pay all accountlines without checking if the sum matches the amount. But I may be mistaken?

In conclusion: I have no objection to pushing this code now. But in this migration project we should try to improve later on.

Passed QA
Comment 43 Kyle M Hall 2016-12-09 17:59:11 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 44 Katrin Fischer 2016-12-12 22:26:28 UTC
This won't get backported to 16.11.x as it is an enhancement and has dependencies that were not backported either.