Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices
Summary: Allow the use of Template Toolkit syntax for slips and notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15276 15277 15278 15969 16883 17976 26663 10708 15280 15705 16942 16960 16985 17246 17960
  Show dependency treegraph
 
Reported: 2015-08-31 12:10 UTC by Kyle M Hall
Modified: 2020-10-13 08:57 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (17.12 KB, patch)
2015-12-02 13:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (17.24 KB, patch)
2015-12-21 14:29 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (11.60 KB, patch)
2016-01-21 14:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (11.68 KB, patch)
2016-01-21 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (17.87 KB, patch)
2016-01-22 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Add Unit Tests (7.65 KB, patch)
2016-01-22 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Make the use of TT a controllable switch, hide it behind a syspref (16.05 KB, patch)
2016-02-10 18:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Make the use of TT a controllable switch, hide it behind a syspref (16.78 KB, patch)
2016-02-10 18:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (17.87 KB, patch)
2016-03-01 12:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Add Unit Tests (7.65 KB, patch)
2016-03-01 12:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Make the use of TT a controllable switch, hide it behind a syspref (16.78 KB, patch)
2016-03-01 12:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (17.87 KB, patch)
2016-03-03 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Add Unit Tests (7.65 KB, patch)
2016-03-03 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Make the use of TT a controllable switch, hide it behind a syspref (16.78 KB, patch)
2016-03-03 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Change method type() to _type() for Koha objects (3.32 KB, patch)
2016-03-03 12:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Change all references to Koha::Borrower to Koha::Patron (3.15 KB, patch)
2016-03-08 18:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (17.92 KB, patch)
2016-03-08 19:32 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Add Unit Tests (7.70 KB, patch)
2016-03-08 19:33 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Make the use of TT a controllable switch, hide it behind a syspref (16.83 KB, patch)
2016-03-08 19:33 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Change method type() to _type() for Koha objects (3.36 KB, patch)
2016-03-08 19:33 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Change all references to Koha::Borrower to Koha::Patron (3.20 KB, patch)
2016-03-08 19:33 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (17.91 KB, patch)
2016-06-23 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Add Unit Tests (7.70 KB, patch)
2016-06-23 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Change method type() to _type() for Koha objects (3.32 KB, patch)
2016-06-23 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Change all references to Koha::Borrower to Koha::Patron (3.20 KB, patch)
2016-06-23 17:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (18.64 KB, patch)
2016-06-24 14:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Add Unit Tests (7.70 KB, patch)
2016-06-24 14:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14757: Add tests for new modules (7.56 KB, patch)
2016-06-24 14:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Don't overload new for Modifications (847 bytes, patch)
2016-06-27 13:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices (18.71 KB, patch)
2016-06-28 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Add Unit Tests (7.77 KB, patch)
2016-06-28 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14757: Add tests for new modules (7.63 KB, patch)
2016-06-28 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14757 [QA Followup] - Don't overload new for Modifications (920 bytes, patch)
2016-06-28 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14757: Remove obsolete occurrence of is_tt (897 bytes, patch)
2016-06-28 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-08-31 12:10:54 UTC
We should enable TT syntax for slips and notices. This could work side by side with the existing syntax parser for a release or two before we officially remove our bespoke parsing code for slips and notices, giving libraries ample time to switch over.
Comment 1 Jonathan Druart 2015-09-03 07:52:30 UTC
Kyle,
This sounds like a big work.
How do you plan to implement that?
Did you have a look at bug 12404?
Comment 2 Kyle M Hall 2015-12-02 13:49:12 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2015-12-21 14:29:11 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-12-23 10:29:34 UTC
Kyle, please provide tests for this new feature.
Comment 5 Kyle M Hall 2016-01-21 14:53:49 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2016-01-21 15:02:51 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2016-01-22 12:42:55 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2016-01-22 12:43:02 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-01-29 10:27:00 UTC
(In reply to Kyle M Hall from comment #0)
> We should enable TT syntax for slips and notices. This could work side by
> side with the existing syntax parser for a release or two before we
> officially remove our bespoke parsing code for slips and notices, giving
> libraries ample time to switch over.

The existing syntax is quite easy to understand/modify for everybody, but the TT syntax is not.
I would like to see a discussion on koha-devel to know if everybody agrees on the removal of the existing syntax, or if we should support both.
Comment 10 Jonathan Druart 2016-01-29 16:21:32 UTC
Comment on attachment 47176 [details] [review]
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices

Review of attachment 47176 [details] [review]:
-----------------------------------------------------------------

::: C4/Letters.pm
@@ +1414,5 @@
> +        issues => {
> +            module   => 'Koha::Checkouts',
> +            singular => 'checkout',
> +            plural   => 'checkouts',
> +            fk       => 'itemnumber',

We should use `pk => issue_id` and update existing calls.
Comment 11 Katrin Fischer 2016-01-30 10:03:37 UTC
> The existing syntax is quite easy to understand/modify for everybody, but
> the TT syntax is not.
> I would like to see a discussion on koha-devel to know if everybody agrees
> on the removal of the existing syntax, or if we should support both.

It would be nice to have all the advantages of TT, but I am also worried that it will make it harder for libraries to change the notices themselves. It currently works quite well, although we have some placeholders that are not well known or not very consistent (<items> vs. <<items.content>> vs. ---- etc.) and need explaining.

I looked at the changes on bug 15280 and that looked not too bad. I think key is to have better documentation to explain the change and make it easier to see which variables can be used where and what additional features this will make available.

I'd be interested to see how this change will affect the editor - no patch there yet on bug 15277, maybe a screenshot?
Comment 12 Bob Birchall 2016-02-02 00:39:42 UTC
(In reply to Kyle M Hall from comment #0)
> We should enable TT syntax for slips and notices. This could work side by
> side with the existing syntax parser for a release or two before we
> officially remove our bespoke parsing code for slips and notices, giving
> libraries ample time to switch over.

I endorse the comments by Jonathan and Katrin that libraries must be able to edit their own notices.  They need a simple interface in which to do that and good documentation.

I'm also concerned about the potential cost of this change, for a support company and also for any large consortium that has notices defined separately by branch.  If every notice in use at every client needs at best examination and at worst to be re-written, that's a bit daunting.  Is that the implication?
Comment 13 Kyle M Hall 2016-02-03 14:34:51 UTC
(In reply to Bob Birchall from comment #12)
> (In reply to Kyle M Hall from comment #0)
> > We should enable TT syntax for slips and notices. This could work side by
> > side with the existing syntax parser for a release or two before we
> > officially remove our bespoke parsing code for slips and notices, giving
> > libraries ample time to switch over.
> 
> I endorse the comments by Jonathan and Katrin that libraries must be able to
> edit their own notices.  They need a simple interface in which to do that
> and good documentation.
> 
> I'm also concerned about the potential cost of this change, for a support
> company and also for any large consortium that has notices defined
> separately by branch.  If every notice in use at every client needs at best
> examination and at worst to be re-written, that's a bit daunting.  Is that
> the implication?

The current plan is to make this work with minimal or no changes! You can check out the mailing list thread for the on-going discussion.
Comment 14 Bob Birchall 2016-02-06 02:28:05 UTC
(In reply to Kyle M Hall from comment #13)

> The current plan is to make this work with minimal or no changes! You can
> check out the mailing list thread for the on-going discussion.

For me, the discussion has led to a great outcome.  I'm happy for this to proceed.  Thank you!
Comment 15 Jonathan Druart 2016-02-08 11:15:22 UTC
Switch to "In discussion' according to the ML thread.
Comment 16 Kyle M Hall 2016-02-10 18:47:06 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2016-02-10 18:48:56 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2016-03-01 12:07:34 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2016-03-01 12:07:41 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2016-03-01 12:07:44 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2016-03-03 12:22:08 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2016-03-03 12:22:20 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2016-03-03 12:22:25 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2016-03-03 12:22:28 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2016-03-08 18:00:20 UTC Comment hidden (obsolete)
Comment 26 Nicole C. Engard 2016-03-08 19:32:56 UTC Comment hidden (obsolete)
Comment 27 Nicole C. Engard 2016-03-08 19:33:03 UTC Comment hidden (obsolete)
Comment 28 Nicole C. Engard 2016-03-08 19:33:08 UTC Comment hidden (obsolete)
Comment 29 Nicole C. Engard 2016-03-08 19:33:13 UTC Comment hidden (obsolete)
Comment 30 Nicole C. Engard 2016-03-08 19:33:17 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2016-03-17 11:09:16 UTC
Kyle,
Could you detail the plan for the dependent bugs please?
You are adding a new pref and a new DB column (letter.is_tt) in this patch but they are not used in other patches.
We cannot update the notices with the TT syntax and set the pref off by default.

I'd like to QA this patch set and the others in one go.
Comment 32 Jonathan Druart 2016-03-23 22:43:32 UTC
That needs some more works.
Comment 33 Katrin Fischer 2016-04-04 22:26:42 UTC
Is the status here correct? Should it be failed QA?
Comment 34 Jonathan Druart 2016-04-05 05:56:36 UTC
(In reply to Katrin Fischer from comment #33)
> Is the status here correct? Should it be failed QA?

More or less the same, we need a new patch.
Comment 35 Kyle M Hall 2016-04-18 16:59:05 UTC
(In reply to Jonathan Druart from comment #31)
> Kyle,
> Could you detail the plan for the dependent bugs please?
> You are adding a new pref and a new DB column (letter.is_tt) in this patch
> but they are not used in other patches.
> We cannot update the notices with the TT syntax and set the pref off by
> default.

The other patches will need to be updated. Are there any changes to this patch set specifically that you need, or shall I go ahead and update all the dependent patches?
Comment 36 Jonathan Druart 2016-04-19 06:55:20 UTC
I don't need anything else (for bug 15705) apart from the patches from this bug report. But I think it would be good to agree on the next steps.
For instance, my note on comment 31 is still valid:
> We cannot update the notices with the TT syntax and set the pref off by default.
Comment 37 Kyle M Hall 2016-04-19 09:58:45 UTC
(In reply to Jonathan Druart from comment #36)
> I don't need anything else (for bug 15705) apart from the patches from this
> bug report. But I think it would be good to agree on the next steps.
> For instance, my note on comment 31 is still valid:
> > We cannot update the notices with the TT syntax and set the pref off by default.

Right, considering the number of blocked patches, I think it would make the most sense for the setting to be enabled by default in this patch set. I'll add a followup to do that.
Comment 38 Nicole C. Engard 2016-06-23 17:40:18 UTC
We were holding on this until 16.05 was released and it's blocking 15969 which we have a partner waiting on. Can we QA this one now?
Comment 39 Kyle M Hall 2016-06-23 17:44:47 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2016-06-23 17:44:56 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2016-06-23 17:44:59 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2016-06-23 17:45:02 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2016-06-24 14:29:40 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2016-06-24 14:29:51 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2016-06-24 14:29:56 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2016-06-24 14:35:24 UTC
Kyle,
I have squashed some patches (they failed qa tools:  FAIL Koha/Borrower/Modification.pm FAIL git manipulation: The file has been added and deleted in the same patchset) and added tests for the new modules.
However, I do not understand why you overload the method 
Koha::Patron::Modifications->new, this should not be needed

Moreover, the methods of Koha::Patron::Modifications should now use snake_case (as they use Koha::Object), but it's outside the scope of this bug.
Comment 47 Kyle M Hall 2016-06-27 13:33:04 UTC
(In reply to Jonathan Druart from comment #46)
> Kyle,
> I have squashed some patches (they failed qa tools:  FAIL
> Koha/Borrower/Modification.pm FAIL git manipulation: The file has been added
> and deleted in the same patchset) and added tests for the new modules.
> However, I do not understand why you overload the method 
> Koha::Patron::Modifications->new, this should not be needed
> 
> Moreover, the methods of Koha::Patron::Modifications should now use
> snake_case (as they use Koha::Object), but it's outside the scope of this
> bug.

I also don't know why I did that ; )

I will post a followup to remove the new() method, as it is not needed. We should file a bug to fix the method names fo Koha::Patron::Modifications.
Comment 48 Kyle M Hall 2016-06-27 13:44:26 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2016-06-28 13:46:26 UTC
Created attachment 52924 [details] [review]
Bug 14757 - Allow the use of Template Toolkit syntax for slips and notices

This bug is the beginning of a conversion from our current bespoke
syntax for slips and notices to Template Toolkit syntax.

This patch is the initial seed which will evolve over time.
With this addition, we can take advantage of our Koha Objects
to greatly simplify the processing of Slips and Notices over time.

Test Plan:
1) Apply this patch
2) Ensure you have the default CHECKOUT notice
3) Check out and return an item for a patron
4) Note the text of the CHECKOUT notice
5) Replace your CHECKOUT notice with the following:

The following items have been checked out:
----
[% biblio.title %]
----
Thank you for visiting [% branch.branchname %].

6) Repeat step 3
7) Note the CHECKOUT notice text matches the previous CHECKOUT notice text

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
New notice syntax works, no koha-qa errors

Signed-off-by: Sean McGarvey <seanm@pascolibraries.org>

Bug 14757 [QA Followup] - Change method type() to _type() for Koha objects

Signed-off-by: Sean McGarvey <seanm@pascolibraries.org>

Bug 14757 [QA Followup] - Change all references to Koha::Borrower to Koha::Patron

Signed-off-by: Sean McGarvey <seanm@pascolibraries.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 50 Jonathan Druart 2016-06-28 13:46:33 UTC
Created attachment 52925 [details] [review]
Bug 14757 [QA Followup] - Add Unit Tests

Signed-off-by: Sean McGarvey <seanm@pascolibraries.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 51 Jonathan Druart 2016-06-28 13:46:41 UTC
Created attachment 52926 [details] [review]
Bug 14757: Add tests for new modules

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 52 Jonathan Druart 2016-06-28 13:46:48 UTC
Created attachment 52927 [details] [review]
Bug 14757 [QA Followup] - Don't overload new for Modifications

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 53 Jonathan Druart 2016-06-28 13:46:56 UTC
Created attachment 52928 [details] [review]
Bug 14757: Remove obsolete occurrence of is_tt

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 54 Jonathan Druart 2016-06-28 13:48:28 UTC
Kyle, I have marked this patchset as Passed QA but we still need a patch to take into account the 3 following tables: aqorders, aqbooksellers, biblioitems.
I think this could be done on another bug report.
Comment 55 Kyle M Hall 2016-07-08 13:51:46 UTC
Pushed to master for 16.11!

(In reply to Jonathan Druart from comment #54)
> Kyle, I have marked this patchset as Passed QA but we still need a patch to
> take into account the 3 following tables: aqorders, aqbooksellers,
> biblioitems.
> I think this could be done on another bug report.

Yes, I think that's an excellent plan. Can you file the bug report(s) for those?

Thanks!
Comment 56 Jonathan Druart 2016-07-08 15:45:18 UTC
(In reply to Kyle M Hall from comment #55)
> Pushed to master for 16.11!
> 
> (In reply to Jonathan Druart from comment #54)
> > Kyle, I have marked this patchset as Passed QA but we still need a patch to
> > take into account the 3 following tables: aqorders, aqbooksellers,
> > biblioitems.
> > I think this could be done on another bug report.
> 
> Yes, I think that's an excellent plan. Can you file the bug report(s) for
> those?

I have opened bug 16883.
Comment 57 Jonathan Druart 2016-07-22 12:07:11 UTC
Kyle, bug 16929 has been introduced by these patches.
Actually it seems that the changes made to Koha::Patron::Modification[s] were not intended. Could you confirm?
Comment 58 Marcel de Rooy 2016-07-22 12:37:47 UTC
(In reply to Jonathan Druart from comment #57)
> Kyle, bug 16929 has been introduced by these patches.
> Actually it seems that the changes made to Koha::Patron::Modification[s]
> were not intended. Could you confirm?

Should be bug 16960.
Comment 59 Petter Goksøyr Åsen 2016-08-30 10:56:55 UTC
I seem to get a lot of failures after this patch.

I can't get HOLD_SLIP to render without error:
SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: unexpected operator 'author' with undef operand at /home/vagrant/kohaclone/Koha/Objects.pm line 168

I hooked it up with debugger and it fails in sub _get_tt_params
Comment 60 Petter Goksøyr Åsen 2016-08-30 11:15:23 UTC
I managed to fix it by changing the config hash in _get_tt_params:

> git diff
diff --git a/C4/Letters.pm b/C4/Letters.pm
index 737c8cd..c1ef404 100644
--- a/C4/Letters.pm
+++ b/C4/Letters.pm
@@ -1437,7 +1437,7 @@ sub _get_tt_params {
             module   => 'Koha::Holds',
             singular => 'hold',
             plural   => 'holds',
-            fk       => [ 'borrowernumber', 'biblionumber' ],
+            pk       => 'reserve_id',
         },
         serial => {
             module   => 'Koha::Serials',


But I don't understand this code very well, so you folks have to look at it!
Comment 61 Jonathan Druart 2016-08-30 12:10:01 UTC
(In reply to Petter Goksøyr Åsen from comment #59)
> I seem to get a lot of failures after this patch.
> 
> I can't get HOLD_SLIP to render without error:
> SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: unexpected operator
> 'author' with undef operand at /home/vagrant/kohaclone/Koha/Objects.pm line
> 168
> 
> I hooked it up with debugger and it fails in sub _get_tt_params

Bug 16942 should fix this problem.
Comment 62 Katrin Fischer 2017-01-04 09:57:42 UTC
Once implemented shoudl also fix bug 10164.
Comment 63 Mason James 2017-05-24 00:57:08 UTC
(In reply to Kyle M Hall from comment #55)
> Pushed to master for 16.11!

Enhancement, skipping for 16.05.x