|
Description
Kyle M Hall (khall)
2016-02-24 14:10:20 UTC
Created attachment 48331 [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 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? (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. 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. Created attachment 49453 [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: Owen Leonard <oleonard@myacpl.org> Paying a fine using the "Pay amount" button works correctly. (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? (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! (In reply to Kyle M Hall from comment #3) > (In reply to Jonathan Druart from comment #2) > > Comment on attachment 48331 [details] [review] [review] [review] > > Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for > > recordpayment > > > > Review of attachment 48331 [details] [review] [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. 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? (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! Created attachment 52138 [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 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. (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. 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. (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. signed-off-by line missing. Created attachment 53883 [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: Owen Leonard <oleonard@myacpl.org> Created attachment 54543 [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: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lari Taskula <larit@student.uef.fi> Created attachment 55870 [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: 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> Created attachment 55871 [details] [review] Bug 15895: Remove useless discard_changes calls Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 16.11! |