|
Description
Kyle M Hall (khall)
2016-03-03 12:15:31 UTC
Created attachment 48618 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Created attachment 48811 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> CONFLICT (content): Merge conflict in misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl CONFLICT (content): Merge conflict in C4/Letters.pm Created attachment 55819 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Created attachment 55820 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Hi Kyle,
This patch does a bit more than advertise :)
1/ __MESSAGE_ID__ could be used everywhere, not only for phone notice. I think this should be covered by tests and documented somewhere (eventually added to the notice editor)
2/
+ if ( $use_tt ) {
+ $letter->{content} =~ s/__MESSAGE_ID__/$message_id/g;
+ print $OUT $letter->{content};
+ }
Don't you think it would be better to retrieve the notice instead of duplicating the substitution? It would be less error prone.
Btw, why does the script output something different if tt syntax is used?
3/ C4::Letters::_process_tt takes a new parameter, it should be covered by tests.
Created attachment 57931 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Created attachment 57933 [details] [review] Bug 15969 - Add unit test of substitution param Created attachment 57934 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Created attachment 57935 [details] [review] Bug 15969 - Fetch the message instead of substituting Created attachment 59923 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Created attachment 59924 [details] [review] Bug 15969 - Add unit test of substitution param Created attachment 59925 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Created attachment 59926 [details] [review] Bug 15969 - Fetch the message instead of substituting Created attachment 59937 [details] [review] Bug 15969 - Fetch the message instead of substituting Created attachment 59940 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Created attachment 59941 [details] [review] Bug 15969 - Fetch the message instead of substituting Created attachment 59942 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Created attachment 59943 [details] [review] Bug 15969 - Add unit test of substitution param Created attachment 59944 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Created attachment 59945 [details] [review] Bug 15969 - Fetch the message instead of substituting Created attachment 59962 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 59963 [details] [review] Bug 15969 - Add unit test of substitution param Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 59964 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 59965 [details] [review] Bug 15969 - Fetch the message instead of substituting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> I'd like to know what it the plan here, I have done a lot of work on bug 17981 to make the different notices work with the TT syntax. This patch will conflict with it. It would also be good to get an independent QA point of view. Applying: Bug 15969 - Add unit test of substitution param fatal: sha1 information is lacking or useless (C4/Letters.pm). Created attachment 71141 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 71142 [details] [review] Bug 15969 - Add unit test of substitution param Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 71143 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 71144 [details] [review] Bug 15969 - Fetch the message instead of substituting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Jonathan Druart from comment #27) > I'd like to know what it the plan here, I have done a lot of work on bug > 17981 to make the different notices work with the TT syntax. > This patch will conflict with it. > > It would also be good to get an independent QA point of view. Let's hold this one off until your bug is pushed to master. At that point I'll revisit this and make whatever changes are necessary to integrate them. It's not pushed to master yet, still in NQA. (In reply to Jonathan Druart from comment #7) > Hi Kyle, > This patch does a bit more than advertise :) > 1/ __MESSAGE_ID__ could be used everywhere, not only for phone notice. I > think this should be covered by tests and documented somewhere (eventually > added to the notice editor) Still valid. > 2/ > + if ( $use_tt ) { > + $letter->{content} =~ s/__MESSAGE_ID__/$message_id/g; > + print $OUT $letter->{content}; > + } > > Don't you think it would be better to retrieve the notice instead of > duplicating the substitution? It would be less error prone. > Btw, why does the script output something different if tt syntax is used? Still need an answer. Does the --tt flag really makes sense? (In reply to Jonathan Druart from comment #27) > It would also be good to get an independent QA point of view. Still needed. I will do it myself if no one else does. Changing status to BLOCKED as it depends on a "does not apply" bug Unblocking as dependency is passed QA! This sentence sounds a little odd to me: +OPTIONAL. If set, processes the notice content as Template Toolkit syntax and sends the content as the CSV line instead +of generated a fixed CSV line. Kyle, can you please answer qustions on comment#34? If you work on the patches, can you please also fix the commit messages (Bug XXXX:) Patch does not apply anymore sorry :( Created attachment 103181 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 103182 [details] [review] Bug 15969 - Add unit test of substitution param Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 103183 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 103184 [details] [review] Bug 15969 - Fetch the message instead of substituting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Turns out, as phone notices are not a thing here at all, I am not sure how this works. In which notice templates (letter codes) do I have to add the TT example code? Oh, and please fix the commit lines if you want to make me happy :) Hi Kyle, I tihnk I need a more detailed test plan here - I have failed to get it working with the existing instructions. How does the letter need to be set up in Koha? (letter code, HTML?, etc.) I was looking to take over on QA here, but the patches no longer apply, unfortunately :( Created attachment 114413 [details] [review] Bug 15969 - Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 114414 [details] [review] Bug 15969 - Add unit test of substitution param Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 114415 [details] [review] Bug 15969 - Unit test use of __MESSAGE_ID__ Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 114416 [details] [review] Bug 15969 - Fetch the message instead of substituting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Processing additional checks
* Commit title does not start with 'Bug XXXXX: ' - 28c2325c47
* Commit title does not start with 'Bug XXXXX: ' - 787cf5cda9
* Commit title does not start with 'Bug XXXXX: ' - 01aa2c4bb8
* Commit title does not start with 'Bug XXXXX: ' - ce321c3ad3
Created attachment 118207 [details] [review] Bug 15969: Allow use of Template Toolkit syntax for Phone Notices By allowing the use of Template Toolkit for generating the outgoing Talking Tech CSV files, we can give libaries more control over the data they send to Itiva. This enhancement would also make it possible to use the script for other services since the output is no longer fixed to a single format. Test Plan: 1) Apply this patch 2) Set up a waiting hold, and overdue checkout, and a non-overdue checkout 3) Generate the CSV file via the script misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl 4) Enable Template Toolkit for the notices and update the notice text to be: [% FILTER remove("\r\n") %] "[% format %]", "[% language %]", "[% type %]", "[% level %]", "[% borrower.cardnumber %]", "[% borrower.title %]", "[% borrower.firstname %]", "[% borrower.surname %]", "[% borrower.phone %]", "[% borrower.email %]", "[% library_code %]", "[% branch.id %]", "[% branch.branchname %]", "[% item.barcode %]", "[% due_date %]", "[% biblio.title %]", "__MESSAGE_ID__" [% END %] 5) Regenerate the notices 6) Compare the originals, note the format is the same Signed-off-by: Sean McGarvey <seanm@pascolibraries.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 118208 [details] [review] Bug 15969: Add unit test of substitution param Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 118209 [details] [review] Bug 15969: Unit test use of __MESSAGE_ID__ Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 118210 [details] [review] Bug 15969: Fetch the message instead of substituting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Katrin Fischer from comment #43) > Turns out, as phone notices are not a thing here at all, I am not sure how > this works. > > In which notice templates (letter codes) do I have to add the TT example > code? Still applies... Also: Test Summary Report ------------------- t/db_dependent/Letters/TemplateToolkit.t (Wstat: 512 Tests: 24 Failed: 1) Failed test: 24 Non-zero exit status: 2 Parse errors: Bad plan. You planned 27 tests but ran 24. Files=1, Tests=24, 6 wallclock secs ( 0.01 usr 0.02 sys + 4.27 cusr 1.13 csys = 5.43 CPU) Result: FAIL Possibly just a reabase error (wrong number) |