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
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.
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.
(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. :)
Created attachment 41872 [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
Works as expected. However, C4/Letters.pm line 648 has forbidden pattern (trailing space chars).
(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.
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
(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.
(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.
Created attachment 42089 [details] [review] [Follow-up] Remove whitespace from line 648 in C4/Letters.pm White space in 'empty' line removed.
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 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.
Testing this on top of bug 16378
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.
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!
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
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.
Btw: you can use <item></item> and leave the <<items.fine>> out too!
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 %]
Created attachment 98819 [details] [review] Bug 13961: Option to include patron's total amount of fines in notices Fixed patch to apply master version.
Created attachment 98820 [details] [review] Bug 13961: Add currency format handling Added currency format handling proposed in comments.
Created attachment 98821 [details] [review] Bug 13961: Add unit test
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>
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>
Created attachment 106194 [details] [review] Bug 13961: Add unit test Sponsored-by: Koha-Suomi Oy Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
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?
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
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
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
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>
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>
Created attachment 116633 [details] [review] Bug 13961: Add unit test Sponsored-by: Koha-Suomi Oy Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
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
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
Bug 24381 might be duplicate of this, I'm not 100% sure because the approach is different.
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>
Test needs fixing but patch is working
(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.".
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?
* Signed-off-by line 'Signed-off-by: Salman Ali <salman@outaouais>' seems invalid
@@ -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 !