Description
Katrin Fischer
2020-01-08 21:37:09 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) 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? 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. I'm sure this used to work... 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.. 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. 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 (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. I need some more of the other accounts bugs to move before I can delegate time to this I'm afraid. 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.
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. Created attachment 106947 [details] [review] Bug 24381: Update sample notices This patch update the sample notices for new installs 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... 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. Created attachment 106951 [details] [review] Bug 24381: Update sample notices This patch update the sample notices for new installs Created attachment 106952 [details] [review] Bug 23481: Update notices if defaults used Created attachment 106953 [details] [review] Bug 24381: Update notices if defaults used Created attachment 106956 [details] [review] Bug 25998: Add 'library' relation to Koha::Account::Line 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. Created attachment 106958 [details] [review] Bug 24381: Update sample notices This patch update the sample notices for new installs Created attachment 106959 [details] [review] Bug 24381: Update notices if defaults used 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 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. Created attachment 106975 [details] [review] Bug 24381: Update sample notices This patch update the sample notices for new installs Created attachment 106976 [details] [review] Bug 24381: Update notices if defaults used 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 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 Martin, can you please rebase? 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. Created attachment 110211 [details] [review] Bug 24381: Update sample notices This patch update the sample notices for new installs Created attachment 110212 [details] [review] Bug 24381: Update notices if defaults used 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 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> 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> Created attachment 110463 [details] [review] Bug 24381: Update notices if defaults used Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> 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? (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. 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> 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> Created attachment 111532 [details] [review] Bug 24381: Update notices if defaults used Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> Created attachment 111534 [details] [review] Bug 24381: (follow-up) Notice improvements 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 :( 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. 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. 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> 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> Created attachment 111592 [details] [review] Bug 24381: Update notices if defaults used Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> Created attachment 111594 [details] [review] Bug 24381: (follow-up) Notice improvements 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. 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. People seemed to generally lose interest in this.. :(.. the work moved to bug 26734 to split it into sensible parts as the code here ended up very customised for each case instead of using a more generalised patturn. Created attachment 116394 [details] [review] Bug 24381: Add payout_amount method to Koha::Account Patch doesn't apply to current master Created attachment 140868 [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> Created attachment 140869 [details] [review] Bug 24381: Update notices if defaults used Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 140870 [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> Created attachment 140871 [details] [review] Bug 24381: (follow-up) Notice improvements Created attachment 140872 [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. Created attachment 140873 [details] [review] Bug 24381: (follow-up) Add USE statements I dropped the first patch, it seemed to have been done already, updated the others slightly, worked in brief testing It appears like there is a problem with the rebase here. I think this block is now being repeated: my $htdocs = C4::Context->config('intrahtdocs'); my ($theme, $lang, $activethemes)= C4::Templates::themelanguage( $htdocs, 'about.tt', 'intranet'); my @includes; foreach (@$activethemes) { push @includes, "$htdocs/$_/$lang/includes"; push @includes, "$htdocs/$_/en/includes" unless $lang eq 'en'; } The QA tool complains about that: FAIL C4/Letters.pm FAIL valid "my" variable $htdocs masks earlier declaration in same scope "my" variable $theme masks earlier declaration in same scope "my" variable $lang masks earlier declaration in same scope "my" variable @includes masks earlier declaration in same scope "my" variable $htdocs masks earlier declaration in same scope "my" variable $theme masks earlier declaration in same scope "my" variable $lang masks earlier declaration in same scope "my" variable @includes masks earlier declaration in same scope I've obsoleted another of the patches that Nick rebased as it's already done in bug 26734... This is another case of a bug loosing momentum due to a distinct lack of testing at the time and I'm struggling to catch back up again.. All we need at this point is the new notices.. all the core code stuff is in... Also, with bug 26689 we can finally split these notices up so they're specific to their credit_type_code and debit_type_code.. so the notices submitted here with various cases for the different types built into the one notice don't really make much sense now.. it's much clearer to have a notice per type instead. The patchset should now work as it stands.. but personally I'd split the notices up to remove the `[% IF ( credit.credit_type_code == 'PAYMENT' ) %]"` and similar blocks and instead put them in new notices 'CREDIT_PAYMENT' for example. Either way.. give it a test and see if you like seeing the itemised summaries.. Created attachment 141619 [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> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 141620 [details] [review] Bug 24381: Update notices if defaults used Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 141621 [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> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 141622 [details] [review] Bug 24381: (follow-up) Notice improvements Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 141623 [details] [review] Bug 24381: (follow-up) Add USE statements Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Thanks for this one Martin. It will make a lot of librarians happy. :) Created attachment 142386 [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> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 142387 [details] [review] Bug 24381: Update notices if defaults used Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 142388 [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> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 142389 [details] [review] Bug 24381: (follow-up) Notice improvements Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 142390 [details] [review] Bug 24381: (follow-up) Add USE statements Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to master for 22.11. Nice work everyone, thanks! Patchset won't apply cleanly to 22.05.x, no backport |