Bug 31373 - Notice template validation is missing INCLUDE_PATH
Summary: Notice template validation is missing INCLUDE_PATH
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 31211
Blocks: 32442
  Show dependency treegraph
 
Reported: 2022-08-16 16:11 UTC by Martin Renvoize
Modified: 2023-06-08 22:32 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31373: Add INCLUDE_PATH to notice validation (2.23 KB, patch)
2022-08-16 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31373: Add INCLUDE_PATH to notice validation (2.28 KB, patch)
2022-08-16 19:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31373: DRY - Use try/catch around _process_tt (3.32 KB, patch)
2022-08-18 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31373: DRY - Use try/catch around _process_tt (3.37 KB, patch)
2022-08-23 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31373: Add INCLUDE_PATH to notice validation (2.38 KB, patch)
2022-09-12 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31373: DRY - Use try/catch around _process_tt (3.47 KB, patch)
2022-09-12 12:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-08-16 16:11:38 UTC
We allow the use of staff client includes in template toolkit notice templates however, the current validation processing is missing this code and thus errantly reports issues with valid notices.
Comment 1 Martin Renvoize 2022-08-16 16:17:41 UTC
Created attachment 139210 [details] [review]
Bug 31373: Add INCLUDE_PATH to notice validation

This is a follow-up to bug 31211. It adds the same INCLUDE_PATH
variables as C4::Templates adds during processing to prevent false
failures.

Test plan
1) Add an INCLUDE to any valid notice, for example:
   [% INCLUDE 'patron-title.inc' patron => patron %]
2) Save and continue
3) Confirm you are faced with a validation error complaining that
   the include file cannot be found
4) Apply this patch
5) Confirm that the reported error has dissapeared
Comment 2 Kyle M Hall 2022-08-16 19:23:07 UTC
Created attachment 139211 [details] [review]
Bug 31373: Add INCLUDE_PATH to notice validation

This is a follow-up to bug 31211. It adds the same INCLUDE_PATH
variables as C4::Templates adds during processing to prevent false
failures.

Test plan
1) Add an INCLUDE to any valid notice, for example:
   [% INCLUDE 'patron-title.inc' patron => patron %]
2) Save and continue
3) Confirm you are faced with a validation error complaining that
   the include file cannot be found
4) Apply this patch
5) Confirm that the reported error has dissapeared

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Jonathan Druart 2022-08-17 09:18:51 UTC
Why do we process html_helpers?

Why are we duplicating C4::Letters::_process_tt?
Comment 4 Martin Renvoize 2022-08-17 09:39:02 UTC
(In reply to Jonathan Druart from comment #3)
> Why do we process html_helpers?

Ha, this crept in from another bug I was working... I found that including biblio-title.inc requires a pre-process of html_helpers and it felt like a good one to add to the default process rather than requiring people to know they need to have it processed at the top in the template itself.  It should probably be in it's own bug.
 
> Why are we duplicating C4::Letters::_process_tt?

So, I did wonder this myself.. I think it's because we don't have any parameters to actually pass in at this point.. so we just want a compile check.. must admit I didn't dig into the depths of the original bug.
Comment 5 Jonathan Druart 2022-08-17 10:00:26 UTC
(In reply to Martin Renvoize from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Why are we duplicating C4::Letters::_process_tt?
> 
> So, I did wonder this myself.. I think it's because we don't have any
> parameters to actually pass in at this point.. so we just want a compile
> check.. must admit I didn't dig into the depths of the original bug.

IMO we should stop as soon as possible and fix the situation, instead of adding more code to the controller.
Comment 6 Jonathan Druart 2022-08-17 10:01:54 UTC
(In reply to Martin Renvoize from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Why do we process html_helpers?
> 
> Ha, this crept in from another bug I was working... I found that including
> biblio-title.inc requires a pre-process of html_helpers and it felt like a
> good one to add to the default process rather than requiring people to know
> they need to have it processed at the top in the template itself.  It should
> probably be in it's own bug.

Preferable to include it explicitly when you need it instead of adding black magic. And it won't help plugins to be compatible between different versions if sometimes it needs to be included, sometimes not.
Comment 7 Martin Renvoize 2022-08-18 14:04:28 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Martin Renvoize from comment #4)
> > (In reply to Jonathan Druart from comment #3)
> > > Why do we process html_helpers?
> > 
> > Ha, this crept in from another bug I was working... I found that including
> > biblio-title.inc requires a pre-process of html_helpers and it felt like a
> > good one to add to the default process rather than requiring people to know
> > they need to have it processed at the top in the template itself.  It should
> > probably be in it's own bug.
> 
> Preferable to include it explicitly when you need it instead of adding black
> magic. And it won't help plugins to be compatible between different versions
> if sometimes it needs to be included, sometimes not.

Fair enough.. it should be very clearly documented somewhere which includes require that helper then so notice authors aren't surprised by them not working.

I'd also remove the KohaDates and Remove_MARC_punctuation additions too as they suffer the same issues.
Comment 8 Katrin Fischer 2022-08-18 14:22:38 UTC
If we remove KohaDates now, that might be a little late. Same for the other TT filter also probably not as widely used. 

What about notices not including the USE already?
Comment 9 Martin Renvoize 2022-08-18 16:23:17 UTC
Created attachment 139406 [details] [review]
Bug 31373: DRY - Use try/catch around _process_tt

This patch reduces the repetition in the tools/letter.pl controller
replaceing it with a try/catch block wrapping a call to
C4::Letters::_process_tt instead.
Comment 10 Martin Renvoize 2022-08-18 16:30:23 UTC
The point I was trying to make with the including html_helpers was that I don't see how it's different from automagically including the other two we include.. to me, this is a well hidden undocumented template thing so made sense to 'just make it easy'.. Anywho.. it should certainly be in it's own bug.

Moving on.. please review my follow-up/alternative (Let's golf it then squash).

I don't like the regex mangling of the croak output but was failing to work out how else to clean up the resulting output to screen... we could remove the croak and get more meaningful in our return from the internal C4::Letter function.. but that also felt a bit funky.
Comment 11 Kyle M Hall 2022-08-23 13:25:14 UTC
Created attachment 139677 [details] [review]
Bug 31373: DRY - Use try/catch around _process_tt

This patch reduces the repetition in the tools/letter.pl controller
replaceing it with a try/catch block wrapping a call to
C4::Letters::_process_tt instead.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Marcel de Rooy 2022-09-12 12:55:57 UTC
Created attachment 140456 [details] [review]
Bug 31373: Add INCLUDE_PATH to notice validation

This is a follow-up to bug 31211. It adds the same INCLUDE_PATH
variables as C4::Templates adds during processing to prevent false
failures.

Test plan
1) Add an INCLUDE to any valid notice, for example:
   [% INCLUDE 'patron-title.inc' patron => patron %]
2) Save and continue
3) Confirm you are faced with a validation error complaining that
   the include file cannot be found
4) Apply this patch
5) Confirm that the reported error has dissapeared

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2022-09-12 12:56:01 UTC
Created attachment 140457 [details] [review]
Bug 31373: DRY - Use try/catch around _process_tt

This patch reduces the repetition in the tools/letter.pl controller
replaceing it with a try/catch block wrapping a call to
C4::Letters::_process_tt instead.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Tomás Cohen Arazi 2022-09-16 14:07:32 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!