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.
Created attachment 11628 [details] [review] 0001-Bug-8378-fine-syntax-not-working-on-overdues-anymore This patch returns the behavior for handling the <fine>USD</fine> tag in overdue_notices.pl It seems this behavior got lost during other changes. At the time of writing this patch, I needed another patch that I had signed off on, so you will need to apply SIGNED-OFF-Bug-8607-FIX-overdues_notices-script-date.patch first to get overdue_notice.pl working. That patch fixed substitution of a date variable that had incorrect quotes, and enables overdues to be returned. Test Plan: 1) I added a student patron (be sure overdues notices can be received with the correct patron type) 2) I checked out a book (in my case i checked out 2 because I wanted to see if multiple were supported) 3) As mysql -uroot -p koha (connect to your test database) > select * from borrowers; ( to find the borrowernumber for your test patron) > select * from issues; ( to see the checkout outs. ) > update issues set date_due='2012-07-01 23:59:00' where borrowernumber=55; ( set the date_due to make the book overdue) 4) login as the patron and see that the checkouts are overdue ( check circulation rules or sysprefs if needed ) 5) I applied SIGNED-OFF-Bug-8607-FIX-overdues_notices-script-date.patch (if you are on master around 2012-08-15) 6) As root in dev environment kohaclone: misc/cronjobs/overdue_notices.pl -n (I got no amount after the Fine: with the currency format code) ... The following item(s) is/are currently overdue: "Paul and his theology " by , 227/.06, Barcode: 111 Fine: USD "Apostolic history and the Gospel" by , , Barcode: 333 Fine: USD ... 7) I applied Bug-8378-fine-syntax-not-working-on-overdues-anymore.patch (what we are really testing) 8) As root in dev environment kohaclone: (be sure the patched C4/Letters.pm is in /usr/share/koha/lib if you use an std install) misc/cronjobs/overdue_notices.pl -n (I got the amount ==> Fine: $0.00 ) ... The following item(s) is/are currently overdue: "Paul and his theology " by , 227/.06, Barcode: 111 Fine: $0.00 "Apostolic history and the Gospel" by , , Barcode: 333 Fine: $0.00 ... Notes: I would apply this patch after Bug8607 to save time on setup steps, and start from step 6) How was it fixed: In overdue_notices.pl the parse_letter routine needed the 'letter' in params to get the <fine> tag and the currency format. It replaced the <fine> tag with <<item.fine>>, which should have been <<items.fine>>. The routine also iterated over the items to format and pushed for use in GetPreparedLetter, but that routine queried the database again for the letter template, and thus would see the <fine> tag and not <<items.fine>>. So I had to factor out GetLetter from GetPreparedLetter, keeping GetPreparedLetter the same behavior, since its called in many places (Reserves.pm, etc). I also added a GetProcessedLetter to Letters.pm that did the same thing as GetPreparedLetter, but accepted a 'letter' param, so it doesn't query the database. I edited overdue_notice.pl to GetLetter, earlier in the code so its retrieved less often, and supply it to parse_letter which originally wanted to do its magic. wajasu
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.
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
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.
Created attachment 11804 [details] [review] Bug-8378-fine-syntax-broken-NFC-and-charset-utf8 After applying Bug 8607's patch and Bug 8378 0001-Bug-8378-fine-syntax-not-working-on-overdues-anymore, this followup patch takes care of NFC and charset issues for utf8: Bug-8378-fine-syntax-broken-NFC-and-charset-utf8.patch 0) configure the KohaAdminEmailAddress syspref (or leave as root@localhost if you don't want to test email). Yout can "select * from message_queue" to see whats queued, and verifiy charset changes as well. 1) create or use a patron WITHOUT primary email address set and with category (such as student) that gets overdue messages. 2) checkout an item for that patron, one with a checkout due date 1 day prior to today 3) checkout an item for that patron, one with a checkout due date 2 days prior to today 4) configure circulation rules (see attachment above) 5) configure overdue actions (Tool->Overdue notice/status triggers) Refer to the attachment for the "First tab" For the "Second" tab, make it the same, except with a delay of 2 6) verify/confogure a overdue ODUE notice template with a <fine> within items. <item>"<<biblio.title>>" by <<biblio.author>>, <<items.itemcallnumber>>, Barcode: <<items.barcode>> Fine: <fine>USD</fine></item> 7) run ./misc/cronjobs/fines.pl (this will calculate fines) 8) There are multiple ways to generate notices: ./misc/cronjobs/overdue_notice.pl ./misc/cronjobs/overdue_notice.pl -t (triggered) (these will generate an email) (if the patron doesn't have a primary email, the email notices are batched up and sent to the KohaAdminEmailAddress syspref) 9) query the message_queue table content column to see what was generated. 10) verify: a) that items show up for the overdues for your patron. "Fine: $1.50" and one with "$3.00" I've seen multiple notices for the same person, or all in one notice. b) that email header Conent-Type: "text/plain"; also has charset="utf-8" for both the main letter body AND attachments. You can see this in the mysql query for the content. If you process the message_queue, sending eamil, you can view source of the email and see the same information. 11) add a primary email address to your patron, and generate notices again (step 8) This will test a different code path (no attachments) and send the notice to the patron email. 12) query the message_queue table to see what is getting emailed. 13) view email for patron to verifiy all Fine: $ curreny symbols appear. 14) Bonus: Bug 8299 - Since the uninitialized matching error were addressed and you did not see them when you ran overdue_notice.pl, you can close bug 8299 as a dup. Note: NFC normalization will apply to other letters/notices as well, and hopefully allow certain UTF8 codes to be stored as well. Note: Bug 8299 would be fixed bby this Bug 8378, since I addressed those warnings. I don't see them anymore with this patch. Note: I did NFC normalization on the attachment filename, since filenames will have unicode chars increasingly in the future. (I think) Note: We use NFC in Record.pm, and other places. That seems to be our standard. wajasu
Created attachment 11805 [details] circulations rules example
Created attachment 11806 [details] overdue actions example
Created attachment 11906 [details] [review] Bug 8378 - <fine> syntax not working on overdues anymore Bug 8378 - <fine> syntax broken NFC and charset utf8 NFC normalize enqueued letters and add content-type charset=utf-8 This prevents utf8 codes from causing mysql to truncate the 'content' from the point of certain codes, when stored in the message_queue table. This was happenning with the currency symbol generated by Locale::Currency:Format currency_format routine. NFC normalization was only done on the attachment content with its content-type containing "text", as in text/plain. For emails AND attachments, the charset="utf-8" was added to the content-type so mail clients would correctly iterate the utf8 codes, thus preventing mobijake. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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.
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.
> 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
Patch pushed to master
Doesn't apply cleanly to 3.8.x, please send a patch that applies on 3.8.x
Created attachment 12286 [details] [review] Bug-8378-v3.8.x-fine-syntax-not-working-on-overdues-.patch I've attached a patch that applies to origin/3.8.x wajasu
Looks like this should be "needs signoff" for the 3.8.x patch.
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.
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.
Note that the patch needing sign off is for 3.8.x only.
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.
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?
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
(our vendor is writing a corrective patch) M. Saby
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.
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
Created attachment 14069 [details] [review] Bug 8378: Fix overdue letter generation In overdue_notices.pl, letter template must not be passed to parse_letter because this sub modifies it, so the first overdue letter is ok but following letters have exactly the same content. This patch make a copy of the template before passing it to parse_letter
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.
Note: this patch is for master
Hi Julian, does this patch also address the fine syntax problem?
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
Hi Katrin, this patch is a follow-up for the original patch that fix the fine syntax problem. Does it answer your question?
Yes, thank you :)
Comment on attachment 14069 [details] [review] Bug 8378: Fix overdue letter generation New Bug created for this patch (Bug 9325)
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.
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.
(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.
(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.
Created attachment 16387 [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.
Created attachment 16388 [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
Created attachment 16390 [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.
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 :-)
Created attachment 16407 [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>
Created attachment 16408 [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>
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.
Created attachment 16513 [details] [review] Bug 8378 - followup - sample_notices.sql updated to <<items.fine>>
Created attachment 16515 [details] [review] [SIGNED OFF] Bug 8378 - followup - sample_notices.sql updated to <<items.fine>> Trivial string substitution. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Created attachment 16518 [details] [review] Bug 8378 - followup - update notices to use <<items.fine>>
Created attachment 16522 [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>
QA comment: 2 last patches modify existing entries in letters table and the sql files for installer. Marked as Passed QA.
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>
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>
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>
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>
Just wanted to say that this is a really important feature for our libraries and pleaaase push :)
This patch has been pushed to master.
Pushed to 3.10.x and 3.8.x will be in 3.10.5 and 3.8.12