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
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
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
(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.
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>
> This patch depends on bug 17585. As I just signed off the 17585, I am continuing with this
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>
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
(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?
(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 ; )
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>
Created attachment 57991 [details] [review] Bug 17586: Move ->get_balance to Koha::Account->balance
(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.
(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.
(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 :)
Pushed to master for 17.05, thanks Jonathan!
This won't get ported back to 16.11.x as it is an enhancement.