Bug 26734 - Convert accounts (monetary) notices to use GetPreparedLetter
Summary: Convert accounts (monetary) notices to use GetPreparedLetter
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
: 28304 (view as bug list)
Depends on: 14233
Blocks: 26689 28368 28374
  Show dependency treegraph
 
Reported: 2020-10-19 08:28 UTC by Martin Renvoize
Modified: 2021-05-28 12:21 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This enhancement updates the existing slip printing code to utilise the internal GetPreparedLetter function as our other notices do. This leads to code clarity improvements and consistency and is part of the path to migrating all notices to use template toolkit consistently as well as exposing more variables to the template system for notices. *WARNING*: We replace any existing notices for the monetary slips with a modern equivalent using template toolkit. Any customised templates will be recorded in the action logs and should be used as inspiration for updating the new slips to include your libraries customizations. Letter codes: ACCOUNT_CREDIT, ACCOUNT_DEBIT
Version(s) released in:
21.05.00


Attachments
Bug 26734: Ratify account slip printing (11.97 KB, patch)
2020-10-19 13:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (9.62 KB, patch)
2020-10-19 13:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (6.65 KB, patch)
2020-10-19 13:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (11.97 KB, patch)
2020-10-20 10:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (10.31 KB, patch)
2020-10-20 10:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (6.72 KB, patch)
2020-10-20 10:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (3.96 KB, patch)
2020-10-20 10:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.39 KB, patch)
2021-02-03 14:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (11.97 KB, patch)
2021-02-03 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (10.31 KB, patch)
2021-02-03 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.05 KB, patch)
2021-02-03 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (3.96 KB, patch)
2021-02-03 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.42 KB, patch)
2021-02-03 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.04 KB, patch)
2021-02-03 15:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.04 KB, patch)
2021-02-03 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (10.38 KB, patch)
2021-02-03 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.12 KB, patch)
2021-02-03 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (4.03 KB, patch)
2021-02-03 15:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.49 KB, patch)
2021-02-03 15:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.28 KB, patch)
2021-02-10 10:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (10.81 KB, patch)
2021-02-10 10:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.21 KB, patch)
2021-02-10 10:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (3.96 KB, patch)
2021-02-10 10:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.70 KB, patch)
2021-02-10 10:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.33 KB, patch)
2021-02-10 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (10.86 KB, patch)
2021-02-10 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.26 KB, patch)
2021-02-10 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (4.01 KB, patch)
2021-02-10 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.76 KB, patch)
2021-02-10 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.33 KB, patch)
2021-02-10 12:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (10.86 KB, patch)
2021-02-10 12:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.26 KB, patch)
2021-02-10 12:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (4.01 KB, patch)
2021-02-10 12:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.76 KB, patch)
2021-02-10 12:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.33 KB, patch)
2021-02-24 14:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (10.86 KB, patch)
2021-02-24 14:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.26 KB, patch)
2021-02-24 14:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (4.01 KB, patch)
2021-02-24 14:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.76 KB, patch)
2021-02-24 14:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.33 KB, patch)
2021-03-12 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (12.51 KB, patch)
2021-03-12 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.26 KB, patch)
2021-03-12 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (4.01 KB, patch)
2021-03-12 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.76 KB, patch)
2021-03-12 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Add missing copyright statement (1.16 KB, patch)
2021-03-12 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Unit tests for C4::Letters (2.03 KB, patch)
2021-03-12 13:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Add 'tendered' and 'change' to notice (2.23 KB, patch)
2021-03-12 14:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.45 KB, patch)
2021-03-19 07:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (12.64 KB, patch)
2021-03-19 07:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.37 KB, patch)
2021-03-19 07:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (4.11 KB, patch)
2021-03-19 07:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.86 KB, patch)
2021-03-19 07:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Add missing copyright statement (1.26 KB, patch)
2021-03-19 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Unit tests for C4::Letters (2.12 KB, patch)
2021-03-19 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Add 'tendered' and 'change' to notice (2.32 KB, patch)
2021-03-19 07:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26734: Ratify account slip printing (12.39 KB, patch)
2021-04-09 08:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter (12.56 KB, patch)
2021-04-09 08:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update sample notices to use standardised syntax (7.32 KB, patch)
2021-04-09 08:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Update existing notices to new syntax (4.07 KB, patch)
2021-04-09 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Replace db update with backup and default (8.81 KB, patch)
2021-04-09 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Add missing copyright statement (1.22 KB, patch)
2021-04-09 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Unit tests for C4::Letters (2.09 KB, patch)
2021-04-09 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Add 'tendered' and 'change' to notice (2.29 KB, patch)
2021-04-09 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (follow-up) Add back 'UPGRADE' type to logviewer (901 bytes, patch)
2021-04-09 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: (QA follow-up) Add warning to database update (1.09 KB, patch)
2021-04-15 15:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Add notices for translations (50.50 KB, patch)
2021-04-23 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26734: Renamed activethemes with _activethemes (1.67 KB, patch)
2021-05-27 12:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26734: [ALT] Renamed activethemes with availablethemes (2.67 KB, patch)
2021-05-27 12:58 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 Martin Renvoize 2020-10-19 08:28:03 UTC
The ACCOUNT_CREDIT and ACCOUNT_DEBIT print slips should be updated to TT syntax.
Comment 1 Martin Renvoize 2020-10-19 10:28:44 UTC
I mispoke, they already use TT, but they do not follow the usual variable names and they use getletter directly when they should really use GetPreparedLetter to ensure they consistently get the same variables as other TT notices. (Like [% today %])
Comment 2 Martin Renvoize 2020-10-19 13:01:47 UTC
Created attachment 111976 [details] [review]
Bug 26734: 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 3 Martin Renvoize 2020-10-19 13:01:53 UTC
Created attachment 111977 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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 4 Martin Renvoize 2020-10-19 13:01:57 UTC
Created attachment 111978 [details] [review]
Bug 26734: Update sample notices to use standardised syntax
Comment 5 Martin Renvoize 2020-10-20 10:53:08 UTC
Created attachment 112023 [details] [review]
Bug 26734: 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 6 Martin Renvoize 2020-10-20 10:53:12 UTC
Created attachment 112024 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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 7 Martin Renvoize 2020-10-20 10:53:16 UTC
Created attachment 112025 [details] [review]
Bug 26734: Update sample notices to use standardised syntax
Comment 8 Martin Renvoize 2020-10-20 10:53:19 UTC
Created attachment 112026 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.
Comment 9 Martin Renvoize 2021-02-02 16:13:40 UTC
This still applies.. be nice to get it off my list.
Comment 10 Jonathan Druart 2021-02-03 12:01:08 UTC
1.
 69         tendered => scalar $input->param('tendered'),

Looks like this var is not used in the notice template.

2.
 65         credits   => $credit_id,
 66         borrowers => $patron->borrowernumber

Should not we remove borrowers/[% patron %] and use credit.patron instead?

3.
+            - "[% PROCESS 'accounts.inc' %]"

There is no precedence for that (using an include file from a notice template).
Don't know if it's a good idea or not. Does it work with translation?
Comment 11 Kyle M Hall 2021-02-03 13:13:55 UTC
I'm getting:

ERROR PROCESSING TEMPLATE: file error - account_type_description: not found at /kohadevbox/koha/members/printfeercpt.pl line 58.
 at /usr/share/perl/5.24/Carp.pm line 167
Comment 12 Martin Renvoize 2021-02-03 13:40:53 UTC
(In reply to Jonathan Druart from comment #10)
> 1.
>  69         tendered => scalar $input->param('tendered'),
> 
> Looks like this var is not used in the notice template.

This is used in the subsequent bug that enhances the printed slip to include 'change' and 'tendered' amounts (These are both in our local slips for customers)

> 
> 2.
>  65         credits   => $credit_id,
>  66         borrowers => $patron->borrowernumber
> 
> Should not we remove borrowers/[% patron %] and use credit.patron instead?

Whilst I agree with this and liked the idea. Katrin muted it saying that the 'patron' (or rather 'borrowers') should always be available at the top level to allow for simple syntax for the end-users writing slips.  I think this is debatable.. but that's why it's there in the first place ;)

> 
> 3.
> +            - "[% PROCESS 'accounts.inc' %]"
> 
> There is no precedence for that (using an include file from a notice
> template).
> Don't know if it's a good idea or not. Does it work with translation?

It's actually there precisely for translation purposes.. though I need to test it.. I'm pretty sure we pass 'lang' through but whether we pass it correctly to refer to the right translated path I'm not sure.. I should take a look, thanks :)
Comment 13 Martin Renvoize 2021-02-03 14:50:27 UTC
Created attachment 116257 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.
Comment 14 Martin Renvoize 2021-02-03 15:00:12 UTC
Created attachment 116259 [details] [review]
Bug 26734: 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 15 Martin Renvoize 2021-02-03 15:00:15 UTC
Created attachment 116260 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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 16 Martin Renvoize 2021-02-03 15:00:18 UTC
Created attachment 116261 [details] [review]
Bug 26734: Update sample notices to use standardised syntax
Comment 17 Martin Renvoize 2021-02-03 15:00:22 UTC
Created attachment 116262 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.
Comment 18 Martin Renvoize 2021-02-03 15:00:25 UTC
Created attachment 116263 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.
Comment 19 Martin Renvoize 2021-02-03 15:02:13 UTC
(In reply to Martin Renvoize from comment #12)

> It's actually there precisely for translation purposes.. though I need to
> test it.. I'm pretty sure we pass 'lang' through but whether we pass it
> correctly to refer to the right translated path I'm not sure.. I should take
> a look, thanks :)

Tested.. I did indeed bake in handling for translations properly :)
Comment 20 Martin Renvoize 2021-02-03 15:03:14 UTC
Final patch changes the database update to be more reliable.. instead of attempting to update modified notices inline, we now take a backup copy to the action_logs and then replace the entire notice with the default.

This should resolve Kyles issue.
Comment 21 Kyle M Hall 2021-02-03 15:21:44 UTC
Created attachment 116272 [details] [review]
Bug 26734: 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 22 Kyle M Hall 2021-02-03 15:39:46 UTC
Created attachment 116275 [details] [review]
Bug 26734: 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 23 Kyle M Hall 2021-02-03 15:39:55 UTC
Created attachment 116276 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2021-02-03 15:39:58 UTC
Created attachment 116277 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2021-02-03 15:40:00 UTC
Created attachment 116278 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2021-02-03 15:40:04 UTC
Created attachment 116279 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Joonas Kylmälä 2021-02-10 09:37:09 UTC
Martin, could you please provide a test plan / what things are most likely to break and should be tested carefully. This is quite big refactoring so I feel like things are gonna break.
Comment 28 Joonas Kylmälä 2021-02-10 10:09:59 UTC
Comment on attachment 116276 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

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

::: C4/Letters.pm
@@ +1526,5 @@
>  
> +    my $htdocs = C4::Context->config('intrahtdocs');
> +    my ($theme, $lang, $activethemes)= C4::Templates::themelanguage( $htdocs, 'about.tt', 'intranet');
> +    my @includes;
> +    foreach (@$activethemes) {

Why are we adding these includes here?
Comment 29 Jonathan Druart 2021-02-10 10:20:35 UTC
(In reply to Joonas Kylmälä from comment #28)
> Comment on attachment 116276 [details] [review] [review]
> Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter
> 
> Review of attachment 116276 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Letters.pm
> @@ +1526,5 @@
> >  
> > +    my $htdocs = C4::Context->config('intrahtdocs');
> > +    my ($theme, $lang, $activethemes)= C4::Templates::themelanguage( $htdocs, 'about.tt', 'intranet');
> > +    my @includes;
> > +    foreach (@$activethemes) {
> 
> Why are we adding these includes here?

See comment 10 point 3. It will pick the translated include files.
Comment 30 Martin Renvoize 2021-02-10 10:47:14 UTC
Created attachment 116644 [details] [review]
Bug 26734: Ratify account slip printing

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

Test plan
Slips should continue to print as they did prior to the patch
1/ Add a manual invoice and manual debit to a patrons account
2/ Print a slip for the manual invoice, compare to a printed slip prior
to the patch.
3/ Print a slip for the manual debit, compare to a printed slip prior to
the patch.
4/ Signoff.
Comment 31 Martin Renvoize 2021-02-10 10:47:17 UTC
Created attachment 116645 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Test plan
1/ Update the ACCOUNT_CREDIT and ACCOUNT_DEBIT notices to use
'accounts.inc' include for account descriptions
1a/ Add `[% PROCESS 'accounts.inc' %]` to the top of the notice
1b/ Replace manual account descriptions with `[% PROCESS
account_type_description account=credit %]`
2/ Print one of each slip and check that descriptions are properly
substituted
3/ Clone the notice to another language
4/ Print the in the other language and note the translated descriptions
are properly substituted.
5/ Signoff
Comment 32 Martin Renvoize 2021-02-10 10:47:21 UTC
Created attachment 116646 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

This patch updates the default notices shiped with Koha to use the new
syntax.

Test plan
1/ Install Koha afresh
2/ Check the notices print as expected
3/ Signoff
Comment 33 Martin Renvoize 2021-02-10 10:47:25 UTC
Created attachment 116647 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.
Comment 34 Martin Renvoize 2021-02-10 10:47:29 UTC
Created attachment 116648 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Test plan
1/ Run the database update
2/ Check that the templates have been replaced by the defaults (for all
languages enabled)
3/ Check that a backup of the original notices was made by naviagating
to view the action log (/tools/viewlog.pl) and selecting 'all modules' +
'all actions'
Comment 35 Martin Renvoize 2021-02-10 12:45:42 UTC
Created attachment 116655 [details] [review]
Bug 26734: Ratify account slip printing

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

Test plan
Slips should continue to print as they did prior to the patch
1/ Add a manual invoice and manual debit to a patrons account
2/ Print a slip for the manual invoice, compare to a printed slip prior
to the patch.
3/ Print a slip for the manual debit, compare to a printed slip prior to
the patch.
4/ Signoff.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Martin Renvoize 2021-02-10 12:45:46 UTC
Created attachment 116656 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Test plan
1/ Update the ACCOUNT_CREDIT and ACCOUNT_DEBIT notices to use
'accounts.inc' include for account descriptions
1a/ Add `[% PROCESS 'accounts.inc' %]` to the top of the notice
1b/ Replace manual account descriptions with `[% PROCESS
account_type_description account=credit %]`
2/ Print one of each slip and check that descriptions are properly
substituted
3/ Clone the notice to another language
4/ Print the in the other language and note the translated descriptions
are properly substituted.
5/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 37 Martin Renvoize 2021-02-10 12:45:49 UTC
Created attachment 116657 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

This patch updates the default notices shiped with Koha to use the new
syntax.

Test plan
1/ Install Koha afresh
2/ Check the notices print as expected
3/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Martin Renvoize 2021-02-10 12:45:52 UTC
Created attachment 116658 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Martin Renvoize 2021-02-10 12:45:56 UTC
Created attachment 116659 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Test plan
1/ Run the database update
2/ Check that the templates have been replaced by the defaults (for all
languages enabled)
3/ Check that a backup of the original notices was made by naviagating
to view the action log (/tools/viewlog.pl) and selecting 'all modules' +
'all actions'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Martin Renvoize 2021-02-10 12:48:15 UTC
Restore the signoff lines I inadvertently wiped out whilst adding a proper test plan to each patch.

I've also slightly updated the final patch to accommodate the changes made by adding bug 14233 as a dependency.  You should now be able to filter to the right logs actions for notices more easily.
Comment 41 Martin Renvoize 2021-02-10 12:56:35 UTC
Created attachment 116663 [details] [review]
Bug 26734: Ratify account slip printing

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

Test plan
Slips should continue to print as they did prior to the patch
1/ Add a manual invoice and manual debit to a patrons account
2/ Print a slip for the manual invoice, compare to a printed slip prior
to the patch.
3/ Print a slip for the manual debit, compare to a printed slip prior to
the patch.
4/ Signoff.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Martin Renvoize 2021-02-10 12:56:38 UTC
Created attachment 116664 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Test plan
1/ Update the ACCOUNT_CREDIT and ACCOUNT_DEBIT notices to use
'accounts.inc' include for account descriptions
1a/ Add `[% PROCESS 'accounts.inc' %]` to the top of the notice
1b/ Replace manual account descriptions with `[% PROCESS
account_type_description account=credit %]`
2/ Print one of each slip and check that descriptions are properly
substituted
3/ Clone the notice to another language
4/ Print the in the other language and note the translated descriptions
are properly substituted.
5/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Martin Renvoize 2021-02-10 12:56:42 UTC
Created attachment 116665 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

This patch updates the default notices shiped with Koha to use the new
syntax.

Test plan
1/ Install Koha afresh
2/ Check the notices print as expected
3/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Martin Renvoize 2021-02-10 12:56:45 UTC
Created attachment 116666 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 45 Martin Renvoize 2021-02-10 12:56:49 UTC
Created attachment 116667 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Test plan
1/ Run the database update
2/ Check that the templates have been replaced by the defaults (for all
languages enabled)
3/ Check that a backup of the original notices was made by naviagating
to view the action log (/tools/viewlog.pl) and selecting 'all modules' +
'all actions'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Martin Renvoize 2021-02-10 14:57:36 UTC
(In reply to Joonas Kylmälä from comment #27)
> Martin, could you please provide a test plan / what things are most likely
> to break and should be tested carefully. This is quite big refactoring so I
> feel like things are gonna break.

Test plans added to patches as discussed... I've also now made it dependant upon bug 14233 which adds a nicer way to filter the action logs for Notices as well as implementing the 'diff' view. Would love some feedback on that one.. I imagine at the very least people will want it put behind a system preference on top of what I've already implimented.
Comment 47 Martin Renvoize 2021-02-24 14:02:58 UTC
Created attachment 117261 [details] [review]
Bug 26734: Ratify account slip printing

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

Test plan
Slips should continue to print as they did prior to the patch
1/ Add a manual invoice and manual debit to a patrons account
2/ Print a slip for the manual invoice, compare to a printed slip prior
to the patch.
3/ Print a slip for the manual debit, compare to a printed slip prior to
the patch.
4/ Signoff.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 48 Martin Renvoize 2021-02-24 14:03:01 UTC
Created attachment 117262 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Test plan
1/ Update the ACCOUNT_CREDIT and ACCOUNT_DEBIT notices to use
'accounts.inc' include for account descriptions
1a/ Add `[% PROCESS 'accounts.inc' %]` to the top of the notice
1b/ Replace manual account descriptions with `[% PROCESS
account_type_description account=credit %]`
2/ Print one of each slip and check that descriptions are properly
substituted
3/ Clone the notice to another language
4/ Print the in the other language and note the translated descriptions
are properly substituted.
5/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 49 Martin Renvoize 2021-02-24 14:03:05 UTC
Created attachment 117263 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

This patch updates the default notices shiped with Koha to use the new
syntax.

Test plan
1/ Install Koha afresh
2/ Check the notices print as expected
3/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 50 Martin Renvoize 2021-02-24 14:03:08 UTC
Created attachment 117264 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 51 Martin Renvoize 2021-02-24 14:03:12 UTC
Created attachment 117265 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Test plan
1/ Run the database update
2/ Check that the templates have been replaced by the defaults (for all
languages enabled)
3/ Check that a backup of the original notices was made by naviagating
to view the action log (/tools/viewlog.pl) and selecting 'all modules' +
'all actions'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Jonathan Druart 2021-03-11 08:26:08 UTC
Marcel, can you QA this one please?
Comment 53 Marcel de Rooy 2021-03-11 09:32:46 UTC
(In reply to Jonathan Druart from comment #52)
> Marcel, can you QA this one please?

Will have a look probably tomorrow
Comment 54 Marcel de Rooy 2021-03-12 09:12:23 UTC
QAing
Comment 55 Marcel de Rooy 2021-03-12 09:33:23 UTC
No license in pos/pay.pl
Unusual copyright in registers.pl
# c 2020 PTFS-Europe Ltd
The abbreviation pos in unintuitive.
Comment 56 Marcel de Rooy 2021-03-12 09:56:09 UTC
TODO Missing a test for change in Letters.pm ?

+    ($table eq 'credits'      )    ? "SELECT * FROM accountlines WHERE   accountlines_id = ?"                         :
+    ($table eq 'debits'       )    ? "SELECT * FROM accountlines WHERE   accountlines_id = ?"                         :
Not consistent

+        tendered => scalar $input->param('tendered'),
+        change   => scalar $input->param('change')
Please clarify these two parameters?

After reading patch 4 I thought: Atomic update is arguable. Will this work on adjusted notices? Not sure.
Patch 5 addressed this concern.

Changing status for feedback/follow-up.
Comment 57 Martin Renvoize 2021-03-12 13:55:26 UTC
(In reply to Marcel de Rooy from comment #56)
> 
> +        tendered => scalar $input->param('tendered'),
> +        change   => scalar $input->param('change')
> Please clarify these two parameters?
> 

>(In reply to Jonathan Druart from comment #10)
>> 1.
>>  69         tendered => scalar $input->param('tendered'),
>> 
>> Looks like this var is not used in the notice template.
>
>
>This is used in the subsequent bug that enhances the printed slip to include >'change' and 'tendered' amounts (These are both in our local slips for customers)
Comment 58 Martin Renvoize 2021-03-12 13:56:17 UTC
Created attachment 118191 [details] [review]
Bug 26734: Ratify account slip printing

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

Test plan
Slips should continue to print as they did prior to the patch
1/ Add a manual invoice and manual debit to a patrons account
2/ Print a slip for the manual invoice, compare to a printed slip prior
to the patch.
3/ Print a slip for the manual debit, compare to a printed slip prior to
the patch.
4/ Signoff.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 59 Martin Renvoize 2021-03-12 13:56:22 UTC
Created attachment 118192 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Test plan
1/ Update the ACCOUNT_CREDIT and ACCOUNT_DEBIT notices to use
'accounts.inc' include for account descriptions
1a/ Add `[% PROCESS 'accounts.inc' %]` to the top of the notice
1b/ Replace manual account descriptions with `[% PROCESS
account_type_description account=credit %]`
2/ Print one of each slip and check that descriptions are properly
substituted
3/ Clone the notice to another language
4/ Print the in the other language and note the translated descriptions
are properly substituted.
5/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 60 Martin Renvoize 2021-03-12 13:56:26 UTC
Created attachment 118193 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

This patch updates the default notices shiped with Koha to use the new
syntax.

Test plan
1/ Install Koha afresh
2/ Check the notices print as expected
3/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 61 Martin Renvoize 2021-03-12 13:56:30 UTC
Created attachment 118194 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 62 Martin Renvoize 2021-03-12 13:56:34 UTC
Created attachment 118195 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Test plan
1/ Run the database update
2/ Check that the templates have been replaced by the defaults (for all
languages enabled)
3/ Check that a backup of the original notices was made by naviagating
to view the action log (/tools/viewlog.pl) and selecting 'all modules' +
'all actions'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 63 Martin Renvoize 2021-03-12 13:56:38 UTC
Created attachment 118196 [details] [review]
Bug 26734: (QA follow-up) Add missing copyright statement
Comment 64 Martin Renvoize 2021-03-12 13:56:42 UTC
Created attachment 118197 [details] [review]
Bug 26734: (QA follow-up) Unit tests for C4::Letters

This patch adds unit tests for the addition of 'include handling' to the
C4::Letters template toolkit functionality.

Test plan
1/ Run t/db_dependant/Letters/TemplateToolkit.t
Comment 65 Martin Renvoize 2021-03-12 13:59:36 UTC
Add a couple of followups as requested.. one adds the missing copyright statement.. not sure how that was missed prior to now.. oops.

I wasn't sure what you meant by 'Unusual copyright in registers.pl'.. doesn't it follow the usual form?

Second followup adds tests for the addition of include file handling in template toolkit based notices. (This gives us nice descriptions for account line types along with supporting translations of those descriptions is the usual way used in templates elsewhere in Koha).

Thanks for testing Marcel :)
Comment 66 Martin Renvoize 2021-03-12 14:02:07 UTC
PS: I had to rebase to get things to apply again.. hence the full re-upload.
Comment 67 Martin Renvoize 2021-03-12 14:23:13 UTC
Created attachment 118200 [details] [review]
Bug 26734: (QA follow-up) Add 'tendered' and 'change' to notice

We include 'tendered' and 'change' parameters in the call to
GetPreparedLetter for the fee receipt slip.. we may as well include them
in the sample notice ;)
Comment 68 Martin Renvoize 2021-03-12 14:24:08 UTC
I decided you were both right.. why leave the tendered/change bits to another bug.. they make sense to be used in the sample notice here.
Comment 69 Marcel de Rooy 2021-03-19 07:13:01 UTC
QAing
Comment 70 Marcel de Rooy 2021-03-19 07:25:44 UTC
Created attachment 118482 [details] [review]
Bug 26734: Ratify account slip printing

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

Test plan
Slips should continue to print as they did prior to the patch
1/ Add a manual invoice and manual debit to a patrons account
2/ Print a slip for the manual invoice, compare to a printed slip prior
to the patch.
3/ Print a slip for the manual debit, compare to a printed slip prior to
the patch.
4/ Signoff.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 71 Marcel de Rooy 2021-03-19 07:25:48 UTC
Created attachment 118483 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Test plan
1/ Update the ACCOUNT_CREDIT and ACCOUNT_DEBIT notices to use
'accounts.inc' include for account descriptions
1a/ Add `[% PROCESS 'accounts.inc' %]` to the top of the notice
1b/ Replace manual account descriptions with `[% PROCESS
account_type_description account=credit %]`
2/ Print one of each slip and check that descriptions are properly
substituted
3/ Clone the notice to another language
4/ Print the in the other language and note the translated descriptions
are properly substituted.
5/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2021-03-19 07:25:51 UTC
Created attachment 118484 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

This patch updates the default notices shiped with Koha to use the new
syntax.

Test plan
1/ Install Koha afresh
2/ Check the notices print as expected
3/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 73 Marcel de Rooy 2021-03-19 07:25:55 UTC
Created attachment 118485 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Marcel de Rooy 2021-03-19 07:25:59 UTC
Created attachment 118486 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Test plan
1/ Run the database update
2/ Check that the templates have been replaced by the defaults (for all
languages enabled)
3/ Check that a backup of the original notices was made by naviagating
to view the action log (/tools/viewlog.pl) and selecting 'all modules' +
'all actions'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 75 Marcel de Rooy 2021-03-19 07:26:03 UTC
Created attachment 118487 [details] [review]
Bug 26734: (QA follow-up) Add missing copyright statement

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 76 Marcel de Rooy 2021-03-19 07:26:07 UTC
Created attachment 118488 [details] [review]
Bug 26734: (QA follow-up) Unit tests for C4::Letters

This patch adds unit tests for the addition of 'include handling' to the
C4::Letters template toolkit functionality.

Test plan
1/ Run t/db_dependant/Letters/TemplateToolkit.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 77 Marcel de Rooy 2021-03-19 07:26:10 UTC
Created attachment 118489 [details] [review]
Bug 26734: (QA follow-up) Add 'tendered' and 'change' to notice

We include 'tendered' and 'change' parameters in the call to
GetPreparedLetter for the fee receipt slip.. we may as well include them
in the sample notice ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 78 Jonathan Druart 2021-04-07 11:06:58 UTC
1. We are not adjusting the installer translated files
2. We are not adjusting notice templates for existing installations

How bad is that?
Comment 79 Jonathan Druart 2021-04-07 12:59:07 UTC
Redoing bug 14233, I dropped:
koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt:[%        CASE 'UPGRADE'     %]Upgrade

It didn't belong there.
Comment 80 Martin Renvoize 2021-04-07 13:01:56 UTC
(In reply to Jonathan Druart from comment #78)
> 1. We are not adjusting the installer translated files
> 2. We are not adjusting notice templates for existing installations
> 
> How bad is that?

I tried hard to adjust the existing notice templates but found it was fraught with error.. hence the move to 'replace' the template but log the change and the entirely of bug 14233 coming out of it so you can see the logged notice change.

I'm pretty sure there's discussion around this above and we came round to thinking this was the cleanest way forward rather than risking breaking peoples notices entirely.. this way they get the modern notice and a record of their original which they can use as inspiration for adapting their customisations back into the modern version.

As for the translations.. good question.. I think I was hoping the yaml stuff might have come to fruition so was leaving that for now..
Comment 81 Martin Renvoize 2021-04-09 08:25:47 UTC
Created attachment 119350 [details] [review]
Bug 26734: Ratify account slip printing

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

Test plan
Slips should continue to print as they did prior to the patch
1/ Add a manual invoice and manual debit to a patrons account
2/ Print a slip for the manual invoice, compare to a printed slip prior
to the patch.
3/ Print a slip for the manual debit, compare to a printed slip prior to
the patch.
4/ Signoff.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 82 Martin Renvoize 2021-04-09 08:25:52 UTC
Created attachment 119351 [details] [review]
Bug 26734: Convert printfeercpt/printinvoice to use GetPreparedLetter

This patch updates C4::Letters to allow use of existing koha template
includes from notices, then updates the printfeercpt and printinvoice
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.

Test plan
1/ Update the ACCOUNT_CREDIT and ACCOUNT_DEBIT notices to use
'accounts.inc' include for account descriptions
1a/ Add `[% PROCESS 'accounts.inc' %]` to the top of the notice
1b/ Replace manual account descriptions with `[% PROCESS
account_type_description account=credit %]`
2/ Print one of each slip and check that descriptions are properly
substituted
3/ Clone the notice to another language
4/ Print the in the other language and note the translated descriptions
are properly substituted.
5/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 83 Martin Renvoize 2021-04-09 08:25:56 UTC
Created attachment 119352 [details] [review]
Bug 26734: Update sample notices to use standardised syntax

This patch updates the default notices shiped with Koha to use the new
syntax.

Test plan
1/ Install Koha afresh
2/ Check the notices print as expected
3/ Signoff

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 84 Martin Renvoize 2021-04-09 08:26:01 UTC
Created attachment 119353 [details] [review]
Bug 26734: Update existing notices to new syntax

This patch adds a database update to replace existing notice variable
used in ACCOUNT_CREDIT and ACCOUNT_DEBIT notices with the modern
equivilents.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 85 Martin Renvoize 2021-04-09 08:26:05 UTC
Created attachment 119354 [details] [review]
Bug 26734: (follow-up) Replace db update with backup and default

The inline update of variable for the notice template was proving
troublesome.. this patch replaces it was a backup of the existing notice
followed by replacing the notice with the default.

Test plan
1/ Run the database update
2/ Check that the templates have been replaced by the defaults (for all
languages enabled)
3/ Check that a backup of the original notices was made by naviagating
to view the action log (/tools/viewlog.pl) and selecting 'all modules' +
'all actions'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 86 Martin Renvoize 2021-04-09 08:26:10 UTC
Created attachment 119355 [details] [review]
Bug 26734: (QA follow-up) Add missing copyright statement

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 87 Martin Renvoize 2021-04-09 08:26:15 UTC
Created attachment 119356 [details] [review]
Bug 26734: (QA follow-up) Unit tests for C4::Letters

This patch adds unit tests for the addition of 'include handling' to the
C4::Letters template toolkit functionality.

Test plan
1/ Run t/db_dependant/Letters/TemplateToolkit.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 88 Martin Renvoize 2021-04-09 08:26:19 UTC
Created attachment 119357 [details] [review]
Bug 26734: (QA follow-up) Add 'tendered' and 'change' to notice

We include 'tendered' and 'change' parameters in the call to
GetPreparedLetter for the fee receipt slip.. we may as well include them
in the sample notice ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 89 Martin Renvoize 2021-04-09 08:26:23 UTC
Created attachment 119358 [details] [review]
Bug 26734: (follow-up) Add back 'UPGRADE' type to logviewer
Comment 90 Jonathan Druart 2021-04-09 14:02:25 UTC
I need more QA opinions about the upgrade step here.
Comment 91 Joonas Kylmälä 2021-04-09 14:36:44 UTC
(In reply to Jonathan Druart from comment #90)
> I need more QA opinions about the upgrade step here.

You mean the breakage of the letter templates? Only other solution I see here is two maintain both template syntaxes in parallel for few releases and deprecate the old one. It sounds tricky solution as well. For our use case breaking this feature after the upgrade until librarian has fixed the situation is OK because the upgrade could be done at a time when the library is still closed and a superlibrarian can go update the credit/debit receipt templates before it is opened. This breakage needs to be widely communicated though so that people can prepare the new templates ahead of time and it won't be a surprise.
Comment 92 Jonathan Druart 2021-04-12 13:32:23 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 93 Martin Renvoize 2021-04-14 13:17:39 UTC
This isn't actually pushed.. just the first commit got pushed by mistake
Comment 94 Jonathan Druart 2021-04-14 13:22:45 UTC
(In reply to Jonathan Druart from comment #92)
> Pushed to master for 21.05, thanks to everybody involved!

Sorry, wrong manip from my side. Only the first patch has been pushed and I didn't mean it (forgot to remove one line from a rebase -i apparently). Will revert in a bit.
Comment 95 Jonathan Druart 2021-04-14 13:41:49 UTC
Reverted, sorry!

So, to clarify, I am still not sure we should push this patchset as it.

Maybe we should 1. Add a warning during the update DB and 2. provide the necessary changes for the translated notices (even if some may be deleted when 21.05 is released).
Comment 96 Joonas Kylmälä 2021-04-14 14:30:04 UTC
(In reply to Jonathan Druart from comment #95)
> Maybe we should 1. Add a warning during the update DB and 2. provide the
> necessary changes for the translated notices (even if some may be deleted
> when 21.05 is released).

I think adding warning is a good idea, I always look those logs when doing updates for anything unexpected and things that need manual intervention.
Comment 97 Martin Renvoize 2021-04-15 15:32:52 UTC
Created attachment 119642 [details] [review]
Bug 26734: (QA follow-up) Add warning to database update

This patch adds a warning to the database update to highlight to the
user which notices have been updated and that a backup exists.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Martin Renvoize 2021-04-23 11:51:42 UTC
Created attachment 120077 [details] [review]
Bug 26734: Add notices for translations

This patch updates the translated sample_notices.sql files for each of
the remaining translations.
Comment 99 Katrin Fischer 2021-04-25 10:34:36 UTC
> So, to clarify, I am still not sure we should push this patchset as it.
> 
> Maybe we should 1. Add a warning during the update DB and 2. provide the
> necessary changes for the translated notices (even if some may be deleted
> when 21.05 is released).

Better late than... I'd also add my voice that making these slips more flexible is a good thing. Breaking changes are always a hard choice. I think they need really good advertising, but sometimes cannot be avoided in a good way.

Release notes notes should be updated I think to have more of a warning sign. 

And it would be great to have some documentation for the new templates/changes to look out for. I think these are the wiki pages, that could be good to add to:

https://wiki.koha-community.org/wiki/Notices_with_Template_Toolkit (especially last section)

And here: https://wiki.koha-community.org/wiki/Notices_and_Slips_Library#Notices_and_slips_using_template_toolkit
Comment 100 Jonathan Druart 2021-04-27 14:24:54 UTC
(In reply to Katrin Fischer from comment #99)
> And it would be great to have some documentation for the new
> templates/changes to look out for. I think these are the wiki pages, that
> could be good to add to:
> 
> https://wiki.koha-community.org/wiki/Notices_with_Template_Toolkit
> (especially last section)
> 
> And here:
> https://wiki.koha-community.org/wiki/
> Notices_and_Slips_Library#Notices_and_slips_using_template_toolkit

Martin, can you deal with that please? Adding the "additional_work_needed", remove it when done.
Comment 101 Jonathan Druart 2021-04-27 14:25:33 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 102 Martin Renvoize 2021-04-27 15:14:59 UTC
(In reply to Jonathan Druart from comment #100)
> (In reply to Katrin Fischer from comment #99)
> > And it would be great to have some documentation for the new
> > templates/changes to look out for. I think these are the wiki pages, that
> > could be good to add to:
> > 
> > https://wiki.koha-community.org/wiki/Notices_with_Template_Toolkit
> > (especially last section)
> > 
> > And here:
> > https://wiki.koha-community.org/wiki/
> > Notices_and_Slips_Library#Notices_and_slips_using_template_toolkit
> 
> Martin, can you deal with that please? Adding the "additional_work_needed",
> remove it when done.

All done :)
Comment 103 Fridolin Somers 2021-04-30 09:00:26 UTC
Enhancement not pushed to 20.11.x
Comment 104 Jonathan Druart 2021-05-27 12:27:21 UTC
Created attachment 121446 [details] [review]
Bug 26734: Renamed activethemes with _activethemes

There was an existing call to $self->activethemes that behave as setter:
   85     $self->activethemes($activethemes);

I am not sure what are the side-effects but it does not look good.

It caused warning in the logs:

Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 279.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 280.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 282.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296
Comment 105 Martin Renvoize 2021-05-27 12:58:25 UTC
Created attachment 121449 [details] [review]
Bug 26734: [ALT] Renamed activethemes with availablethemes

There was an existing call to $self->activethemes that behave as setter:
   85     $self->activethemes($activethemes);

I am not sure what are the side-effects but it does not look good.

It caused warning in the logs:

Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 279.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 280.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 282.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 106 Jonathan Druart 2021-05-27 13:10:37 UTC
Follow-up pushed to master.
Comment 107 Jonathan Druart 2021-05-28 12:21:16 UTC
*** Bug 28304 has been marked as a duplicate of this bug. ***