Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment
Summary: Add Koha::Account module, use Koha::Account::pay internally for recordpayment
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 15675
Blocks: 15894 15896 15899 15900 15901 15902 15903
  Show dependency treegraph
 
Reported: 2016-02-24 14:10 UTC by Kyle M Hall
Modified: 2017-12-07 22:17 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (14.16 KB, patch)
2016-02-24 14:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (13.68 KB, patch)
2016-03-23 16:14 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (13.53 KB, patch)
2016-06-07 15:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (13.58 KB, patch)
2016-08-02 10:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (13.63 KB, patch)
2016-08-17 15:44 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 15895 - Add Koha::Account module, use Koha::Account::pay internally for recordpayment (13.70 KB, patch)
2016-09-28 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15895: Remove useless discard_changes calls (1.01 KB, patch)
2016-09-28 11:07 UTC, Jonathan Druart
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:10:20 UTC

    
Comment 1 Kyle M Hall 2016-02-24 14:16:46 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-02-29 10:25:21 UTC
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?
Comment 3 Kyle M Hall 2016-03-07 19:25:02 UTC
(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.
Comment 4 Kyle M Hall 2016-03-07 19:26:53 UTC
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.
Comment 5 Owen Leonard 2016-03-23 16:14:28 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-03-23 21:16:53 UTC
(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?
Comment 7 Kyle M Hall 2016-03-23 22:05:35 UTC
(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!
Comment 8 Jonathan Druart 2016-04-18 13:30:24 UTC
(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.
Comment 9 Jonathan Druart 2016-04-18 13:37:53 UTC
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?
Comment 10 Kyle M Hall 2016-04-20 13:22:47 UTC
(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!
Comment 11 Kyle M Hall 2016-06-07 15:10:35 UTC Comment hidden (obsolete)
Comment 12 Srdjan Jankovic 2016-07-13 05:24:34 UTC
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.
Comment 13 Kyle M Hall 2016-07-13 10:42:43 UTC
(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.
Comment 14 Srdjan Jankovic 2016-07-15 00:57:32 UTC
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.
Comment 15 Kyle M Hall 2016-07-15 10:21:49 UTC
(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.
Comment 16 Jonathan Druart 2016-08-02 09:21:39 UTC
signed-off-by line missing.
Comment 17 Kyle M Hall 2016-08-02 10:22:56 UTC Comment hidden (obsolete)
Comment 18 Lari Taskula 2016-08-17 15:44:06 UTC
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>
Comment 19 Jonathan Druart 2016-09-28 11:07:27 UTC
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>
Comment 20 Jonathan Druart 2016-09-28 11:07:31 UTC
Created attachment 55871 [details] [review]
Bug 15895: Remove useless discard_changes calls

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Kyle M Hall 2016-09-29 11:14:19 UTC
Pushed to master for 16.11!