The upgrade mechanism really needs to run the same dep check which is run on a clean install... every time.

And error trapping has never been our strong point, but really should become so.

(In reply to Marc Véron from comment #0)
> After the upgrade to 3.19.00.035 the mainpage of the 
> cgi-bin/koha/mainpage.pl  -> Software error: Can't locate PDF/FromHTML.pm in
> @INC
> 
> However, other pages are available, e.g.
> cgi-bin/koha/circ/circulation-home.pl
> 
> There were missing dependencies, see comment on Bug 8007
> 
>   * libhtml-tidy-perl
>   * libgraphics-colornames-perl
>   * libpdf-fromhtml-perl (build needed)
>   * libpdf-writer-perl (build needed)
> 
> However, the depenencies are not listed on the About page (Perl modules).
> 
> The crash on mainpage.pl occurs because does use Koha::Borrower::Discharge
> Maybe this could be handled more gracefully if the dependencies are not
> installed.

The PerlDependencies.pm show only PDF::FromHTML module and is set to don't required.
    'PDF::FromHTML' => {
        'usage'    => 'Discharge generation',
        'required' => '0',
        'min_ver'  => '0.31',
    },

The other modules are requiered by PDF::FromHTML and indeed the error its launched by mainpage.pl at line 32. This is because the use of Koha::Borrower::Discharge.

Maybe Robin can do something to build the packages.


Regards

This bug represent a problem when doing a new installation.

When execute perl koha_perl_deps.pl -u -m gives a table:

Module Name    Installed Version   Required Version   Module is Required
PDF::FromHTML  *                   0.31               No

If you ignore the module and installion haven't done, before doing all steps the software crash, showing us the software error page.

I propose to change the module to required since people without a experience on Koha will become confused.

I add Katrin and Tomás, since this represent a critical bug for 3.20 release

I added a note to bug 8007:

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 :)

I've added some patches using quilt that fix the encoding issues. For an intro to using quilt, go here:

http://pkg-perl.alioth.debian.org/howto/quilt.html

(In reply to Robin Sheat from comment #7)
> I've added some patches using quilt that fix the encoding issues. For an
> intro to using quilt, go here:
> 
> http://pkg-perl.alioth.debian.org/howto/quilt.html

Thanks Robin. Can you point me to some reference docs on how to build the package from source (for testing purposees)? I'm only familiar with dh-make-perl and always used --cpan.

(In reply to Tomás Cohen Arazi from comment #8)
> Thanks Robin. Can you point me to some reference docs on how to build the
> package from source (for testing purposees)? I'm only familiar with
> dh-make-perl and always used --cpan.

I wrote a getting-started wiki page:

http://wiki.koha-community.org/wiki/Building_Debian_Dependencies

I have provided a counter patch, which I think is better. :)

(In reply to M. Tompsett from comment #11)
> I have provided a counter patch, which I think is better. :)

+1

(In reply to M. Tompsett from comment #11)
> I have provided a counter patch, which I think is better. :)

+1

Do you think that UT t/db_dependent/Borrower_Discharge.t will not be able to validate this patch ?

(In reply to Fridolin SOMERS from comment #14)
> Do you think that UT t/db_dependent/Borrower_Discharge.t will not be able to
> validate this patch ?

cnighswonger@cnighswonger-dt:~/Repositories/koha.dev (bugs/bug_14103)$ prove t/db_dependent/Borrower_Discharge.t
t/db_dependent/Borrower_Discharge.t .. SELECT COUNT( * ) FROM borrowers me WHERE ( userid = ? ): 'myfirstname.mysurname'
SELECT me.categorycode, me.description, me.enrolmentperiod, me.enrolmentperioddate, me.upperagelimit, me.dateofbirthrequired, me.finetype, me.bulk, me.enrolmentfee, me.overduenoticerequired, me.issuelimit, me.reservefee, me.hidelostitems, me.category_type, me.BlockExpiredPatronOpacActions, me.default_privacy FROM categories me WHERE ( me.categorycode = ? ): 'S'
INSERT INTO issues ( auto_renew, borrowernumber, branchcode, date_due, issuedate, itemnumber, onsite_checkout) VALUES ( ?, ?, ?, ?, ?, ?, ? ): '0', '51', 'CPL', '2015-05-27 23:59:00', '2015-05-06 06:35:37', '1', '0'
t/db_dependent/Borrower_Discharge.t .. 1/? SELECT me.borrower, me.needed, me.validated FROM discharges me WHERE ( ( me.needed IS NOT NULL AND me.validated IS NULL ) ):
t/db_dependent/Borrower_Discharge.t .. ok
All tests successful.
Files=1, Tests=5, 13 wallclock secs ( 0.02 usr  0.02 sys +  1.74 cusr  0.22 csys =  2.00 CPU)
Result: PASS

(In reply to Chris Nighswonger from comment #15)
> (In reply to Fridolin SOMERS from comment #14)
> > Do you think that UT t/db_dependent/Borrower_Discharge.t will not be able to
> > validate this patch ?
> 
> cnighswonger@cnighswonger-dt:~/Repositories/koha.dev (bugs/bug_14103)$ prove
> t/db_dependent/Borrower_Discharge.t

Opps. I just realized that this test does not appear to test the sub in question. I'll be out today, but will look at adding a test for it if no one gets to it before I get back.

FTR, I still support this second fix over the first. Keeping required deps to a minimum should be a priority for us.

(In reply to Chris Nighswonger from comment #17)

> FTR, I still support this second fix over the first. Keeping required deps
> to a minimum should be a priority for us.

I totally agree with you, please when send the another patch for unit test do obsolete the first one.

Comment on attachment 38837 [details] [review]
Bug 14103 - Discharge management: Software error on main page after upgrade

I am obsoleting the first one.
Please follow the test plans in comment #16 and comment #19. :)
Enjoy.

(In reply to Chris Nighswonger from comment #1)
> The upgrade mechanism really needs to run the same dep check which is run on
> a clean install... every time.
> 
> And error trapping has never been our strong point, but really should become
> so.

I so agree with this.

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

Created attachment 38955 [details] [review]
[PASSED QA] Bug 14103: PDF::FromHTML is conditionally required on UseDischarge systempreference

Since this is only in generate_as_pdf of Koha::Borrowers::Discharge, it made
sense to change it from a use to a require (avoiding a compile time error),
and placing it inside the eval where the PDF is generated, thus capturing the
problem in the koha error log.

TEST PLAN
---------
1) Log in to staff client
2) Koha Administration -> Global system preferences -> Local Use
3) Lower the Version number by 1, but DO NOT CLICK SAVE.
4) At the command line, sudo apt-get purge libpdf-fromhtml-perl
5) Back in the staff client, click save.
6) Every looks great until you attempt to log in. KABOOM!
7) Apply patch
8) Log in to staff client
   -- YAY!
9) Koha Administration -> Global system preferences -> Patrons
10) Make use the useDischarge is set to Allow
11) Find a patron with nothing checked out.
12) There is now a Discharge tab on the left at the bottom.
13) Click the 'Generate Discharge' button.
    -- nice error message
    -- check koha error log file for the ugly @INC missing message
14) At the command line, reinstall PDF::FromHTML
    (I built my own with cpan2deb: sudo dpkg -i /path/to/libpdf-fromhtml-perl_...)
15) In the staff client, click the 'Generate Discharge' button again.
    -- PDF is created as expected. All is well.
16) run koha qa test tools.

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

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

Created attachment 38956 [details] [review]
[PASSED QA] Bug 14103: Improved testing coverage

As per Chris' comment #17, this improves the testing coverage.

Same test plan as comment #16, but add:

TEST PLAN SUPPLEMENT
--------------------
1) $ prove -v t/db_dependent/Borrower_Discharge.t
   -- notice only 5 tests, and generate_as_pdf not tested.
2) Apply this patch.
3) Retest as per comment #16
   -- this will test the error and no error cases
      tweaked in Koha/Borrower/Discharge.pm
4) remove PDF::HTML
   $ sudo apt-get purge libpdf-fromhtml-perl
5) $ prove -v t/db_dependent/Borrower_Discharge.t
   -- 7 tests, all successful
6) add PDF::HTML
   $ sudo dpkg -i /path/to/libpdf-fromhtml-perl...
   -- depends on how you get it. vary accordingly.
7) $ prove -v t/db_dependent/Borrower_Discharge.t
   -- 7 tests, all successful
   -- Note that is was a different 7th test. :)
8) koha qa test tools

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

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

Patches pushed to master.

Thanks Mark!