What is a "discharge" in this context?

In France a "quitus" (translated here by "discharge") is needed if you want to register in a state-owned company (library or university).
It means the patron don't have any document in his possession / is in order with his previous library.

When I try to test this, I get the following error: An error has occured while generating the discharge. Please retry later or contact your administrator.

However, the process created the file 2420130215.tar.gz and the empty directory '24' ( i.e. the borrowernumber ) in the discharges directory.

Do you have the xhtml2pdf command available?
It is provided by the python-pisa package.

(In reply to comment #5)
> Do you have the xhtml2pdf command available?
> It is provided by the python-pisa package.

Nope, my bad. I missed that part of the test plan.

I'm still having problems even after installing that package:

[Fri Feb 15 11:56:37 2013] [error] [client 192.168.1.30] Traceback (most recent call last):, referer: http://staff.kohadev/cgi-bin/koha/members/discharge.pl?borrowernumber=18
[Fri Feb 15 11:56:37 2013] [error] [client 192.168.1.30]   File "/usr/bin/xhtml2pdf", line 5, in <module>, referer: http://staff.kohadev/cgi-bin/koha/members/discharge.pl?borrowernumber=18
[Fri Feb 15 11:56:37 2013] [error] [client 192.168.1.30]     from pkg_resources import load_entry_point, referer: http://staff.kohadev/cgi-bin/koha/members/discharge.pl?borrowernumber=18
[Fri Feb 15 11:56:37 2013] [error] [client 192.168.1.30] ImportError: No module named pkg_resources, referer: http://staff.kohadev/cgi-bin/koha/members/discharge.pl?borrowernumber=18
[Fri Feb 15 11:56:37 2013] [error] [client 192.168.1.30] tar: Removing leading `/' from member names, referer: http://staff.kohadev/cgi-bin/koha/members/discharge.pl?borrowernumber=18
[Fri Feb 15 11:56:37 2013] [error] [client 192.168.1.30] [Fri Feb 15 11:56:37 2013] discharge.pl: readline() on unopened filehandle at /home/koha/kohaclone/C4/Discharges.pm line 68., referer: http://staff.kohadev/cgi-bin/koha/members/discharge.pl?borrowernumber=18

I have note tested this problem. So failed qa

Sorry. I've made a mistake. This on is in need signal off

In a sandbox :
- pro : when I click on the discharges button, nothing happen.
- opac : there is an error "could not been sent"

Is there a problem with sandbox ?

Tested on my VM : patch does not apply
Please also consider Pierre's comment.

warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql

Mathieu Saby

I am worried about libraries taking this not seriously or just missing that they should be doing that:

- For a security reason create a .htaccess in your dischargePath with
  the following content:
      Options -Indexes

Could we provide a standard path and a .htaccess file?

(In reply to comment #13)
> I am worried about libraries taking this not seriously or just missing that
> they should be doing that:
> 
> - For a security reason create a .htaccess in your dischargePath with
>   the following content:
>       Options -Indexes
> 
> Could we provide a standard path and a .htaccess file?

I don't think so. This file should be placed in the directory pointed out by the syspref dischargePath. Moreover the file should have specific rights (www-data). We can do that in a patch.

I complete the RFC page on the wiki.

(In reply to comment #14)
> (In reply to comment #13)
> > I am worried about libraries taking this not seriously or just missing that
> > they should be doing that:
> > 
> > - For a security reason create a .htaccess in your dischargePath with
> >   the following content:
> >       Options -Indexes
> > 
> > Could we provide a standard path and a .htaccess file?
> 
> I don't think so. This file should be placed in the directory pointed out by
> the syspref dischargePath. Moreover the file should have specific rights
> (www-data). We can do that in a patch.
> 
> I complete the RFC page on the wiki.


This can be done! My first iteration of the plugins feature did this. You'll need to modify the installation files to create var/lib/discharges or such, and put the .htaccess file there. You can check out early versions of the plugins patch to see how to do it. The .htaccess I believe will need to go somewhere in skel skel/var/lib/koha/discharges, and I think that would give a standard path. If you want to have a nonstandard path, just symlink var/lib/discharges to your nonstandard path.

(In reply to comment #16)
> This can be done! My first iteration of the plugins feature did this. You'll
> need to modify the installation files to create var/lib/discharges or such,
> and put the .htaccess file there. You can check out early versions of the
> plugins patch to see how to do it. The .htaccess I believe will need to go
> somewhere in skel skel/var/lib/koha/discharges, and I think that would give
> a standard path. If you want to have a nonstandard path, just symlink
> var/lib/discharges to your nonstandard path.

Done, thank you Kyle.

I don't think it applies cleanly.
mtompset@ubuntu:~/kohaclone2$ git bz apply 8007
Bug 8007 - Discharge management

17484 - Bug 8007: Discharge management
17485 - Bug 8007: Add a discharges example file

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8007: Discharge management
Using index info to reconstruct a base tree...
<stdin>:587: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging misc/cronjobs/gather_print_notices.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/de-DE/mandatory/sample_notices.sql
Auto-merging C4/Members.pm
Auto-merging C4/Letters.pm
Auto-merging C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8007: Discharge management
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-8007-Discharge-management-8LjEoI.patch

Patch applied cleanly, go forth and signoff

Testing this.

It's a good feature. Our university students need to get a 'discharge' certificate (we call it "libre deuda"). The best we can do with Koha is print a circulation slip that shows no items on loan.

Test
1) Patches applied, a few touchs on updatedatabase.pl 
(using CheckVersion($DBversion) instead of C4::Context->preference("Version") < TransformToNum($DBversion), add Bug number to update message)

2) Enabled sysprefs, created dir, etc.
3) OPAC, patron view Discarged tab (I think it could be "my discharge" or "discharges" with lower case for consistency)
4) OPAC, patron with an issue can't generate discharge, but the message do not state that fact, only tells that can´t be generated
5) OPAC, patron without issue can request discharge, mail to library sent
6) STAFF, Discharge tab is Upercase, in sync with others
7) STAFF, trying to generate discharge for patron with issue states that fact.
8) STAFF, patron without issue...

This is when I run into problems. Staff interface only tells "An error has ..."

Files are created!!
(
kohaclone$ ls -l ../kohadev/discharges/52
total 8
-rw-r--r-- 1 www-data www-data  668 sep 29 16:50 discharge-2013-09-29.html
-rw-r--r-- 1 www-data www-data 2801 sep 29 16:50 discharge-2013-09-29.pdf
)

Log shows this:
discharge.pl: readline() on unopened filehandle at /home/bgkriegel/kohaclone/C4/Discharges.pm line 68., referer: http://staffdev.koha-community.org.ar/cgi-bin/koha/members/discharge.pl?borrowernumber=52

Looking into the code (line 68) I fund that you read files in user dir like

my $pdf_path = qq{$dischargePath/$borrowernumber/*.pdf};
my @files = <$pdf_path>;

I can replicate the error message using

perl -e 'use Modern::Perl; my $p = qq{52/*.pdf}; my @f = <$p>;'
readline() on unopened filehandle at -e line 1.
[with or without files]

So, all stops trying to read files.

I think this is a bug, or perhaps I'm doing something wrong.

(In reply to Bernardo Gonzalez Kriegel from comment #25)
> Testing this.

Thanks Bernardo for testing!

> I think this is a bug, or perhaps I'm doing something wrong.

Yes, it is a bug. I will provide a followup which will fix 3 issues.

Worked! :)

Last question:
On a dev install, koha-tmpl is accesible from oapc and staff,
but on a packages/tar install DocumentRoot for opac and staff
do not overlap.

opac  -> DocumentRoot /usr/share/koha/opac/htdocs
staff -> DocumentRoot /usr/share/koha/intranet/htdocs

I solved this doing a symlink of discharges dir to both htdocs
but this must be stated on instructions

Do you plan some followup to remove these files?
I think that if the patron is lifted from a discharge
the files must be removed, or make them inaccesible (from opac)

(In reply to Bernardo Gonzalez Kriegel from comment #28)
> Worked! :)
> 
> Last question:
> On a dev install, koha-tmpl is accesible from oapc and staff,
> but on a packages/tar install DocumentRoot for opac and staff
> do not overlap.
> 
> opac  -> DocumentRoot /usr/share/koha/opac/htdocs
> staff -> DocumentRoot /usr/share/koha/intranet/htdocs
> 
> I solved this doing a symlink of discharges dir to both htdocs
> but this must be stated on instructions

I think this feature should be installed/configured by a sysadmin.
Paths have to be adapted depending each installation/preference.

> Do you plan some followup to remove these files?

No, currently I don't have followups for this feature.

Hi Jonathan, 

I am sorry, but the patch does no longer apply. Some questions I had from looking at the code/patches:

1) Can we ensure that patrons can't access discharge notices from other patrons by guessing the file name? 

I know this would be a rewrite, but keep wondering if this could be done dynamically and without storing files. Like have a flag that the user is allowed to download a discharge notice with date x and then generate the pdf file dynamically. 

2) The email send to the library is not translatable. Instead of sending an email to the library, it could show up on the dashboard.

3) Can you provide unit tests for the new module and routines please?

4) I think this bit will have to be changed for the new handling of debarments/restrictions:

+        # (this is quite ugly, but this is how borrowers seems to be permanently debarred)
+        # FIXME: Perl strings are not translatable atm, so comment is written in english
+        C4::Members::DebarMember($borrowernumber, '9999-12-31', "Discharge generated by $librarianname on $date");

5) Question: Which cronjobs are needed for this?

Comment on attachment 24026 [details] [review]
Bug 8007: Discharge management

Review of attachment 24026 [details] [review]:
-----------------------------------------------------------------

Just a couple of quick comments, I'll do more.

I have to say, I am really not a fan of the ::DataObject:: and ::Service:: idea. I may be overruled on that I guess. I'd prefer a Discharge class I think.

::: Koha/DataObject/Discharge.pm
@@ +24,5 @@
> +    );
> +
> +    bless $self, $class;
> +    return $self;
> +}

You don't need this, Class::Accessor does this new for you.
Including populating the accessors. Thats one of the reasons for using it :)

You just call it like this. 
my $discharge = Koha::DataObject::Discharge->new({borrowernumber => $number, needed => $needed, validated => $validated});

@@ +54,5 @@
> +    my $data = $rs->single( { borrower => $self->borrowernumber } );
> +    $self->needed( $data->needed );
> +    $self->validated( $data->validated );
> +    return $self;
> +}

You could have based this class on Koha::Database and done things like

use base qw/Koha::Database/;

sub fetch {
    my $self = shift;
    my $self->schema->resultset('Discharge')->single( { borrower => $self->borrowernumber});

etc

I am a little worried setting a packagewide rs is not safe when run under persistence. But I would need to check that. DBIX::Class is smart, and does pooling and connection reuse for us, we don't need to do it ourselves.

I would recommend ditching the DataObject::Discharge and go with directly using DBIC with Koha::Database. It seems like an unnecessary layer of overhead.

(In reply to Chris Cormack from comment #41)
> I have to say, I am really not a fan of the ::DataObject:: and ::Service::
> idea. I may be overruled on that I guess. I'd prefer a Discharge class I
> think.

I believe in our developer discussions at KohaCon13 the basic consensus was that the DataObject layer was not needed and was just over-complicating everything and adding unnecessary overhead ( it was only in our initial designs to join biblio and biblioitems virtually ).

Jonathan: first, let me thank you for engaging with DBIC.

(In reply to Kyle M Hall from comment #43)
> (In reply to Chris Cormack from comment #41)
> > I have to say, I am really not a fan of the ::DataObject:: and ::Service::
> > idea. I may be overruled on that I guess. I'd prefer a Discharge class I
> > think.
> 
> I believe in our developer discussions at KohaCon13 the basic consensus was
> that the DataObject layer was not needed and was just over-complicating
> everything and adding unnecessary overhead ( it was only in our initial
> designs to join biblio and biblioitems virtually ).

I agree.  There are *some* specific cases, most notably bibs, where an intermediate class might be useful to deal with legacy defects in the current data model, and some cases where complicated business logic suggests a container class, but I am not in favor of creating DataObject classes or the like automatically.

Thanks all 3 of you for your looks! 

The idea of Service and DataObject was to separate methods for a discharge object from others routines which are not directly linked to the object ("Manager").

Don't you think if we put all the code in a Koha::Discharge package we will get back to a new Koha namespace with big files where responsibilities are not enough separated?

What is the problem with having a simple business object? I don't understand where the complexity is added, to me it is simplified. Should I put all methods/routines from Service::Discharge and DataObject::Discharge to Koha::Discharge?

I personally think we should not call Koha::Database in pl files, do we agree with that?

If you think it is not the way to do, where can I find a RFC / consensus on how developers should develop using oop into Koha?

I'm even all right with benign dictatorship saying how it's going to be from here on out, but it would be good to have firm guidelines in place...

I have refactored the work of Jonathan in order to adapt it to the different comments I have read on this bug report.
I have tried to find the best compromise between organization, heaviness and simplicity to finally integrate this improvement in Koha.

In Opac, I had a software error:

Can't locate HTML/HTMLDoc.pm in @INC (@INC contains: /usr/share/kohaclone /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at /usr/share/kohaclone/Koha/Borrower/Discharge.pm line 6

In order to be able to generate PDF from HTML with Perl, you need to install first the Perl module HTML::HTMLDoc

Even if this enhancement is highly usefull for a end-of-year school time and we are at the beginning of it, it's a very valuable one.

Bernardo, any chance you could test it again and sign-off ? Chris/Katrin/Galen/Kyle, comments from the QA POV ? Jonathan has risen a topic about DBIx::Class on koha-devel, comments welcomed. I would really like to have a positive issue on this matter, to know how to write code, not only to know how to not write code !
(hint: chris, katrin, Jonathan, Bernardo, Tomas and I will be in Cordoba, -not kyle, I don't know for Galen-)

Patch rebased against master.
This new feature does not need any other patch to be tested.

Hi, patch applies and works.

One question: From it's documentation, HTML::HTMLDoc does no support UTF8 :)

From http://search.cpan.org/~mfrankl/HTML-HTMLDoc-0.07/lib/HTML/HTMLDoc.pm
"
set_charset($charset)

defines the charset for the output document. The following charsets are currenty supported: cp-874 cp-1250 cp-1251 cp-1252 cp-1253 cp-1254 cp-1255 cp-1256 cp-1257 cp-1258 iso-8859-1 iso-8859-2 iso-8859-3 iso-8859-4 iso-8859-5 iso-8859-6 iso-8859-7 iso-8859-8 iso-8859-9 iso-8859-14 iso-8859-15 koi8-r
"

Tested editing DISCHARGE letter and adding some diacritics, 
For áéíóúñÑ äà I got áéÃ−óúñà äà on PDF

We need 11944...

Would it be possible to use PDF::fromHTML?
(http://search.cpan.org/~autrijus/PDF-FromHTML-0.12/lib/PDF/FromHTML.pm)

With that we can set correct encoding and font, for example DejaVuSans,
and have a decent international support

(In reply to Bernardo Gonzalez Kriegel from comment #65)
> Would it be possible to use PDF::fromHTML?
> (http://search.cpan.org/~autrijus/PDF-FromHTML-0.12/lib/PDF/FromHTML.pm)

Now the discharge use this module.

(In reply to Bernardo Gonzalez Kriegel from comment #64)
> We need 11944...

I submitted an optional patch ( 8007: Compatibility with bug 11944 ). You can test this patch with or without bug 11944.
Both solutions should work.

Patches rebased.

Patch tested with a sandbox, by Lucie <lucie.rousseaux@dracenie.com>

Workflow question: After the patron has printed a discharge notice from the OPAC he can go to the library and check out books again, so the discharge would then be invalid. How can we deal with that?

(In reply to Katrin Fischer from comment #93)
> Workflow question: After the patron has printed a discharge notice from the
> OPAC he can go to the library and check out books again, so the discharge
> would then be invalid. How can we deal with that?

I would expect a restriction "Discharged" (similar to manual restricitions).

It would display a message like:
Restricted: Patron's account is restricted with the explanation:
Discharched

(In reply to Marc Véron from comment #94)
> I would expect a restriction "Discharged" (similar to manual restricitions).
> 
> It would display a message like:
> Restricted: Patron's account is restricted with the explanation:
> Discharched

Should work like that (looking at the code). I suspect the updatedb entry was not executed.

General tests: 
 - discharge a patron as a librarian: ok
 - ask for a discharge as a patron: ok.

For the specific issue of restricting the patron:
 - in both cases, once the discharge was generated, the patron was restricted with : 
   - Type: Discharge
   - Comment: <none>
   - Expiration: Indefinite.

Restriction works for me. I'm putting this enh back in Signed-Off.

Created attachment 37030 [details] [review]
[PASSED QA] Bug 8007: Discharge - DB changes

This patch adds:
- syspref useDischarge
- the new table discharges
- the discharge notice

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 37031 [details] [review]
[PASSED QA] Bug 8007: Discharge - DBIx changes

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 37032 [details] [review]
[PASSED QA] Bug 8007: Discharge - Glue

This patch adds:
- links to the new pages.
- syspref description
- links on the main page (intranet)
- the DISCHARGE type for debarment

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 37033 [details] [review]
[PASSED QA] Bug 8007: Discharge management

This patch is the main patch. It adds new package and files for the new
pages (opac-discharge, members/discharge and members/discharges).

At the intranet, it is now possible to generate a discharge for a patron.
At the opac, a patron can request a discharge and a discharge if it has
been validated by a librarian.

Requirements:
    The perl module PDF::FromHTML

New sysprefs:
 - useDischarge: Allows librarians to discharge borrowers and borrowers
   to request a discharge

New letter with a letter_code DISCHARGE.

Test plan:
- Switch on the syspref useDischarge.
- Verify a new tab appears in the patron page (intranet and opac).
- Verify the discharge cannot be generated if the patron has issues.
- Verify the patron can request a discharge from it's opac area.
- The request appears on the main page (intranet).
- Generate the discharge from the intranet.
- Try to download it (from the opac and the intranet).

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 37034 [details] [review]
[PASSED QA] Bug 8007: Discharge - Add unit tests

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 37035 [details] [review]
[PASSED QA] Bug 8007 : Discharge - Adding Perl dependencie

HTML::HTMLDoc is required in order to generate the discharges in PDF format.

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 37036 [details] [review]
[PASSED QA] Bug 8007: Replace HTML::HTMLDoc with PDF::FromHTML

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 37037 [details] [review]
[PASSED QA] Bug 8007: Compatibility with bug 11944

This patch should be applied to work with bug 11944.
If you don't have 11944 yet, just don't apply this patch.

Note that the line
  binmode(STDOUT);
in pl files is mandatory.

It comes from bug 11944 that adds the line
  binmode( STDOUT, ":utf8" );
in C4::Templates::gettemplate.

Signed-off-by: Lucie <lucie.rousseaux@dracenie.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Ok, I hate you guys :-D

Couldn't you just choose a PDF library that is shipped with Debian? :-D
I successfully built both
- libpdf-writer-perl
- libpdf-fromhtml-perl
on Jessie but I need to check with Robin. Maybe he would let me build those. I put this one on hold for a couple days (it works, I like it).

There's also a new non-Perl dependency python-pisa. It definitely needs to be added to control.in on the koha-deps section for this to be pushed.

(In reply to Tomás Cohen Arazi from comment #106)
> Couldn't you just choose a PDF library that is shipped with Debian? :-D

Seconded!

> I successfully built both
> - libpdf-writer-perl
> - libpdf-fromhtml-perl
> on Jessie but I need to check with Robin. Maybe he would let me build those.
> I put this one on hold for a couple days (it works, I like it).

It'll need to build on Wheezy, too. That might be fine, or maybe it won't be. I'll have a look when I get some other people-wanting-dependencies-that-don't-exist issues sorted. I am curious why we need multiple PDF writing things though, is it not possible to standardise around one? (and fair enough if it's not, but I'd like it to be considered before just adding in dependencies.)

Tomás: You can build the packages if you like, so long as they would get into debian :) (or, to be honest, if they pass lintian with pedantic=yes and the pkg-perl profile applied, that's usually 90% of the work done right there. It's about time I started scripting more of the deploy process, and someone else doing stuff with it would make that more necessary.)

> There's also a new non-Perl dependency python-pisa. It definitely needs to
> be added to control.in on the koha-deps section for this to be pushed.

Gar. We literally just removed python as a dependency. Like, a couple of weeks ago. Oh well.

Unrelated to the packaging, and I'm not sure if this is a problem, but it looks like files are generated as borrnum/borrnum+date.tar.gz, and this is a totally predictable filename. Is this an OK thing to have?

Also, should paths be a system preference? Should library staff be able to change the web paths and file paths on the system? That seems like something that is out of the scope of library policy and well into the scope of systems admin, and so defined in koha-conf.xml. My litmus test there is "does it make sense for library staff to be able to change this.", and in a case like this, I contend that it's meaningless. It also makes it harder for installation processes to set it to something that suits the distribution.

Actually, the more I think about it, the more of a problem that last part is, as it means there's no way to have a "default works" situation, and there's no way you could expect library staff to fill out a path on the system.

Additionally, this should be creating directories under /var/lib/koha/instancename/ as part of koha-create-dirs to put these files, and alias to them in the appropriate apache template.

Other misc things:

* discharges.pl has no error handling, it just blindly attempts to open a file and send it. What if the file doesn't exist? It looks like you'll then end up with people saving those PDF files that really contain an error message that confuses everyone.
* related, there are other places where files are opened and everything is assumed to be OK. Can we stop assuming that everything is going to be OK? It causes too many problems when it's not. Code defensively, handle errors sensibly.
* the RFC talks about mailing things, but I can't see in the patches where that happens, does it exist? I was trying to verify it was using the mailqueue, as the RFC implies (but doesn't say) that it doesn't.

I'm going to make it failed because there's a good few things in here that need to be considered.

(In reply to Tomás Cohen Arazi from comment #106)
(In reply to Robin Sheat from comment #107)

Hi Tomás and Robin,

As you can see, this bug report has been opened a while ago. And I rewrote the original patch (from Matthias) to make it cleaner.

The wiki page is outdated, I will put a warning on it. Sorry about that (I have forgotten the existence of this page...)

Almost all of your concerns have been addressed/answered in the comments, but I admit 100 comments is a lot to read :)

First, the dependencies. Have a look at comment 64 and comment 65, the packaged perl modules don't seem to work. python-pisa works as needed but as it's a python dependency I preferred to remove it (so python vs non packaged perl module).

Then the paths, the pdf file is generated on the fly (see Katrin's concerns on comment 64). The html and pdf files are generated using tmpnam.

> * the RFC talks about mailing things, but I can't see in the patches where
> that happens, does it exist? I was trying to verify it was using the
> mailqueue, as the RFC implies (but doesn't say) that it doesn't.

This has been abandoned.

The most recent "doc" is the commit messages of the last (non-obsoleted) patches.

I spent days on this development, starting 2 years ago, and I have rewritten it several times. It starts to make me crazy...

Back to Passed QA to let you see my comment.

OK, that does address many of my concerns, though I'd like better error checking still.

I don't mind adding a dependency like this, it was more a grump about having just got rid of python☺

Will have another proper look tomorrow.

Created attachment 38597 [details] [review]
Bug 8007: Add error handling when generating the pdf

If error occurs when generating the pdf, it would be better to get an
encapsulated error instead of the "software error" message in the pdf
file.
To test this patch I added this change:

b/Koha/Borrower/Discharge.pm
-115,6 +115,7 @@ sub generate_as_pdf {
     say $html_fh $html_content;
     close $html_fh;
     my $pdf = PDF::FromHTML->new( encoding => 'utf-8' );
+    $html_path .= "poeut";
     $pdf->load_file( $html_path );
     $pdf->convert;

FTR:

- It doesn't need python-pisa :-D (this was removed at some point)
- The new dependencies for this one are:
  * libhtml-tidy-perl
  * libgraphics-colornames-perl
  * libpdf-fromhtml-perl (build needed)
  * libpdf-writer-perl (build needed)

Feature pushed to master.

Thanks Yohann and Jonathan!

(In reply to Tomás Cohen Arazi from comment #112)
> FTR:
> 
> - It doesn't need python-pisa :-D (this was removed at some point)
> - The new dependencies for this one are:
>   * libhtml-tidy-perl
>   * libgraphics-colornames-perl
>   * libpdf-fromhtml-perl (build needed)
>   * libpdf-writer-perl (build needed)

I'm a bit confused... 

It appears that libgraphics-colornames-perl and libpdf-writer-perl are dependencies for libpdf-fromhtml-perl. However, I don't see libhtml-tidy-perl in the code? I notice that Image::Size is a dependency of PDF::FromHtml though.

Also, why is PDF::FromHTML marked as not required in the koha_perl_deps.pl? As soon as you log in to Koha on master, you're faced with:

"Can't locate PDF/FromHTML.pm in @INC (you may need to install the PDF::FromHTML module)", since Koha::Borrower::Discharge loads on mainpage.pl...

Do we just need a follow-up patch for C4::Installer::PerlDependencies to mark it as required?

(In reply to David Cook from comment #114)
> (In reply to Tomás Cohen Arazi from comment #112)
> > FTR:
> > 
> > - It doesn't need python-pisa :-D (this was removed at some point)
> > - The new dependencies for this one are:
> >   * libhtml-tidy-perl
> >   * libgraphics-colornames-perl
> >   * libpdf-fromhtml-perl (build needed)
> >   * libpdf-writer-perl (build needed)
> 
> I'm a bit confused... 
> 
> It appears that libgraphics-colornames-perl and libpdf-writer-perl are
> dependencies for libpdf-fromhtml-perl. However, I don't see
> libhtml-tidy-perl in the code? I notice that Image::Size is a dependency of
> PDF::FromHtml though.
> 
> Also, why is PDF::FromHTML marked as not required in the koha_perl_deps.pl?
> As soon as you log in to Koha on master, you're faced with:
> 
> "Can't locate PDF/FromHTML.pm in @INC (you may need to install the
> PDF::FromHTML module)", since Koha::Borrower::Discharge loads on
> mainpage.pl...
> 
> Do we just need a follow-up patch for C4::Installer::PerlDependencies to
> mark it as required?

My bad about libhtml-tidy-perl. I already had it installed, so I didn't see it as a dependency. I assume it's a dependency of PDF::FromHTML?

PDF::Writer is pretty straightforward, but PDF::FromHTML is not high enough quality to be a Debian package so is going to take a bit of time to prepare packages for (I'm not willing to take on the technical debt of being a sole maintainer, I'm afraid, so am going to insist that it's ready for debian before I put it into the Koha repo.)

Here's the list of errors it produces:
http://paste.koha-community.org/86

If you want to make some patches to fix any of this, especially the manpage errors, that would help this process along a whole lot.

The git repo is here:
ssh://git.debian.org/git/pkg-perl/packages/libpdf-fromhtml-perl.git

Tomás, this does mean that master isn't going to be able to be built properly until this is resolved.

PS: good practice for anyone who wants to learn about what's needed to package things correctly, which I encourage as then it means less to do for me :)

(In reply to Robin Sheat from comment #116)
> PDF::Writer is pretty straightforward, but PDF::FromHTML is not high enough
> quality to be a Debian package so is going to take a bit of time to prepare
> packages for (I'm not willing to take on the technical debt of being a sole
> maintainer, I'm afraid, so am going to insist that it's ready for debian
> before I put it into the Koha repo.)

If Debian does not like it, why are we using it?
Maybe changing the dependency to a better alternative is not that much work?

(In reply to Marcel de Rooy from comment #117)
> (In reply to Robin Sheat from comment #116)
> > PDF::Writer is pretty straightforward, but PDF::FromHTML is not high enough
> > quality to be a Debian package so is going to take a bit of time to prepare
> > packages for (I'm not willing to take on the technical debt of being a sole
> > maintainer, I'm afraid, so am going to insist that it's ready for debian
> > before I put it into the Koha repo.)
> 
> If Debian does not like it, why are we using it?
> Maybe changing the dependency to a better alternative is not that much work?

You can read back the 100+ comments above.
I did not find a better alternative, otherwise I would have used it.

(In reply to Marcel de Rooy from comment #117)
> If Debian does not like it, why are we using it?
> Maybe changing the dependency to a better alternative is not that much work?

Oh, it won't be a large amount of work to make the package acceptable, it's mostly that it has a lot of incorrectly formatted/missing documentation.* It's just that between fixing things for jessie, and some other things, it might be a bit of time before I get to it. So, I included instructions for someone else to make some progress on polishing it up.

* something I feel that we can't fault it for, but in this case we need to fix it** :)
** I'll probably submit these patches to the upstream project, too.

libpdf-writer-perl is currently in the Debian NEW queue, and in the Koha repository. libpdf-fromhtml-perl is being a more recalcitrant. I'm hoping that by spending some time on it today, I'll succeed it making it build correctly, and if I'm really lucky it'll work in wheezy too.

(In reply to Robin Sheat from comment #120)
> libpdf-writer-perl is currently in the Debian NEW queue, and in the Koha
> repository. libpdf-fromhtml-perl is being a more recalcitrant. I'm hoping
> that by spending some time on it today, I'll succeed it making it build
> correctly, and if I'm really lucky it'll work in wheezy too.

...and after days of wailing and gnashing of teeth, both modules are in a releasable state.

Yay!

Comment on attachment 37034 [details] [review]
[PASSED QA] Bug 8007: Discharge - Add unit tests

Review of attachment 37034 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Borrower_Discharge.t
@@ +18,5 @@
> +
> +$dbh->do(q|DELETE FROM discharges|);
> +
> +C4::Context->_new_userenv('xxx');
> +C4::Context::set_userenv(0, 0, 0, 'firstname', 'surname', 'CPL', 'CPL', '', '', '');

This was pushed after bug 10900. Creating another report to correct this.