Bug 13961 - Option to include patron's total amount of fines in notices
Summary: Option to include patron's total amount of fines in notices
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-07 13:37 UTC by Katrin Fischer
Modified: 2023-11-01 19:26 UTC (History)
15 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 13961: Option to include patron's total amount of fines in notices (1.88 KB, patch)
2015-08-25 06:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Screenshot from diff view - white chars in empty line 648 in C4/letters.pm (9.61 KB, image/png)
2015-08-28 13:04 UTC, Marc Véron
Details
[Follow-up] Remove whitespace from line 648 in C4/Letters.pm (911 bytes, patch)
2015-08-28 15:34 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13961: Option to include patron's total amount of fines in notices (2.10 KB, patch)
2016-04-21 14:22 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13961: Option to include patron's total amount of fines in notices (2.21 KB, patch)
2020-02-13 12:42 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Add currency format handling (2.27 KB, patch)
2020-02-13 12:45 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Add unit test (3.15 KB, patch)
2020-02-13 12:45 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Option to include patron's total amount of fines in notices (2.27 KB, patch)
2020-06-23 09:40 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 13961: Add currency format handling (2.33 KB, patch)
2020-06-23 09:40 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 13961: Add unit test (3.21 KB, patch)
2020-06-23 09:40 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 13961: Fix QA issues (1.99 KB, patch)
2020-07-07 10:15 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Check that borrowernumber isn't a reference (1.80 KB, patch)
2020-07-07 10:16 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Option to include patron's total amount of fines in notices (2.27 KB, patch)
2021-02-10 07:36 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Add currency format handling (2.33 KB, patch)
2021-02-10 07:36 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Add unit test (3.19 KB, patch)
2021-02-10 07:37 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Fix QA issues (1.99 KB, patch)
2021-02-10 07:38 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Check that borrowernumber isn't a reference (1.80 KB, patch)
2021-02-10 07:38 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 13961: Check that borrowernumber isn't a reference (1.83 KB, patch)
2021-06-11 15:12 UTC, Salman Ali
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2015-04-07 13:37:07 UTC
It would be nice if we had the option to include information about the patron's total of fines in notices.

- overdues (fine per item can be printed, but no total)
- checkin
- checkout
- predue
- due
Comment 1 Joonas Kylmälä 2015-08-20 11:25:17 UTC
Koha -> Tools -> Notices, you mean? Would there be one one row per patron and there would be all the info? I'm not sure if I understood right what you suggested.
Comment 2 Katrin Fischer 2015-08-24 13:25:15 UTC
Just looking for a new placeholder. We got <<items.fine>> to display the fine per item. And we have something like <<today>> to display today's date. The suggestion would be to have something like <<totalfine>> or similar to print the total amount owed by a patron in the letter. So a single placeholder - not one to be repeated in the letter.
Comment 3 Joonas Kylmälä 2015-08-24 13:47:45 UTC
(In reply to Katrin Fischer from comment #2)
> Just looking for a new placeholder. We got <<items.fine>> to display the
> fine per item. And we have something like <<today>> to display today's date.
> The suggestion would be to have something like <<totalfine>> or similar to
> print the total amount owed by a patron in the letter. So a single
> placeholder - not one to be repeated in the letter.

Yeah, that would great. Actually we just spoke about that kind of feature in our library. I think I could add that quickly there. :)
Comment 4 Joonas Kylmälä 2015-08-25 06:29:17 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-08-28 08:42:38 UTC
Works as expected.
However, C4/Letters.pm line 648 has forbidden pattern (trailing space chars).
Comment 6 Joonas Kylmälä 2015-08-28 09:15:44 UTC
(In reply to Marc Véron from comment #5)
> Works as expected.
> However, C4/Letters.pm line 648 has forbidden pattern (trailing space chars).

I don't see any in that line.
Comment 7 Marc Véron 2015-08-28 13:04:54 UTC Comment hidden (obsolete)
Comment 8 Joonas Kylmälä 2015-08-28 13:42:09 UTC
(In reply to Marc Véron from comment #7)
> Created attachment 42077 [details]
> Screenshot from diff view - white chars in empty line 648 in C4/letters.pm
> 
> Hi Joonas,
> 
> There are some space chars in an empty line, see screenshot from diff view.
> 
> Marc

When I apply the patch and try to edit with Gedit, Atom, Nano none of these editors see the whitespace. Can you please make a follow-up where you remove it because I'm not able to.
Comment 9 Joonas Kylmälä 2015-08-28 13:46:30 UTC
(In reply to Joonas Kylmälä from comment #8)
> (In reply to Marc Véron from comment #7)
> > Created attachment 42077 [details]
> > Screenshot from diff view - white chars in empty line 648 in C4/letters.pm
> > 
> > Hi Joonas,
> > 
> > There are some space chars in an empty line, see screenshot from diff view.
> > 
> > Marc
> 
> When I apply the patch and try to edit with Gedit, Atom, Nano none of these
> editors see the whitespace. Can you please make a follow-up where you remove
> it because I'm not able to.

I think the problem is with the bugzilla's diff shower because in my patch I didn't even modify that line.
Comment 10 Marc Véron 2015-08-28 15:34:00 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2016-04-21 14:22:35 UTC
Created attachment 50517 [details] [review]
Bug 13961: Option to include patron's total amount of fines in notices

Possibility to show patrons's total amount of fines in notices.

Test plan:
1. Apply patch
2. Modify some notice in Tools -> notices & slips
3. See that in the left side of where one can edit message's body is now
   also a new attribute that one can include to the text:
   borrowers.totalfine
4. Include that to the text and see it showing the patron's total amount
   of fines.

Sponsored-by: Vaara-kirjastot

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Marc Véron 2016-04-21 14:28:02 UTC
Comment on attachment 42089 [details] [review]
[Follow-up] Remove whitespace from line 648 in C4/Letters.pm

QA tools no longer complain regarding first patch. Obsoleting this follow-up.
Comment 13 Katrin Fischer 2016-04-28 12:48:28 UTC
Testing this on top of bug 16378
Comment 14 Kyle M Hall 2016-04-28 13:03:50 UTC
Please add a unit test for this addition.

Also, could you please change the key needed from borrowers.totalfine to something like borrowers.account_balance? I think that would be much more readable and explanatory.
Comment 15 Katrin Fischer 2016-04-28 13:15:18 UTC
I was also looking into this and have another note: Please take a look in Overdues.pm at how items.fine is formatted:

            $item->{'fine'} = currency_format($currency_format, "$fine", FMT_SYMBOL);
            # if active currency isn't correct ISO code fallback to sprintf
            $item->{'fine'} = sprintf('%.2f', $fine) unless $item->{'fine'};

I think we should use the same formatting for all money amounts if possible - especially as it's very likely that people will want to use those 2 together!
Comment 16 Ray Delahunty 2016-05-11 14:45:33 UTC
Are we sure that <<items.fines>> is working as it should? We are on 3.20.01 and we recently changed ODUE to use <item></item> allowing nice layout rather than the horridly formatted <<items.content>> variable. But often (not always) the fine amount wasn’t pulling through. Also to our horror, after the change sometimes only the first overdue would be listed, and not the full list. The consensus here is that the <<items.fine>> tag can cause problems when generating notices and we have reverted to <<items.content>>. This has resolved issues with notices being truncated after the first item. (We can find no pattern for why some of these fail but we suspect it is related to the <<items.fine>> variable.)
Ray Delahunty
University of the Arts London
Comment 17 Katrin Fischer 2016-05-11 16:18:15 UTC
Hi, this bug is about a different feature, so it would be good to open a separate bug report, maybe with a 'see also' link to this one if you want to.

We have been using the items.fine variable without any problems for quite some time now - although not in 3.20, but in 3.18 and 3.22.

One reson for the fine not printing can be the sequence the cronjobs are being run. fines.pl should be run before overdue_notices.pl (currently not the default, see bug 11144). Or that you haven't defined a proper ISO code for your active currency.
Comment 18 Katrin Fischer 2016-05-11 16:19:08 UTC
Btw: you can use <item></item> and leave the <<items.fine>> out too!
Comment 19 Josef Moravec 2019-03-25 15:27:14 UTC
In ISSUESLIP ans ISSUEQSLIP is possible to do this:


[% USE Price %]

# your normal issue slip content

[% SET balance = borrower.account.balance %]
[% IF balance > 0 %]
Patron has debit of [% balance | $Price %].
[% END %]
[% IF balance < 0 %]
Patron has credit of [% balance | $Price %].
[% END %]
Comment 20 Emmi Takkinen 2020-02-13 12:42:06 UTC
Created attachment 98819 [details] [review]
Bug 13961: Option to include patron's total amount of fines in notices

Fixed patch to apply master version.
Comment 21 Emmi Takkinen 2020-02-13 12:45:14 UTC
Created attachment 98820 [details] [review]
Bug 13961: Add currency format handling

Added currency format handling proposed in comments.
Comment 22 Emmi Takkinen 2020-02-13 12:45:58 UTC
Created attachment 98821 [details] [review]
Bug 13961: Add unit test
Comment 23 ByWater Sandboxes 2020-06-23 09:40:07 UTC
Created attachment 106192 [details] [review]
Bug 13961: Option to include patron's total amount of fines in notices

Possibility to show patrons's total amount of fines in notices.

Test plan:
1. Apply patch
2. Modify some notice in Tools -> notices & slips
3. See that in the left side of where one can edit message's body is now
   also a new attribute that one can include to the text:
   borrowers.totalfine
4. Include that to the text and see it showing the patron's total amount
   of fines.

Sponsored-by: Vaara-kirjastot

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 24 ByWater Sandboxes 2020-06-23 09:40:11 UTC
Created attachment 106193 [details] [review]
Bug 13961: Add currency format handling

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 25 ByWater Sandboxes 2020-06-23 09:40:14 UTC
Created attachment 106194 [details] [review]
Bug 13961: Add unit test

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 26 Katrin Fischer 2020-07-04 21:14:23 UTC
Hi Emmi,

1) there are 2 problems hightlighted by QA tools:

 FAIL	C4/Letters.pm
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).  

 FAIL	t/db_dependent/Letters.t
   FAIL	  critic



2) Also, the tests are failing for me:

Test Summary Report
-------------------
t/db_dependent/Letters.t (Wstat: 512 Tests: 74 Failed: 2)
  Failed tests:  63, 67
  Non-zero exit status: 2

Can you please check?
Comment 27 Emmi Takkinen 2020-07-07 10:15:42 UTC
Created attachment 106623 [details] [review]
Bug 13961: Fix QA issues

This patch fixes following QA issues:

 FAIL	C4/Letters.pm
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).

 FAIL	t/db_dependent/Letters.t
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).

Sponsored-by: Koha-Suomi Oy
Comment 28 Emmi Takkinen 2020-07-07 10:16:31 UTC
Created attachment 106624 [details] [review]
Bug 13961: Check that borrowernumber isn't a reference

Some SendAlerts() tests in Letters.t failed due SQL::Abstract errors,
caused by HASH sometimes set as borrowernumber.

Added code to check if borrowernumber is defined and it isn't a reference.
To test prove t/db_dependent/Letters.t

Sponsored-by: Koha-Suomi Oy
Comment 29 Mazen Khallaf 2021-01-19 21:46:47 UTC
Unable to apply patch (step 1)

$ git bz apply 13961
Bug 13961 - Option to include patron's total amount of fines in notices

106192 - Bug 13961: Option to include patron's total amount of fines in notices
106193 - Bug 13961: Add currency format handling
106194 - Bug 13961: Add unit test
106623 - Bug 13961: Fix QA issues
106624 - Bug 13961: Check that borrowernumber isn't a reference

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 13961: Option to include patron's total amount of fines in notices
Applying: Bug 13961: Add currency format handling
error: sha1 information is lacking or useless (C4/Letters.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 13961: Add currency format handling
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-13961-Add-currency-format-handling-LIVbPh.patch
Comment 30 Emmi Takkinen 2021-02-10 07:36:09 UTC
Created attachment 116631 [details] [review]
Bug 13961: Option to include patron's total amount of fines in notices

Possibility to show patrons's total amount of fines in notices.

Test plan:
1. Apply patch
2. Modify some notice in Tools -> notices & slips
3. See that in the left side of where one can edit message's body is now
   also a new attribute that one can include to the text:
   borrowers.totalfine
4. Include that to the text and see it showing the patron's total amount
   of fines.

Sponsored-by: Vaara-kirjastot

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 31 Emmi Takkinen 2021-02-10 07:36:42 UTC
Created attachment 116632 [details] [review]
Bug 13961: Add currency format handling

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 32 Emmi Takkinen 2021-02-10 07:37:28 UTC
Created attachment 116633 [details] [review]
Bug 13961: Add unit test

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 33 Emmi Takkinen 2021-02-10 07:38:04 UTC
Created attachment 116634 [details] [review]
Bug 13961: Fix QA issues

This patch fixes following QA issues:

 FAIL	C4/Letters.pm
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).

 FAIL	t/db_dependent/Letters.t
   FAIL	  critic
		# Variables::ProhibitConditionalDeclarations: Got 1 violation(s).

Sponsored-by: Koha-Suomi Oy
Comment 34 Emmi Takkinen 2021-02-10 07:38:37 UTC
Created attachment 116635 [details] [review]
Bug 13961: Check that borrowernumber isn't a reference

Some SendAlerts() tests in Letters.t failed due SQL::Abstract errors,
caused by HASH sometimes set as borrowernumber.

Added code to check if borrowernumber is defined and it isn't a reference.
To test prove t/db_dependent/Letters.t

Sponsored-by: Koha-Suomi Oy
Comment 35 Joonas Kylmälä 2021-02-10 09:43:27 UTC
Bug 24381 might be duplicate of this, I'm not 100% sure because the approach is different.
Comment 36 Salman Ali 2021-06-11 15:12:12 UTC
Created attachment 121854 [details] [review]
Bug 13961: Check that borrowernumber isn't a reference

Some SendAlerts() tests in Letters.t failed due SQL::Abstract errors,
caused by HASH sometimes set as borrowernumber.

Added code to check if borrowernumber is defined and it isn't a reference.
To test prove t/db_dependent/Letters.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Salman Ali <salman@outaouais>
Comment 37 Salman Ali 2021-06-11 15:13:45 UTC
Test needs fixing but patch is working
Comment 38 Emmi Takkinen 2021-07-23 09:14:09 UTC
(In reply to Salman Ali from comment #37)
> Test needs fixing but patch is working

Do mean tests aren't working? At least for me running prove t/db_dependent/Letters.t returns "All tests successful.".
Comment 39 Kyle M Hall 2021-09-10 15:45:19 UTC
I'm wondering if this is really necessary given the solution provided by Josef. If we implement this, it means adding yet another database query that will only be used in a few cases.

That solution from Josef was:
---
[% USE Price %]

# your normal issue slip content

[% SET balance = borrower.account.balance %]
[% IF balance > 0 %]
Patron has debit of [% balance | $Price %].
[% END %]
[% IF balance < 0 %]
Patron has credit of [% balance | $Price %].
[% END %]
---

Is there a slip where this doesn't work? This functions on any slip with a borrower object. If there is a slip where this doesn't work, it seems more sensible to add the borrower object instead, right?
Comment 40 Marcel de Rooy 2021-10-07 13:45:44 UTC
        * Signed-off-by line 'Signed-off-by: Salman Ali <salman@outaouais>' seems invalid
Comment 41 Marcel de Rooy 2021-10-07 13:49:19 UTC
@@ -660,6 +660,12 @@ sub GetPreparedLetter {
     my $OPACBaseURL = C4::Context->preference('OPACBaseURL');
     $letter->{content} =~ s/<<OPACBaseURL>>/$OPACBaseURL/go;

+    my $borrowernumber = $tables->{borrowers};
+    my $lines = Koha::Account::Lines->search({ borrowernumber => $borrowernumber  });
+    my ($totalfine) = $lines->total_outstanding;
+    $totalfine = sprintf("%.2f", $totalfine);
+    $letter->{content} =~ s/<<borrowers.totalfine>>/$totalfine/go;

See earlier comments. We should not be doing that at this code location.

> Added code to check if borrowernumber is defined and it isn't a reference.

I think we should address that problem at the source, not here.

Please adjust. Also look at comment39. Thanks !