[Bug 15896] New: Use Koha::Account::pay internally for recordpayment
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Bug ID: 15896 Summary: Use Koha::Account::pay internally for recordpayment Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com Blocks| |15894 Depends on| |15895 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15894 [Bug 15894] Unify all account fine payment functions https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 [Bug 15895] Add Koha::Account module, use Koha::Account::pay internally for recordpayment -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Use Koha::Account::pay |Use Koha::Account::pay |internally for |internally for makepayment |recordpayment | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 48332 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48332&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15897 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15897 [Bug 15897] Use Koha::Account::pay internally for recordpayment_selectaccts -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15905 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15905 [Bug 15905] Remove use of makepayment -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15906 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15906 [Bug 15906] Remove use of makepayment in paycollect.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15907 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15907 [Bug 15907] Remove use of makepayment in opac/opac-account-pay-paypal-return.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |mtompset@hotmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #2 from M. Tompsett <mtompset@hotmail.com> --- Oops... forgot to apply 15895. Sorry. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #3 from Owen Leonard <oleonard@myacpl.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #5 from Florent Mara <florent.mara@gmail.com> --- Created attachment 51920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51920&action=edit bug 15896 - A few tests for Koha::Account -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Florent Mara <florent.mara@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |florent.mara@gmail.com --- Comment #6 from Florent Mara <florent.mara@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|15905 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15905 [Bug 15905] Remove use of makepayment -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #48332|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 52139 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52139&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #9 from Josef Moravec <josef.moravec@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52139|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 53314 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53314&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #12 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 53782 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53782&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53314|0 |1 is obsolete| | --- Comment #13 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 53783 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53783&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51920|0 |1 is obsolete| | --- Comment #14 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 53784 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53784&action=edit bug 15896 - A few tests for Koha::Account -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53782|0 |1 is obsolete| | --- Comment #15 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 53785 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53785&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53783|0 |1 is obsolete| | --- Comment #16 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 53786 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53786&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53785|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53784|0 |1 is obsolete| | --- Comment #17 from Marc Véron <veron@veron.ch> --- Created attachment 55924 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55924&action=edit bug 15896 - A few tests for Koha::Account Signed-off-by: Marc Véron <veron@veron.ch> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53786|0 |1 is obsolete| | --- Comment #18 from Marc Véron <veron@veron.ch> --- Created attachment 55925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55925&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #20 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55924|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #22 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|15906 | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15906 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15906 [Bug 15906] Remove use of makepayment in paycollect.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15898 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Will come back to the testing argument of Jonathan. First add a followup to resolve the two fines-problem of bug 15906. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Depends on| |15906 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15906 [Bug 15906] [ONLY PUSH TOGETHER WITH 15896] Remove use of makepayment in paycollect.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 57175 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57175&action=edit Bug 15896 - Unit Tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57175|0 |1 is obsolete| | --- Comment #26 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 57176 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57176&action=edit Bug 15896 - Unit Tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57177 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57177&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57176|0 |1 is obsolete| | --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57178 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57178&action=edit Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55925|0 |1 is obsolete| | --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57182 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57182&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57177|0 |1 is obsolete| | --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57183 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57183&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57178|0 |1 is obsolete| | --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57184 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57184&action=edit Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #32 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57185 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57185&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57182|0 |1 is obsolete| | --- Comment #33 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57186 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57186&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57183|0 |1 is obsolete| | --- Comment #34 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57187 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57187&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57184|0 |1 is obsolete| | --- Comment #35 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57188 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57188&action=edit Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57185|0 |1 is obsolete| | --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57189 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57189&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Aarg Corrected Veron now and lost my own signoff line again.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57186|0 |1 is obsolete| | --- Comment #38 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57190 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57190&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57187|0 |1 is obsolete| | --- Comment #39 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57191 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57191&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57188|0 |1 is obsolete| | --- Comment #40 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57192 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57192&action=edit Bug 15896 - Unit Tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57189|0 |1 is obsolete| | --- Comment #41 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57193 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57193&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #42 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|15907 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15907 [Bug 15907] Remove use of makepayment in opac/opac-account-pay-paypal-return.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #43 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 17.05, thanks Marcel! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Bug 15896 depends on bug 15906, which changed state. Bug 15906 Summary: [ONLY PUSH TOGETHER WITH 15896] Remove use of makepayment in paycollect.pl https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15906 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|Pushed to Master |RESOLVED Resolution|--- |FIXED --- Comment #44 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- This won't get backported to 16.11.x as it is an enhancement and has dependencies that were not backported either. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20631 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20631 [Bug 20631] C4::Acounts claims to use ReturnLostItem but doesn't -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org