| Summary: | Can't print basket group order pdf. | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Frédérick Capovilla <fcapovilla> |
| Component: | Acquisitions | Assignee: | Katrin Fischer <katrin.fischer> |
| Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
| Severity: | blocker | ||
| Priority: | P2 | CC: | chris, m.de.rooy, paul.poulain |
| Version: | 3.6 | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 6369 | ||
| Attachments: |
Bug 7138: Can't print basket group order pdf
Bug 7138: Can't print basket group order pdf Signed patch Signed patch |
||
|
Description
Frédérick Capovilla
2011-11-01 21:33:06 UTC
I can confirm this bug, updating severity! Created attachment 6153 [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. 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> Created attachment 6161 [details] [review] Signed patch Created attachment 6162 [details] [review] Signed patch Oops, wrong file. 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 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) OK. Setting status back. 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.
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. (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.. 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
|