Bug 24381 - ACCOUNT_CREDIT and ACCOUNT_DEBIT slip not printing information about paid fines/fees
Summary: ACCOUNT_CREDIT and ACCOUNT_DEBIT slip not printing information about paid fin...
Status: BLOCKED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 25998 26398 26689 22761 22809
Blocks: 13985 14825
  Show dependency treegraph
 
Reported: 2020-01-08 21:37 UTC by Katrin Fischer
Modified: 2020-10-20 13:29 UTC (History)
10 users (show)

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


Attachments
Bug 24381: Ratify account slip printing (12.10 KB, patch)
2020-07-16 11:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update sample notices (11.64 KB, patch)
2020-07-16 11:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Ratify account slip printing (12.10 KB, patch)
2020-07-16 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update sample notices (15.84 KB, patch)
2020-07-16 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23481: Update notices if defaults used (11.88 KB, patch)
2020-07-16 13:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notices if defaults used (11.93 KB, patch)
2020-07-16 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25998: Add 'library' relation to Koha::Account::Line (942 bytes, patch)
2020-07-16 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Ratify account slip printing (12.10 KB, patch)
2020-07-16 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update sample notices (15.84 KB, patch)
2020-07-16 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notices if defaults used (11.93 KB, patch)
2020-07-16 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notice params (3.36 KB, patch)
2020-07-16 16:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Ratify account slip printing (12.10 KB, patch)
2020-07-17 07:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update sample notices (15.84 KB, patch)
2020-07-17 07:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notices if defaults used (11.93 KB, patch)
2020-07-17 07:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notice params (3.36 KB, patch)
2020-07-17 07:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Ratify account slip printing (11.99 KB, patch)
2020-09-16 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update sample notices (15.84 KB, patch)
2020-09-16 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notices if defaults used (11.93 KB, patch)
2020-09-16 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notice params (3.36 KB, patch)
2020-09-16 14:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Ratify account slip printing (12.06 KB, patch)
2020-09-21 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24381: Update sample notices (15.91 KB, patch)
2020-09-21 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24381: Update notices if defaults used (12.00 KB, patch)
2020-09-21 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24381: Update notice params (3.43 KB, patch)
2020-09-21 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24381: Ratify account slip printing (12.04 KB, patch)
2020-10-13 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update sample notices (15.90 KB, patch)
2020-10-13 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notices if defaults used (11.99 KB, patch)
2020-10-13 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notice params (3.42 KB, patch)
2020-10-13 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: (follow-up) Notice improvements (13.53 KB, patch)
2020-10-13 11:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Ratify account slip printing (12.04 KB, patch)
2020-10-14 11:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update sample notices (15.90 KB, patch)
2020-10-14 11:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notices if defaults used (11.99 KB, patch)
2020-10-14 11:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Update notice params (3.42 KB, patch)
2020-10-14 11:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: (follow-up) Notice improvements (13.56 KB, patch)
2020-10-14 11:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24381: Convert printfeercpt to use GetPreparedLetter (6.33 KB, patch)
2020-10-14 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2020-01-08 21:37:09 UTC
While the templates suggests it should print the fines/fees paid/changed by the payment, this doesn't work for me on current master.
Comment 2 Katrin Fischer 2020-01-08 21:52:23 UTC
I think what we should have in the table for each:

ACCOUNT_CREDIT
Fee receipt - list of fines paid (print is on payment line)

ACCOUNT_DEBIT
Invoice - list of payments made (print is on fine/fee)
Comment 3 Jonathan Druart 2020-01-14 11:17:06 UTC
I do not understand, can you explain a bit more please? Or maybe attach a screenshot?

I see the Print button for each line, the letter displays only 1 line. Do you expect all the lines to be displayed? In that case the button should be moved somewhere else?
Comment 4 Katrin Fischer 2020-01-14 11:48:20 UTC
It might get clearer if we start with the payment receipts printed from a 'payment' line.

At the moment, it will have:

- a table with the column heading: Description of charges
- Below always 1 line only with "Payment"
- And of course... there is not much to total

What I think we should have, and we can already see this in the 'details' for the payment and in the sample notice for PAYMENT notices:

- a table with the column heading: Description of charges
- A list of the fines the payment paid off, so something like:
  - Rental fee X
  - Fine for ...
- The total of the payment

I've tried to use the TT syntax from the payment emails, but couldn't get it to work.
Comment 5 Martin Renvoize 2020-01-14 14:52:42 UTC
I'm sure this used to work...
Comment 6 Martin Renvoize 2020-01-14 15:14:30 UTC
Right.. I think I know what you guys want... but I'm pretty confident it wasn't different in 18.11 unless I'm missing a bug somewhere.. the conversion from hard coded to a notices template did not change the output going back to my notes..
Comment 7 Martin Renvoize 2020-01-14 15:17:45 UTC
Now that bug 23354 has been pushed to master, could you take a look at the template included for a RECEIPT slipt for the Point Of Sale receipts and confirm that would conform to the data you're looking for in the default slip here too..

If so, I can create a patch to correct that for new installs.. I think the reason we didn't add such a slip during the change to slips was to preserve existing receipt look and feel during upgrades.
Comment 8 Martin Renvoize 2020-01-14 15:22:25 UTC
Whilst we're here.. can someone explain to me why we need 4 distinct notices for this?

ACCOUNT_CREDIT - printfeercpt
ACCOUNT_DEBIT - printinvoice
ACCOUNT_PAYMENT - What is this for?
ACCOUNT_WRITEOFF - What is this for?

RECEIPT - pos/printreceipt
Comment 9 Katrin Fischer 2020-01-14 21:12:58 UTC
(In reply to Martin Renvoize from comment #8)
> Whilst we're here.. can someone explain to me why we need 4 distinct notices
> for this?

Interesting question!
 
> ACCOUNT_CREDIT - printfeercpt
> ACCOUNT_DEBIT - printinvoice

These are for the slips. I have to say I keep confusing debit and credit, but i think:

CREDIT = Payment
DEBIT = Invoice

> ACCOUNT_PAYMENT - What is this for? 
> ACCOUNT_WRITEOFF - What is this for?

Sending emails to the patron, similar to checkout/checkin slips. It was a major step forward as at that time the slips were still not customizable. - see UseEmailReceipts system preference.

> RECEIPT - pos/printreceipt

I think there is a good precedence to have different templates for printed slips, so that makes sense to me. Also while we could probably combine payment, writeoff notice and invoice notice are different things.

I have not idea about RECEIPT yet - but this one you should know about ;)

We have tested ACCOUNT_ emails and they do what we need on the slips. I'd prefere if they all shared the same names for objects/variables or this will be really messy to train and explain.
Comment 10 Martin Renvoize 2020-02-07 14:19:07 UTC
I need some more of the other accounts bugs to move before I can delegate time to this I'm afraid.
Comment 11 Martin Renvoize 2020-07-15 14:03:59 UTC
OK, coming back to this..

>We have tested ACCOUNT_ emails and they do what we need on the slips. I'd prefere if they all shared the same names for objects/variables or this will be really messy to train and explain.

I agree.. it would be great to unify the syntax's and have the default template all take a similar form... the issue with this to date has been backwards compatibility...

I'm now considering how we can move to a consistent approach without having huge amounts of redundant code for backward compatibility.. any suggestions greatly welcomed.
Comment 12 Martin Renvoize 2020-07-16 11:10:38 UTC
Created attachment 106946 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.
Comment 13 Martin Renvoize 2020-07-16 11:10:42 UTC
Created attachment 106947 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs
Comment 14 Martin Renvoize 2020-07-16 11:14:56 UTC
So.. this works really nicely for new installs now and the code is significantly cleaner in my opinion.. but I need to now work out how to support backward compatibility with older notices...
Comment 15 Martin Renvoize 2020-07-16 13:37:23 UTC
Created attachment 106950 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.
Comment 16 Martin Renvoize 2020-07-16 13:37:27 UTC
Created attachment 106951 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs
Comment 17 Martin Renvoize 2020-07-16 13:37:30 UTC
Created attachment 106952 [details] [review]
Bug 23481: Update notices if defaults used
Comment 18 Martin Renvoize 2020-07-16 14:05:42 UTC
Created attachment 106953 [details] [review]
Bug 24381: Update notices if defaults used
Comment 19 Martin Renvoize 2020-07-16 16:03:40 UTC
Created attachment 106956 [details] [review]
Bug 25998: Add 'library' relation to Koha::Account::Line
Comment 20 Martin Renvoize 2020-07-16 16:03:44 UTC
Created attachment 106957 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.
Comment 21 Martin Renvoize 2020-07-16 16:03:48 UTC
Created attachment 106958 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs
Comment 22 Martin Renvoize 2020-07-16 16:03:51 UTC
Created attachment 106959 [details] [review]
Bug 24381: Update notices if defaults used
Comment 23 Martin Renvoize 2020-07-16 16:03:56 UTC
Created attachment 106960 [details] [review]
Bug 24381: Update notice params

If the update to catch defaults fails, we need to attempt to update
renamed template variables
Comment 24 Martin Renvoize 2020-07-17 07:04:08 UTC
Created attachment 106974 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.
Comment 25 Martin Renvoize 2020-07-17 07:04:12 UTC
Created attachment 106975 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs
Comment 26 Martin Renvoize 2020-07-17 07:04:16 UTC
Created attachment 106976 [details] [review]
Bug 24381: Update notices if defaults used
Comment 27 Martin Renvoize 2020-07-17 07:04:20 UTC
Created attachment 106977 [details] [review]
Bug 24381: Update notice params

If the update to catch defaults fails, we need to attempt to update
renamed template variables
Comment 28 Martin Renvoize 2020-07-17 07:09:01 UTC
Test plan

Fresh Install
1/ Install a koha fresh and allow the sample notices to be installed
2/ Create some debits on a patrons account
3/ 'Print' the debit and verify the print works and looks reasonable
4/ Create some credits on a patrons account
5/ 'Print' the credit and verify the print works and looks reasonable
6/ Apply a credit against some debits
7/ Print both 'credit' and 'debit' and confirm the receipts look reasonable

Upgrade (With default notices)
1/ Perform an upgrade where the notices were still set to their defaults prior to upgrade
2/ Check that the notices have been upgraded to the new defaults

Upgrade (With modified notices)
1/ Perform an upgrade where the notices have been modified locally
2/ Check that the notice customisations are intact
3/ Check that the notices still print correctly
Comment 29 Katrin Fischer 2020-09-16 06:20:45 UTC
Martin, can you please rebase?
Comment 30 Martin Renvoize 2020-09-16 14:33:21 UTC
Created attachment 110210 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.
Comment 31 Martin Renvoize 2020-09-16 14:33:27 UTC
Created attachment 110211 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs
Comment 32 Martin Renvoize 2020-09-16 14:33:33 UTC
Created attachment 110212 [details] [review]
Bug 24381: Update notices if defaults used
Comment 33 Martin Renvoize 2020-09-16 14:33:42 UTC
Created attachment 110213 [details] [review]
Bug 24381: Update notice params

If the update to catch defaults fails, we need to attempt to update
renamed template variables
Comment 34 Kyle M Hall 2020-09-21 12:56:38 UTC
Created attachment 110461 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Kyle M Hall 2020-09-21 12:56:51 UTC
Created attachment 110462 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Kyle M Hall 2020-09-21 12:56:57 UTC
Created attachment 110463 [details] [review]
Bug 24381: Update notices if defaults used

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 37 Kyle M Hall 2020-09-21 12:57:02 UTC
Created attachment 110464 [details] [review]
Bug 24381: Update notice params

If the update to catch defaults fails, we need to attempt to update
renamed template variables

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Joonas Kylmälä 2020-10-06 10:26:07 UTC
Before this patch on koha-testing-docker if I add two manual fees and click on the print button on either one of the fees then it shows that fee as itemized. After applying this patch, running updatedatabase and restart_all I only see the total amount and not details for this fee. I think this is not expected, after this patch all the fee lines are supposed to be shown, right?
Comment 39 Martin Renvoize 2020-10-12 16:44:09 UTC
(In reply to Joonas Kylmälä from comment #38)
> Before this patch on koha-testing-docker if I add two manual fees and click
> on the print button on either one of the fees then it shows that fee as
> itemized. After applying this patch, running updatedatabase and restart_all
> I only see the total amount and not details for this fee. I think this is
> not expected, after this patch all the fee lines are supposed to be shown,
> right?

You are quit right.. seems I didn't test the cases where we haven't actually applied any credits against the debt.. or vica versa.  Those don't looked great right now.

I'm working on a better notice template to replace it with.
Comment 40 Martin Renvoize 2020-10-13 11:31:35 UTC
Created attachment 111530 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Martin Renvoize 2020-10-13 11:31:40 UTC
Created attachment 111531 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Martin Renvoize 2020-10-13 11:31:49 UTC
Created attachment 111532 [details] [review]
Bug 24381: Update notices if defaults used

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Martin Renvoize 2020-10-13 11:31:55 UTC
Created attachment 111533 [details] [review]
Bug 24381: Update notice params

If the update to catch defaults fails, we need to attempt to update
renamed template variables

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Martin Renvoize 2020-10-13 11:32:00 UTC
Created attachment 111534 [details] [review]
Bug 24381: (follow-up) Notice improvements
Comment 45 Martin Renvoize 2020-10-13 11:37:42 UTC
To Test

The updated templates are used for a plethora of different cases and I'd like some feedback as to whether they're an improvement for each.

ACCOUNT_DEBIT
* This one is used for accountlines of debit_type_code 'ACCOUNT', 'ACCOUNT_RENEW', 'LOST', 'MANUAL', 'NEW_CARD', 'OVERDUE', 'PROCESSING', 'RENT', 'RENT_DAILY', 'RENT_RENEW', 'RENT_DAILY_RENEW', 'RESERVE', 'RESERVE_EXPIRED', 'PAYOUT' and any other locally added debit types.  Other than the 'PAYOUT' type, which I believe may warrant it's own template entirely I'm reasonably happy.  I am, however interested in feedback regarding how they look prior to and post the debt being paid (partially, fully in one and fully over multiple payments)

ACCOUNT_CREDIT
* This one is used for accountlines of credit_type_code 'PAYMENT', 'WRITEOFF', 'FORGIVEN', 'CREDIT', 'LOST_FOUND', 'REFUND' and any other locally added credit types. Again, I'm interested in feedback regarding how they look before and after they have been applied to debts.

ACCOUNT_PAYMENT
* I can't remember what this one is used for :(
Comment 46 Joonas Kylmälä 2020-10-14 08:12:27 UTC
Martin, the changes you made to the template in the last patch are quite huge in my opinion, if you don't mind I would like to set this to Need Signoff to have someone review the last patch before having the QA team check for final time.
Comment 47 Martin Renvoize 2020-10-14 08:48:10 UTC
As an additional note, whilst working on the templates I noticed that the [% today %] substitution doesn't work in this notice as it does elsewhere..

After a discussion with Jonathan, we noted that these print slips do not go via GetPreparedLetter and rather call the private getletter function directly and then pass the resultant template into a process_tt block in the print template.

We deceded we aught to work out a better way to handle this.
Comment 48 Martin Renvoize 2020-10-14 11:04:25 UTC
Created attachment 111590 [details] [review]
Bug 24381: Ratify account slip printing

This patch simplifies the members/print*.pl controller scripts to only
pass around the minimum required data for the templates.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 49 Martin Renvoize 2020-10-14 11:04:31 UTC
Created attachment 111591 [details] [review]
Bug 24381: Update sample notices

This patch update the sample notices for new installs

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 50 Martin Renvoize 2020-10-14 11:04:37 UTC
Created attachment 111592 [details] [review]
Bug 24381: Update notices if defaults used

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 51 Martin Renvoize 2020-10-14 11:04:46 UTC
Created attachment 111593 [details] [review]
Bug 24381: Update notice params

If the update to catch defaults fails, we need to attempt to update
renamed template variables

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Martin Renvoize 2020-10-14 11:04:55 UTC
Created attachment 111594 [details] [review]
Bug 24381: (follow-up) Notice improvements
Comment 53 Martin Renvoize 2020-10-14 11:05:05 UTC
Created attachment 111595 [details] [review]
Bug 24381: Convert printfeercpt to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt slip print option
to use GetPreparedLetter rather than calling getletter directly.

As part of this work, we also add credits and debits handling to the
_parseletter_sth and _get_tt_params routines in C4::Letters to allow for
recognisable variable names in the notice template.
Comment 54 Martin Renvoize 2020-10-14 11:06:41 UTC
OK, back to NSO.. that final followup is a proof of concept for converting the first slip to use GetPreparedLetter as aposed to calling getletter directly and then parting the evaluating the tt from inside the template.