Bug 8378 - <fine> syntax not working on overdues anymore
Summary: <fine> syntax not working on overdues anymore
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Jonathan Druart
URL:
Keywords: regression
Depends on: 8607
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-06 11:51 UTC by Nicole C. Engard
Modified: 2013-11-29 08:51 UTC (History)
15 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:


Attachments
0001-Bug-8378-fine-syntax-not-working-on-overdues-anymore (6.15 KB, patch)
2012-08-15 18:44 UTC, wajasu
Details | Diff | Splinter Review
Bug-8378-fine-syntax-broken-NFC-and-charset-utf8 (3.43 KB, patch)
2012-08-23 18:58 UTC, wajasu
Details | Diff | Splinter Review
circulations rules example (132.05 KB, image/png)
2012-08-23 19:28 UTC, wajasu
Details
overdue actions example (19.62 KB, image/png)
2012-08-23 19:29 UTC, wajasu
Details
Bug 8378 - <fine> syntax not working on overdues anymore (9.15 KB, patch)
2012-08-31 12:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug-8378-v3.8.x-fine-syntax-not-working-on-overdues-.patch (9.47 KB, patch)
2012-09-17 16:37 UTC, wajasu
Details | Diff | Splinter Review
Bug 8378: Fix overdue letter generation (1.20 KB, patch)
2012-12-13 10:58 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8378 - show all items columns and new items.fine (1.04 KB, patch)
2013-03-19 13:43 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8378 - <fine> in overdues changed to <<items.fine>> (2.84 KB, patch)
2013-03-19 13:46 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8378 - show all items columns and new items.fine (1.04 KB, patch)
2013-03-19 13:48 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8378 - <fine> in overdues changed to <<items.fine>> (3.05 KB, patch)
2013-03-19 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 8378 - show all items columns and new items.fine (1.21 KB, patch)
2013-03-19 15:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 8378 - followup - sample_notices.sql updated to <<items.fine>> (32.11 KB, patch)
2013-03-20 13:16 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
[SIGNED OFF] Bug 8378 - followup - sample_notices.sql updated to <<items.fine>> (32.29 KB, patch)
2013-03-20 13:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 8378 - followup - update notices to use <<items.fine>> (1.45 KB, patch)
2013-03-20 13:48 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8378 - followup - update notices to use <<items.fine>> (1.51 KB, patch)
2013-03-20 14:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 8378 - <fine> in overdues changed to <<items.fine>> (3.11 KB, patch)
2013-03-20 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8378 - show all items columns and new items.fine (1.27 KB, patch)
2013-03-20 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8378 - followup - sample_notices.sql updated to <<items.fine>> (32.35 KB, patch)
2013-03-20 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8378 - followup - update notices to use <<items.fine>> (1.58 KB, patch)
2013-03-20 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-07-06 11:51:36 UTC
When you use the <fine>USD</fine> syntax in overdue notices it used to show the fines in USD - if you use <fine>GBP</fine> it used to show the fines in GBP ... now it shows nothing.
Comment 1 wajasu 2012-08-15 18:44:59 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2012-08-20 22:10:55 UTC
Hi wajasu,

I am sorry, but this is not working for me.

My test case:
- 1 overdue item 1 day overdue
- 2 overdue items 2 days overdue

My notice:
<item>"<<biblio.title>>" by <<biblio.author>>, <<items.itemcallnumber>>, Barcode: <<items.barcode>> Fine: <fine>GBP</fine></item>

Some other configuration:
- Notice triggers: 1 and 2 days using ODUE notice
- Circulation rules: charging interval 1 day, fine 1.50

Running the fine script generated the correct fines - 1.50 and 3.00.

Next I tried creating the notices without your patch, using overdues.pl with -t.

I got 1 notice with all 3 items listed, which was semi-correct. (It shoud have given me 2 notices, one 1st notice, one 2nd... but this goes on another bug, because that's how it works in 3.6 and how it should work for our libraries.)
Fines were not printed out.

After I applied your patch, I got 2 notices (which is better at first glance), but
- the fine was not printed out and
- each notice only listed 1 item. One of those should have listed 2 items.

Hope my test case is understandable for you, sorry to fail QA.

All was tested on top of 8607.
Comment 3 Katrin Fischer 2012-08-20 22:34:40 UTC
I redid my tests because I didn't notice at first that it was shortly after midnight. 

I have to correct this:

>I got 1 notice with all 3 items listed, which was semi-correct. (It shoud have >given me 2 notices, one 1st notice, one 2nd... but this goes on another bug, >because that's how it works in 3.6 and how it should work for our libraries.)
>Fines were not printed out.

Because it works fine :) After adjusting my delays I got 2 notices, one listing 2 items, one listing 1 item. So the problem that remains to be solved here is what I noted for the notices after applying the patch:

- fines still not printed
- every notice only listing 1 item
Comment 4 Katrin Fischer 2012-08-21 19:56:49 UTC
Hi wajasu,

I have done some more tests.

1) overdues.pl -t -n
The data output on command line is all nice and correct.
All items are listed and the fine amounts show correctly.

2) overdues.pl -t
The notices written into the message_queue table in the database are not correct. Not all items are listed and fine amount is not shown.

That's not good.
Comment 5 wajasu 2012-08-23 18:58:03 UTC Comment hidden (obsolete)
Comment 6 wajasu 2012-08-23 19:28:58 UTC
Created attachment 11805 [details]
circulations rules example
Comment 7 wajasu 2012-08-23 19:29:56 UTC
Created attachment 11806 [details]
overdue actions example
Comment 8 Kyle M Hall 2012-08-31 12:00:05 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-08-31 12:01:29 UTC
Ran through test plan before and after applying patch. Verified that fine syntax does not work pre-patch and does work post-patch for both direct emails and emails to the KohaAdminEmailAddress.

I also squashed both patches together before uploading, for ease of use.
Comment 10 Mason James 2012-09-14 12:35:37 UTC
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
 
patch looks good, passing QA…

$ koha-qa.pl  

testing 1 commit(s) (applied to commit 0acebb8)
 * d0c8d9e Bug 8378 - <fine> syntax not working on overdues anymore
      C4/Letters.pm
      misc/cronjobs/overdue_notices.pl

* C4/Letters.pm                                                            OK
* misc/cronjobs/overdue_notices.pl                                         OK
Comment 11 Paul Poulain 2012-09-15 05:07:50 UTC
Patch pushed to master
Comment 12 Chris Cormack 2012-09-16 09:00:00 UTC
Doesn't apply cleanly to 3.8.x, please send a patch that applies on 3.8.x
Comment 13 wajasu 2012-09-17 16:37:30 UTC Comment hidden (obsolete)
Comment 14 Magnus Enger 2012-09-27 07:00:51 UTC
Looks like this should be "needs signoff" for the 3.8.x patch.
Comment 15 Magnus Enger 2012-09-27 07:02:24 UTC
Comment on attachment 11906 [details] [review]
Bug 8378 - <fine> syntax not working on overdues anymore

This patch has been pushed. Marking it as obsolete to avoid confusion with the 3.8.x patch.
Comment 16 wajasu 2012-10-02 22:04:15 UTC
What the RMmaintenance mgr for production 3.8  wants is not only to test the overdue_notice.pl, but also that other notices are still being sent (not breaking).  This fix NFC normalizes the enqueued letters so that certain unicode symbols would not cause the letter to be truncated when stored.  The fine currency symbol was causing truncations of the letter in mysql.

This fix also enabled utf8 to be shown correctly in email clients (for attachments).

Note: This fix is already on master, hopefully on track for 3.10.  And should show up in notice realted testing prior to 3.10.
Comment 17 Jared Camins-Esakov 2012-11-07 02:45:46 UTC
Note that the patch needing sign off is for 3.8.x only.
Comment 18 Colin Campbell 2012-11-23 16:40:59 UTC
I've had some problems with this patch. With it applied I found overdues for multiple users getting the same user's overdue message text, eg user 1 gets their correct message but users 2, 3 and 4 get it as well. reverting the patch corrected this. I've not tracked down the cause as yet.
Comment 19 Chris Cormack 2012-12-08 06:47:45 UTC
Holding off on applying this to 3.8.x, as Colin's findings worry me, can someone else test on master to make sure this patch is safe?
Comment 20 mathieu saby 2012-12-09 17:12:07 UTC
Hello
I can confirm the problem. We noticed it some weeks ago, and we discovered this week it was caused by this patch.

M. Saby
Rennes 2 University
Comment 21 mathieu saby 2012-12-09 17:15:00 UTC
(our vendor is writing a corrective patch)
M. Saby
Comment 22 Jonathan Druart 2012-12-12 13:45:41 UTC
I can't reproduce the issue but I certainly don't have enough information.

If the pushed patch introduces a bug I think it should be revert in order to let wajasu resubmit another patch.
Comment 23 mathieu saby 2012-12-12 13:57:11 UTC
We had to revert bz8378 in Rennes 2 to get overdues working properly. So I can confirm this patch is broken somewhere.

In overdue_notice.pl I noticed the following problem :
I checked the value of the variable $letter_template just before and after this piece of code :
 my $letter = parse_letter(
                    { letter_code => $overdue_rules->{"letter$i"},
                        letter => $letter_template,

=>
The value of $letter_template was modified after the call to parse_letter, which is not normal I presume.

In sub parse_letter, the function C4::Letters::GetProcessedLetter is called.
And this function was altered by bz8378.

BEFORE applying bz8378, in sub parse_letter, the value of $param->{letter} is unchanged after it passed as an argument to GetProcessedLetter.

AFTER bz8378, the value of $param->{letter} is changed after it passed as an argument to GetProcessedLetter.

I hope it could help

M. Saby
Rennes 2 University
Comment 24 Julian Maurice 2012-12-13 10:58:41 UTC Comment hidden (obsolete)
Comment 25 Julian Maurice 2012-12-13 11:04:03 UTC
Steps to reproduce the bug:
1. make 2 checkouts with 2 different borrowers of the same categorycode
2. make these checkouts become overdue "update issues set date_due='XXXX-XX-XX XX:XX' where borrowernumber IN(X,Y)"
3. run misc/cronjobs/overdue_notices.pl -n
You should obtain the same letter twice.
4. Now apply the patch and re-run misc/cronjobs/overdue_notices.pl -n
You should obtain 2 different letters.
Comment 26 Julian Maurice 2012-12-13 11:05:53 UTC
Note: this patch is for master
Comment 27 Katrin Fischer 2012-12-13 11:20:12 UTC
Hi Julian, does this patch also address the fine syntax problem?
Comment 28 mathieu saby 2012-12-13 11:30:28 UTC
Hello
Maybe your patch work, I did not check, but is it normal that C4::Letters::GetProcessedLetter changes the value of the template passed to it?
If we keep this behavior, I fear it will cause other bugs somewhere else.

M. Saby
Comment 29 Julian Maurice 2012-12-13 11:39:30 UTC
Hi Katrin, this patch is a follow-up for the original patch that fix the fine syntax problem. Does it answer your question?
Comment 30 Katrin Fischer 2012-12-13 12:07:55 UTC
Yes, thank you :)
Comment 31 Jonathan Druart 2012-12-27 15:47:52 UTC
Comment on attachment 14069 [details] [review]
Bug 8378: Fix overdue letter generation

New Bug created for this patch (Bug 9325)
Comment 32 Jared Camins-Esakov 2012-12-28 03:38:12 UTC
I have reverted this patch from master, as it was the cause of the much larger problem described in bug 9325. A new patch to restore the <fine> syntax would be welcomed.
Comment 33 Maxime Pelletier 2013-03-12 21:46:30 UTC
Can I resurrect this ? Problem seems still present in master. This patch followed by the one in bug 9325 fixes it.
I can attach a patch merging both if needed.
Comment 34 Jared Camins-Esakov 2013-03-12 21:52:44 UTC
(In reply to comment #33)
> Can I resurrect this ? Problem seems still present in master. This patch
> followed by the one in bug 9325 fixes it.
> I can attach a patch merging both if needed.

I would prefer the fix Chris described on 9325.
Comment 35 Maxime Pelletier 2013-03-12 22:04:11 UTC
(In reply to comment #34)
> (In reply to comment #33)
> > Can I resurrect this ? Problem seems still present in master. This patch
> > followed by the one in bug 9325 fixes it.
> > I can attach a patch merging both if needed.
> 
> I would prefer the fix Chris described on 9325.

That simple fix doesn't work. See first line of the bug :

> In overdue_notices.pl the parse_letter routine needed the 'letter' in params to > get the <fine> tag and the currency format.

Without this patch there is no $params->{'letter'} to replace.
Comment 36 Dobrica Pavlinusic 2013-03-19 13:43:34 UTC Comment hidden (obsolete)
Comment 37 Dobrica Pavlinusic 2013-03-19 13:46:28 UTC Comment hidden (obsolete)
Comment 38 Dobrica Pavlinusic 2013-03-19 13:48:49 UTC Comment hidden (obsolete)
Comment 39 Dobrica Pavlinusic 2013-03-19 14:16:23 UTC
This is hopefully simple and non-controversial way to fix fines. It doesn't do anything with encoding, but that can be submitted as separate patch.

wajasu, I hope you don't mind that I hijacked your bug, Katrin made me do it :-)
Comment 40 Tomás Cohen Arazi 2013-03-19 15:16:47 UTC Comment hidden (obsolete)
Comment 41 Tomás Cohen Arazi 2013-03-19 15:17:07 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2013-03-20 12:53:31 UTC
QA comment:

Works great.
However .sql files should be replaced (sample_notices.sql for each languages).

It would be great to replace existing values in DB (loop on letters and replace <fine>XXX</fine> with <<items.fine>>).

Marked as Failed QA.
Comment 43 Dobrica Pavlinusic 2013-03-20 13:16:03 UTC Comment hidden (obsolete)
Comment 44 Tomás Cohen Arazi 2013-03-20 13:25:54 UTC Comment hidden (obsolete)
Comment 45 Dobrica Pavlinusic 2013-03-20 13:48:29 UTC Comment hidden (obsolete)
Comment 46 Tomás Cohen Arazi 2013-03-20 14:01:45 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2013-03-20 14:44:54 UTC
QA comment:

2 last patches modify existing entries in letters table and the sql files for installer.

Marked as Passed QA.
Comment 48 Jonathan Druart 2013-03-20 14:45:10 UTC
Created attachment 16528 [details] [review]
Bug 8378 - <fine> in overdues changed to <<items.fine>>

This patch reintroduces fines in overdue_notices.pl with minimal
changes to exsiting code.

It DOES changes template syntax, it used to be <fine>USD</fine> but
currency was not recalculated and used only to select format, so
now we are using active system currency because values are in it
anyway.

Example notify template:

<item>"<<biblio.title>>" by <<biblio.author>>, <<items.itemcallnumber>>,
Barcode: <<items.barcode>> Fine: <<items.fine>> </item>

If your active currency doesn't have valid ISO code this code will
fallback to sprintf with two decimal digits, and you can insert
currency symbol in template itself (currency field is not editable
through web interface, so this fallback might be useful for existing
installations).

Test scenario:
1. configure fine amount under "Circulation and fines rules"
   for patron category and item type
2. checkout item with correct type to partron in correct
   category with due date set to yesterday
3. verify that overdue notice uses new <<items.fine>> tag
4. run ./misc/cronjobs/fines.pl to calculate fines
5. run ./misc/cronjobs/overdue_notices.pl -n
   and verify that fine amount is included

Notes: Tested using the default ODUE notice, changing:
<fines>USD</fines>
for
<<items.fine>>

Everything worked as expected with finesmode=production set.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 49 Jonathan Druart 2013-03-20 14:45:14 UTC
Created attachment 16529 [details] [review]
Bug 8378 - show all items columns and new items.fine

This allows users to select any columns from items and adds new
items.fine field introduced by previous patch to user interface.

Note: This works as expected. items.fine appears as an option and gets inserted in the notice template as expected.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 50 Jonathan Druart 2013-03-20 14:45:21 UTC
Created attachment 16530 [details] [review]
Bug 8378 - followup - sample_notices.sql updated to <<items.fine>>

Trivial string substitution.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 51 Jonathan Druart 2013-03-20 14:45:26 UTC
Created attachment 16531 [details] [review]
Bug 8378 - followup - update notices to use <<items.fine>>

Didn't break on well-formed notices, and fixed those with problems

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 52 Katrin Fischer 2013-03-21 16:18:33 UTC
Just wanted to say that this is a really important feature for our libraries and pleaaase push :)
Comment 53 Jared Camins-Esakov 2013-03-22 01:56:35 UTC
This patch has been pushed to master.
Comment 54 Chris Cormack 2013-03-29 07:16:34 UTC
Pushed to 3.10.x and 3.8.x will be in 3.10.5 and 3.8.12