Bug 31069

Summary: Did you mean? in the OPAC - links have <span> tags
Product: Koha Reporter: David Nind <david>
Component: OPACAssignee: Victor Grousset/tuxayo <victor>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: minor    
Priority: P5 - low CC: arthur.suzuki, lucas, martin.renvoize, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30629
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This removes <span> tags incorrectly displayed around the links for options available when 'Did you mean?' is enabled (for example, 'Search also for related subjects').
Version(s) released in:
22.11.00, 22.05.05
Bug Depends on: 30733    
Bug Blocks:    
Attachments: Screenshot - OPAC search showing <span> tags for Did you mean?
Bug 31069: fix <span> in OPAC search suggestions (Did you mean)
Bug 31069: fix <span> in OPAC search suggestions (Did you mean)
Bug 31069: fix <span> in OPAC search suggestions (Did you mean)
Bug 31069: Remove ExplodedTerms dependency on templates

Description David Nind 2022-06-30 00:24:19 UTC
Created attachment 136756 [details]
Screenshot - OPAC search showing <span> tags for Did you mean?

In master, when "Did you mean?" is enabled <span> tags are exposed (see attached screenshot).

To replicate:

1. Enable "Did you mean?":
   . go to Administration > Additional parameters > Did you mean?
   . put a tick in each plugin and save the configuration
2. In the OPAC, search for 'open source'

This is showing only in master, and not in 22.05.x.
Comment 1 Victor Grousset/tuxayo 2022-06-30 03:00:45 UTC
oops, it must be from bug 30733 ^^"
Comment 2 Victor Grousset/tuxayo 2022-06-30 03:04:18 UTC
> This is showing only in master, and not in 22.05.x.

We can expect it to be in 22.05.02 since it ships 30733.
Comment 3 Victor Grousset/tuxayo 2022-07-09 03:42:49 UTC
Created attachment 137444 [details] [review]
Bug 31069: fix <span> in OPAC search suggestions (Did you mean)

== test plan ==
1. Enable "Did you mean?":
   . go to Administration > Additional parameters > Did you mean?
   . put a tick in each plugin and save the configuration
2. In the OPAC, search for 'open source'
3. Without the patch you see escaped <span> after "Did you mean:"
Comment 4 Victor Grousset/tuxayo 2022-07-09 03:43:35 UTC
Same fix as bug 30629
Comment 5 David Nind 2022-07-09 09:24:48 UTC
Created attachment 137446 [details] [review]
Bug 31069: fix <span> in OPAC search suggestions (Did you mean)

== test plan ==
1. Enable "Did you mean?":
   . go to Administration > Additional parameters > Did you mean?
   . put a tick in each plugin and save the configuration
2. In the OPAC, search for 'open source'
3. Without the patch you see escaped <span> after "Did you mean:"

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 Martin Renvoize 2022-07-14 09:25:59 UTC
Created attachment 137698 [details] [review]
Bug 31069: fix <span> in OPAC search suggestions (Did you mean)

== test plan ==
1. Enable "Did you mean?":
   . go to Administration > Additional parameters > Did you mean?
   . put a tick in each plugin and save the configuration
2. In the OPAC, search for 'open source'
3. Without the patch you see escaped <span> after "Did you mean:"

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-07-14 09:26:30 UTC
All works, PQA
Comment 8 Tomás Cohen Arazi 2022-07-18 13:31:33 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 9 Tomás Cohen Arazi 2022-07-18 14:52:19 UTC
This broke tests:

t/db_dependent/SuggestionEngine_ExplodedTerms.t

Please fix ASAP.
Comment 10 Tomás Cohen Arazi 2022-07-18 15:39:02 UTC
Created attachment 137824 [details] [review]
Bug 31069: Remove ExplodedTerms dependency on templates

This patch removes the use of templates and CGI in ExplodedTerms by using the
Koha::I18N library.

The functionality on the package is too simple, and messing with the
template paths complexity on the tests was too much, given we have a
nice way to have translatable strings at the package level.

This patch does that, and cleans up the test file as well, that required
complex template and CGI mocking to run properly.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/SuggestionEngine_ExplodedTerms.t
=> FAIL: It fails in master
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass
4. Follow the original test plan
=> SUCCESS: Things work
5. Enjoy the rest of the day and forget this ever happened

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Victor Grousset/tuxayo 2022-07-22 01:43:53 UTC
Why did the patch broke SuggestionEngine_ExplodedTerms.t btw? I don't get it and might break other things if I use Koha::I18N in other places to clean up strings.
Comment 12 Tomás Cohen Arazi 2022-07-22 11:07:16 UTC
(In reply to Victor Grousset/tuxayo from comment #11)
> Why did the patch broke SuggestionEngine_ExplodedTerms.t btw? I don't get it
> and might break other things if I use Koha::I18N in other places to clean up
> strings.

It was probably a unique case in the codebase. A template that is used from a .pm file that didn't use get_template_and_user that also had really complex and fragile mocks in the tests...
Comment 13 Victor Grousset/tuxayo 2022-07-23 03:49:50 UTC
wow
thanks, no worries then ^^
Comment 14 Lucas Gass 2022-08-23 17:19:33 UTC
Backported to 22.05.x for 22.05.05
Comment 15 Arthur Suzuki 2022-08-26 12:08:03 UTC
Can't reproduce the bug on 21.11.
feel free to ask if backport needed.
Comment 16 Victor Grousset/tuxayo 2022-08-28 01:01:53 UTC
To spare testing time you can check the "Depends on:" field of the ticket. Here it links to bug 30733 which isn't in 21.11