Description
Ivan Dziuba
2021-01-21 14:48:27 UTC
Created attachment 115696 [details] [review] Bug 27510: Download the report in HTML format TEST PLAN: 1. Apply this patch 2. Update database ./updatedatabase.pl Now we have new preference 'ReportUserCSS' where we can add style for our HTML report. 3. Go on the page - <intranet>/reports/guided_reports.pl 4. Download -> Document HTML I don't see any changes to the template in this patch. Shouldn't there be a new menu item under the "Download" button? Created attachment 115702 [details] [review] Bug 27510: Download the report in HTML format Bug 27510: Download the report in HTML format. Added to Download list this option. (In reply to Owen Leonard from comment #3) > I don't see any changes to the template in this patch. Shouldn't there be a > new menu item under the "Download" button? Added! Thanks. Really like the idea of this. I've applied the patch on a sandbox and restarted services, but I get an error. Steps 1-2 are fine, and ReportUserCSS shows in the system preferences. When attempting step 3, the failure below shows on all of the guided_reports.pl pages: Could not compile /kohadevbox/koha/reports/guided_reports.pl: Can't locate HTML/Table.pm in @INC (you may need to install the HTML::Table module) (@INC contains: /kohadevbox/koha /kohadevbox/koha/installer /kohadevbox/koha/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/friday/plugins) at /kohadevbox/koha/reports/guided_reports.pl line 42. BEGIN failed--compilation aborted at /kohadevbox/koha/reports/guided_reports.pl line 42. at /usr/share/perl5/CGI/Compile.pm line 132 If it's a patch that can't be applied to a sandbox, revert the status and I'll let someone with a devbox test. :) (In reply to Sally from comment #6) > Really like the idea of this. I've applied the patch on a sandbox and > restarted services, but I get an error. > > Steps 1-2 are fine, and ReportUserCSS shows in the system preferences. > > When attempting step 3, the failure below shows on all of the > guided_reports.pl pages: > > Could not compile /kohadevbox/koha/reports/guided_reports.pl: Can't locate > HTML/Table.pm in @INC (you may need to install the HTML::Table module) (@INC > contains: /kohadevbox/koha /kohadevbox/koha/installer > /kohadevbox/koha/lib/installer /etc/perl > /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 > /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 > /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 > /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base > /var/lib/koha/friday/plugins) at /kohadevbox/koha/reports/guided_reports.pl > line 42. > BEGIN failed--compilation aborted at > /kohadevbox/koha/reports/guided_reports.pl line 42. > at /usr/share/perl5/CGI/Compile.pm line 132 > > If it's a patch that can't be applied to a sandbox, revert the status and > I'll let someone with a devbox test. :) You need to install the HTML::Table module. Thanks! I think this looks good. I think the label for the menu item should say "HTML file" instead of "Document HTML." My only other concern is that the generated file isn't a complete HTML document... No <html>, no <body>, etc. Does that matter? The file opens fine in a browser. (In reply to Owen Leonard from comment #8) > I think this looks good. I think the label for the menu item should say > "HTML file" instead of "Document HTML." > > My only other concern is that the generated file isn't a complete HTML > document... No <html>, no <body>, etc. > > Does that matter? The file opens fine in a browser. If that matter I can add all tags. Thanks! I think it would be good if the file validated :) Created attachment 116079 [details] [review] Bug 27510: Download the report in HTML format Bug 27510: Download the report in HTML format. Added to Download list this option. Bug 27510: Download the report in HTML format. The file is validated. Created attachment 116091 [details] [review] Bug 27510: Download the report in HTML format Bug 27510: Download the report in HTML format. Fix bugs with utf-8. Created attachment 116849 [details] Bug 27510: Download the report in HTML format. Created attachment 116850 [details] [review] Bug 27510: Download the report in HTML format Bug 27510: Download the report in HTML format. Fix bugs with utf-8. Bug 27510: Download the report in HTML format. Added [% PROCESS params %] in URL You need install the module HTML::Table (In reply to Ivan Dziuba from comment #15) > You need install the module HTML::Table Hi Ivan, I've added he dependency keyword - have you checked that this is available as package on Debian? Please also add a test plan to the commit message. I still quite like the idea of this. I think it would be helpful for staff users and others that have no access to the Reports module, but could profit from the links in an HTML export file. It looks like HTML::Table is packaged, at least in Debian 9.13: $ sudo apt-cache policy libhtml-table-perl libhtml-table-perl: Installert: (ingen) Kandidat: 2.08a-3 Versjonstabell: 2.08a-3 500 500 http://mirrors.linode.com/debian stretch/main amd64 Packages So what is needed here? - A test plan in the commit message - To add libhtml-table-perl to Koha's dpendencies Should adding the dep be part of the patch, or is that something the RM adds? Created attachment 146135 [details] [review] Bug 27510: Download the report in HTML format TEST PLAN: 1. Apply this patch 2. Update database ./updatedatabase.pl Now we have new preference 'ReportUserCSS' where we can add style for our HTML report. 3. Go on the page - <intranet>/reports/guided_reports.pl 4. Download -> Document HTML Signed-off-by: Magnus Enger <magnus@libriotech.no> I have fixed some conflicts and put in the test plan that was on Bugzilla. Feature works as advertised. HTML file can be downloaded and is a full HTML document. Formatting can be done with the syspref. Links generated in SQL carry over to the document (but links need to be absolute to work, of course.) Created attachment 146142 [details] [review] Bug 27510: Followup - Change "Document HTML" to "HTML file" as requested by Owen - Add HTML::Table to cpanfile Not sure if my followup should be one or two patches? I am also taking a chance on leaving this at "signed off". Please reset to "needs signoff" if that is thought necessary. Shouldn't this just use a Template Toolkit based template to generate the output instead of HTML::Table and a bunch of html embedded in a perl script? Shouldn't this just use a Template Toolkit based template to generate the output instead of HTML::Table and a bunch of html embedded in a perl script? (In reply to Kyle M Hall from comment #24) > Shouldn't this just use a Template Toolkit based template to generate the > output instead of HTML::Table and a bunch of html embedded in a perl script? Yes, that does sound like a sensible idea. Ivan, could you find the time to redo the patch with a template for generating the table? Just a note that if HTML::Table is added to cpanfile, HTML::Table 2.08a will break the About page. (In reply to Kyle M Hall from comment #24) > Shouldn't this just use a Template Toolkit based template to generate the > output instead of HTML::Table and a bunch of html embedded in a perl script? Did I imagine you did this somewhere already Kyle? I'm struggling to find the bug if you did.. if so, we could close this as a duplicate (In reply to Martin Renvoize from comment #27) > (In reply to Kyle M Hall from comment #24) > > Shouldn't this just use a Template Toolkit based template to generate the > > output instead of HTML::Table and a bunch of html embedded in a perl script? > > Did I imagine you did this somewhere already Kyle? I'm struggling to find > the bug if you did.. if so, we could close this as a duplicate Bug 34136 could definitely be used as an alternative to this though this patchset is for guided reports and mine is for SQL reports. |