Bug 15278

Summary: [Omnibus] Port default slips and notices from bespoke syntax to Template Toolkit
Product: Koha Reporter: Kyle M Hall <kyle>
Component: NoticesAssignee: Emily Lamancusa <emily.lamancusa>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: black23, cbrannon, dcook, emily.lamancusa, george, jonathan.druart, josef.moravec, lisette.scheer, lisettepalouse+koha, lucas, martin.renvoize, severine.queune, veron
Version: unspecifiedKeywords: roadmap_24_05
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34472
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36470
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15283, 15284, 35925, 36020, 36106, 36107, 36109, 36110, 36112, 36113, 36114, 36124, 36125, 36126, 36127, 36253, 36254, 36255, 36256, 36257, 36258, 36259, 36260, 36588, 36608, 14757, 15280, 15282, 17961    
Bug Blocks: 15279    

Description Kyle M Hall 2015-12-02 13:11:49 UTC
This bug is an omnibus for converting all default slips and notices from our current bespoke syntax to Template Toolkit.
Comment 1 Martin Renvoize 2019-07-19 14:51:56 UTC
It would be great to see this moving again.. I've resurrected 15283 as a starting point.
Comment 2 Jonathan Druart 2020-11-11 08:19:57 UTC
Could we get a team work on this one and target 21.05? Anyone interested?
Comment 3 Katrin Fischer 2020-11-11 21:12:38 UTC
Not so much for programming maybe, but can throw my hat in for testing and QA.
Comment 4 Katrin Fischer 2020-11-11 21:12:59 UTC
On one condition: we should accompany porting by writing up Documentation and improving the editor!
Comment 5 Katrin Fischer 2020-11-11 21:13:54 UTC
And if there are more people interested, it might be good to have a quick meeting mapping out the steps that need to be done before getting started.
Comment 6 Martin Renvoize 2020-11-11 21:53:21 UTC
I'm certainly onboard with this.. as discussed a few times.

Would be great to share the load as there's lots of notices to work through and corrections to the underlying code to go with them I've found.
Comment 7 Lucas Gass 2023-05-24 13:40:33 UTC
I would love to see this get back on the roadmap, perhaps for 23.11.
Comment 8 George Williams (NEKLS) 2023-05-24 16:05:42 UTC
(In reply to Katrin Fischer from comment #4)
> On one condition: we should accompany porting by writing up Documentation
> and improving the editor!

I'd like to see this move forward again, too.  The documentation part is really important for us librarians.
Comment 9 Christopher Brannon 2023-08-04 15:17:30 UTC
I would also like to see this move forward.  We have little documentation to work with, and are at the mercy of developers to give us nuggets of information on TT coding.
Comment 10 Lisette Scheer 2023-12-18 21:57:21 UTC
I agree that documentation is an essential part of this process. 

This is definitely something I'd love to see get moving again.
Comment 11 Emily Lamancusa 2024-03-08 16:49:45 UTC
All notices that still use the old bespoke syntax in their default notice text now have a linked bug on this omnibus. I grouped similar notices together in some cases when I felt it made sense, though they can be further split out in dependent bugs if needed.

(Reminder that here we are focused only on updating the default notice text and making sure that all notices CAN be rendered with Template Toolkit, not on documentation (see bug 34472) or deprecating the existing functionality (bug 15279))

Many of the notices should be largely trivial to update to Template Toolkit syntax - the underlying code is already there, and it's just a matter of replacing a few lines in the sample notice itself. However, there are some that will need work on the underlying code before they can be recreated with Template Toolkit.

Should be trivial to update:
Bug 35925 - Port default purchase suggestion notices to Template Toolkit
Bug 36020 - Port default recall notices to Template Toolkit
Bug 36106 - Port default PREDUE and DUE notices to Template Toolkit
Bug 36110 - Port default OVERDUES_SLIP to Template Toolkit syntax
Bug 36112 - Port default CHECKOUT_NOTE notice to Template Toolkit syntax
Bug 36113 - Port default RENEWAL notice to Template Toolkit syntax
Bug 36114 - Port default TRANSFERSLIP notice to Template Toolkit syntax
Bug 36124 - Port default Article Request notices to Template Toolkit syntax
Bug 36125 - Port default HOLD_SLIP notice to Template Toolkit syntax
Bug 36126 - Port default HOLD notice to Template Toolkit syntax
Bug 36127 - Port default HOLDPLACED and HOLD_CHANGED notices to Template Toolkit syntax
Bug 36253 - Port default acquisition notices to Template Toolkit syntax
Bug 36255 - Port default OPAC_REG_VERIFY notice to Template Toolkit syntax
Bug 36256 - Port default MEMBERSHIP_EXPIRY notice to Template Toolkit syntax
Bug 36257 - Port default DISCHARGE notice to Template Toolkit syntax 
Bug 36259 - Port default SERIAL_ALERT notice to Template Toolkit syntax
Bug 36260 - Port default PROBLEM_REPORT notice to Template Toolkit syntax


Need work on the underlying code:
Bug 15283 - Switch default ISSUEQSLIP notice to Template Toolkit
Bug 15284 - Switch default ISSUESLIP notice to Template Toolkit
Bug 36107 - Port default DUEDGST and PREDUEDGST notices to Template Toolkit
Bug 36109 - Port default ODUE notice to Template Toolkit
Bug 36254 - Port default PASSWORD_RESET and STAFF_PASSWORD_RESET notices to Template Toolkit syntax
Bug 36258 - Port default SHARE_ACCEPT and SHARE_INVITE notices to Template Toolkit syntax
Comment 12 Emily Lamancusa 2024-03-08 17:17:36 UTC
One more thing to consider: do we just update the default text in sample_notices.yml for new installations at this point, or do we also include dbrevs for existing installations that use the default notice text?

Normally, we don't update the default notice text for existing installations unless there is a breaking change. However, since the goal is to eventually deprecate the old syntax, which will be a breaking change, it might be better to update the defaults on both new and existing installations together... It will be unavoidable for libraries with customized notices to need to update their own notices eventually. However, I'm sure there are libraries who rely on the default notices and may not be familiar with customizing their notices at all, and I could see it being a major issue if a large number of their notices, which they haven't touched, stop working.

One could argue that we should wait until we actually introduce the breaking change to update the default notice text on existing installations, but IMO it makes more sense to do it now rather than tracking down what the default used to be sometime later. (Granted, we will still probably miss some installations that have older "default" versions of notices whose defaults have already been changed in the past, but I still think we should prevent problems as much as we can now.)

The downside is that it makes the current task harder, and introduces a chance of bugs/regressions...but I still think it's worth it to avoid even more difficulties later.
Comment 13 Christopher Brannon 2024-03-08 17:33:23 UTC
(In reply to Emily Lamancusa from comment #12)
> One more thing to consider: do we just update the default text in
> sample_notices.yml for new installations at this point, or do we also
> include dbrevs for existing installations that use the default notice text?
> 
> Normally, we don't update the default notice text for existing installations
> unless there is a breaking change. However, since the goal is to eventually
> deprecate the old syntax, which will be a breaking change, it might be
> better to update the defaults on both new and existing installations
> together... It will be unavoidable for libraries with customized notices to
> need to update their own notices eventually. However, I'm sure there are
> libraries who rely on the default notices and may not be familiar with
> customizing their notices at all, and I could see it being a major issue if
> a large number of their notices, which they haven't touched, stop working.
> 
> One could argue that we should wait until we actually introduce the breaking
> change to update the default notice text on existing installations, but IMO
> it makes more sense to do it now rather than tracking down what the default
> used to be sometime later. (Granted, we will still probably miss some
> installations that have older "default" versions of notices whose defaults
> have already been changed in the past, but I still think we should prevent
> problems as much as we can now.)
> 
> The downside is that it makes the current task harder, and introduces a
> chance of bugs/regressions...but I still think it's worth it to avoid even
> more difficulties later.

Could you keep the old syntax functional for existing users and give them a version or two to update their notices/slips?  Maybe only change the default notices that have not been modified from install?  It seems reasonable to give existing users time to clean up their own notices with the new tools.  That way, if there are any complications or something missed, they some wiggle room before the old stuff goes away.
Comment 14 Emily Lamancusa 2024-03-08 18:31:35 UTC
(In reply to Christopher Brannon from comment #13)
> Could you keep the old syntax functional for existing users and give them a
> version or two to update their notices/slips?  Maybe only change the default
> notices that have not been modified from install?  It seems reasonable to
> give existing users time to clean up their own notices with the new tools. 
> That way, if there are any complications or something missed, they some
> wiggle room before the old stuff goes away.

Yes, that's basically the idea. The old syntax will still be fully functional for now. We have Bug 15279 to remove the functionality eventually, but that bug is blocked and won't happen until after we update the installation defaults (this bug), create a reference guide (bug 34472), and improve the notice editor (bug 15277) so that users actually have the tools to make good use of Template Toolkit syntax.

The questions I'm trying to get at now are:
1) If a library hasn't modified a particular notice from install, should we still make them responsible for updating it themselves, or should we update it automatically in that case (since it should be relatively straightforward to check for the old default template and replace it with the new default template)?

2) If we do update it automatically, should we do it now, or wait until the old syntax actually becomes nonfunctional?
Comment 15 Christopher Brannon 2024-03-08 18:40:07 UTC
(In reply to Emily Lamancusa from comment #14)
> (In reply to Christopher Brannon from comment #13)
> > Could you keep the old syntax functional for existing users and give them a
> > version or two to update their notices/slips?  Maybe only change the default
> > notices that have not been modified from install?  It seems reasonable to
> > give existing users time to clean up their own notices with the new tools. 
> > That way, if there are any complications or something missed, they some
> > wiggle room before the old stuff goes away.
> 
> Yes, that's basically the idea. The old syntax will still be fully
> functional for now. We have Bug 15279 to remove the functionality
> eventually, but that bug is blocked and won't happen until after we update
> the installation defaults (this bug), create a reference guide (bug 34472),
> and improve the notice editor (bug 15277) so that users actually have the
> tools to make good use of Template Toolkit syntax.
> 
> The questions I'm trying to get at now are:
> 1) If a library hasn't modified a particular notice from install, should we
> still make them responsible for updating it themselves, or should we update
> it automatically in that case (since it should be relatively straightforward
> to check for the old default template and replace it with the new default
> template)?
> 
> 2) If we do update it automatically, should we do it now, or wait until the
> old syntax actually becomes nonfunctional?

1) You should update it automatically if it hasn't been modified.
2) Right away.  This gives people more exposure to how it is used.
Comment 16 Katrin Fischer 2024-03-08 22:43:03 UTC
Hi all,

first of all: Emily, you are awesome and we don't tell you nearly often enough!

Something that hasn't come up here yet, but we need to keep in mind is translations and they complicate things a bit.

We install translated notices now, which makes it much harder to tell which ones are "unchanged". And on the other side it makes it harder to insert new nice notices for everyone. It's even hard to determine which language was used when installing or which language a notice is in. 

Some other ideas:
* Have a page on the wiki with the new nice notice sample templates, so it's easy for libraries to get them (sometimes you might just break something and want to go a step back).
* Have a deprecation period for the old syntax to help people switch. I think that's almost a given.
* Visually highlight notices in the old syntax that require updating?
* Maybe a wild idea: Could we have something like a "reset to sample notice" button? If it could get the translated version from the YAML files that would be pretty awesome too.
Comment 17 Lucas Gass 2024-03-08 22:54:11 UTC
> * Maybe a wild idea: Could we have something like a "reset to sample notice"
> button? If it could get the translated version from the YAML files that
> would be pretty awesome too.

+1 to this!
Comment 18 George Williams (NEKLS) 2024-03-08 22:56:19 UTC
> * Maybe a wild idea: Could we have something like a "reset to sample notice"
> button? If it could get the translated version from the YAML files that
> would be pretty awesome too.

+1 here too.  Awesome idea.
Comment 19 Jonathan Druart 2024-03-11 12:50:30 UTC
Aren't most templates easy to adjust?
Like << borrowers.attribute >> becomes [% patron.attribute %].

We could replace them one by one. If there are still <<>> after the replacement happening in the db rev then we don't update it and warn about the deprecation.
Comment 20 Emily Lamancusa 2024-03-19 18:57:30 UTC
> first of all: Emily, you are awesome and we don't tell you nearly often
> enough!

Aw, thank you =)

> Something that hasn't come up here yet, but we need to keep in mind is
> translations and they complicate things a bit.
> 
> We install translated notices now, which makes it much harder to tell which
> ones are "unchanged". And on the other side it makes it harder to insert new
> nice notices for everyone. It's even hard to determine which language was
> used when installing or which language a notice is in. 

Ah, I didn't realize that. Good to know! It sounds like Jonathan's suggestion of replacing the tags one by one is the way to go here. That would be out of scope for this bug, so we'll limit ourselves to simply changing the text in the YAML file for now (plus whatever work needs to be done to make those last few notices fully TT-compatible).


> * Maybe a wild idea: Could we have something like a "reset to sample notice"
> button? If it could get the translated version from the YAML files that
> would be pretty awesome too.

Also +1 to this!
Comment 21 Katrin Fischer 2024-03-20 21:28:34 UTC
I think just replacing the markers is not enough, as sometimes we need to make sure to add a [% FOREACH %] loop in the right spots as well (ACQORDER; *DGST; ...) It might work, but needs some thought. 

I think having nice sample notices is a great first step and then we can figure out the next ones.
Comment 22 Emily Lamancusa 2024-03-21 15:47:48 UTC
Hmmm, I hadn't actually thought about replacing the digest syntax... [% FOREACH %] doesn't work for digest-building, because the ---- marker doesn't represent a piece of logic or substitution internal to one notice, but rather it's metadata that tells the code how to parse and store the prepared message so that a new message can be combined properly with an old message after the fact, without duplicating the parts that should be the same:

> sub _metadata {
>    my ($letter) = @_;
>    if ($letter->{content} =~ /----/) {
>        my ($header, $body, $footer) = split(/----\r?\n?/, $letter->{content});
>        return {
>            header => $header,
>            body   => [$body],
>            footer => $footer,
>        };
>    } else {
>        return {
>            header => '',
>            body   => [$letter->{content}],
>            footer => '',
>        };
>    }
> }

Is that something we can (usefully) replace with Template Toolkit? (this may also be a "next step" question, to avoid making the perfect the enemy of the good)
Comment 23 Martin Renvoize 2024-03-21 17:49:52 UTC
It's not just the custom --- handling if digests that need thought.. there is also a customer loop syntax in our <<>> style I believe.. that and the special handling for attributes code conversion stuff that I'm already also working on.

As for the --- digest thing, I feel like the logic for that one is backwards and strange.. we should be generating the notice all in one hit at a later point in time rather than queuing a notice and then munging extra stuff into it after the fact at various times.


Food for thought.  Happy to grab a call some time to discuss the things I've been contemplating around this area for some time now if it helps
Comment 24 Lisette Scheer 2024-03-21 19:27:59 UTC
(In reply to Martin Renvoize from comment #23)

> As for the --- digest thing, I feel like the logic for that one is backwards
> and strange.. we should be generating the notice all in one hit at a later
> point in time rather than queuing a notice and then munging extra stuff into
> it after the fact at various times.

The --- digest thing also doesn't make much sense to me. 
 With putting it together at one point in time, would that mean that say for hold notices when notices send every hour that before that happened it would build and check for the patron any waiting holds that had been set to waiting since the last one? 

Or perhaps we could set some sort of flag when the hold is trapped that gets cleared when the notice is sent?
Comment 25 Katrin Fischer 2024-04-01 11:35:31 UTC
(In reply to Lisette Scheer from comment #24)
> (In reply to Martin Renvoize from comment #23)
> 
> > As for the --- digest thing, I feel like the logic for that one is backwards
> > and strange.. we should be generating the notice all in one hit at a later
> > point in time rather than queuing a notice and then munging extra stuff into
> > it after the fact at various times.
> 
> The --- digest thing also doesn't make much sense to me. 
>  With putting it together at one point in time, would that mean that say for
> hold notices when notices send every hour that before that happened it would
> build and check for the patron any waiting holds that had been set to
> waiting since the last one? 
> 
> Or perhaps we could set some sort of flag when the hold is trapped that gets
> cleared when the notice is sent?

I'd love to change the mechanic on how these notices are generated, but we should move this to a separate bug:

Bug 36470 - Generate CHECKOUT, CHECKIN and HOLD digest notices should be generated at once, not amended over time

When talking about the loops, I was actually thinking of the ACQORDER notice, as I had recently worked on that one :) It also has a weird way to detect repeatable sections, that does not even rely on the <order> tags, as you would think. Problem for another bug too.