Hi Nicole, is it possible to add the current date in the discharge letter? I don't think so. That's why my report. Nicole, did you notice that the pdf generation uses the NoticeCSS pref to style it? I did not - good to know - but I'm not sure that everything can be done with CSS - can it? Nicole It depends on what you want to do - did you try to add the logo? You could either try adding it in the HTML of the notice or with CSS. Katrin, are you sure CSS is used ? I see the code uses PDF::FromHTML to produce the pd file or the documentation says "it is not designed to utilise CSS." (http://search.cpan.org/~audreyt/PDF-FromHTML-0.31/lib/PDF/FromHTML.pm) Not totally sure - we haven't played much with the discharge notices so far. We have added a few line breaks so far, but no more formatting I think. (In reply to Sophie MEYNIEUX from comment #6) > I see the code uses PDF::FromHTML to produce the pd file or the > documentation says "it is not designed to utilise CSS." My tests confirm this. Neither linked or inline CSS (using style attributes) affects the output. As far as I can tell the inclusion of the NoticeCSS stylesheet does nothing. As said on comment 6, PDF::FromHTML doesn't handle CSS. I was able to convert an HTML to a PDF on Perl using PDF::WebKit which in turn uses wkhtmltopdf: https://stackoverflow.com/a/62443025/4970442 I used the repo version (old), which need an X Server installed, but the latest versions are headless (does not require X server). (In reply to Claudio from comment #1) > Hi Nicole, is it possible to add the current date in the discharge letter? I might be a little late, but you can use <<today>> to include current date. I'll give this a try :-D (In reply to Pablo AB from comment #9) > As said on comment 6, PDF::FromHTML doesn't handle CSS. > > I was able to convert an HTML to a PDF on Perl using PDF::WebKit which in > turn uses wkhtmltopdf: https://stackoverflow.com/a/62443025/4970442 > > I used the repo version (old), which need an X Server installed, but the > latest versions are headless (does not require X server). It still requires too many things related to X, and systemd. Will go with something more lightweight. Created attachment 144863 [details]
How it looks with this patches :-D
Created attachment 144864 [details] [review] Bug 14251: Allow use of CSS in discharge letter The discharge feature relies PDF::FromHTML which explicitly doesn't handle CSS [1]. This patch makes it use the `weasyprint` command-line tool to generate the PDF. This tool handles CSS correctly. To test: 1. Install weasyprint: $ apt install weasyprint 2. Add some style to your DISCHARGE letter. I used: <<today>> <h1>Discharge confirmation</h1> <p style="background-color: yellow;"><<branches.branchname>> certifies that the following borrower:<br> <<borrowers.firstname>> <<borrowers.surname>> (cardnumber: <<borrowers.cardnumber>>)<br> has returned all items.</p> 3. Have some non-latin chars on the patron name. I picked 'Henry Acevedo' and added 'Δοκιμή' as picked from bug 23589. Only to check no regressions. 4. Enable the 'UseDischarge' syspref 5. Go to Henry's detail page 6. Choose More > Discharge and Generate the discharge => SUCCESS: - Style is applied to the PDF - Greek characters are displayed correctly 7. Run the tests: $ kshell k$ prove -v t/db_dependent/Patron/Borrower_Discharge.t => SUCCESS: The rewritten tests pass! 8. Remove weasyprint: $ apt remove weasyprint 9. Repeat 7 => SUCCESS: Tests pass, relevant test is skipped because of missing weasyprint 10. Sign off :-D [1] https://metacpan.org/pod/PDF::FromHTML#CAVEATS Created attachment 144865 [details] [review] Bug 14251: Add warning in about.pl for missing weasyprint To test: 1. Have weasyprint installed 2. Go to the About > System information page => SUCCESS: No warning about weasyprint 3. Uninstall weasyprint 4. Repeat 2 => SUCCESS: A warning is displayed and makes sense 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 144866 [details] [review] Bug 14251: Add weasyprint dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I can't reproduce the step. Hi Solene, I just checked and the patches apply to master cleanly. Do you have another question about this one? I have the same message in system information ("No warnings") when I install and uninstall weasyprint. Ps: the patch has been applied before. Oh, didn't notice this was failed because of the about page! Created attachment 146168 [details] [review] Bug 14251: Allow use of CSS in discharge letter The discharge feature relies PDF::FromHTML which explicitly doesn't handle CSS [1]. This patch makes it use the `weasyprint` command-line tool to generate the PDF. This tool handles CSS correctly. To test: 1. Install weasyprint: $ apt install weasyprint 2. Add some style to your DISCHARGE letter. I used: <<today>> <h1>Discharge confirmation</h1> <p style="background-color: yellow;"><<branches.branchname>> certifies that the following borrower:<br> <<borrowers.firstname>> <<borrowers.surname>> (cardnumber: <<borrowers.cardnumber>>)<br> has returned all items.</p> 3. Have some non-latin chars on the patron name. I picked 'Henry Acevedo' and added 'Δοκιμή' as picked from bug 23589. Only to check no regressions. 4. Enable the 'UseDischarge' syspref 5. Go to Henry's detail page 6. Choose More > Discharge and Generate the discharge => SUCCESS: - Style is applied to the PDF - Greek characters are displayed correctly 7. Run the tests: $ kshell k$ prove -v t/db_dependent/Patron/Borrower_Discharge.t => SUCCESS: The rewritten tests pass! 8. Remove weasyprint: $ apt remove weasyprint 9. Repeat 7 => SUCCESS: Tests pass, relevant test is skipped because of missing weasyprint 10. Sign off :-D [1] https://metacpan.org/pod/PDF::FromHTML#CAVEATS Created attachment 146169 [details] [review] Bug 14251: Add warning in about.pl for missing weasyprint To test: 1. Have weasyprint installed 2. Go to the About > System information page => SUCCESS: No warning about weasyprint 3. Uninstall weasyprint 4. Repeat 2 => SUCCESS: A warning is displayed and makes sense 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 146170 [details] [review] Bug 14251: Add weasyprint dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Two things from testing: 1. The CSS changes are only reflected in the PDF if they are made in the Email section of the notice. If you add them to the Print section, then the inline CSS is ignored. 2. The tests fail for me after removing weasyprint (step 9 of the test plan): prove -v t/db_dependent/Patron/Borrower_Discharge.t t/db_dependent/Patron/Borrower_Discharge.t .. 1..23 ok 1 - A patron with issues cannot be discharged ok 2 - No request done if patron has issues ok 3 - No discharge done if patron has issues ok 4 - There is no pending discharge request ok 5 - There is no validated discharge ok 6 - A patron without issues can be discharged ok 7 - Confirm failure when lacking borrower number ok 8 - The patron has been discharged ok 9 - The patron has been debarred after discharge ok 10 - There are 3 validated discharges ok 11 - There is 1 validated discharge for a given patron ok 12 - There is 2 validated discharges for a given branchcode ok 13 - The debarment has been lifted ok 14 - The patron is not discharged after the restriction has been lifted ok 15 - There is a pending discharge request (second time) ok 16 - There is no pending discharge request (second time) ok 17 # skip Skipping because weasyprint is not installed not ok 18 - Failed call to run() prints the generated error ok 19 - undef returned if failed run # Failed test 'Failed call to run() prints the generated error' # at t/db_dependent/Patron/Borrower_Discharge.t line 141. # found warning: weasyprint not found! at /kohadevbox/koha/Koha/Patron/Discharge.pm line 151. # expected to find warning: Some error ok 20 - Expected failure because of missing weasyprint ok 21 - undef returned if missing weasyprint ok 22 - Discharge request sent # Subtest: search_limited 1..4 ok 1 - With permission, all discharges are visible ok 2 - With permission, all discharges are visible ok 3 - Without permission, only discharge from our group are visible ok 4 - Without permission, only discharge from our group are visible ok 23 - search_limited # Looks like you failed 1 test of 23. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/23 subtests (less 1 skipped subtest: 21 okay) Test Summary Report ------------------- t/db_dependent/Patron/Borrower_Discharge.t (Wstat: 256 Tests: 23 Failed: 1) Failed test: 18 Non-zero exit status: 1 Files=1, Tests=23, 2 wallclock secs ( 0.02 usr 0.00 sys + 1.55 cusr 0.28 csys = 1.85 CPU) Result: FAIL Created attachment 146295 [details] [review] Bug 14251: Allow use of CSS in discharge letter The discharge feature relies PDF::FromHTML which explicitly doesn't handle CSS [1]. This patch makes it use the `weasyprint` command-line tool to generate the PDF. This tool handles CSS correctly. To test: 1. Install weasyprint: $ apt install weasyprint 2. Add some style to your DISCHARGE letter. I used: <<today>> <h1>Discharge confirmation</h1> <p style="background-color: yellow;"><<branches.branchname>> certifies that the following borrower:<br> <<borrowers.firstname>> <<borrowers.surname>> (cardnumber: <<borrowers.cardnumber>>)<br> has returned all items.</p> 3. Have some non-latin chars on the patron name. I picked 'Henry Acevedo' and added 'Δοκιμή' as picked from bug 23589. Only to check no regressions. 4. Enable the 'UseDischarge' syspref 5. Go to Henry's detail page 6. Choose More > Discharge and Generate the discharge => SUCCESS: - Style is applied to the PDF - Greek characters are displayed correctly 7. Run the tests: $ kshell k$ prove -v t/db_dependent/Patron/Borrower_Discharge.t => SUCCESS: The rewritten tests pass! 8. Remove weasyprint: $ apt remove weasyprint 9. Repeat 7 => SUCCESS: Tests pass, relevant test is skipped because of missing weasyprint 10. Sign off :-D [1] https://metacpan.org/pod/PDF::FromHTML#CAVEATS Created attachment 146296 [details] [review] Bug 14251: Add warning in about.pl for missing weasyprint To test: 1. Have weasyprint installed 2. Go to the About > System information page => SUCCESS: No warning about weasyprint 3. Uninstall weasyprint 4. Repeat 2 => SUCCESS: A warning is displayed and makes sense 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 146297 [details] [review] Bug 14251: Add weasyprint dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 146310 [details] [review] Bug 14251: Allow use of CSS in discharge letter The discharge feature relies PDF::FromHTML which explicitly doesn't handle CSS [1]. This patch makes it use the `weasyprint` command-line tool to generate the PDF. This tool handles CSS correctly. To test: 1. Install weasyprint: $ apt install weasyprint 2. Add some style to your DISCHARGE letter. I used: <<today>> <h1>Discharge confirmation</h1> <p style="background-color: yellow;"><<branches.branchname>> certifies that the following borrower:<br> <<borrowers.firstname>> <<borrowers.surname>> (cardnumber: <<borrowers.cardnumber>>)<br> has returned all items.</p> 3. Have some non-latin chars on the patron name. I picked 'Henry Acevedo' and added 'Δοκιμή' as picked from bug 23589. Only to check no regressions. 4. Enable the 'UseDischarge' syspref 5. Go to Henry's detail page 6. Choose More > Discharge and Generate the discharge => SUCCESS: - Style is applied to the PDF - Greek characters are displayed correctly 7. Run the tests: $ kshell k$ prove -v t/db_dependent/Patron/Borrower_Discharge.t => SUCCESS: The rewritten tests pass! 8. Remove weasyprint: $ apt remove weasyprint 9. Repeat 7 => SUCCESS: Tests pass, relevant test is skipped because of missing weasyprint 10. Sign off :-D [1] https://metacpan.org/pod/PDF::FromHTML#CAVEATS Signed-off-by: David Nind <david@davidnind.com> Created attachment 146311 [details] [review] Bug 14251: Add warning in about.pl for missing weasyprint To test: 1. Have weasyprint installed 2. Go to the About > System information page => SUCCESS: No warning about weasyprint 3. Uninstall weasyprint 4. Repeat 2 => SUCCESS: A warning is displayed and makes sense 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 146312 [details] [review] Bug 14251: Add weasyprint dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 147731 [details] [review] Bug 14251: Allow use of CSS in discharge letter The discharge feature relies PDF::FromHTML which explicitly doesn't handle CSS [1]. This patch makes it use the `weasyprint` command-line tool to generate the PDF. This tool handles CSS correctly. To test: 1. Install weasyprint: $ apt install weasyprint 2. Add some style to your DISCHARGE letter. I used: <<today>> <h1>Discharge confirmation</h1> <p style="background-color: yellow;"><<branches.branchname>> certifies that the following borrower:<br> <<borrowers.firstname>> <<borrowers.surname>> (cardnumber: <<borrowers.cardnumber>>)<br> has returned all items.</p> 3. Have some non-latin chars on the patron name. I picked 'Henry Acevedo' and added 'Δοκιμή' as picked from bug 23589. Only to check no regressions. 4. Enable the 'UseDischarge' syspref 5. Go to Henry's detail page 6. Choose More > Discharge and Generate the discharge => SUCCESS: - Style is applied to the PDF - Greek characters are displayed correctly 7. Run the tests: $ kshell k$ prove -v t/db_dependent/Patron/Borrower_Discharge.t => SUCCESS: The rewritten tests pass! 8. Remove weasyprint: $ apt remove weasyprint 9. Repeat 7 => SUCCESS: Tests pass, relevant test is skipped because of missing weasyprint 10. Sign off :-D [1] https://metacpan.org/pod/PDF::FromHTML#CAVEATS Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 147732 [details] [review] Bug 14251: Add warning in about.pl for missing weasyprint To test: 1. Have weasyprint installed 2. Go to the About > System information page => SUCCESS: No warning about weasyprint 3. Uninstall weasyprint 4. Repeat 2 => SUCCESS: A warning is displayed and makes sense 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 147733 [details] [review] Bug 14251: Add weasyprint dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Just an FYI, weasyprint is not a very performant application. I recently surveyed most available foss html to pdf options out there. I ended up using wkhtmltopdf to generate 5000 page pdfs in under a minute each. That being said, we are only generating a page or two with weasyprint so that shouldn't matter, and it appears that weasyprint is more actively maintained. Another option would be to use headless chrome (https://developer.chrome.com/blog/headless-chrome/) but I see no compelling reason to use that over weasyprint at this time. (In reply to Kyle M Hall from comment #34) > Just an FYI, weasyprint is not a very performant application. I recently > surveyed most available foss html to pdf options out there. I ended up using > wkhtmltopdf to generate 5000 page pdfs in under a minute each. I don't remember exactly right now, but I recall wkhtmltopdf could use weasyprint as backend, or latex. And the latter was like 800MB in dependencies. I agree we should move forward with this, and even centralize PDF generation in a class and make the v backend swappable until we find the perfect tool. I didn't know about chrome/chromium headless. > I don't remember exactly right now, but I recall wkhtmltopdf could use > weasyprint as backend, or latex. And the latter was like 800MB in > dependencies. Sounds like you are thinking of Pandoc (https://pandoc.org/). Pushed to master for 23.05. Nice work everyone, thanks! Enhancement - not backporting to 22.11.x Nice work everyone! Seems like back end stuff. I still reviewed the discharge sections in the manual to at least update the screenshots... Reviewing about.pl and noticed the missing weasyprint on an upgraded system. Like often happens, it looks like it's in debian/control.in but not debian/control. We need to get better at managing packaged dependencies... Also, weasyprint installs an additional 57 packaged dependencies totally up 504MB. Sometimes, we quibble over adding 1 Perl dependency, but then this huge one gets added with little discussion. Might've been better to make this optional. Keeping the original code and then optionally using weasyprint if you wanted better HTML to PDF support. Hmm... too late now I guess. Also probably would've been better to create a Koha::HtmlToPDF module or something like that, so that it could be re-used in other parts of Koha. For instance, I imagine we could add this into the label creator at some stage. (In reply to David Cook from comment #40) > Reviewing about.pl and noticed the missing weasyprint on an upgraded system. > Like often happens, it looks like it's in debian/control.in but not > debian/control. We need to get better at managing packaged dependencies... I just added it the other week - did you check main? But I've also just been in an extra cranky mood today and commenting at 11:47pm doesn't help [U+1F605]. I'll try to add some more positive comments... I used wkhtmltopdf for years as a CLI tool, but I've found in recent years Webkit doesn't render many webpages very well anymore. Complex HTML/CSS/Javascript that looks good in Chrome often wouldn't look good in the Webkit used by wkhtmltopdf. Still... a pretty good tool overall. Headless Chrome is great, since it's the closest to the Chrome user experience, but... as a CLI tool it's a bit limited in terms of options. My overall preference these days for HTML to PDF is either Puppeteer or Playwright. While there are bindings out there (not for Perl though I think), I use Node.js to control them, and I have them use Chrome under the head (stored in their node_modules directory). I've found these tools to produce the most accurate outputs. However, Puppeteer, Playwright, and Chrome are probably all too heavyweight for Koha. While I moaned about weasyprint having 500MB of dependencies, they're mostly Python libraries and low level libraries. And they're all packaged up. As Kyle said... if weasyprint works fine then I wouldn't rock the boat. (In reply to Katrin Fischer from comment #43) > (In reply to David Cook from comment #40) > > Reviewing about.pl and noticed the missing weasyprint on an upgraded system. > > Like often happens, it looks like it's in debian/control.in but not > > debian/control. We need to get better at managing packaged dependencies... > > I just added it the other week - did you check main? Ah cheers. I thought that I had an up-to-date main, but it appears that I didn't! My bad.
> > I just added it the other week - did you check main?
>
> Ah cheers. I thought that I had an up-to-date main, but it appears that I
> didn't! My bad.
It was a very recent change. I remember I wondered about that additional one showing up in list to the ones I had expected.
|
Created attachment 39365 [details] contents of letter Would be nice if the discharge letter was a bit more styled - maybe a library logo to make it look like letterhead? Or at least a few more line breaks.