Bug 21770 - t::lib::QA::TemplateFilters should allow html_entity in href
Summary: t::lib::QA::TemplateFilters should allow html_entity in href
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 21393 21740
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-06 13:32 UTC by Tomás Cohen Arazi
Modified: 2019-10-14 19:57 UTC (History)
5 users (show)

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


Attachments
Bug 21770: t::lib::QA::TemplateFilters should allow html_entity in href (2.08 KB, patch)
2018-11-06 13:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21770: Add tests for html_entity (2.47 KB, patch)
2018-11-06 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 21770: Allow html_entity for href (1.00 KB, patch)
2018-11-06 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21770: t::lib::QA::TemplateFilters should allow html_entity in href (2.15 KB, patch)
2018-11-06 20:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21770: Add tests for html_entity (2.54 KB, patch)
2018-11-06 20:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 21770: Allow html_entity for href (1.08 KB, patch)
2018-11-06 20:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21770: Add tests for html_entity (2.58 KB, patch)
2018-11-07 17:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21770: Allow html_entity for href (1.11 KB, patch)
2018-11-07 17:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 (kidclamp) 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.