Bug 22588 - Simplify getting account information in opac and self checkout module
Summary: Simplify getting account information in opac and self checkout module
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Josef Moravec
QA Contact: Testopia
URL:
Keywords:
Depends on: 21772
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-27 08:50 UTC by Josef Moravec
Modified: 2019-04-26 11:49 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00


Attachments
Bug 22588: Simplify getting fines information in opac and selfcheckout (11.26 KB, patch)
2019-03-27 09:03 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22588: Simplify getting fines information in opac and selfcheckout (11.28 KB, patch)
2019-03-27 12:42 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22588: (follow-up) Simplify getting accounts even more (3.66 KB, patch)
2019-03-28 09:01 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22588: Simplify getting fines information in opac and selfcheckout (11.37 KB, patch)
2019-04-03 12:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22588: (follow-up) Simplify getting accounts even more (3.71 KB, patch)
2019-04-03 12:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22588: Simplify getting fines information in opac and selfcheckout (11.18 KB, patch)
2019-04-15 15:43 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22588: (follow-up) Simplify getting accounts even more (3.70 KB, patch)
2019-04-15 15:43 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2019-03-27 08:50:11 UTC

    
Comment 1 Josef Moravec 2019-03-27 09:03:16 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>
Comment 2 Owen Leonard 2019-03-27 12:42:15 UTC
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>
Comment 3 Josef Moravec 2019-03-27 13:11:51 UTC
> 
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Thanks for testing Owen!
Comment 4 Tomás Cohen Arazi 2019-03-27 17:51:28 UTC
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;
Comment 5 Josef Moravec 2019-03-28 09:00:14 UTC
(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
Comment 6 Josef Moravec 2019-03-28 09:01:51 UTC
Created attachment 87131 [details] [review]
Bug 22588: (follow-up) Simplify getting accounts even more
Comment 7 Katrin Fischer 2019-04-03 12:18:40 UTC
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>
Comment 8 Katrin Fischer 2019-04-03 12:18:43 UTC
Created attachment 87351 [details] [review]
Bug 22588: (follow-up) Simplify getting accounts even more

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Josef Moravec 2019-04-15 15:43:07 UTC
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>
Comment 10 Josef Moravec 2019-04-15 15:43:10 UTC
Created attachment 88022 [details] [review]
Bug 22588: (follow-up) Simplify getting accounts even more

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Josef Moravec 2019-04-15 15:43:34 UTC
Rebased on master
Comment 12 Nick Clemens 2019-04-16 13:26:45 UTC
Awesome work all!

Pushed to master for 19.05
Comment 13 Martin Renvoize 2019-04-26 11:49:33 UTC
Enhancement will not be backported to 18.11.x series.