Bug 7138 - Can't print basket group order pdf.
Summary: Can't print basket group order pdf.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.6
Hardware: All All
: P2 blocker (vote)
Target Milestone: Oldversions
Assignee: Katrin Fischer
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 6369
  Show dependency treegraph
 
Reported: 2011-11-01 21:33 UTC by Frédérick Capovilla
Modified: 2018-12-16 19:32 UTC (History)
4 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:
Version(s) released in:


Attachments
Bug 7138: Can't print basket group order pdf (1.21 KB, patch)
2011-11-02 23:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7138: Can't print basket group order pdf (1.27 KB, patch)
2011-11-03 02:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Signed patch (2.64 KB, patch)
2011-11-03 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Signed patch (1.56 KB, patch)
2011-11-03 09:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédérick Capovilla 2011-11-01 21:33:06 UTC
When trying to print a basket group as a pdf file, an invalid pdf file is returned.

The returned file contains this error :
Undefined subroutine &main::printpdf called at /home/koha/kohagit/acqui/basketgroup.pl line 275.

The problem seems to happen because pdfformat::layout3pages doesn't load correctly on line 190 of basketgroup.pl. It's in an eval and the warnings are ignored.

A possible way to fix the problem would be to put require and import inside of evals :

eval "require $pdfformat";
eval "import $pdfformat";
Comment 1 Katrin Fischer 2011-11-02 22:14:12 UTC
I can confirm this bug, updating severity!
Comment 2 Katrin Fischer 2011-11-02 23:54:07 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2011-11-03 02:31:50 UTC
Created attachment 6154 [details] [review]
Bug 7138: Can't print basket group order pdf

PDF print of basket groups is broken.

To test:

1) Make sure OrderPdfFormat is set to pdfformat::layout2pages or pdfformat::layout3pages
2) Create a basket with orders
3) Close the basket and create a basket group checking the checkbox
4) Print the basket group as PDF

Before patch the file is broken and when opened in an editor contains an error message.
After the patch the PDF should be generated correctly again.

Thx to Chris for helping me to fix the problem.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 4 Marcel de Rooy 2011-11-03 09:02:08 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2011-11-03 09:03:34 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2011-11-03 09:06:55 UTC
QA: Combined require and import into use. Added a warn statement.

Solves the actual problem of printing PDF. But we still have several warnings in the logfile. Such as:

[Thu Nov 03 09:52:28 2011] [error] [client 192.87.126.61] [Thu Nov  3 09:52:28 2011] basketgroup.pl: Use of uninitialized value in multiplication (*) at /usr/share/koha/testclone/acqui/basketgroup.pl line 246., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=5

[Thu Nov 03 09:52:28 2011] [error] [client 192.87.126.61] [Thu Nov  3 09:52:28 2011] basketgroup.pl: Use of uninitialized value in join or string at pdfformat/layout2pages.pm line 192., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=5

[Thu Nov 03 09:52:28 2011] [error] [client 192.87.126.61] [Thu Nov  3 09:52:28 2011] basketgroup.pl: Use of uninitialized value in join or string at pdfformat/layout2pages.pm line 230., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=5

[Thu Nov 03 09:52:28 2011] [error] [client 192.87.126.61] [Thu Nov  3 09:52:28 2011] basketgroup.pl: Use of uninitialized value in concatenation (.) or string at pdfformat/layout2pages.pm line 94., referer: http://libdevelop.rijksmuseum.nl:8009/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=5
Comment 7 Chris Cormack 2011-11-03 09:24:33 UTC
I don't think we should edit someones patch, and leave their name as the committer. If a follow up is needed, do that, or a counter patch. But editing someones work and leaving it under their name isn't right.

This will fail perl::critic tests again now also
not ok 5 - Test::Perl::Critic for "acqui/basketgroup.pl"
#   Failed test 'Test::Perl::Critic for "acqui/basketgroup.pl"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 110.
#
# Perl::Critic found these violations in "acqui/basketgroup.pl":
# Expression form of "eval" at line 189, column 9.  See page 161 of PBP.  (Severity: 5)
Comment 8 Marcel de Rooy 2011-11-03 09:26:04 UTC
OK. Setting status back.
Comment 9 Marcel de Rooy 2011-11-03 09:32:25 UTC
I restored Chris patch for this report and made my addition obsolete.

I just have two points at the code:
1) The construction with an eval require within an eval, followed by the import does not really look great. Apparently, it does not conflict with perl critic.
2) An empty if($@) {} after these lines could be corrected by this patch too.
Comment 10 Chris Cormack 2011-11-03 09:36:34 UTC
Heh, it's not my patch, its Katrin's I just signed off on it :)

And by changing it, i'd now signed off on yours without even seeing it .. which is pretty tricky of me :)

Yep a follow up with a warn wouldn't be a bad thing.
Comment 11 Marcel de Rooy 2011-11-03 09:59:27 UTC
(In reply to comment #10)
> Heh, it's not my patch, its Katrin's I just signed off on it :)
True of course.
> And by changing it, i'd now signed off on yours without even seeing it .. which
> is pretty tricky of me :)
Clear. I would only change a patch this way if it is quite trivial. Which seemed to me the case here. But no problem.

Just checking the perl critic thing: Where I wrote: "Apparently, it does not conflict with perl critic.", I now see that the original patch just violates the same rule of stringy eval.
So, maybe it should be solved differently..
Comment 12 Paul Poulain 2011-11-09 16:17:57 UTC
This one is not MAJ, it's BLO : the pdf can't be printed anymore and there is no way to do it another way.

* upgrading severity
* tested the patch, the problem is now fixed
* passed perlcritic
* QA comment: none, it's a 2 line patch. I just don't understand why we need eval{ eval ... }} as Marcel noticed in comment 
* I apply the patch, but a follow-up would be usefull to fix the warnings in the logs (but it's not related to this bug

So, patch pushed, please test