Bug 31069 - Did you mean? in the OPAC - links have <span> tags
Summary: Did you mean? in the OPAC - links have <span> tags
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Victor Grousset/tuxayo
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 30733
Blocks:
  Show dependency treegraph
 
Reported: 2022-06-30 00:24 UTC by David Nind
Modified: 2023-06-08 22:26 UTC (History)
5 users (show)

See Also:
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


Attachments
Screenshot - OPAC search showing <span> tags for Did you mean? (189.51 KB, image/png)
2022-06-30 00:24 UTC, David Nind
Details
Bug 31069: fix <span> in OPAC search suggestions (Did you mean) (1.37 KB, patch)
2022-07-09 03:42 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 31069: fix <span> in OPAC search suggestions (Did you mean) (1.42 KB, patch)
2022-07-09 09:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 31069: fix <span> in OPAC search suggestions (Did you mean) (1.49 KB, patch)
2022-07-14 09:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31069: Remove ExplodedTerms dependency on templates (9.91 KB, patch)
2022-07-18 15:39 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 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