Kyle,
This sounds like a big work.
How do you plan to implement that?
Did you have a look at bug 12404?

Kyle, please provide tests for this new feature.

(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 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.

> 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?

(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?

(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.

(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!

Switch to "In discussion' according to the ML thread.

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.

That needs some more works.

Is the status here correct? Should it be failed QA?

(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.

(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?

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.

(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.

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?

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.

(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.

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>

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>

Created attachment 52926 [details] [review]
Bug 14757: Add tests for new modules

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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>

Created attachment 52928 [details] [review]
Bug 14757: Remove obsolete occurrence of is_tt

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

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.

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!

(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.

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?

(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.

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

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!

(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.

Once implemented shoudl also fix bug 10164.

(In reply to Kyle M Hall from comment #55)
> Pushed to master for 16.11!

Enhancement, skipping for 16.05.x