Bug 14251 - Allow use of CSS in discharge letter
Summary: Allow use of CSS in discharge letter
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords: dependency
Depends on:
Blocks: 32892
  Show dependency treegraph
 
Reported: 2015-05-21 16:48 UTC by Nicole C. Engard
Modified: 2023-12-28 20:42 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
With this patch, it is now possible to add CSS formatting to the discharge letter. CSS styling can be added directly in the DISCHARGE letter in the Notices and slips tool. The CSS stylesheet added in the NoticeCSS system preference will also be applied to the discharge PDF. Note that the discharge functionality now requires the weasyprint module.
Version(s) released in:
23.05.00


Attachments
contents of letter (17.21 KB, image/png)
2015-05-21 16:48 UTC, Nicole C. Engard
Details
How it looks with this patches :-D (73.78 KB, image/png)
2022-12-28 14:20 UTC, Tomás Cohen Arazi
Details
Bug 14251: Allow use of CSS in discharge letter (6.48 KB, patch)
2022-12-28 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Add warning in about.pl for missing weasyprint (4.01 KB, patch)
2022-12-28 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Add weasyprint dependency (954 bytes, patch)
2022-12-28 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Allow use of CSS in discharge letter (6.48 KB, patch)
2023-02-03 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Add warning in about.pl for missing weasyprint (6.08 KB, patch)
2023-02-03 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Add weasyprint dependency (954 bytes, patch)
2023-02-03 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Allow use of CSS in discharge letter (6.47 KB, patch)
2023-02-06 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Add warning in about.pl for missing weasyprint (6.08 KB, patch)
2023-02-06 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Add weasyprint dependency (954 bytes, patch)
2023-02-06 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14251: Allow use of CSS in discharge letter (6.52 KB, patch)
2023-02-07 09:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 14251: Add warning in about.pl for missing weasyprint (6.12 KB, patch)
2023-02-07 09:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 14251: Add weasyprint dependency (1000 bytes, patch)
2023-02-07 09:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 14251: Allow use of CSS in discharge letter (6.57 KB, patch)
2023-03-03 19:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14251: Add warning in about.pl for missing weasyprint (6.28 KB, patch)
2023-03-03 19:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14251: Add weasyprint dependency (1.03 KB, patch)
2023-03-03 19:55 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 Nicole C. Engard 2015-05-21 16:48:47 UTC
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.
Comment 1 Claudio Costales 2015-12-27 17:16:41 UTC
Hi Nicole, is it possible to add the current date in the discharge letter?
Comment 2 Nicole C. Engard 2016-01-25 21:32:36 UTC
I don't think so.  That's why my report.
Comment 3 Jonathan Druart 2016-03-16 09:25:50 UTC
Nicole, did you notice that the pdf generation uses the NoticeCSS pref to style it?
Comment 4 Nicole C. Engard 2016-05-12 15:52:17 UTC
I did not - good to know - but I'm not sure that everything can be done with CSS - can it?

Nicole
Comment 5 Katrin Fischer 2016-05-12 17:37:21 UTC
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.
Comment 6 Sophie MEYNIEUX 2016-08-17 13:44:33 UTC
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)
Comment 7 Katrin Fischer 2016-08-17 14:14:29 UTC
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.
Comment 8 Owen Leonard 2019-02-22 13:56:21 UTC
(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.
Comment 9 Pablo AB 2020-06-18 06:02:25 UTC
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).
Comment 10 Dobrica Pavlinusic 2020-09-02 10:25:00 UTC
(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.
Comment 11 Tomás Cohen Arazi 2022-12-28 12:35:41 UTC
I'll give this a try :-D
Comment 12 Tomás Cohen Arazi 2022-12-28 12:36:39 UTC
(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.
Comment 13 Tomás Cohen Arazi 2022-12-28 14:20:05 UTC
Created attachment 144863 [details]
How it looks with this patches :-D
Comment 14 Tomás Cohen Arazi 2022-12-28 14:20:47 UTC
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
Comment 15 Tomás Cohen Arazi 2022-12-28 14:20:53 UTC
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>
Comment 16 Tomás Cohen Arazi 2022-12-28 14:20:59 UTC
Created attachment 144866 [details] [review]
Bug 14251: Add weasyprint dependency

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 solene.ngamga 2023-01-09 20:48:01 UTC
I can't reproduce the step.
Comment 18 Katrin Fischer 2023-01-09 20:52:30 UTC
Hi Solene, I just checked and the patches apply to master cleanly. Do you have another question about this one?
Comment 19 solene.ngamga 2023-01-09 21:23:45 UTC
I have the same message in system information ("No warnings") when I install and uninstall weasyprint.
Ps: the patch has been applied before.
Comment 20 Tomás Cohen Arazi 2023-01-30 14:41:10 UTC
Oh, didn't notice this was failed because of the about page!
Comment 21 Tomás Cohen Arazi 2023-02-03 18:24:09 UTC
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
Comment 22 Tomás Cohen Arazi 2023-02-03 18:24:16 UTC
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>
Comment 23 Tomás Cohen Arazi 2023-02-03 18:24:24 UTC
Created attachment 146170 [details] [review]
Bug 14251: Add weasyprint dependency

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 David Nind 2023-02-04 18:03:23 UTC
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
Comment 25 Tomás Cohen Arazi 2023-02-06 18:23:54 UTC
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
Comment 26 Tomás Cohen Arazi 2023-02-06 18:24:27 UTC
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>
Comment 27 Tomás Cohen Arazi 2023-02-06 18:24:56 UTC
Created attachment 146297 [details] [review]
Bug 14251: Add weasyprint dependency

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 David Nind 2023-02-07 09:51:20 UTC
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>
Comment 29 David Nind 2023-02-07 09:51:26 UTC
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>
Comment 30 David Nind 2023-02-07 09:51:32 UTC
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>
Comment 31 Kyle M Hall 2023-03-03 19:55:07 UTC
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>
Comment 32 Kyle M Hall 2023-03-03 19:55:14 UTC
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>
Comment 33 Kyle M Hall 2023-03-03 19:55:17 UTC
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>
Comment 34 Kyle M Hall 2023-03-15 14:05:00 UTC
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.
Comment 35 Tomás Cohen Arazi 2023-03-15 15:35:47 UTC
(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.
Comment 36 Kyle M Hall 2023-03-15 15:40:46 UTC
> 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/).
Comment 37 Tomás Cohen Arazi 2023-03-27 10:06:03 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 38 Matt Blenkinsop 2023-04-11 09:01:20 UTC
Enhancement - not backporting to 22.11.x

Nice work everyone!
Comment 39 Caroline Cyr La Rose 2023-05-24 15:08:39 UTC
Seems like back end stuff. I still reviewed the discharge sections in the manual to at least update the screenshots...