Bug 8687 - Improvements in overdue_notices.pl
Summary: Improvements in overdue_notices.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: simith.doliveira
QA Contact: Jonathan Druart
URL:
Keywords:
: 9208 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-08-24 17:46 UTC by Frédérick Capovilla
Modified: 2016-06-21 21:37 UTC (History)
16 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Improvements in the overdue_notices.pl script (4.17 KB, patch)
2012-08-24 17:46 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Bug 8687: Improvements in the overdue_notices.pl script (4.24 KB, patch)
2012-08-24 23:17 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug-8667-cleaned-up-perlcritic-complaints-aligned-usage with POD doc (1.97 KB, patch)
2012-09-20 21:46 UTC, wajasu
Details | Diff | Splinter Review
Bug-8667 cleaned up perlcritic complaints, aligned usage with POD doc (2.12 KB, patch)
2012-10-05 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8687: Improvements in the overdue_notices.pl script (4.33 KB, patch)
2012-10-05 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug-8667 cleaned up perlcritic complaints, aligned usage with POD doc (2.12 KB, patch)
2012-10-05 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
patch for master combining the other 2 and adding back the BEGIN (4.38 KB, patch)
2013-04-22 21:12 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Improvements in the overdue_notices.pl script (new version) (4.39 KB, patch)
2013-05-03 21:09 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
Improvements in overdue_notices.pl script (Newer version) (5.60 KB, patch)
2013-11-25 22:17 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
[PATCH][SIGNED OFF] Bug 8687: Improvements in overdue_notices.pl script (Newer version) (5.95 KB, patch)
2013-11-29 21:45 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 8687 - Improvements in overdue_notices.pl (4.54 KB, patch)
2014-07-31 13:42 UTC, simith.doliveira
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8687 - Improvements in overdue_notices.pl (4.61 KB, patch)
2014-08-01 11:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8687 - Improvements in overdue_notices.pl (4.54 KB, patch)
2014-08-04 13:22 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 8687 - Improvements in overdue_notices.pl (4.54 KB, patch)
2014-08-04 18:41 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 8687 - Overdues: Add utf-8 support to HTML and additional fields to CSV output (5.85 KB, patch)
2014-08-16 09:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8687 - Overdues: Add utf-8 support to HTML and additional fields to CSV output (5.85 KB, patch)
2014-08-16 11:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 8687 - Overdues: Add utf-8 support to HTML and additional fields to CSV output (6.00 KB, patch)
2014-08-16 19:59 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédérick Capovilla 2012-08-24 17:46:37 UTC Comment hidden (obsolete)
Comment 1 Frédérick Capovilla 2012-08-24 17:47:15 UTC
Patch sponsored by the CCSR ( http://www.ccsr.qc.ca )
Comment 2 Chris Cormack 2012-08-24 18:55:12 UTC
Don't forget to set Needs Signoff Frédérick.
Comment 3 Chris Cormack 2012-08-24 23:17:41 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-08-24 23:19:19 UTC
Works well, I especially like the utf8 change, which is almost a bugfix.
Comment 5 Jonathan Druart 2012-08-27 10:46:10 UTC
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.
Comment 6 Paul Poulain 2012-09-04 16:20:26 UTC
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
Comment 7 Mark Tompsett 2012-09-20 16:05:32 UTC
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.
Comment 8 wajasu 2012-09-20 21:46:43 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-10-05 14:33:14 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-10-05 14:33:32 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2012-10-05 14:33:40 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-11-13 13:32:45 UTC
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.
Comment 13 Maxime Pelletier 2013-04-22 21:12:55 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-04-26 19:04:48 UTC
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.
Comment 15 Frédérick Capovilla 2013-05-03 21:09:50 UTC Comment hidden (obsolete)
Comment 16 I'm just a bot 2013-09-18 09:59:11 UTC
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".
Comment 17 Frédérick Capovilla 2013-11-25 22:17:15 UTC Comment hidden (obsolete)
Comment 18 Mathieu Saby 2013-11-29 21:41:11 UTC
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
Comment 19 Mathieu Saby 2013-11-29 21:45:37 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2013-12-13 14:17:23 UTC
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.
Comment 21 Jonathan Druart 2013-12-13 14:18:22 UTC
Marked as Failed QA.
Feel free to set out arguments if you disagree.
Comment 22 Katrin Fischer 2013-12-28 08:54:16 UTC
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?
Comment 23 simith.doliveira 2014-07-31 13:42:40 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-08-01 11:22:51 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2014-08-01 11:29:44 UTC
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.
Comment 26 simith.doliveira 2014-08-04 13:22:21 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2014-08-04 13:35:52 UTC
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
Comment 28 simith.doliveira 2014-08-04 18:41:30 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2014-08-04 18:58:00 UTC
As it was already tested by Kyle and has a sign off, I am switching back to sigend off - thx Simith!
Comment 30 Katrin Fischer 2014-08-16 08:57:28 UTC
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.
Comment 31 Katrin Fischer 2014-08-16 09:09:37 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2014-08-16 11:17:14 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2014-08-16 11:19:20 UTC
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.
Comment 34 Katrin Fischer 2014-08-16 19:59:20 UTC
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.
Comment 35 Katrin Fischer 2014-08-18 22:03:35 UTC
*** Bug 9208 has been marked as a duplicate of this bug. ***
Comment 36 Tomás Cohen Arazi 2014-10-19 14:43:22 UTC
Patch pushed to master.

Thanks Simith.