Bug 38089

Summary: Fix incorrect regular expression from bug 33478 and move styles to head
Product: Koha Reporter: Martin Renvoize (ashimema) <martin.renvoize>
Component: NoticesAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Pushed to stable --- QA Contact: Nick Clemens (kidclamp) <nick>
Severity: major    
Priority: P5 - low CC: aleisha, aspencatteam, caroline.cyr-la-rose, katrin.fischer, Laura.escamilla, lucas, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.06
Circulation function:
Bug Depends on: 33478    
Bug Blocks: 33260    
Attachments: Bug 38089: Move style to head and correct regular expression
Bug 38089: Move style to head and correct regular expression
Bug 38089: (QA follow-up) Ensure style works on members/printslip.pl
Bug 38089: Move style to head and correct regular expression
Bug 38089: (QA follow-up) Ensure style works on members/printslip.pl
Bug 38089: (follow-up) More fixes for errors in original submission

Description Martin Renvoize (ashimema) 2024-10-04 07:29:27 UTC
The regular expression introduced in bug 33478 outright replaces all CSS selectors in your local styles.

This means that whatever styles you've defined will be applied to ALL elements of the notice template and not the specific one's you're attempting to target.
Comment 1 Martin Renvoize (ashimema) 2024-10-04 07:30:53 UTC
We should replace it with:

[% slip.style.replace('([^\{\}]+?)\s*\{', '#slip_' _ slip.id _ ' $1 {') | $raw %]

This will prepend the $slip_ID selector to any and all local selectors instead of replacing them
Comment 2 Martin Renvoize (ashimema) 2024-10-04 07:31:37 UTC
I also propose moving the <style> tags up into the <head> where they belong, as not all browsers support such tags inside the <body> and it's against the HTML standard to have them there.
Comment 3 Martin Renvoize (ashimema) 2024-10-04 08:28:37 UTC
Created attachment 172380 [details] [review]
Bug 38089: Move style to head and correct regular expression

This patch moves the local style sections from the body to the head
where they belong and it also updates the regular expression used to
prepend the slip_id id to css selectors rather than replace them
Comment 4 Nick Clemens (kidclamp) 2024-10-04 16:49:58 UTC
Created attachment 172421 [details] [review]
Bug 38089: Move style to head and correct regular expression

This patch moves the local style sections from the body to the head
where they belong and it also updates the regular expression used to
prepend the slip_id id to css selectors rather than replace them

To test:
1 - Create a new notice in the 'Patron (custom slips)' module called 'Testing'
2 - Set print content as:
    Try the  <span class="styled">styling</span> stuff.
3 - Set the 'Format' as:
    .styled {color:red;font-weight: bold;}
4 - Set system preference IntrnaetSlipPrinterJS to "//test"
    http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=IntranetSlipPrinterJS
    This will prevent auto printing popup
5 - Go to a patron and click Print -> Print Testing
6 - Note incorrect styling
7 - Apply patch, restart all
8 - Print Testing again
9 - Confirm styling is correct
Comment 5 Nick Clemens (kidclamp) 2024-10-04 16:50:01 UTC
Created attachment 172422 [details] [review]
Bug 38089: (QA follow-up) Ensure style works on members/printslip.pl

When moving the style up, it was moved before the code that handles printing a single slip, i.e.
taking the variables and forming a slip hash.

This patch moves that code to the top and additionally passes through the CODE of the notice so the ID is not blank
Comment 6 Martin Renvoize (ashimema) 2024-10-07 14:10:59 UTC
Created attachment 172464 [details] [review]
Bug 38089: Move style to head and correct regular expression

This patch moves the local style sections from the body to the head
where they belong and it also updates the regular expression used to
prepend the slip_id id to css selectors rather than replace them

To test:
1 - Create a new notice in the 'Patron (custom slips)' module called 'Testing'
2 - Set print content as:
    Try the  <span class="styled">styling</span> stuff.
3 - Set the 'Format' as:
    .styled {color:red;font-weight: bold;}
4 - Set system preference IntrnaetSlipPrinterJS to "//test"
    http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=IntranetSlipPrinterJS
    This will prevent auto printing popup
5 - Go to a patron and click Print -> Print Testing
6 - Note incorrect styling
7 - Apply patch, restart all
8 - Print Testing again
9 - Confirm styling is correct

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2024-10-07 14:11:01 UTC
Created attachment 172465 [details] [review]
Bug 38089: (QA follow-up) Ensure style works on members/printslip.pl

When moving the style up, it was moved before the code that handles printing a single slip, i.e.
taking the variables and forming a slip hash.

This patch moves that code to the top and additionally passes through the CODE of the notice so the ID is not blank

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2024-10-07 14:28:25 UTC
Created attachment 172473 [details] [review]
Bug 38089: (follow-up) More fixes for errors in original submission

The original submission missed a number of slip types that are print on
demand without queueing.

These all need an "id" passing in to the template and in some cases we
were also missing passing in the new style parameter too (preservation)
Comment 9 Katrin Fischer 2024-10-11 08:23:44 UTC
Strictly speaking we are missing either a SO or a QA person here ;)
Comment 10 Katrin Fischer 2024-10-11 09:16:04 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 11 Lucas Gass (lukeg) 2024-11-18 22:56:07 UTC
Backported to 24.05.x for upcoming 24.05.06