|
Description
Josef Moravec
2019-03-27 08:50:11 UTC
Created attachment 87049 [details] [review] Bug 22588: Simplify getting fines information in opac and selfcheckout Test plan: 0) Have patron with some fines and credits 1) Go to opac and log in 2) Go to 'your summary' page and confirm it shows the right amount and description for both, credit and debit 3) Go to 'your fines' page and confirm it show right information 4) Go to self checkout page and confirm the fines are right here Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 87065 [details] [review] Bug 22588: Simplify getting fines information in opac and selfcheckout Test plan: 0) Have patron with some fines and credits 1) Go to opac and log in 2) Go to 'your summary' page and confirm it shows the right amount and description for both, credit and debit 3) Go to 'your fines' page and confirm it show right information 4) Go to self checkout page and confirm the fines are right here Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> >
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Thanks for testing Owen!
I like the idea in general but:
- shouldn't we be using line.{is_debit|is_credit} instead of comparing line.amount > 0? Keep in mind that we found pathological cases in the wild where the amountoutstanding got flipped. We better rely on the methods we have.
- The fact that we aren't using $account->outstanding_debits and $account->outstanding_credits but instead keep searching for all accountlines makes some noise to me. I guess it is not straightforward as we want to display all outstanding debits and credits ordered by date. Just something to think about a bit. Maybe we should add amountountstanding => { '!=' => 0 } to the search line.
- We could replace the search with
my $account = $patron->account;
my $total = $account->balance;
my $lines = $account->lines;
(In reply to Tomás Cohen Arazi from comment #4) > I like the idea in general but: > - shouldn't we be using line.{is_debit|is_credit} instead of comparing > line.amount > 0? Keep in mind that we found pathological cases in the wild > where the amountoutstanding got flipped. We better rely on the methods we > have. Definitely yes, I don't know how could I forget these methods... I'll add a follow-up for this > - The fact that we aren't using $account->outstanding_debits and > $account->outstanding_credits but instead keep searching for all > accountlines makes some noise to me. I guess it is not straightforward as we > want to display all outstanding debits and credits ordered by date. Just > something to think about a bit. Maybe we should add amountountstanding => { > '!=' => 0 } to the search line. Maybe we should think a bit more, what we wan't to show to users in accounts information table - now it is something like cash book, but is that right way of presenting account lines to user? Do they understand it enough? > - We could replace the search with > > my $account = $patron->account; > my $total = $account->balance; > my $lines = $account->lines; Will be done in follow-up Created attachment 87131 [details] [review] Bug 22588: (follow-up) Simplify getting accounts even more Created attachment 87350 [details] [review] Bug 22588: Simplify getting fines information in opac and selfcheckout Test plan: 0) Have patron with some fines and credits 1) Go to opac and log in 2) Go to 'your summary' page and confirm it shows the right amount and description for both, credit and debit 3) Go to 'your fines' page and confirm it show right information 4) Go to self checkout page and confirm the fines are right here Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 87351 [details] [review] Bug 22588: (follow-up) Simplify getting accounts even more Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88021 [details] [review] Bug 22588: Simplify getting fines information in opac and selfcheckout Test plan: 0) Have patron with some fines and credits 1) Go to opac and log in 2) Go to 'your summary' page and confirm it shows the right amount and description for both, credit and debit 3) Go to 'your fines' page and confirm it show right information 4) Go to self checkout page and confirm the fines are right here Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 88022 [details] [review] Bug 22588: (follow-up) Simplify getting accounts even more Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Rebased on master Awesome work all! Pushed to master for 19.05 Enhancement will not be backported to 18.11.x series. |