Bug 22761

Summary: Move "Fee receipt" from template to a slip
Product: Koha Reporter: Kyle M Hall <kyle>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: black23, costalc, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, lisettepalouse+koha, m.de.rooy, martin.renvoize, nengard, nick, oleonard, rbit
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement allows the 'Fee receipt' to be configurable by the library in the Tools->Notices and slips page
Version(s) released in:
19.05.00
Bug Depends on:    
Bug Blocks: 22809    
Attachments: Bug 22761: Add ability to use print version of ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF" slip
Bug 22761: Add notice based on existing template
Bug 22761: Remove hard coded version from template
Bug 22761: Add ability to use print version of ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF" slip
Bug 22761: Add notice based on existing template
Bug 22761: Remove hard coded version from template
printinvoice.pl
Bug 22761: Add ability to use print version of ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF" slip
Bug 22761: Add notice based on existing template
Bug 22761: Remove hard coded version from template
Bug 22761: Add ability to use print version of ACCOUNT_CREDIT slip
Bug 22761: Add notice based on existing template
Bug 22761: Remove hard coded version from template
Bug 22761: Add ability to use print version of ACCOUNT_CREDIT slip
Bug 22761: Add notice based on existing template
Bug 22761: Remove hard coded version from template
Bug 22761: (QA follow-up) Add notice translations
Bug 22761: (QA follow-up) Add description to DB update

Description Kyle M Hall 2019-04-23 18:11:46 UTC
Some libraries would like to be able to customize the "Fee receipt". It makes sense to move the content of members/printfeercpt.pl to a the print section of the ACCOUNT_PAYMENT slip.
Comment 1 Katrin Fischer 2019-04-23 18:20:46 UTC
*** Bug 10708 has been marked as a duplicate of this bug. ***
Comment 2 Katrin Fischer 2019-04-23 18:21:52 UTC
I've chosen to mark the other bug a duplicate in this case to have a fresh start. Making this the print version of the ACCOUNT_PAYMENT slip makes sense on first glance, but I wonder how best to deal with libraries that want to have the print on demand but not activate the email functionality.
Comment 3 Katrin Fischer 2019-04-23 18:22:36 UTC
Maybe a different slip template would work better? And leave the print on ACCOUNT_PAYMENT for a letter?
Comment 4 Kyle M Hall 2019-04-23 18:46:15 UTC
Created attachment 88552 [details] [review]
Bug 22761: Add ability to use print version of ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF" slip
Comment 5 Kyle M Hall 2019-04-23 18:46:23 UTC
Created attachment 88553 [details] [review]
Bug 22761: Add notice based on existing template
Comment 6 Kyle M Hall 2019-04-23 18:46:25 UTC
Created attachment 88554 [details] [review]
Bug 22761: Remove hard coded version from template
Comment 7 Kyle M Hall 2019-04-23 19:05:19 UTC
Created attachment 88557 [details] [review]
Bug 22761: Add ability to use print version of ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF" slip
Comment 8 Kyle M Hall 2019-04-23 19:05:28 UTC
Created attachment 88558 [details] [review]
Bug 22761: Add notice based on existing template
Comment 9 Kyle M Hall 2019-04-23 19:05:31 UTC
Created attachment 88559 [details] [review]
Bug 22761: Remove hard coded version from template
Comment 10 Kyle M Hall 2019-04-23 19:06:51 UTC
(In reply to Katrin Fischer from comment #3)
> Maybe a different slip template would work better? And leave the print on
> ACCOUNT_PAYMENT for a letter?

Good thinking. I've updated the patch set. Once it has passed qa, I'll add the additional translated versions of the slip in a followup!
Comment 11 Claudio Costales 2019-04-23 19:14:53 UTC
Great! It would be nice to be able to customize members/printinvoice.pl too (see image attached)
Comment 12 Claudio Costales 2019-04-23 19:15:55 UTC
Created attachment 88560 [details]
printinvoice.pl
Comment 13 Michal Denar 2019-04-23 19:18:19 UTC
Hi Kyle,
any test plan?

Than you.
Comment 14 Kyle M Hall 2019-04-23 19:23:01 UTC
Test Plan:
1) Apply this patch set
2) Restart all the things
3) Use the "print" button beside a *payment*
4) You should not see any difference from before the patches were applied
Comment 15 Kyle M Hall 2019-04-23 19:23:50 UTC
(In reply to Claudio from comment #11)
> Great! It would be nice to be able to customize members/printinvoice.pl too
> (see image attached)

I'm planning on submitting another patch set on another bug for printinvoice.pl ;)
Comment 16 Claudio Costales 2019-04-23 19:30:43 UTC
(In reply to Kyle M Hall from comment #15)
> (In reply to Claudio from comment #11)
> > Great! It would be nice to be able to customize members/printinvoice.pl too
> > (see image attached)
> 
> I'm planning on submitting another patch set on another bug for
> printinvoice.pl ;)

Thanks Kyle! (in spanish: Respiro aliviado!)
Comment 17 Michal Denar 2019-04-23 19:54:51 UTC
Hi kyle,
thank you for test plan. 
I've found problem. Print button on "red" paymets works well. But on "green" files open blank page. Please see video: http://g.recordit.co/shRFPo3XB9.gif
Comment 18 Kyle M Hall 2019-04-24 00:19:19 UTC
(In reply to Michal Denar from comment #17)
> Hi kyle,
> thank you for test plan. 
> I've found problem. Print button on "red" paymets works well. But on "green"
> files open blank page. Please see video: http://g.recordit.co/shRFPo3XB9.gif

I'm completely unable to reproduce this. Can you test this latest version of the patch set? If you are still seeing this can you provide specific steps to reproduce what you are seeing?

Thanks!
Kyle
Comment 19 Kyle M Hall 2019-04-24 00:23:20 UTC
(In reply to Michal Denar from comment #17)
> Hi kyle,
> thank you for test plan. 
> I've found problem. Print button on "red" paymets works well. But on "green"
> files open blank page. Please see video: http://g.recordit.co/shRFPo3XB9.gif

On further review of your gif, it seems pretty clear that you don't have the slip configured. I think there was an sql error in the old patch set that has been corrected in this new one!
Comment 20 Michal Denar 2019-04-24 13:40:17 UTC
Created attachment 88622 [details] [review]
Bug 22761: Add ability to use print version of ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF" slip

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 21 Michal Denar 2019-04-24 13:40:21 UTC
Created attachment 88623 [details] [review]
Bug 22761: Add notice based on existing template

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 22 Michal Denar 2019-04-24 13:40:24 UTC
Created attachment 88624 [details] [review]
Bug 22761: Remove hard coded version from template

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 23 Kyle M Hall 2019-04-24 14:33:34 UTC
Created attachment 88629 [details] [review]
Bug 22761: Add ability to use print version of ACCOUNT_CREDIT slip

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2019-04-24 14:33:44 UTC
Created attachment 88630 [details] [review]
Bug 22761: Add notice based on existing template

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2019-04-24 14:33:48 UTC
Created attachment 88631 [details] [review]
Bug 22761: Remove hard coded version from template

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2019-04-24 14:34:19 UTC
Forgot to update the patch title when I changed the letter code.
Comment 27 Jonathan Druart 2019-04-29 13:28:44 UTC
Comment on attachment 88629 [details] [review]
Bug 22761: Add ability to use print version of ACCOUNT_CREDIT slip

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/printfeercpt.tt
@@ +19,5 @@
>  
>  <div id="receipt">
> +
> +[% IF letter %]
> +    [% letter.content | evaltt %]

evaltt?

::: members/printfeercpt.pl
@@ +83,2 @@
>  
>  $template->param(

There are more parameters than before, I would expect less.
Is that correct?
Comment 28 Katrin Fischer 2019-04-30 05:58:33 UTC
(In reply to Jonathan Druart from comment #27)
> Comment on attachment 88629 [details] [review] [review]
> Bug 22761: Add ability to use print version of ACCOUNT_CREDIT slip
> 
> Review of attachment 88629 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/printfeercpt.tt
> @@ +19,5 @@
> >  
> >  <div id="receipt">
> > +
> > +[% IF letter %]
> > +    [% letter.content | evaltt %]
> 
> evaltt?

I've found this: http://www.template-toolkit.org/docs/manual/Filters.html#section_eval_evaltt

I assume that letter.content contains TT markup in that case?

 
> ::: members/printfeercpt.pl
> @@ +83,2 @@
> >  
> >  $template->param(
> 
> There are more parameters than before, I would expect less.
> Is that correct?

Should we move this to failed QA to get a quick answer from Kyle or can we continue here?
Comment 29 Kyle M Hall 2019-04-30 11:08:33 UTC
(In reply to Katrin Fischer from comment #28)
> (In reply to Jonathan Druart from comment #27)
> > Comment on attachment 88629 [details] [review] [review] [review]
> > Bug 22761: Add ability to use print version of ACCOUNT_CREDIT slip
> > 
> > Review of attachment 88629 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/printfeercpt.tt
> > @@ +19,5 @@
> > >  
> > >  <div id="receipt">
> > > +
> > > +[% IF letter %]
> > > +    [% letter.content | evaltt %]
> > 
> > evaltt?
> 
> I've found this:
> http://www.template-toolkit.org/docs/manual/Filters.html#section_eval_evaltt
> 
> I assume that letter.content contains TT markup in that case?
> 
>  
> > ::: members/printfeercpt.pl
> > @@ +83,2 @@
> > >  
> > >  $template->param(
> > 
> > There are more parameters than before, I would expect less.
> > Is that correct?
> 
> Should we move this to failed QA to get a quick answer from Kyle or can we
> continue here?

There are more parameters because I made the slip compatible with the existing syntax for the ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF slips.
Comment 30 Kyle M Hall 2019-04-30 11:37:53 UTC
> I've found this:
> http://www.template-toolkit.org/docs/manual/Filters.html#section_eval_evaltt
> 
> I assume that letter.content contains TT markup in that case?

You are correct!
Comment 31 Martin Renvoize 2019-05-02 11:29:16 UTC
Created attachment 89222 [details] [review]
Bug 22761: Add ability to use print version of ACCOUNT_CREDIT slip

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2019-05-02 11:29:19 UTC
Created attachment 89223 [details] [review]
Bug 22761: Add notice based on existing template

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2019-05-02 11:29:23 UTC
Created attachment 89224 [details] [review]
Bug 22761: Remove hard coded version from template

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2019-05-02 11:29:26 UTC
Created attachment 89225 [details] [review]
Bug 22761: (QA follow-up) Add notice translations

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2019-05-02 11:29:58 UTC
Works well for me, no regressions found and the changes all make sense.

Passing QA
Comment 36 Martin Renvoize 2019-05-02 11:34:24 UTC
Created attachment 89227 [details] [review]
Bug 22761: (QA follow-up) Add description to DB update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Nick Clemens 2019-05-10 14:53:13 UTC
Awesome work all!

Pushed to master for 19.05
Comment 38 Jonathan Druart 2019-05-10 17:59:38 UTC
Some notes about this:
1. The TT content has been moved but without being adjusted.
For instance:
- Should not we use library.branchname instead of LibraryName (otherwise we will have to pass 'LibraryName' to all our templates)?
- Branches.GetName( patron.branchcode ), should not we prefer patron.library.branchname?

If we do not know what is sent/used in the template, it will be a nightmare to provide backward compatibility in next versions.

2. About evaltt, I really do not understand why we need it, the template has been parsed already, we should not need to parse it again, I think the content must be  displayed with $raw

3. +    library     => C4::Context::mybranch,
so library will be the branchcode, whereas we are expecting a Koha::Library object

(In reply to Kyle M Hall from comment #29)
> > > ::: members/printfeercpt.pl
> > > @@ +83,2 @@
> > > >  
> > > >  $template->param(
> > > 
> > > There are more parameters than before, I would expect less.
> > > Is that correct?
> There are more parameters because I made the slip compatible with the
> existing syntax for the ACCOUNT_PAYMENT/ACCOUNT_WRITEOFF slips.

I am sorry but I still do not understand that part. The variable we do not see in the template will not be used, so for instance 'credit' is useless.
Comment 39 Jonathan Druart 2019-05-10 18:23:16 UTC
GetPreparedLetter is actually not called, hence the confusion.
Is there a good reason not to?
Comment 40 Michal Denar 2019-05-13 12:18:19 UTC
*** Bug 17551 has been marked as a duplicate of this bug. ***
Comment 41 Martin Renvoize 2019-05-17 13:33:11 UTC
Enhancement will not be backported to 18.11.x series
Comment 42 Martin Renvoize 2019-07-19 06:39:43 UTC
*** Bug 23325 has been marked as a duplicate of this bug. ***