Bug 17586 - Add the Koha::Account->balance method
Summary: Add the Koha::Account->balance method
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)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17585
Blocks: 17588 20942
  Show dependency treegraph
 
Reported: 2016-11-08 15:33 UTC by Jonathan Druart
Modified: 2018-06-14 13:47 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17596: Add the Koha::Account::Lines->get_balance method (2.67 KB, patch)
2016-11-08 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17586: Add the Koha::Account::Lines->get_balance method (2.62 KB, patch)
2016-11-08 15:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17586: Add the Koha::Account::Lines->get_balance method (2.69 KB, patch)
2016-12-02 09:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17586: Add the Koha::Account::Lines->get_balance method (2.73 KB, patch)
2016-12-02 15:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17586: Add the Koha::Account::Lines->get_balance method (2.73 KB, patch)
2016-12-06 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17586: Move ->get_balance to Koha::Account->balance (2.93 KB, patch)
2016-12-06 08:14 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 Jonathan Druart 2016-11-08 15:33:00 UTC

    
Comment 1 Jonathan Druart 2016-11-08 15:52:11 UTC
Created attachment 57337 [details] [review]
Bug 17596: Add the Koha::Account::Lines->get_balance method

Test plan:
  prove t/db_dependent/Accounts.t
should return green
Comment 2 Jonathan Druart 2016-11-08 15:53:00 UTC
Created attachment 57338 [details] [review]
Bug 17586: Add the Koha::Account::Lines->get_balance method

Test plan:
  prove t/db_dependent/Accounts.t
should return green
Comment 3 Mika 2016-11-29 22:18:24 UTC
Tests failed

t/db_dependent/Accounts.t .. 1/20     # No tests run!

#   Failed test 'No tests run for subtest "get_balance"'
#   at t/db_dependent/Accounts.t line 392.
The method get_account_lines is not covered by tests!# Looks like your test exited with 255 just after 20.
t/db_dependent/Accounts.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/20 subtests 

Test Summary Report
-------------------
t/db_dependent/Accounts.t (Wstat: 65280 Tests: 20 Failed: 1)
  Failed test:  20
  Non-zero exit status: 255
Files=1, Tests=20,  2 wallclock secs ( 0.03 usr  0.00 sys +  1.97 cusr  0.06 csys =  2.06 CPU)
Result: FAIL
Comment 4 Jonathan Druart 2016-11-30 07:36:02 UTC
(In reply to Mika from comment #3)
> Tests failed
> 
> t/db_dependent/Accounts.t .. 1/20     # No tests run!
> 
> #   Failed test 'No tests run for subtest "get_balance"'
> #   at t/db_dependent/Accounts.t line 392.
> The method get_account_lines is not covered by tests!# Looks like your test
> exited with 255 just after 20.
> t/db_dependent/Accounts.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
> Failed 1/20 subtests 
> 
> Test Summary Report
> -------------------
> t/db_dependent/Accounts.t (Wstat: 65280 Tests: 20 Failed: 1)
>   Failed test:  20
>   Non-zero exit status: 255
> Files=1, Tests=20,  2 wallclock secs ( 0.03 usr  0.00 sys +  1.97 cusr  0.06
> csys =  2.06 CPU)
> Result: FAIL

This patch depends on bug 17585.
Comment 5 Josef Moravec 2016-12-02 09:14:29 UTC
Created attachment 57906 [details] [review]
[SIGNED-OFF] Bug 17586: Add the Koha::Account::Lines->get_balance method

Test plan:
  prove t/db_dependent/Accounts.t
should return green

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 6 Josef Moravec 2016-12-02 09:15:20 UTC
> This patch depends on bug 17585.

As I just signed off the 17585, I am continuing with this
Comment 7 Tomás Cohen Arazi 2016-12-02 15:43:03 UTC
Created attachment 57929 [details] [review]
Bug 17586: Add the Koha::Account::Lines->get_balance method

Test plan:
  prove t/db_dependent/Accounts.t
should return green

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Kyle M Hall 2016-12-02 19:26:44 UTC
I don't want to be *too* much of a pedant, but wouldn't this method be better in Koha::Account? I see there are a good number of patches that depend on this, so if that change will require many further changes, I'd be happy with just a final bug at the end of the chain to move it to Koha::Account.

Let me know what you think and reset the status accordingly.

Kyle
Comment 9 Jonathan Druart 2016-12-05 08:29:53 UTC
(In reply to Kyle M Hall from comment #8)
> I don't want to be *too* much of a pedant, but wouldn't this method be
> better in Koha::Account? I see there are a good number of patches that
> depend on this, so if that change will require many further changes, I'd be
> happy with just a final bug at the end of the chain to move it to
> Koha::Account.

How would you write this line in that case:
  my $balance = $patron->get_account_lines->get_balance;
(from the patch on bug 17588).

Maybe just $patron->get_account_balance then?
Comment 10 Kyle M Hall 2016-12-05 15:57:04 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Kyle M Hall from comment #8)
> > I don't want to be *too* much of a pedant, but wouldn't this method be
> > better in Koha::Account? I see there are a good number of patches that
> > depend on this, so if that change will require many further changes, I'd be
> > happy with just a final bug at the end of the chain to move it to
> > Koha::Account.
> 
> How would you write this line in that case:
>   my $balance = $patron->get_account_lines->get_balance;
> (from the patch on bug 17588).
> 
> Maybe just $patron->get_account_balance then?

I would do $patron->account()->balance() with account() returning the Koha::Account object, and balance() returning the account balance. Of course, I'm a fan of being succinct ; )
Comment 11 Jonathan Druart 2016-12-06 08:14:14 UTC
Created attachment 57990 [details] [review]
Bug 17586: Add the Koha::Account::Lines->get_balance method

Test plan:
  prove t/db_dependent/Accounts.t
should return green

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Jonathan Druart 2016-12-06 08:14:19 UTC
Created attachment 57991 [details] [review]
Bug 17586: Move ->get_balance to Koha::Account->balance
Comment 13 Jonathan Druart 2016-12-06 08:16:32 UTC
(In reply to Kyle M Hall from comment #10)
> I would do $patron->account()->balance() with account() returning the
> Koha::Account object, and balance() returning the account balance. Of
> course, I'm a fan of being succinct ; )

I am not 100% this change is the right thing to do but we could go like that to start.
For now, the Koha::Account module's responsibilities are not clearly defined and it's not easy to guess what it will become.
Comment 14 Kyle M Hall 2016-12-06 10:43:04 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Kyle M Hall from comment #10)
> > I would do $patron->account()->balance() with account() returning the
> > Koha::Account object, and balance() returning the account balance. Of
> > course, I'm a fan of being succinct ; )
> 
> I am not 100% this change is the right thing to do but we could go like that
> to start.
> For now, the Koha::Account module's responsibilities are not clearly defined
> and it's not easy to guess what it will become.

That's fine. I just wanted to put that thought out. We may have a better notion of where such a method would go after the fact and we can make that change at the end rather then have to modify all the dependent patches.
Comment 15 Jonathan Druart 2016-12-06 12:04:12 UTC
(In reply to Kyle M Hall from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > (In reply to Kyle M Hall from comment #10)
> > > I would do $patron->account()->balance() with account() returning the
> > > Koha::Account object, and balance() returning the account balance. Of
> > > course, I'm a fan of being succinct ; )
> > 
> > I am not 100% this change is the right thing to do but we could go like that
> > to start.
> > For now, the Koha::Account module's responsibilities are not clearly defined
> > and it's not easy to guess what it will become.
> 
> That's fine. I just wanted to put that thought out. We may have a better
> notion of where such a method would go after the fact and we can make that
> change at the end rather then have to modify all the dependent patches.

I have modified the dependent patches, so let's go with it :)
Comment 16 Kyle M Hall 2016-12-16 14:50:40 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 17 Katrin Fischer 2016-12-18 20:53:50 UTC
This won't get ported back to 16.11.x as it is an enhancement.