[Bug 15895] New: Add Koha::Account module, replace recordpayment with Koha::Account::pay
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Bug ID: 15895 Summary: Add Koha::Account module, replace recordpayment with Koha::Account::pay 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=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15675 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15675 [Bug 15675] Add issue_id column to accountlines and use it for updating fines -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15894 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15894 [Bug 15894] Unify all account fine payment functions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add Koha::Account module, |Add Koha::Account module, |replace recordpayment with |use Koha::Account::pay |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=15895 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=15895 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 48331 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48331&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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15896 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896 [Bug 15896] 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=15895 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=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15899 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15899 [Bug 15899] Remove the use of recordpayment -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15900 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15900 [Bug 15900] Remove use of recordpayment in ProcessOfflinePayment -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15901 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15901 [Bug 15901] Remove use of recordpayment in C4::SIP::ILS::Transaction::FeePayment -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15902 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15902 [Bug 15902] Remove use of recordpayment in process_koc.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15903 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15903 [Bug 15903] Remove use of recordpayment 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=15895 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 48331 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=48331 Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment Review of attachment 48331: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=15895&attachment=48331) ----------------------------------------------------------------- ::: t/db_dependent/Accounts.t @@ +158,5 @@
+ my $line1 = Koha::Account::Line->new({ borrowernumber => $borrower->borrowernumber, amountoutstanding => 100 })->store(); + my $line2 = Koha::Account::Line->new({ borrowernumber => $borrower->borrowernumber, amountoutstanding => 200 })->store(); + $line1->_result->discard_changes; + $line2->_result->discard_changes;
Why do you need these 2 calls? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #2)
Comment on attachment 48331 [details] [review] Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment
Review of attachment 48331 [details] [review]: -----------------------------------------------------------------
::: t/db_dependent/Accounts.t @@ +158,5 @@
+ my $line1 = Koha::Account::Line->new({ borrowernumber => $borrower->borrowernumber, amountoutstanding => 100 })->store(); + my $line2 = Koha::Account::Line->new({ borrowernumber => $borrower->borrowernumber, amountoutstanding => 200 })->store(); + $line1->_result->discard_changes; + $line2->_result->discard_changes;
Why do you need these 2 calls?
The discard_changes call updates the objects from the database. Without them, any default values inserted by MySQL are not available in the initial object. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- As an aside, maybe we should just add a reset() ( or maybe refresh() ) method to Koha::Object so we don't have to go through _result() which is specific to our implementation and breaks encapsulation. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Bug 15895 depends on bug 15675, which changed state. Bug 15675 Summary: Add issue_id column to accountlines and use it for updating fines https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15675 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=15895 Owen Leonard <oleonard@myacpl.org> 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=15895 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #48331|0 |1 is obsolete| | --- Comment #5 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 49453 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49453&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: Owen Leonard <oleonard@myacpl.org> Paying a fine using the "Pay amount" button works correctly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Kyle M Hall from comment #4)
As an aside, maybe we should just add a reset() ( or maybe refresh() ) method to Koha::Object so we don't have to go through _result() which is specific to our implementation and breaks encapsulation.
Just a thought: should not we update Koha::Object->store to make it returns an updated/refreshed object? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #6)
(In reply to Kyle M Hall from comment #4)
As an aside, maybe we should just add a reset() ( or maybe refresh() ) method to Koha::Object so we don't have to go through _result() which is specific to our implementation and breaks encapsulation.
Just a thought: should not we update Koha::Object->store to make it returns an updated/refreshed object?
It's not a bad idea, but given that we've not needed such behavior yet, it would probably be best to make it an optional method. I imagine there would be an associated cost for refetching the data from the database. I would be good to run some benchmarks, if there is no appreciable speed difference I'd say we should definitely do what you propose! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Kyle M Hall from comment #3)
(In reply to Jonathan Druart from comment #2)
Comment on attachment 48331 [details] [review] [review] Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment
Review of attachment 48331 [details] [review] [review]: -----------------------------------------------------------------
::: t/db_dependent/Accounts.t @@ +158,5 @@
+ my $line1 = Koha::Account::Line->new({ borrowernumber => $borrower->borrowernumber, amountoutstanding => 100 })->store(); + my $line2 = Koha::Account::Line->new({ borrowernumber => $borrower->borrowernumber, amountoutstanding => 200 })->store(); + $line1->_result->discard_changes; + $line2->_result->discard_changes;
Why do you need these 2 calls?
The discard_changes call updates the objects from the database. Without them, any default values inserted by MySQL are not available in the initial object.
Actually the 2 objects $line1 and $line2 are not reused later, I don't think they are useful. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Kyle, since this patch is the bases for a rewrite of the Account module and that other patches are not signed off yet, will you agree if we wait for the next release before pushing it? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #9)
Kyle, since this patch is the bases for a rewrite of the Account module and that other patches are not signed off yet, will you agree if we wait for the next release before pushing it?
Yes, I think that's a good plan! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 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=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #49453|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 52138 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52138&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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |srdjan@catalyst.net.nz --- Comment #12 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- I know this has been signed off, but I think there's no such thing as "an Account", therefore pay() should go to Koha::Patron. In that case, rather than letting surrounding code look for account lines, it can support passing criteria, and make a accountlines() call to get the lines. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Srdjan Jankovic from comment #12)
I know this has been signed off, but I think there's no such thing as "an Account", therefore pay() should go to Koha::Patron. In that case, rather than letting surrounding code look for account lines, it can support passing criteria, and make a accountlines() call to get the lines.
I don't think having the pay method be part of Koha::Patron makes any more sense than this. If it were to be moved anywhere, I think maybe moving it to Koha::AccountLines would more appropriate if anything. I'd be happy to move the method to a difference class after we've got these pushed to master. The dependency chain for this is complex and I would prefer not to introduce such changes until the tail end of the patch series. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #14 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- I suggested Koha::Patron for the reason that you never pay for multiple patrons' fees. Patron->accountlines() makes sure of that. Koha::AccountLines is better than Koha::Account because there's only one method call. I still think Patron->pay() is superior. The only argument against is you may not need the borrowers record at that stage, but that's not very likely, and can be made irrelevant with loading Patron with account lines preloaded or something. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Srdjan Jankovic from comment #14)
I suggested Koha::Patron for the reason that you never pay for multiple patrons' fees. Patron->accountlines() makes sure of that.
Koha::AccountLines is better than Koha::Account because there's only one method call.
I still think Patron->pay() is superior. The only argument against is you may not need the borrowers record at that stage, but that's not very likely, and can be made irrelevant with loading Patron with account lines preloaded or something.
I understand. Due to the complexity of the bug series ( https://bugs.koha-community.org/bugzilla3/showdependencygraph.cgi?id=15895 ) I'd like to wait until the patch set has all been pushed to master or at least passed qa. At that time I can submit a bug and patch to make this change. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- signed-off-by line missing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Kyle M Hall <kyle@bywatersolutions.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=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #52138|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 53883 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53883&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: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Lari Taskula <larit@student.uef.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53883|0 |1 is obsolete| | --- Comment #18 from Lari Taskula <larit@student.uef.fi> --- Created attachment 54543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54543&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: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lari Taskula <larit@student.uef.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15895 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54543|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 55870 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55870&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: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lari Taskula <larit@student.uef.fi> Signed-off-by: Jonathan Druart <jonathan.druart@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=15895 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 55871 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55871&action=edit Bug 15895: Remove useless discard_changes calls Signed-off-by: Jonathan Druart <jonathan.druart@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=15895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org