Patch sponsored by the CCSR ( http://www.ccsr.qc.ca )

Don't forget to set Needs Signoff Frédérick.

Works well, I especially like the utf8 change, which is almost a bugfix.

QA Comment:

Add 2 field for the csv output (cardnumber and phone) and add the utf8 support for the html output.

Seems good.

Marked as Passed QA.

QA comment: sorry, but this patch introduces a perlcritic error:

I/O layer ":utf8" used at line 367, column 5.  Use ":encoding(UTF-8)" to get strict validation.  (Severity: 5)


(before your patch there is also:
"require" statement with library name as string at line 29, column 12.  Use a bareword instead.  (Severity: 5)
 which is still here after the patch, not a problem

There are several scripts in the bin directory that use similar logic that trigger the last perlcritic error. My rough idea was to do a "push @INC" for the variable part, that way the require could just be the library name itself. Haven't tested it. Just a brain storming thought I had.

QA Comments:

wajasu: we don't want to delete the BEGIN bloc.
Could you resubmit your last patch with reintroduce it please ?
QA's rules had change :) Now we accept the eval { require "$FindBin::Bin/../kohalib.pl" };
(see the discussion on koha-devel: [QA] 2 short QA questions)

Marked as Failed QA.

Global symbol "$phone" requires explicit package name at misc/cronjobs/overdue_notices.pl line 600.
Global symbol "$cardnumber" requires explicit package name at misc/cronjobs/overdue_notices.pl line 601.

Applying: Bug 8687: Improvements in the overdue_notices.pl script
Using index info to reconstruct a base tree...
M	misc/cronjobs/overdue_notices.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/cronjobs/overdue_notices.pl
CONFLICT (content): Merge conflict in misc/cronjobs/overdue_notices.pl
Patch failed at 0001 Bug 8687: Improvements in the overdue_notices.pl script
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Before the patch:
name;surname;address1;address2;zipcode;city;country;email;itemcount;itemsinfo
bébé;saby;"15 rie";;;;;math.saby_15@gmail.com;1;"22/11/2013	Palladio	013559A	Lionello Puppi	2273
"
thefirstname;thename;"rue X";"batiment 2";75000;PARIS;France;;1;"22/11/2013	Souvenirs d'enfance et de jeunesse	125305F	Ernest Renan	2272
"
mathieu;saby;;;;;;math.saby_15@gmail.com;1;"07/11/2013	Lecture de Valéry2	0281972092	par James R. Lawler	2301
"



After the patch:
name;surname;address1;address2;zipcode;city;country;email;phone;cardnumber;itemcount;itemsinfo;branchname;letternumber
bébé;saby;"15 rie";;;;;math.saby_15@gmail.com;;6;1;"2273	29/11/2013	22/11/2013	Palladio	013559A	Lionello Puppi	W 11246	BU centrale
";"BU centrale";1
thefirstname;thename;"rue X";"batiment 2";75000;PARIS;France;;060606060606;3;1;"2272	29/11/2013	22/11/2013	Souvenirs d'enfance et de jeunesse	125305F	Ernest Renan	605634	BU centrale
";"BU centrale";1
mathieu;saby;;;;;;math.saby_15@gmail.com;;1;1;"2301	29/11/2013	07/11/2013	Lecture de Valéry2	0281972092	par James R. Lawler	606395	BU centrale
";"BU centrale";3

It seems ok.

Except there seems to be an extra linefeed before the closing " of the item information (but it was an existing bug before your patch).

Regarding UTF-8 : I tried to put chinese text in my notices, and to generate it before your patch. Chinese text is displayed properly. After your patch also, but I don't see any change.
Is utf8 display of notices broken in current master?

I sign off, as this patch improves overdue_notices.pl without breaking anything.

Mathieu

Hello Frédérick,
Your patch modify the value of itemcontent, is it intentional? I don't think it is a good idea to change this default value.

Marked as Failed QA.
Feel free to set out arguments if you disagree.

I am stumbling over this bug report again and again, wondering what it is about as the subject is not really telling much. After reading the comments I think this would have been a bit better as 2 separate bug reports, one for the CSV and one for the HTML output change.

I agree with Jonathan about the change to the itemscontent default as this would be an unexpected change for a lot of libraries. Also itemnumber and branchname are not useful for a lot of libraries.

-my $itemscontent = join( ',', qw( date_due title barcode author itemnumber ) );
+my $itemscontent = join( ',', qw( itemnumber issuedate date_due title barcode author itemcallnumber branchname ) );

It looks like this is hiding a 3rd feature even:
+SELECT biblio.*, items.*, issues.*, biblioitems.itemtype, TO_DAYS($date)-TO_DAYS(date_due) AS days_overdue, branchname
+  FROM issues,items,biblio, biblioitems, branches b

Making ist possible to put the branchname instead of the branchcode into the output of items?

Hi Simith, I see that Kyle passed QA on this, but could you maybe reattach a patch with a commit message? A short description of what this patch changes/adds to the script? I think this has gone quite a long way since the beginning so most of the comments here do no longer apply.

Please provide a step by step test plan and a summary of the changes/consequences of your patch. There are hints in the wiki about how to write the 'good' commit message: http://wiki.koha-community.org/wiki/Commit_messages

As it was already tested by Kyle and has a sign off, I am switching back to sigend off - thx Simith!

Please include the test plan in your commit message next time - it shows up in bugzilla, but is not in the patch. Also please obsolete the old patch, when attaching a new one - thx! 

I am going to fix it for this.

Fixed a conflict with some patches recently pushed - I have problems getting the overdues working currently - CSV only has the header line and no other entries. I will try again later if I can get this to cooperate.

Created attachment 30875 [details] [review]
[PASSED QA] Bug 8687 - Overdues: Add utf-8 support to HTML and additional fields to CSV output

This patch contains small improvements made in the overdue_notices.pl script.

- UTF-8 support in the HTML output
- Add the phone, cardnumber, branchname and letternumber columns
  to the csv output

Testing:

I) Create an issue overdue (intranet):
0) Enter in a patron account, add some characters like
   ユーザー別サイト in his address
1) Check if in Patron category administration the overdue column
   is YES for his code
2) In Tools -> Notice triggers define first delay = 7, letter
   Overdue notice. Nothing for second ni third
3) Check if Tools -> Notices and slips, code overdue is available
4) checkout a book with a retard more than 7 days.
5) Check if Adminstration -> Defining circulation and fine rules
   for all libraries has fine rules for his patron category,
   item type book.

II) Before applying the patch
0) Run misc/cronjobs/overdue_notices.pl -n -html .
1) Open the file notices_"date".html in a brower
2) Validate the characters are not correct

III) After applying the patch
0) Run misc/cronjobs/overdue_notices.pl -n -html .
1) Open the file notices_"date".html in a brower
2) Validate the characters are correct
3) Run misc/cronjobs/overdue_notices.pl -n -csv temp.csv
4) Validate phone, cardnumber, branchname and letternumber
   columns and values

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes tests and QA script.
Works as described, no problems found.

*** Bug 9208 has been marked as a duplicate of this bug. ***

Patch pushed to master.

Thanks Simith.