Bug 8007 - Discharge management
Summary: Discharge management
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Matthias Meusburger
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14250 16463 14368 15823 16127 16464
  Show dependency treegraph
 
Reported: 2012-04-25 09:47 UTC by claire.hernandez@biblibre.com
Modified: 2017-06-14 22:02 UTC (History)
19 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this: 2013-09-29 00:00:00
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 8007: Discharge management (64.48 KB, patch)
2013-01-09 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (64.47 KB, patch)
2013-03-21 13:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (64.58 KB, patch)
2013-03-27 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (64.60 KB, patch)
2013-04-16 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Add a discharges example file (1.04 KB, patch)
2013-04-16 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (64.89 KB, patch)
2013-09-24 07:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Add a discharges example file (1.04 KB, patch)
2013-09-24 07:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: QA Followup (5.13 KB, patch)
2013-09-24 07:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Followup: fix read dir error (2.61 KB, patch)
2013-09-30 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8007: Discharge management (65.47 KB, patch)
2013-09-30 17:43 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8007: Add a discharges example file (1.11 KB, patch)
2013-09-30 17:43 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8007: QA Followup (5.20 KB, patch)
2013-09-30 17:43 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8007: Followup: fix read dir error (2.71 KB, patch)
2013-09-30 17:44 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8007: Followup: fix read dir error (2.71 KB, patch)
2013-09-30 17:48 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 8007: Discharge - DB changes (33.24 KB, patch)
2014-01-08 17:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DBIx changes (5.21 KB, patch)
2014-01-08 17:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Add unit tests (3.19 KB, patch)
2014-01-08 17:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (23.29 KB, patch)
2014-01-08 17:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Glue (9.74 KB, patch)
2014-01-08 17:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DB changes (38.62 KB, patch)
2014-08-04 14:17 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 8007: Discharge - DBIx changes (4.06 KB, patch)
2014-08-04 14:17 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 8007: Discharge - Glue (10.98 KB, patch)
2014-08-04 14:18 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 8007: Discharge management (20.16 KB, patch)
2014-08-04 14:18 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 8007: Discharge - Add unit tests (3.14 KB, patch)
2014-08-04 14:18 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 8007 : Discharge - Adding Perl dependencie (834 bytes, patch)
2014-08-20 08:26 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 8007: Discharge - DB changes (38.12 KB, patch)
2014-12-01 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DBIx changes (4.06 KB, patch)
2014-12-01 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Glue (10.32 KB, patch)
2014-12-01 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (20.16 KB, patch)
2014-12-01 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Add unit tests (3.14 KB, patch)
2014-12-01 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007 : Discharge - Adding Perl dependencie (833 bytes, patch)
2014-12-01 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DB changes (38.12 KB, patch)
2014-12-24 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DBIx changes (4.06 KB, patch)
2014-12-24 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Glue (10.32 KB, patch)
2014-12-24 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (20.16 KB, patch)
2014-12-24 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Add unit tests (3.14 KB, patch)
2014-12-24 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007 : Discharge - Adding Perl dependencie (833 bytes, patch)
2014-12-24 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Replace HTML::HTMLDoc with PDF::FromHTML (1.92 KB, patch)
2014-12-24 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Compatibility with bug 11944 (2.33 KB, patch)
2014-12-24 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DB changes (35.09 KB, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DBIx changes (4.06 KB, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Glue (10.26 KB, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge management (20.16 KB, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - Add unit tests (3.14 KB, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007 : Discharge - Adding Perl dependencie (833 bytes, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Replace HTML::HTMLDoc with PDF::FromHTML (1.92 KB, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Compatibility with bug 11944 (2.33 KB, patch)
2015-02-26 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Discharge - DB changes (35.16 KB, patch)
2015-03-03 11:22 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 8007: Discharge - DBIx changes (4.11 KB, patch)
2015-03-03 11:22 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 8007: Discharge - Glue (10.31 KB, patch)
2015-03-03 11:22 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 8007: Discharge management (20.21 KB, patch)
2015-03-03 11:23 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 8007: Discharge - Add unit tests (3.19 KB, patch)
2015-03-03 11:23 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 8007 : Discharge - Adding Perl dependencie (892 bytes, patch)
2015-03-03 11:23 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 8007: Replace HTML::HTMLDoc with PDF::FromHTML (1.98 KB, patch)
2015-03-03 11:23 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 8007: Compatibility with bug 11944 (2.39 KB, patch)
2015-03-03 11:23 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Discharge - DB changes (35.61 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Discharge - DB changes (35.61 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Discharge - DBIx changes (4.18 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Discharge - Glue (10.29 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Discharge management (20.29 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Discharge - Add unit tests (3.27 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007 : Discharge - Adding Perl dependencie (976 bytes, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Replace HTML::HTMLDoc with PDF::FromHTML (2.05 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 8007: Compatibility with bug 11944 (2.46 KB, patch)
2015-03-20 12:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8007: Add error handling when generating the pdf (6.52 KB, patch)
2015-04-28 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8007: Add error handling when generating the pdf (6.78 KB, patch)
2015-04-28 09:03 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description claire.hernandez@biblibre.com 2012-04-25 09:47:26 UTC
discharge management : on the patron page, add a discharge button. If the patron has no more check-outs, print the discharge. Cancel any hold, suspend the patron. The discharge is saved in PDF and accessible by the patron itself from OPAC. 


Documentation will be added here:
http://wiki.koha-community.org/wiki/Discharge_RFC
If you have things to discuss please, use the wiki discussion page.
Comment 1 Jonathan Druart 2013-01-09 14:03:37 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2013-01-09 14:05:01 UTC
What is a "discharge" in this context?
Comment 3 Jonathan Druart 2013-01-09 14:41:09 UTC
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.
Comment 4 Kyle M Hall 2013-02-15 13:47:37 UTC
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.
Comment 5 Jonathan Druart 2013-02-15 15:54:11 UTC
Do you have the xhtml2pdf command available?
It is provided by the python-pisa package.
Comment 6 Kyle M Hall 2013-02-15 16:43:27 UTC
(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.
Comment 7 Kyle M Hall 2013-02-15 16:57:17 UTC
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
Comment 8 Jonathan Druart 2013-03-21 13:25:59 UTC Comment hidden (obsolete)
Comment 9 Pierre Angot 2013-03-21 15:24:31 UTC
I have note tested this problem. So failed qa
Comment 10 Pierre Angot 2013-03-21 15:28:34 UTC
Sorry. I've made a mistake. This on is in need signal off
Comment 11 Pierre Angot 2013-03-21 16:39:56 UTC
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 ?
Comment 12 mathieu saby 2013-03-21 21:41:29 UTC
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
Comment 13 Katrin Fischer 2013-03-21 22:51:58 UTC
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?
Comment 14 Jonathan Druart 2013-03-27 14:52:51 UTC
(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.
Comment 15 Jonathan Druart 2013-03-27 14:53:27 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2013-04-12 13:44:43 UTC
(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.
Comment 17 Jonathan Druart 2013-04-16 09:12:47 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2013-04-16 09:12:53 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2013-04-16 09:13:43 UTC
(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.
Comment 20 M. Tompsett 2013-09-23 17:11:55 UTC
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
Comment 21 Jonathan Druart 2013-09-24 07:22:41 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2013-09-24 07:22:49 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2013-09-24 07:22:55 UTC Comment hidden (obsolete)
Comment 24 I'm just a bot 2013-09-29 06:12:08 UTC
Patch applied cleanly, go forth and signoff
Comment 25 Bernardo Gonzalez Kriegel 2013-09-29 21:14:41 UTC
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.
Comment 26 Jonathan Druart 2013-09-30 13:13:25 UTC
(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.
Comment 27 Jonathan Druart 2013-09-30 13:13:56 UTC Comment hidden (obsolete)
Comment 28 Bernardo Gonzalez Kriegel 2013-09-30 14:58:52 UTC
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)
Comment 29 Bernardo Gonzalez Kriegel 2013-09-30 17:43:31 UTC Comment hidden (obsolete)
Comment 30 Bernardo Gonzalez Kriegel 2013-09-30 17:43:42 UTC Comment hidden (obsolete)
Comment 31 Bernardo Gonzalez Kriegel 2013-09-30 17:43:56 UTC Comment hidden (obsolete)
Comment 32 Bernardo Gonzalez Kriegel 2013-09-30 17:44:12 UTC Comment hidden (obsolete)
Comment 33 Bernardo Gonzalez Kriegel 2013-09-30 17:48:38 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2013-10-01 12:21:04 UTC
(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.
Comment 35 Katrin Fischer 2013-11-20 22:16:33 UTC
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 36 Jonathan Druart 2014-01-08 17:46:42 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2014-01-08 17:46:52 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2014-01-08 17:47:08 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2014-01-08 17:47:23 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2014-01-08 17:47:31 UTC Comment hidden (obsolete)
Comment 41 Chris Cormack 2014-01-08 18:45:50 UTC
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.
Comment 42 Kyle M Hall 2014-01-08 19:04:40 UTC
I would recommend ditching the DataObject::Discharge and go with directly using DBIC with Koha::Database. It seems like an unnecessary layer of overhead.
Comment 43 Kyle M Hall 2014-01-08 19:07:59 UTC
(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 ).
Comment 44 Galen Charlton 2014-01-08 19:18:54 UTC
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.
Comment 45 Jonathan Druart 2014-01-09 10:40:12 UTC
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?
Comment 46 David Cook 2014-03-26 06:59:55 UTC
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...
Comment 47 Yohann Dufour 2014-08-04 14:17:06 UTC Comment hidden (obsolete)
Comment 48 Yohann Dufour 2014-08-04 14:17:56 UTC Comment hidden (obsolete)
Comment 49 Yohann Dufour 2014-08-04 14:18:03 UTC Comment hidden (obsolete)
Comment 50 Yohann Dufour 2014-08-04 14:18:09 UTC Comment hidden (obsolete)
Comment 51 Yohann Dufour 2014-08-04 14:18:15 UTC Comment hidden (obsolete)
Comment 52 Yohann Dufour 2014-08-18 14:08:41 UTC
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.
Comment 53 Marc Véron 2014-08-19 17:04:56 UTC
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
Comment 54 Yohann Dufour 2014-08-20 07:04:39 UTC
In order to be able to generate PDF from HTML with Perl, you need to install first the Perl module HTML::HTMLDoc
Comment 55 Yohann Dufour 2014-08-20 08:26:10 UTC Comment hidden (obsolete)
Comment 56 Paul Poulain 2014-09-15 15:36:29 UTC
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-)
Comment 57 Jonathan Druart 2014-12-01 13:30:28 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2014-12-01 13:30:33 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2014-12-01 13:30:38 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2014-12-01 13:30:43 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2014-12-01 13:30:47 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2014-12-01 13:30:52 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2014-12-01 13:33:20 UTC
Patch rebased against master.
This new feature does not need any other patch to be tested.
Comment 64 Bernardo Gonzalez Kriegel 2014-12-01 14:23:22 UTC
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...
Comment 65 Bernardo Gonzalez Kriegel 2014-12-01 19:08:27 UTC
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
Comment 66 Jonathan Druart 2014-12-24 11:05:58 UTC Comment hidden (obsolete)
Comment 67 Jonathan Druart 2014-12-24 11:06:18 UTC Comment hidden (obsolete)
Comment 68 Jonathan Druart 2014-12-24 11:06:40 UTC Comment hidden (obsolete)
Comment 69 Jonathan Druart 2014-12-24 11:06:57 UTC Comment hidden (obsolete)
Comment 70 Jonathan Druart 2014-12-24 11:07:15 UTC Comment hidden (obsolete)
Comment 71 Jonathan Druart 2014-12-24 11:07:26 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2014-12-24 11:07:31 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2014-12-24 11:07:35 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2014-12-24 11:09:54 UTC
(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.
Comment 75 Jonathan Druart 2015-02-26 15:05:11 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2015-02-26 15:05:17 UTC Comment hidden (obsolete)
Comment 77 Jonathan Druart 2015-02-26 15:05:21 UTC Comment hidden (obsolete)
Comment 78 Jonathan Druart 2015-02-26 15:05:26 UTC Comment hidden (obsolete)
Comment 79 Jonathan Druart 2015-02-26 15:05:30 UTC Comment hidden (obsolete)
Comment 80 Jonathan Druart 2015-02-26 15:05:34 UTC Comment hidden (obsolete)
Comment 81 Jonathan Druart 2015-02-26 15:05:40 UTC Comment hidden (obsolete)
Comment 82 Jonathan Druart 2015-02-26 15:05:45 UTC Comment hidden (obsolete)
Comment 83 Jonathan Druart 2015-02-26 15:06:06 UTC
Patches rebased.
Comment 84 sandboxes@biblibre.com 2015-03-03 11:21:16 UTC
Patch tested with a sandbox, by Lucie <lucie.rousseaux@dracenie.com>
Comment 85 sandboxes@biblibre.com 2015-03-03 11:22:50 UTC Comment hidden (obsolete)
Comment 86 sandboxes@biblibre.com 2015-03-03 11:22:54 UTC Comment hidden (obsolete)
Comment 87 sandboxes@biblibre.com 2015-03-03 11:22:58 UTC Comment hidden (obsolete)
Comment 88 sandboxes@biblibre.com 2015-03-03 11:23:02 UTC Comment hidden (obsolete)
Comment 89 sandboxes@biblibre.com 2015-03-03 11:23:07 UTC Comment hidden (obsolete)
Comment 90 sandboxes@biblibre.com 2015-03-03 11:23:11 UTC Comment hidden (obsolete)
Comment 91 sandboxes@biblibre.com 2015-03-03 11:23:15 UTC Comment hidden (obsolete)
Comment 92 sandboxes@biblibre.com 2015-03-03 11:23:19 UTC Comment hidden (obsolete)
Comment 93 Katrin Fischer 2015-03-06 15:29:27 UTC
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?
Comment 94 Marc Véron 2015-03-06 15:44:26 UTC
(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
Comment 95 Jonathan Druart 2015-03-10 09:42:13 UTC
(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.
Comment 96 Matthias Meusburger 2015-03-10 15:45:31 UTC
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.
Comment 97 Kyle M Hall 2015-03-20 12:58:12 UTC Comment hidden (obsolete)
Comment 98 Kyle M Hall 2015-03-20 12:58:31 UTC
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>
Comment 99 Kyle M Hall 2015-03-20 12:58:38 UTC
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>
Comment 100 Kyle M Hall 2015-03-20 12:58:41 UTC
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>
Comment 101 Kyle M Hall 2015-03-20 12:58:44 UTC
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>
Comment 102 Kyle M Hall 2015-03-20 12:58:48 UTC
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>
Comment 103 Kyle M Hall 2015-03-20 12:58:51 UTC
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>
Comment 104 Kyle M Hall 2015-03-20 12:58:54 UTC
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>
Comment 105 Kyle M Hall 2015-03-20 12:58:57 UTC
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>
Comment 106 Tomás Cohen Arazi 2015-04-27 19:34:45 UTC
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.
Comment 107 Robin Sheat 2015-04-28 00:27:37 UTC
(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.
Comment 108 Jonathan Druart 2015-04-28 07:41:39 UTC
(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.
Comment 109 Robin Sheat 2015-04-28 08:07:36 UTC
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.
Comment 110 Jonathan Druart 2015-04-28 09:02:11 UTC Comment hidden (obsolete)
Comment 111 Jonathan Druart 2015-04-28 09:03:23 UTC
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;
Comment 112 Tomás Cohen Arazi 2015-04-28 18:06:56 UTC
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)
Comment 113 Tomás Cohen Arazi 2015-04-30 19:43:01 UTC
Feature pushed to master.

Thanks Yohann and Jonathan!
Comment 114 David Cook 2015-05-05 00:44:22 UTC
(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?
Comment 115 David Cook 2015-05-05 02:27:10 UTC
(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?
Comment 116 Robin Sheat 2015-05-05 04:46:03 UTC
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 :)
Comment 117 Marcel de Rooy 2015-05-07 11:11:58 UTC
(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?
Comment 118 Jonathan Druart 2015-05-07 11:21:27 UTC
(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.
Comment 119 Robin Sheat 2015-05-08 01:24:49 UTC
(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.
Comment 120 Robin Sheat 2015-05-19 22:58:11 UTC
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.
Comment 121 Robin Sheat 2015-05-20 03:15:00 UTC
(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.
Comment 122 Katrin Fischer 2015-05-21 20:42:11 UTC
Yay!
Comment 123 M. Tompsett 2015-06-03 19:11:16 UTC
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.