Bug 19456 - Some pages title tag contains html
Summary: Some pages title tag contains html
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Josef Moravec
QA Contact: Kyle M Hall
URL:
Keywords:
: 19491 19553 (view as bug list)
Depends on: 14039
Blocks: 18403
  Show dependency treegraph
 
Reported: 2017-10-13 05:59 UTC by Josef Moravec
Modified: 2018-12-03 20:04 UTC (History)
9 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
Bug19456: Make patron-title ability to be generated with or without html tags (20.21 KB, patch)
2017-10-13 06:51 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug19456: Make patron-title ability to be generated with or without html tags (20.27 KB, patch)
2017-10-13 14:26 UTC, Simon Pouchol
Details | Diff | Splinter Review
Bug 19456: Make patron-title ability to be generated with or without html tags (20.27 KB, patch)
2017-10-27 13:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19456: Make patron-title ability to be generated with or without html tags (12.88 KB, patch)
2017-10-27 19:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19456: Make patron-title ability to be generated with or without html tags (12.88 KB, patch)
2017-10-27 19:54 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19456: Make patron-title ability to be generated with or without html tags (12.99 KB, patch)
2017-11-13 16:54 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2017-10-13 05:59:17 UTC
This comes from patron-title.inc
Comment 1 Josef Moravec 2017-10-13 06:51:33 UTC
Created attachment 68044 [details] [review]
Bug19456: Make patron-title ability to be generated with or without html tags

Modified pages:
circ/circulation.pl
circ/circulation_batch_checkouts.pl
members/boraccount.pl
members/files.pl
members/holdshistory.pl
members/housebound.pl
members/moremember.pl
members/notices.pl
members/purchase-suggestions.pl
members/readingrec.pl
members/routing-lists.pl
members/statistics.pl
tools/viewlog.pl

Test plan:
0) Do not apply the patch
1) Have a patron with title/salution filled in
2) Confirm bug, go for example to circ/circulation page and see there is
html in <title> tag (you can see it in your browser page/window title)
3) Apply the patch
4) Go through circulation/patron pages (see modified page above) and
confirm there is no html in <title> tag, but on the page itself the
salutation should be in <span class="patron-title">
Comment 2 Simon Pouchol 2017-10-13 14:26:29 UTC
Created attachment 68081 [details] [review]
Bug19456: Make patron-title ability to be generated with or without html tags

Modified pages:
circ/circulation.pl
circ/circulation_batch_checkouts.pl
members/boraccount.pl
members/files.pl
members/holdshistory.pl
members/housebound.pl
members/moremember.pl
members/notices.pl
members/purchase-suggestions.pl
members/readingrec.pl
members/routing-lists.pl
members/statistics.pl
tools/viewlog.pl

Test plan:
0) Do not apply the patch
1) Have a patron with title/salution filled in
2) Confirm bug, go for example to circ/circulation page and see there is
html in <title> tag (you can see it in your browser page/window title)
3) Apply the patch
4) Go through circulation/patron pages (see modified page above) and
confirm there is no html in <title> tag, but on the page itself the
salutation should be in <span class="patron-title">

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>
Comment 3 Aleisha Amohia 2017-10-18 22:00:59 UTC
*** Bug 19491 has been marked as a duplicate of this bug. ***
Comment 4 Kyle M Hall 2017-10-27 13:28:43 UTC
Created attachment 68702 [details] [review]
Bug 19456: Make patron-title ability to be generated with or without html tags

Modified pages:
circ/circulation.pl
circ/circulation_batch_checkouts.pl
members/boraccount.pl
members/files.pl
members/holdshistory.pl
members/housebound.pl
members/moremember.pl
members/notices.pl
members/purchase-suggestions.pl
members/readingrec.pl
members/routing-lists.pl
members/statistics.pl
tools/viewlog.pl

Test plan:
0) Do not apply the patch
1) Have a patron with title/salution filled in
2) Confirm bug, go for example to circ/circulation page and see there is
html in <title> tag (you can see it in your browser page/window title)
3) Apply the patch
4) Go through circulation/patron pages (see modified page above) and
confirm there is no html in <title> tag, but on the page itself the
salutation should be in <span class="patron-title">

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>
Comment 5 Jonathan Druart 2017-10-27 14:43:53 UTC
1/ QA script detected a missing %
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt
   FAIL   valid_template
                parse error - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt line 109: unexpected token (])
  [% INCLUDE 'patron-title.inc' invert_name = 1 use_html = 1 ] [% END %]


2/ So basically we want use_html everywhere but in title tags, right?
If this is correct, I would prefer to do the reverse: add a do_not_use_html flag when patron-title.inc is used in title tags. That could be easily caught with `git grep patron-title.inc|grep '<title>'`
Comment 6 Josef Moravec 2017-10-27 19:13:37 UTC
Created attachment 68787 [details] [review]
Bug 19456: Make patron-title ability to be generated with or without html tags

Modified pages:
circ/circulation.pl
circ/circulation_batch_checkouts.pl
members/boraccount.pl
members/files.pl
members/holdshistory.pl
members/housebound.pl
members/moremember.pl
members/notices.pl
members/purchase-suggestions.pl
members/readingrec.pl
members/routing-lists.pl
members/statistics.pl

Test plan:
0) Do not apply the patch
1) Have a patron with title/salution filled in
2) Confirm bug, go for example to circ/circulation page and see there is
html in <title> tag (you can see it in your browser page/window title)
3) Apply the patch
4) Go through circulation/patron pages (see modified page above) and
confirm there is no html in <title> tag, but on the page itself the
salutation should be in <span class="patron-title">
Comment 7 Josef Moravec 2017-10-27 19:14:35 UTC
(In reply to Jonathan Druart from comment #5)
> 1/ QA script detected a missing %
>  FAIL  
> koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation_batch_checkouts.tt
>    FAIL   valid_template
>                 parse error -
> /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/circ/
> circulation_batch_checkouts.tt line 109: unexpected token (])
>   [% INCLUDE 'patron-title.inc' invert_name = 1 use_html = 1 ] [% END %]
> 

Fixed in new patch, now it passes qa tools

> 
> 2/ So basically we want use_html everywhere but in title tags, right?
> If this is correct, I would prefer to do the reverse: add a do_not_use_html
> flag when patron-title.inc is used in title tags. That could be easily
> caught with `git grep patron-title.inc|grep '<title>'`

You are right, the ratio is 23 in page content versus 12 in title tag
I changed it.

Followup would rewrite almost everything anyway, so I decided to make new patch and ask again for sign off.
Comment 8 David Bourgault 2017-10-27 19:54:27 UTC
Created attachment 68790 [details] [review]
Bug 19456: Make patron-title ability to be generated with or without html tags

Modified pages:
circ/circulation.pl
circ/circulation_batch_checkouts.pl
members/boraccount.pl
members/files.pl
members/holdshistory.pl
members/housebound.pl
members/moremember.pl
members/notices.pl
members/purchase-suggestions.pl
members/readingrec.pl
members/routing-lists.pl
members/statistics.pl

Test plan:
0) Do not apply the patch
1) Have a patron with title/salution filled in
2) Confirm bug, go for example to circ/circulation page and see there is
html in <title> tag (you can see it in your browser page/window title)
3) Apply the patch
4) Go through circulation/patron pages (see modified page above) and
confirm there is no html in <title> tag, but on the page itself the
salutation should be in <span class="patron-title">
Comment 9 David Bourgault 2017-10-27 19:55:02 UTC
Worked as planned, QA tool green across the board.
Comment 10 Jonathan Druart 2017-11-02 11:24:48 UTC
*** Bug 19553 has been marked as a duplicate of this bug. ***
Comment 11 Jonathan Druart 2017-11-02 11:40:24 UTC
(In reply to David Bourgault from comment #9)
> Worked as planned, QA tool green across the board.

Do not forget to add your 'signed-off-by' line to the patch.
Comment 12 Nick Clemens 2017-11-13 16:54:02 UTC
Created attachment 69131 [details] [review]
Bug 19456: Make patron-title ability to be generated with or without html tags

Modified pages:
circ/circulation.pl
circ/circulation_batch_checkouts.pl
members/boraccount.pl
members/files.pl
members/holdshistory.pl
members/housebound.pl
members/moremember.pl
members/notices.pl
members/purchase-suggestions.pl
members/readingrec.pl
members/routing-lists.pl
members/statistics.pl

Test plan:
0) Do not apply the patch
1) Have a patron with title/salution filled in
2) Confirm bug, go for example to circ/circulation page and see there is
html in <title> tag (you can see it in your browser page/window title)
3) Apply the patch
4) Go through circulation/patron pages (see modified page above) and
confirm there is no html in <title> tag, but on the page itself the
salutation should be in <span class="patron-title">

Signed-off-by: David Bourgalt <david.bourgault@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Jonathan Druart 2017-12-07 12:39:53 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Nick Clemens 2017-12-14 12:30:23 UTC
Pushed to stable for 17.11.01, awesome work all!
Comment 15 Fridolin Somers 2017-12-20 08:12:38 UTC
I will push to next 17.05.x release because of string freeze.
Comment 16 Fridolin Somers 2018-01-03 13:41:55 UTC
Ah the patch does not apply easily.
I it not a major problem, I choose not to backport to 17.05.x.