Bug 14103 - PDF::FromHTML headaches related to optional discharging
Summary: PDF::FromHTML headaches related to optional discharging
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Héctor Eduardo Castro Avalos
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 14165 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-04-30 21:06 UTC by Marc Véron
Modified: 2015-12-03 22:11 UTC (History)
10 users (show)

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


Attachments
Bug 14103 - Discharge management: Software error on main page after upgrade (1.56 KB, patch)
2015-05-05 03:59 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 14103 - PDF::FromHTML is conditionally required on UseDischarge systempreference (2.27 KB, patch)
2015-05-06 05:25 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14103: PDF::FromHTML is conditionally required on UseDischarge systempreference (2.34 KB, patch)
2015-05-06 10:38 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 14103 - Improved testing coverage (3.70 KB, patch)
2015-05-06 18:29 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14103: Improved testing coverage (3.77 KB, patch)
2015-05-07 12:04 UTC, Chris Nighswonger
Details | Diff | Splinter Review
[PASSED QA] Bug 14103: PDF::FromHTML is conditionally required on UseDischarge systempreference (2.41 KB, patch)
2015-05-07 16:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14103: Improved testing coverage (3.83 KB, patch)
2015-05-07 16:03 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-04-30 21:06:00 UTC
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.
Comment 1 Chris Nighswonger 2015-05-04 19:54:47 UTC
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.
Comment 2 Héctor Eduardo Castro Avalos 2015-05-04 21:47:26 UTC
(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
Comment 3 Héctor Eduardo Castro Avalos 2015-05-05 03:07:30 UTC
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.
Comment 4 Héctor Eduardo Castro Avalos 2015-05-05 03:59:59 UTC Comment hidden (obsolete)
Comment 5 Héctor Eduardo Castro Avalos 2015-05-05 04:02:00 UTC
I add Katrin and Tomás, since this represent a critical bug for 3.20 release
Comment 6 Robin Sheat 2015-05-05 05:07:11 UTC
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 :)
Comment 7 Robin Sheat 2015-05-05 05:45:05 UTC
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
Comment 8 Tomás Cohen Arazi 2015-05-05 18:51:02 UTC
(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.
Comment 9 Robin Sheat 2015-05-05 23:08:05 UTC
(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
Comment 10 M. Tompsett 2015-05-06 05:25:14 UTC Comment hidden (obsolete)
Comment 11 M. Tompsett 2015-05-06 05:26:42 UTC
I have provided a counter patch, which I think is better. :)
Comment 12 Chris Nighswonger 2015-05-06 09:48:20 UTC
(In reply to M. Tompsett from comment #11)
> I have provided a counter patch, which I think is better. :)

+1
Comment 13 Fridolin SOMERS 2015-05-06 10:21:20 UTC
(In reply to M. Tompsett from comment #11)
> I have provided a counter patch, which I think is better. :)

+1
Comment 14 Fridolin SOMERS 2015-05-06 10:23:36 UTC
Do you think that UT t/db_dependent/Borrower_Discharge.t will not be able to validate this patch ?
Comment 15 Chris Nighswonger 2015-05-06 10:36:04 UTC
(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
Comment 16 Chris Nighswonger 2015-05-06 10:38:13 UTC Comment hidden (obsolete)
Comment 17 Chris Nighswonger 2015-05-06 10:43:59 UTC
(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.
Comment 18 Héctor Eduardo Castro Avalos 2015-05-06 17:55:13 UTC
(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 19 M. Tompsett 2015-05-06 18:29:00 UTC Comment hidden (obsolete)
Comment 20 M. Tompsett 2015-05-06 18:30:24 UTC
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.
Comment 21 David Cook 2015-05-07 00:59:25 UTC
(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.
Comment 22 Chris Nighswonger 2015-05-07 11:47:40 UTC
*** Bug 14165 has been marked as a duplicate of this bug. ***
Comment 23 Chris Nighswonger 2015-05-07 12:04:33 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2015-05-07 16:02:51 UTC
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>
Comment 25 Kyle M Hall 2015-05-07 16:03:00 UTC
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>
Comment 26 Tomás Cohen Arazi 2015-05-07 17:23:52 UTC
Patches pushed to master.

Thanks Mark!