Jenkins fails (run 287) on a test in t/db_dependent/Letters/TemplateToolkit.t: With the historical syntax: # Your request for an article from tQYRS (c3Av58O0P5xkkIGu) has been canceled for the following reason: With the TT syntax: # Your request for an article from tQYRS_ (c3Av58O0P5xkkIGu) has been canceled for the following reason: The last character of the biblio's title has been removed because it's a punctuation character. It comes from: C4::Letters::_parseletter 893 $val =~ s/\p{P}$// if $val && $table=~/biblio/; The same replacement is done for patron's attributes too. With the TT syntax we retrieve the objects and send them to the template, without fetching the attributes. To restore this behaviour we could apply this regex on all the fields of the biblio objects, but it could be perf killer (we will process fields that we will certainly not used in the template). I thought about the TT filter we could apply on all the variables we display, but we will not know where come from the data, and we need to apply this change only for the biblio table. Any other ideas?
Should we identify the few fields this really is about? Like at least biblio.title. And then run the same substitutions in parseletter and the template?
(In reply to Jonathan Druart from comment #0) > Jenkins fails (run 287) on a test in > t/db_dependent/Letters/TemplateToolkit.t: > > With the historical syntax: > # Your request for an article from tQYRS (c3Av58O0P5xkkIGu) has been > canceled for the following reason: > > With the TT syntax: > # Your request for an article from tQYRS_ (c3Av58O0P5xkkIGu) has > been canceled for the following reason: > > The last character of the biblio's title has been removed because it's a > punctuation character. > It comes from: C4::Letters::_parseletter > 893 $val =~ s/\p{P}$// if $val && $table=~/biblio/; > > The same replacement is done for patron's attributes too. > > With the TT syntax we retrieve the objects and send them to the template, > without fetching the attributes. > To restore this behaviour we could apply this regex on all the fields of the > biblio objects, but it could be perf killer (we will process fields that we > will certainly not used in the template). > I thought about the TT filter we could apply on all the variables we > display, but we will not know where come from the data, and we need to apply > this change only for the biblio table. > > Any other ideas? I think there are two valid options: 1) Remove line 893 ( what is it's purpose? ) 2) Add a TT filter like you suggest. I think if we just apply it to any TT syntax that contains ".biblio" within TT tags ( something like /^\[%.*biblio\..*%\]$/ ) it would be safe.
(In reply to Kyle M Hall from comment #2) > 1) Remove line 893 ( what is it's purpose? ) Remove MARC punctuation that seems to end the sentence. too early :)
Created attachment 68990 [details] [review] Bug 19578: Remove MARC punctuation in notices (TT syntax)
Created attachment 68991 [details] [review] Bug 19578: Remove MARC punctuation in notices (TT syntax)
Marcel, what do you think about this approach?
(In reply to Jonathan Druart from comment #6) > Marcel, what do you think about this approach? Looks quite good. Is it possible to further reduce the number of fields in biblio and biblioitems to run this Remove_MARC_punctuation on in some pragmatic way?
(In reply to Marcel de Rooy from comment #7) > (In reply to Jonathan Druart from comment #6) > > Marcel, what do you think about this approach? > > Looks quite good. Is it possible to further reduce the number of fields in > biblio and biblioitems to run this Remove_MARC_punctuation on in some > pragmatic way? Yes, we could apply the regex only with a list of fields. But then the 2 syntaxes will no give the same results.
(In reply to Jonathan Druart from comment #8) > (In reply to Marcel de Rooy from comment #7) > > (In reply to Jonathan Druart from comment #6) > > > Marcel, what do you think about this approach? > > > > Looks quite good. Is it possible to further reduce the number of fields in > > biblio and biblioitems to run this Remove_MARC_punctuation on in some > > pragmatic way? > > Yes, we could apply the regex only with a list of fields. But then the 2 > syntaxes will no give the same results. Which can be solved too ?
I would not recommend to make them diverge and I would not recommend to keep maintaining the 2 syntaxes... So let's keep behaviour as it for now :)
I think it seems the safest bet to keep the current behavour and do the processing for all fields from biblio and biblioitems. Could you provide a test plan Jonathan? Not quite sure yet how to work with the TT syntax in notices.
Test plan: - Confirm that the new tests pass. That should be enough to confirm this change make sense. Test plan (manual): - Create a biblio with a title ending with a punctuation (like "with_punctuation_"), or any other fields of biblio/biblioitem - Generate a notice which will display this field (CHECKIN for instance) Use the historical syntax and the TT syntax, both should display the title without the punctuation character at the end CHECKIN historical: The following items have been checked in: ---- <<biblio.title>> ---- CHECKIN TT syntax: The following items have been checked in: ---- [% biblio.title %] ----
The tests don't pass for me :( kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Letters/TemplateToolkit.t t/db_dependent/Letters/TemplateToolkit.t .. ERROR PROCESSING TEMPLATE: plugin error - Remove_MARC_punctuation: plugin not found at t/db_dependent/Letters/TemplateToolkit.t line 104. # Looks like your test exited with 2 before it could output anything. t/db_dependent/Letters/TemplateToolkit.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 18/18 subtests Test Summary Report ------------------- t/db_dependent/Letters/TemplateToolkit.t (Wstat: 512 Tests: 0 Failed: 0) Non-zero exit status: 2 Parse errors: Bad plan. You planned 18 tests but ran 0. FAIL C4/Letters.pm OK critic OK forbidden patterns OK git manipulation FAIL pod empty =head3 *** ERROR: in file C4/Letters.pm FAIL pod coverage POD is missing for add_tt_filters POD is missing for getletter OK spelling OK valid
Created attachment 73585 [details] [review] Bug 19578: Remove MARC punctuation in notices (TT syntax) Jenkins fails (run 287) on a test in t/db_dependent/Letters/TemplateToolkit.t: With the historical syntax: # Your request for an article from tQYRS (c3Av58O0P5xkkIGu) has been canceled for the following reason: With the TT syntax: # Your request for an article from tQYRS_ (c3Av58O0P5xkkIGu) has been canceled for the following reason: The last character of the biblio's title has been removed because it's a punctuation character. It comes from: C4::Letters::_parseletter 893 $val =~ s/\p{P}$// if $val && $table=~/biblio/; The same replacement is done for patron's attributes too. Test plan: - Confirm that the new tests pass. That should be enough to confirm this change make sense. Test plan (manual): - Create a biblio with a title ending with a punctuation (like "with_punctuation_"), or any other fields of biblio/biblioitem - Generate a notice which will display this field (CHECKIN for instance) Use the historical syntax and the TT syntax, both should display the title without the punctuation character at the end CHECKIN historical: The following items have been checked in: ---- <<biblio.title>> ---- CHECKIN TT syntax: The following items have been checked in: ---- [% biblio.title %] ----
Katrin, sorry I have forgotten to add the new file (TT plugin) for the patch. I have also fixed the POD issue.
Created attachment 73596 [details] [review] Bug 19578: Remove MARC punctuation in notices (TT syntax) Jenkins fails (run 287) on a test in t/db_dependent/Letters/TemplateToolkit.t: With the historical syntax: # Your request for an article from tQYRS (c3Av58O0P5xkkIGu) has been canceled for the following reason: With the TT syntax: # Your request for an article from tQYRS_ (c3Av58O0P5xkkIGu) has been canceled for the following reason: The last character of the biblio's title has been removed because it's a punctuation character. It comes from: C4::Letters::_parseletter 893 $val =~ s/\p{P}$// if $val && $table=~/biblio/; The same replacement is done for patron's attributes too. Test plan: - Confirm that the new tests pass. That should be enough to confirm this change make sense. Test plan (manual): - Create a biblio with a title ending with a punctuation (like "with_punctuation_"), or any other fields of biblio/biblioitem - Generate a notice which will display this field (CHECKIN for instance) Use the historical syntax and the TT syntax, both should display the title without the punctuation character at the end CHECKIN historical: The following items have been checked in: ---- <<biblio.title>> ---- CHECKIN TT syntax: The following items have been checked in: ---- [% biblio.title %] ---- Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 73602 [details] [review] Bug 19578: Remove MARC punctuation in notices (TT syntax) Jenkins fails (run 287) on a test in t/db_dependent/Letters/TemplateToolkit.t: With the historical syntax: # Your request for an article from tQYRS (c3Av58O0P5xkkIGu) has been canceled for the following reason: With the TT syntax: # Your request for an article from tQYRS_ (c3Av58O0P5xkkIGu) has been canceled for the following reason: The last character of the biblio's title has been removed because it's a punctuation character. It comes from: C4::Letters::_parseletter 893 $val =~ s/\p{P}$// if $val && $table=~/biblio/; The same replacement is done for patron's attributes too. Test plan: - Confirm that the new tests pass. That should be enough to confirm this change make sense. Test plan (manual): - Create a biblio with a title ending with a punctuation (like "with_punctuation_"), or any other fields of biblio/biblioitem - Generate a notice which will display this field (CHECKIN for instance) Use the historical syntax and the TT syntax, both should display the title without the punctuation character at the end CHECKIN historical: The following items have been checked in: ---- <<biblio.title>> ---- CHECKIN TT syntax: The following items have been checked in: ---- [% biblio.title %] ---- Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 18.05, thanks to everybody involved!
Pushed to stable for 17.11.06 Awesome work all!
Pushed to 17.05.x for v17.05.12