Created attachment 48332 [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
Oops... forgot to apply 15895. Sorry.
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.
(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!
Created attachment 51920 [details] [review] bug 15896 - A few tests for Koha::Account
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.
Created attachment 52139 [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
(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!
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
(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
Created attachment 53314 [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
Created attachment 53782 [details] [review] Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment This is the first patch in a series to unify all payment functions into a single method. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Accounts.t 3) Test fine payment via the "Pay amount" button Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 53783 [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>
Created attachment 53784 [details] [review] bug 15896 - A few tests for Koha::Account
Created attachment 53785 [details] [review] Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment This is the first patch in a series to unify all payment functions into a single method. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Accounts.t 3) Test fine payment via the "Pay amount" button Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
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>
Created attachment 55924 [details] [review] bug 15896 - A few tests for Koha::Account Signed-off-by: Marc Véron <veron@veron.ch>
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>
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
(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.
(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 :)
(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.
(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.
Will come back to the testing argument of Jonathan. First add a followup to resolve the two fines-problem of bug 15906.
Created attachment 57175 [details] [review] Bug 15896 - Unit Tests
Created attachment 57176 [details] [review] Bug 15896 - Unit Tests
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>
Created attachment 57178 [details] [review] Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
Created attachment 57184 [details] [review] Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
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>
Created attachment 57188 [details] [review] Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
Aarg Corrected Veron now and lost my own signoff line again..
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>
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>
Created attachment 57192 [details] [review] Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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
Pushed to master for 17.05, thanks Marcel!
This won't get backported to 16.11.x as it is an enhancement and has dependencies that were not backported either.