Bug 19578 - TT syntax for notices - There is no way to pre-process DB fields
Summary: TT syntax for notices - There is no way to pre-process DB fields
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17961
  Show dependency treegraph
 
Reported: 2017-11-03 18:40 UTC by Jonathan Druart
Modified: 2019-10-14 19:58 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19578: Remove MARC punctuation in notices (TT syntax) (3.96 KB, patch)
2017-11-06 18:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19578: Remove MARC punctuation in notices (TT syntax) (3.96 KB, patch)
2017-11-06 18:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19578: Remove MARC punctuation in notices (TT syntax) (6.57 KB, patch)
2018-04-03 13:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19578: Remove MARC punctuation in notices (TT syntax) (6.61 KB, patch)
2018-04-04 05:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19578: Remove MARC punctuation in notices (TT syntax) (6.69 KB, patch)
2018-04-04 10:28 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-11-03 18:40:33 UTC
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?
Comment 1 Marcel de Rooy 2017-11-06 07:34:26 UTC
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?
Comment 2 Kyle M Hall 2017-11-06 11:31:49 UTC
(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.
Comment 3 Marcel de Rooy 2017-11-06 11:37:10 UTC
(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 :)
Comment 4 Jonathan Druart 2017-11-06 18:36:13 UTC
Created attachment 68990 [details] [review]
Bug 19578: Remove MARC punctuation in notices (TT syntax)
Comment 5 Jonathan Druart 2017-11-06 18:41:43 UTC
Created attachment 68991 [details] [review]
Bug 19578: Remove MARC punctuation in notices (TT syntax)
Comment 6 Jonathan Druart 2017-11-06 18:43:30 UTC
Marcel, what do you think about this approach?
Comment 7 Marcel de Rooy 2017-11-07 13:10:05 UTC
(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?
Comment 8 Jonathan Druart 2017-11-07 13:33:23 UTC
(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.
Comment 9 Marcel de Rooy 2017-11-07 13:37:23 UTC
(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 ?
Comment 10 Jonathan Druart 2017-11-07 14:15:52 UTC
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 :)
Comment 11 Katrin Fischer 2018-04-01 12:22:25 UTC
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.
Comment 12 Jonathan Druart 2018-04-02 14:49:09 UTC
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 %]
----
Comment 13 Katrin Fischer 2018-04-03 05:52:36 UTC
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
Comment 14 Jonathan Druart 2018-04-03 13:25:45 UTC
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 %]
----
Comment 15 Jonathan Druart 2018-04-03 13:27:40 UTC
Katrin, sorry I have forgotten to add the new file (TT plugin) for the patch.
I have also fixed the POD issue.
Comment 16 Katrin Fischer 2018-04-04 05:46:09 UTC
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>
Comment 17 Kyle M Hall 2018-04-04 10:28:18 UTC
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>
Comment 18 Jonathan Druart 2018-04-04 19:07:42 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 19 Nick Clemens 2018-05-14 21:15:15 UTC
Pushed to stable for 17.11.06

Awesome work all!
Comment 20 Fridolin Somers 2018-05-22 10:38:17 UTC
Pushed to 17.05.x for v17.05.12