Bug 21770

Summary: t::lib::QA::TemplateFilters should allow html_entity in href
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, josef.moravec, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21731
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21393, 21740    
Bug Blocks:    
Attachments: Bug 21770: t::lib::QA::TemplateFilters should allow html_entity in href
Bug 21770: Add tests for html_entity
[ALTERNATIVE-PATCH] Bug 21770: Allow html_entity for href
Bug 21770: t::lib::QA::TemplateFilters should allow html_entity in href
Bug 21770: Add tests for html_entity
[ALTERNATIVE-PATCH] Bug 21770: Allow html_entity for href
Bug 21770: Add tests for html_entity
Bug 21770: Allow html_entity for href

Description Tomás Cohen Arazi 2018-11-06 13:32:17 UTC
In some cases the href is used in JavaScript to build UI components and shouldn't be URI encoded.
Comment 1 Tomás Cohen Arazi 2018-11-06 13:36:01 UTC
Created attachment 81980 [details] [review]
Bug 21770: t::lib::QA::TemplateFilters should allow html_entity in href

Bug 21740 highlighted the situation in which the href is not a link (and
thus requiring to be URI/URL encoded). This was fixed on the bug, but
the xt/find-missing-filters.t tests fail on it, becuase they enforce
uri filters on href without exceptions.

This patch adds the exception for those href's starting with '#'

To test:
- Run:
  $ kshell
 k$ prove xt/find-missing-filters.t
=> FAIL: Tests fail
- Apply this patch
- Run:
 k$ prove xt/find-missing-filters.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 2 Tomás Cohen Arazi 2018-11-06 13:36:41 UTC
This patch is submitted to highlight the issue. A better approach can be done, though.
Comment 3 Jonathan Druart 2018-11-06 14:04:39 UTC
Created attachment 81983 [details] [review]
Bug 21770: Add tests for html_entity
Comment 4 Jonathan Druart 2018-11-06 14:04:43 UTC
Created attachment 81984 [details] [review]
[ALTERNATIVE-PATCH] Bug 21770: Allow html_entity for href

We certainly should not expect this filter to be used much for href, but
we have one occurrence that needs it.

Test plan:
t/template_filters.t and xt/find-missing-filters.t must return green
Comment 5 Kyle M Hall 2018-11-06 20:31:43 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2018-11-06 20:32:02 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2018-11-06 20:32:07 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2018-11-06 21:23:11 UTC
The 3 patches should not be picked, the last one is an alternative patch for the first one.
Comment 9 Tomás Cohen Arazi 2018-11-07 17:05:53 UTC
Created attachment 82041 [details] [review]
Bug 21770: Add tests for html_entity

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2018-11-07 17:05:58 UTC
Created attachment 82042 [details] [review]
Bug 21770: Allow html_entity for href

We certainly should not expect this filter to be used much for href, but
we have one occurrence that needs it.

Test plan:
t/template_filters.t and xt/find-missing-filters.t must return green

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2018-11-07 17:07:20 UTC
I liked my patch because it was more clear to me. But as the author for the tests, I prefer Jonathan's approach.
Comment 12 Nick Clemens 2018-11-08 13:27:33 UTC
Awesome work all!

Pushed to master for 18.11
Comment 13 Martin Renvoize 2018-11-16 13:38:21 UTC
Bugfix on enhancement bug not in 18.05.x series.