|
Description
Martin Renvoize (ashimema)
2020-10-19 08:28:03 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 %]) 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. 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. Created attachment 111978 [details] [review] Bug 26734: Update sample notices to use standardised syntax 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. 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. Created attachment 112025 [details] [review] Bug 26734: Update sample notices to use standardised syntax 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. This still applies.. be nice to get it off my list. 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?
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 (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 :) 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. 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. 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. Created attachment 116261 [details] [review] Bug 26734: Update sample notices to use standardised syntax 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. 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. (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 :) 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. 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> 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> 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> Created attachment 116277 [details] [review] Bug 26734: Update sample notices to use standardised syntax Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> 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> 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 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? (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. 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. 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 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 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. 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' 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> 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> 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> 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> 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> 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. 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> 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> 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> 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> 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> (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. 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> 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> 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> 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> 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> Marcel, can you QA this one please? (In reply to Jonathan Druart from comment #52) > Marcel, can you QA this one please? Will have a look probably tomorrow QAing No license in pos/pay.pl Unusual copyright in registers.pl # c 2020 PTFS-Europe Ltd The abbreviation pos in unintuitive. 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.
(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) 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> 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> 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> 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> 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> Created attachment 118196 [details] [review] Bug 26734: (QA follow-up) Add missing copyright statement 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 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 :) PS: I had to rebase to get things to apply again.. hence the full re-upload. 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 ;) 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. QAing 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> 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> 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> 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> 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> 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> 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> 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> 1. We are not adjusting the installer translated files 2. We are not adjusting notice templates for existing installations How bad is that? Redoing bug 14233, I dropped: koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt:[% CASE 'UPGRADE' %]Upgrade It didn't belong there. (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.. 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 119358 [details] [review] Bug 26734: (follow-up) Add back 'UPGRADE' type to logviewer I need more QA opinions about the upgrade step here. (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. Pushed to master for 21.05, thanks to everybody involved! This isn't actually pushed.. just the first commit got pushed by mistake (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. 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). (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. 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> 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. > 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 (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. Pushed to master for 21.05, thanks to everybody involved! (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 :) Enhancement not pushed to 20.11.x 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 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> Follow-up pushed to master. *** Bug 28304 has been marked as a duplicate of this bug. *** |