Bug 11603

Summary: Gather print notices: add csv and ods export
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: NoticesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: f.demians, katrin.fischer, kyle, matthias.meusburger, mik, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11678
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12810
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 10832, 11209    
Bug Blocks: 11678, 12810, 21560    
Attachments: Bug 11603: Preparation step, cleaning up
Bug 11603: Gather print notices - add a csv parameter
Bug 11603: Gather print notices - add a ods parameter
Bug 11603: Refactoring
Bug 11603: Fix delimiter on launching csv2ods
Bug 11603: If a letter code is given, filenames should contain it
Bug 11603: Preparation step, cleaning up
Bug 11603: Gather print notices - add a csv parameter
Bug 11603: Gather print notices - add a ods parameter
Bug 11603: If a letter code is given, filenames should contain it
Bug 11603: Fix encoding issues (bug 11944 is now pushed)
Bug 11603 Add --send|--nosend, fix stuf
[SIGNED-OFF] Bug 11603: Fix encoding issues (bug 11944 is now pushed)
[SIGNED-OFF] Bug 11603: If a letter code is given, filenames should contain it
[SIGNED-OFF] Bug 11603: Gather print notices - add a ods parameter
[SIGNED-OFF] Bug 11603: Gather print notices - add a csv parameter
[SIGNED-OFF] Bug 11603: Preparation step, cleaning up
[SIGNED-OFF] Bug 11603: Preparation step, cleaning up
[SIGNED-OFF] Bug 11603: Gather print notices - add a csv parameter
[SIGNED-OFF] Bug 11603: Gather print notices - add a ods parameter
[SIGNED-OFF] Bug 11603: If a letter code is given, filenames should contain it
[SIGNED-OFF] Bug 11603: Fix encoding issues (bug 11944 is now pushed)
Bug 11603 Add --send|--nosend, fix stuf
Bug 11678: Gather print notices: add --email parameter
Bug 11603: Some minor documentation changes
Bug 11603: Preparation step, cleaning up
Bug 11603: Gather print notices - add a csv parameter
Bug 11603: Gather print notices - add a ods parameter
Bug 11603: If a letter code is given, filenames should contain it
Bug 11603: Fix encoding issues (bug 11944 is now pushed)
Bug 11603 Add --send|--nosend, fix stuf
Bug 11603: Some minor documentation changes
Bug 11603: (follow-up) pod
Bug 11603: Change the output filenames
[PASED QA] Bug 11603: Preparation step, cleaning up
[PASSED QA] Bug 11603: Gather print notices - add a csv parameter
[PASSED QA] Bug 11603: Gather print notices - add a ods parameter
[PASSED QA] Bug 11603: If a letter code is given, filenames should contain it
[PASSED QA] Bug 11603: Fix encoding issues (bug 11944 is now pushed)
[PASSED QA] Bug 11603 Add --send|--nosend, fix stuf
Bug 11603: Some minor documentation changes
[PASSED QA] Bug 11603: (follow-up) pod
[PASSED QA] Bug 11603: Change the output filenames
Bug 11603: Preparation step, cleaning up
Bug 11603: Gather print notices - add a csv parameter
Bug 11603: Gather print notices - add a ods parameter
Bug 11603: If a letter code is given, filenames should contain it
Bug 11603: Fix encoding issues (bug 11944 is now pushed)
Bug 11603 Add --send|--nosend, fix stuf
Bug 11603: Some minor documentation changes
Bug 11603: (follow-up) pod
Bug 11603: Change the output filenames

Description Jonathan Druart 2014-01-23 08:31:59 UTC
I completely refactored the gather_print_notices cronjob in order to allow the generation of csv and ods files.

You can have a look at the wip on the BibLibre repo:
http://git.biblibre.com/?p=kohac;a=shortlog;h=refs/heads/sanop/ft/MT13212
Comment 1 Jonathan Druart 2014-02-04 14:16:00 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-02-04 14:16:06 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-02-04 14:16:11 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-02-04 14:16:16 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-02-26 11:44:12 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-02-26 12:43:26 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-04-28 14:55:09 UTC
This patch is blocked by bug 11679. The ods generation should be rewritten using OpenOffice::OODoc.
Comment 8 Jonathan Druart 2014-08-19 13:00:54 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-08-19 13:01:19 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-08-19 13:01:24 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-08-19 13:01:29 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-08-19 13:01:52 UTC
(In reply to Jonathan Druart from comment #7)
> This patch is blocked by bug 11679. The ods generation should be rewritten
> using OpenOffice::OODoc.

Done in the last patch set.
Comment 13 Frédéric Demians 2015-04-13 06:30:31 UTC
Thank you so much Jonathan for pushing to a better handling of print messages.

Isn't there something that could be improve conceptually or from an
architectural perspective? (always possible of course :-) On the one hand,
there is C4::Letters module, and on the other hand process_message_queue.pl and
gather_print_notices.pl using these module.  Both .pl scripts are doing the
same thing: gathering messages from the queue, and process them (send them, and
switch there status from pending to sent or failed). For
process_message_queue.pl, there is just one call to
C4::Letter::SendQueuedMessages. When gather_print_notices.pl do all kind of
processing, and then call C4::Letter internal function like
_set_message_status.  Shouldn't be logical just to merge both .pl scripts, and
put all code in SendQueueMessage?

SendQueueMessage interface could be extended with more parameters. For example:

SendQueueMessage({
  mtt => 'print',
  split => '0|1',
  dir => '/tmp',
  email_to => 'a@a.fr',
  format => 'html|csv',
  groupby => ['branch', 'letter']
});
Comment 14 Jonathan Druart 2015-04-13 08:28:05 UTC
(In reply to Frédéric Demians from comment #13)
> Thank you so much Jonathan for pushing to a better handling of print
> messages.
> 
> Isn't there something that could be improve conceptually or from an
> architectural perspective? (always possible of course :-)

Yes of course, and that was the idea.
But as you can see, I have submitted this bug and bug 11678 1 year ago and nothing has moved yet...
All should be done with small steps, and these 2 bugs are 2 steps.
The idea is to have only 1 script to sent notices, I would also see misc/cronjobs/printoverdues.sh merged in it.
Comment 15 Frédéric Demians 2015-04-14 06:43:41 UTC
It doesn't work if --letter_code parameter isn't given to the script.

There is an encoding problem with html output. I get warnings about wide character and the result isn't utf8 encoded. Maybe '>:encoding(utf8)' is required.

It would be nice to have a --send|--nosend, defaulted to --nosend, in order to control whether the messages status is changed from 'pending' to 'sent'. This would ease testing.
Comment 16 Jonathan Druart 2015-04-14 09:07:45 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-04-14 09:09:08 UTC
(In reply to Frédéric Demians from comment #15)
> It doesn't work if --letter_code parameter isn't given to the script.

What do you mean? If you use --csv or --ods, you have to provide a letter_code, otherwise generating a csv file does not have sense.

> There is an encoding problem with html output. I get warnings about wide
> character and the result isn't utf8 encoded. Maybe '>:encoding(utf8)' is
> required.

Should be fixed with the last patch, please retest html, csv and ods.

> It would be nice to have a --send|--nosend, defaulted to --nosend, in order
> to control whether the messages status is changed from 'pending' to 'sent'.
> This would ease testing.

Sorry but I don't plan to provide anything else than bug fixes until it's pushed.
Comment 18 Frédéric Demians 2015-04-14 11:11:49 UTC
(In reply to Jonathan Druart from comment #17)

> > It doesn't work if --letter_code parameter isn't given to the script.

> What do you mean? If you use --csv or --ods, you have to provide a
> letter_code, otherwise generating a csv file does not have sense.

It doesn't work with --html. This produces no html file:

  ./gather_print_notices.pl -s --html <mydir>

This works properly:

  ./gather_print_notices.pl -s --html <mydir> --letter_code XXX

I will fix that.

> Should be fixed with the last patch, please retest html, csv and ods.

It works. Thanks.

> > It would be nice to have a --send|--nosend, defaulted to --nosend, in order
> > to control whether the messages status is changed from 'pending' to 'sent'.
> > This would ease testing.

> Sorry but I don't plan to provide anything else than bug fixes until it's
> pushed.

No demand, no offense :-)
Comment 19 Jonathan Druart 2015-04-14 11:21:19 UTC
(In reply to Frédéric Demians from comment #18)
> (In reply to Jonathan Druart from comment #17)
> 
> > > It doesn't work if --letter_code parameter isn't given to the script.
> 
> > What do you mean? If you use --csv or --ods, you have to provide a
> > letter_code, otherwise generating a csv file does not have sense.
> 
> It doesn't work with --html. This produces no html file:
> 
>   ./gather_print_notices.pl -s --html <mydir>
> 
> This works properly:
> 
>   ./gather_print_notices.pl -s --html <mydir> --letter_code XXX
> 
> I will fix that.

Ha yes, because of the "filter by letter_code" in any case :)
Comment 20 Frédéric Demians 2015-04-14 11:23:18 UTC Comment hidden (obsolete)
Comment 21 Frédéric Demians 2015-04-14 11:23:48 UTC Comment hidden (obsolete)
Comment 22 Frédéric Demians 2015-04-14 11:24:17 UTC Comment hidden (obsolete)
Comment 23 Frédéric Demians 2015-04-14 11:24:46 UTC Comment hidden (obsolete)
Comment 24 Frédéric Demians 2015-04-14 11:25:18 UTC Comment hidden (obsolete)
Comment 25 Frédéric Demians 2015-04-14 11:25:52 UTC Comment hidden (obsolete)
Comment 26 Frédéric Demians 2015-04-14 11:30:14 UTC
For me, it functional, well documented, and add a way to do something
worthwhile with 'print' notifications. I've tested all script options with
success producing usable HTML, CSV, and OOffice files.
Comment 27 Jonathan Druart 2015-04-14 11:57:58 UTC
Thanks Frédéric.
There is something wrong the the patches order. I don't know how you did that but they are in the wrong order :)
Comment 28 Frédéric Demians 2015-04-14 18:39:47 UTC Comment hidden (obsolete)
Comment 29 Frédéric Demians 2015-04-14 18:40:07 UTC Comment hidden (obsolete)
Comment 30 Frédéric Demians 2015-04-14 18:40:23 UTC Comment hidden (obsolete)
Comment 31 Frédéric Demians 2015-04-14 18:40:35 UTC Comment hidden (obsolete)
Comment 32 Frédéric Demians 2015-04-14 18:40:53 UTC Comment hidden (obsolete)
Comment 33 Frédéric Demians 2015-04-14 18:41:00 UTC Comment hidden (obsolete)
Comment 34 Frédéric Demians 2015-04-14 18:43:07 UTC
(In reply to Jonathan Druart from comment #27)
> Thanks Frédéric.
> There is something wrong the the patches order. I don't know how you did
> that but they are in the wrong order :)

Sorry. I git bz on me. I verified that it's fixed now.
Comment 35 Frédéric Demians 2015-04-14 19:28:25 UTC Comment hidden (obsolete)
Comment 36 Frédéric Demians 2015-04-14 19:29:36 UTC
Comment on attachment 37837 [details] [review]
Bug 11678: Gather print notices: add --email parameter

Wrong patch attachment!
Comment 37 Katrin Fischer 2015-04-23 00:04:18 UTC Comment hidden (obsolete)
Comment 38 Katrin Fischer 2015-04-23 00:06:41 UTC
I like this! (But need a bit more time to play with it :( )

Some first notes from reading the docs (yay!) and code:

1) I see there are 2 copyright statements and licenses, one at the top and one at the bottom. Should this be cleaned up?

2) Is this statement about the file name still true?
The generated filename will be holdnotices-TODAY.[csv|html|ods] or holdnotices-TODAY-BRANCHCODE.[csv|html|ods] if the --split parameter is given.

3) I have attached some suggestions for the documentation - please take a look and obsolete if you don't agree :)
Comment 39 Jonathan Druart 2015-04-23 07:38:40 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2015-04-23 07:38:44 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2015-04-23 07:38:47 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2015-04-23 07:38:52 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2015-04-23 07:38:55 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2015-04-23 07:38:59 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2015-04-23 07:39:02 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2015-04-23 07:39:06 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2015-04-23 07:41:04 UTC
(In reply to Katrin Fischer from comment #38)
> I like this! (But need a bit more time to play with it :( )
> 
> Some first notes from reading the docs (yay!) and code:
> 
> 1) I see there are 2 copyright statements and licenses, one at the top and
> one at the bottom. Should this be cleaned up?

It depends on how you have fixed the first conflict :)

> 2) Is this statement about the file name still true?
> The generated filename will be holdnotices-TODAY.[csv|html|ods] or
> holdnotices-TODAY-BRANCHCODE.[csv|html|ods] if the --split parameter is
> given.

Fixed

> 3) I have attached some suggestions for the documentation - please take a
> look and obsolete if you don't agree :)

Ok for me, thanks!
Comment 48 Katrin Fischer 2015-04-23 09:17:57 UTC
1) Regression test
Change of the file name when no options specified but the directory:
From: holdnotices-<date>.html
ToL notices_hold-<date>.html

As by default all notices (including overdues) are in the file, maybe we could rename it to be more general just: notices_<date>.html? (trivial)

2) -s splits by branch

The output is identical.

3) --nosend works - helpful in testing!

4) --letter_code generates a nicely named file with only those notices!

5) --ods creates a warn:
Use of uninitialized value in lc at /usr/share/perl5/OpenOffice/OODoc/File.pm line 378.

6) --csv
This one is giving me some trouble:

First I thought it was the concatenation:

cardnumber:patron:email:item
1:Koha <borrowers.surname>>:patron@localhost:

But this is also not giving all data:
cardnumber:patron:patron:email:item
1:Koha:<borrowers.surname>>:patron@localhost:

Barcode and surname are missing, but set in the database.

We are pretty close here - it's a great enhancement.
Comment 49 Jonathan Druart 2015-04-23 11:27:27 UTC Comment hidden (obsolete)
Comment 50 Katrin Fischer 2015-04-23 11:59:54 UTC
(In reply to Katrin Fischer from comment #48)

> 1) 
File names make sense now.

> 6) --csv
> This one is giving me some trouble:

This is solved, I had an error in my thinking about the template - using <<items.barcode>> in ODUE doesn't work without <item></item>. 

The remaining problem is now with the ods export, working on it with Joubu:

> 5) --ods creates a warn:
> Use of uninitialized value in lc at
> /usr/share/perl5/OpenOffice/OODoc/File.pm line 378.
Comment 51 Katrin Fischer 2015-04-23 12:28:55 UTC Comment hidden (obsolete)
Comment 52 Katrin Fischer 2015-04-23 12:29:03 UTC Comment hidden (obsolete)
Comment 53 Katrin Fischer 2015-04-23 12:29:11 UTC Comment hidden (obsolete)
Comment 54 Katrin Fischer 2015-04-23 12:29:15 UTC Comment hidden (obsolete)
Comment 55 Katrin Fischer 2015-04-23 12:29:20 UTC Comment hidden (obsolete)
Comment 56 Katrin Fischer 2015-04-23 12:29:25 UTC Comment hidden (obsolete)
Comment 57 Katrin Fischer 2015-04-23 12:29:29 UTC Comment hidden (obsolete)
Comment 58 Katrin Fischer 2015-04-23 12:29:34 UTC Comment hidden (obsolete)
Comment 59 Katrin Fischer 2015-04-23 12:29:38 UTC Comment hidden (obsolete)
Comment 60 Katrin Fischer 2015-04-23 12:34:58 UTC
The warn only appeared on my system, but the file was still created. For me this is not a blocker currently.

This patch is a big improvement over the existing script, while I couldn't detect any regressions. It adds useful options. The file name changes slightly, but this is a fix to a bug - the file doesn't only include holds messages nowadays and hasn't for quite a while. The file name properly reflects that now. (bug 12810)

There seem to be some issues in our way to enqueue messages and in determining the mtt = print - but they are outside the scope of this script, that just deals with the results of the processes done before.
Comment 61 Tomás Cohen Arazi 2015-05-06 15:08:03 UTC
Can someone rebase this one ASAP? I'm sorry...
Comment 62 Jonathan Druart 2015-05-11 10:06:44 UTC
Created attachment 39033 [details] [review]
Bug 11603: Preparation step, cleaning up

This patch refactores and adds some good practices:
- use Modern::Perl
- use Pod::Usage
- add POD

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 63 Jonathan Druart 2015-05-11 10:06:49 UTC
Created attachment 39034 [details] [review]
Bug 11603: Gather print notices - add a csv parameter

This patch adds:
- the ability to generate a csv file instead of a html file.
- a letter_code parameter.

From now you are able to generate a csv file for print notices.

Imagine a template notice defined as:
cardnumber:patron:email:item
<<borrowers.cardnumber>>:<<borrowers.firstname>> <borrowers.surname>>:<<borrowers.email>>:<<items.barcode>>

You would like to generate a csv file and not a html file.

Test plan:
- define your ODUE notice for the print template as:

cardnumber:patron:email:item
<<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item>

- define overdues rules for a patron category
- check 2 items out using a due date in order to generate the overdue
  notices
- check these 2 items in
- launch the overdue_notices script
- the message_queue table should now contain 2 new entries
- launch the gather_print_notices cronjob with the following parameters:
 perl misc/cronjobs/gather_print_notices.pl /tmp/test --csv
 --letter_code=OVERDUE --letter_code=CHECKIN
you should get an error

 perl misc/cronjobs/gather_print_notices.pl /tmp/test --csv
you should get an error

 perl misc/cronjobs/gather_print_notices.pl /tmp/test --csv
 --letter_code=OVERDUE -d=:
 will produce 1 csv file in your /tmp/test directory
- verify the csv file is correct and contain only 1 csv header column.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
QA note: Keep in mind that you can use all placeholders for the
csv that you can use for the normal templates. If you normally
get the item information from <item></item> you need to use that.
If you can use <<item.barcode>> directly, you can also do so
in the csv.
Comment 64 Jonathan Druart 2015-05-11 10:06:54 UTC
Created attachment 39035 [details] [review]
Bug 11603: Gather print notices - add a ods parameter

This patch adds:
- the ability to generate an ods file

From now you are able to generate a ods file for print notices.

You would like to generate a csv file and not a html file.

Test plan:
- same as previous patch but test the following parameters:
 perl misc/cronjobs/gather_print_notices.pl /tmp/test --ods
 --letter_code=OVERDUE -d=:
you should get an error because csv2ods is not installed.
Follow the installation instructions and try again the command.

A ods file should be generated in your /tmp/test directory.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 65 Jonathan Druart 2015-05-11 10:06:58 UTC
Created attachment 39036 [details] [review]
Bug 11603: If a letter code is given, filenames should contain it

If you choose to generate print notices for a specific letter code, the
generated files should be distinct.

The use case is: you want to process print notice for letter codes:
overdue1, overdue2 and overdue3.

The cronjobs will be:

perl misc/cronjobs/gather_print_notices.pl
/tmp --letter_code=overdue1 --csv --ods --html --delimiter=";"

perl misc/cronjobs/gather_print_notices.pl
/tmp --letter_code=overdue2 --csv --ods --html --delimiter=";"

perl misc/cronjobs/gather_print_notices.pl
/tmp --letter_code=overdue3 --csv --ods --html --delimiter=";"

without this patch, the 2 first files will be erased.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 66 Jonathan Druart 2015-05-11 10:07:02 UTC
Created attachment 39037 [details] [review]
Bug 11603: Fix encoding issues (bug 11944 is now pushed)

Signed-off-by: Frederic Demians <f.demians@tamil.fr>
  No more encoding issue with html file, no problem with csv|ods

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 67 Jonathan Druart 2015-05-11 10:07:07 UTC
Created attachment 39038 [details] [review]
Bug 11603 Add --send|--nosend, fix stuf

Fix --html without --letter_code
Fix --ods which was producing a 2 lines ods file

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 68 Jonathan Druart 2015-05-11 10:07:11 UTC
Created attachment 39039 [details] [review]
Bug 11603: Some minor documentation changes

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 69 Jonathan Druart 2015-05-11 10:07:15 UTC
Created attachment 39040 [details] [review]
Bug 11603: (follow-up) pod

The filenames will change

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 70 Jonathan Druart 2015-05-11 10:07:20 UTC
Created attachment 39041 [details] [review]
Bug 11603: Change the output filenames

The output filename is notices_all_<date>.[html|csv|ods] if no
letter_code parameter is given.
If 1 is given: notices_<letter_code>_<date>.[html|csv|ods]
If 1+ are given:
noties_<letter_code1>_..._<letter_codeN>_<date>.[html|csv|ods]

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 71 Tomás Cohen Arazi 2015-06-01 17:34:43 UTC
Patches pushed to master.

Good job Jonathan!