Bug 15434 - Make search results breadcrumbs human readable
Summary: Make search results breadcrumbs human readable
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Srdjan Jankovic
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-29 06:50 UTC by Srdjan Jankovic
Modified: 2016-12-01 21:57 UTC (History)
6 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
Bug 15434: search-xlate.inc - search criteria definition table (18.71 KB, patch)
2015-12-29 06:58 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 15434: Search criteria definition table (10.42 KB, patch)
2015-12-31 02:20 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 15434: Search criteria definition table (10.42 KB, patch)
2016-01-15 00:39 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 15434: Search criteria definition table (10.48 KB, patch)
2016-01-19 14:49 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[PASSED QA] Bug 15434: Search criteria definition table (10.54 KB, patch)
2016-03-13 22:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15434: Follow-up - Add more index terms (3.28 KB, patch)
2016-03-13 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15434: Follow-up - Add more index terms (3.34 KB, patch)
2016-03-13 23:37 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 15434: Follow-up - Add more index terms (3.32 KB, patch)
2016-04-28 22:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15434: Follow-up - Add more index terms (3.15 KB, patch)
2016-04-28 22:33 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2015-12-29 06:50:46 UTC
Humanise the breadcrumb text (or otherwise summarise what was searched for in a human readable format). The current formatting can lead to breadcrumbs such as Results of search for 'kw,wrdl: test 'with limit(s): 'holdingbranch:CPL su-to:Personality tests.'
Comment 1 Srdjan Jankovic 2015-12-29 06:58:56 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2015-12-29 20:13:11 UTC
Hi Srdjan, I am a bit worried about translatability - did you check by chance if the moved strings show up in updated po files?
Comment 3 Srdjan Jankovic 2015-12-29 23:35:55 UTC
Right, I can see a problem with separating "phrase". Will rework.
Comment 4 Katrin Fischer 2015-12-29 23:46:14 UTC
Thx a lot!
Comment 5 Srdjan Jankovic 2015-12-31 02:20:51 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2015-12-31 02:28:52 UTC
Template translations are a monster. Tried to add support for translations in assignments/function calls within [% %] tags, but gave up when the pain became unbearable.
Chickened out and used javascript, _() is clean and easy. The only thing that I don't like is the fact that I had to put all strings in opac-bottom.inc. Please comment.
Comment 7 Marc Véron 2015-12-31 07:39:14 UTC
Hi Srdjan, I like this feature.

One question:
If I search for "test" I get as breadcrumb:
Results of search for 'Keyword: test'
(As expected)

Then I click on: Did you mean: Search also for broader subjects
The breadcrumb is now:
Results of search for 'Keyword: su-br=test'

Similar for related / narrower subjects

Is that intended? I see su-br, su-na, su-rl in the JavaScript, though.
Comment 8 Srdjan Jankovic 2016-01-04 01:35:16 UTC
As far as I can see it is a bug either in
* modules/svc/suggestion.tt, line 37, that composes query as q=[indexed query], or in
* C4::Search::parseQuery() that cannot deal with that kind of query

I think that buildQuery() actually builds an invalid query, but I'd certainly need an authoritative opinion.
Comment 9 Nicole C. Engard 2016-01-14 15:20:55 UTC
Applying: Bug 15434: Search criteria definition table
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
Failed to merge in the changes.
Patch failed at 0001 Bug 15434: Search criteria definition table
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-15434-Search-criteria-definition-table-QMpnoE.patch
Comment 10 Srdjan Jankovic 2016-01-15 00:39:39 UTC Comment hidden (obsolete)
Comment 11 Nicole C. Engard 2016-01-15 14:11:26 UTC
That new patch doesn't apply either :(

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 15434: Search criteria definition table
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
	koha-tmpl/opac-tmpl/bootstrap/js/search.js
Please move or remove them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0001 Bug 15434: Search criteria definition table
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-15434-Search-criteria-definition-table-K9mxpF.patch
08:10 ~/kohaclone (bug15434-qa %|AM u=)$
Comment 12 David Cook 2016-01-18 23:03:22 UTC
(In reply to Srdjan Jankovic from comment #8)
> As far as I can see it is a bug either in
> * modules/svc/suggestion.tt, line 37, that composes query as q=[indexed
> query], or in
> * C4::Search::parseQuery() that cannot deal with that kind of query
> 
> I think that buildQuery() actually builds an invalid query, but I'd
> certainly need an authoritative opinion.

I used to know the answer to this one but I can't remember now.

That said:

1) buildQuery() is terribad
2) I think there might be some Javascript which adds the "su-br, su-na, su-rl" stuff and I recall it also being terribad. 

The whole of search needs an overhaul...

The other thing with this patch is that it doesn't include all CCL qualifiers, so hand-made queries will still show as the codes instead of the descriptions. Of course, providing descriptive labels for all CCL qualifiers is probably outside the scope of this bug...
Comment 13 Srdjan Jankovic 2016-01-18 23:40:04 UTC
> The whole of search needs an overhaul...

Oh yes...

> 
> The other thing with this patch is that it doesn't include all CCL
> qualifiers, so hand-made queries will still show as the codes instead of the
> descriptions. Of course, providing descriptive labels for all CCL qualifiers
> is probably outside the scope of this bug...

I can add that if you (or someone else) gives me the list. Or anyone can write a follow-up to add them, that's not a big thing.
Comment 14 Srdjan Jankovic 2016-01-19 01:19:04 UTC
(In reply to Nicole C. Engard from comment #11)
> That new patch doesn't apply either :(
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 15434: Search criteria definition table
> Using index info to reconstruct a base tree...
> Falling back to patching base and 3-way merge...
> error: The following untracked working tree files would be overwritten by
> merge:
> 	koha-tmpl/opac-tmpl/bootstrap/js/search.js
> Please move or remove them before you can merge.
> Aborting
> Failed to merge in the changes.
> Patch failed at 0001 Bug 15434: Search criteria definition table
> When you have resolved this problem run "git bz apply --continue".
> If you would prefer to skip this patch, instead run "git bz apply --skip".
> To restore the original branch and stop patching run "git bz apply --abort".
> Patch left in /tmp/Bug-15434-Search-criteria-definition-table-K9mxpF.patch
> 08:10 ~/kohaclone (bug15434-qa %|AM u=)$

Not sure why. I'm getting no errors:
srdjan@srdjan:~/koha/dev (bug_15434_wr251001)$ git checkout kc/master
Note: checking out 'kc/master'.
...
srdjan@srdjan:~/koha/dev ((e7e1da8...))$ git bz apply 15434
Bug 15434 - Make search results breadcrumbs human readable

46674 - Bug 15434: Search criteria definition table

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 15434: Search criteria definition table
srdjan@srdjan:~/koha/dev ((084446e...))$

Maybe your tree was not clean for some reason? Can you please check git status after checking out master, maybe there are some leftovers? That sometimes happens to me when changing branches.
Comment 15 David Cook 2016-01-19 06:00:14 UTC
(In reply to Srdjan Jankovic from comment #13)
> I can add that if you (or someone else) gives me the list. Or anyone can
> write a follow-up to add them, that's not a big thing.

Cool. I don't have time at the moment, but I'll keep that in mind. I've been thinking about writing a parser for ccl.properties, so if I ever pull that off, that could be useful.
Comment 16 Nicole C. Engard 2016-01-19 14:49:36 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-03-11 11:03:00 UTC
Comment on attachment 46887 [details] [review]
Bug 15434: Search criteria definition table

Review of attachment 46887 [details] [review]:
-----------------------------------------------------------------

Don't you think it would be good to keep the existing phrasing somewhere (on hover for instance)?

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc
@@ +133,5 @@
> +        };
> +        var limit_xlate = {
> +            "mc-itype": _("Type"),
> +            "ln": _("Language"),
> +            "branch": _("Branch"),

=> "Library"

@@ +136,5 @@
> +            "ln": _("Language"),
> +            "branch": _("Branch"),
> +            "aud": _("Audience"),
> +            "l-format": _("Format"),
> +            "ctype": _("Content Type"),

=> "Content type"
Comment 18 Jonathan Druart 2016-03-11 11:07:48 UTC
(In reply to Srdjan Jankovic from comment #13)
> > The other thing with this patch is that it doesn't include all CCL
> > qualifiers, so hand-made queries will still show as the codes instead of the
> > descriptions. Of course, providing descriptive labels for all CCL qualifiers
> > is probably outside the scope of this bug...
> 
> I can add that if you (or someone else) gives me the list. Or anyone can
> write a follow-up to add them, that's not a big thing.

ltrn, rltrn, ext, st-year, st-date-normalized
Comment 19 Katrin Fischer 2016-03-11 11:47:02 UTC
Hi Joubu and Srdjan, I had started on this during a train trip recently and have a follow-up for some additional phrases (facets, advanced search, qualifiers). Could post tonight if that would be helpful.
Comment 20 Katrin Fischer 2016-03-13 22:29:43 UTC
Created attachment 49074 [details] [review]
[PASSED QA] Bug 15434: Search criteria definition table

Collected search criteria and limit labels in opac-bottom.inc, so it
can be used for both making search criteria selects and displaying back
selections on the result screen.

*-opac-bootstrap.po translation files need to be changed:
- references to opac-advanced-search.tt need to be updated to
  opac-bottom.inc
- messages and translations need  -s removed

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2016-03-13 22:30:14 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2016-03-13 22:30:56 UTC
Follow-up attached - Jonathan and Srjdan, can you check?
Comment 23 Srdjan Jankovic 2016-03-13 23:37:33 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2016-03-14 00:21:44 UTC
I think there might still be some missing, but I feel it's a good start and we can improve on it step by step. Right now it should hopefully already cover most searches done by patrons.
Comment 25 Jonathan Druart 2016-03-14 16:01:59 UTC
Select Language= French:
  Results of search  with limit(s): 'Language *:fr'
The * does not seem to be expected.
Comment 26 Srdjan Jankovic 2016-03-14 23:26:17 UTC
(In reply to Jonathan Druart from comment #25)

Hi Jonathan,

> Select Language= French:
>   Results of search  with limit(s): 'Language *:fr'

Can you please tell me how did you get that *. When I select French from the drop-down, I get "limit=ln%2Crtrn%3Afre" in the url which shows as "with limit(s): 'Language *:fre'" in the breadcrumbs.
Comment 27 Jonathan Druart 2016-03-15 08:29:57 UTC
(In reply to Srdjan Jankovic from comment #26)
> (In reply to Jonathan Druart from comment #25)
> 
> Hi Jonathan,
> 
> > Select Language= French:
> >   Results of search  with limit(s): 'Language *:fr'
> 
> Can you please tell me how did you get that *. When I select French from the
> drop-down, I get "limit=ln%2Crtrn%3Afre" in the url which shows as "with
> limit(s): 'Language *:fre'" in the breadcrumbs.

Same as me.
So it's expected, that's what you mean?
Comment 28 Srdjan Jankovic 2016-03-16 04:01:20 UTC
Ah right, that's what Katrin wanted to replace rtrn.
Katrin, can you please defend your choice of "*".
Comment 29 Katrin Fischer 2016-03-16 05:28:43 UTC
My fault - sorry! Change can be removed from patch set.
I was thinking it would be nice to show a 'translated version' of rtrn, wrdl, phr etc. and did some experimenting, forgot to undo.
Comment 30 Katrin Fischer 2016-04-28 22:32:39 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2016-04-28 22:33:50 UTC
Created attachment 50964 [details] [review]
Bug 15434: Follow-up - Add more index terms

Adds more index names, especially for
- search for location and collection from adv search page
- facet limits
- authority number search from detail pages

Also adds a space after the colon for limits.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Comment 32 Katrin Fischer 2016-04-28 22:36:05 UTC
Found a loose end here... removed the * that caused the problems from my follow-up. Please reconsider!
Comment 33 David Cook 2016-04-28 23:34:49 UTC
I haven't had time to go over this bug... but I don't really understand it.

We build the breadcrumbs... so why are we doing this client-side? Why aren't we doing this server-side?
Comment 34 Srdjan Jankovic 2016-05-05 01:58:19 UTC
> We build the breadcrumbs... so why are we doing this client-side? Why aren't
> we doing this server-side?

I could have done it both ways, but these days I prefer client side.
* As a sever side dude, I prefer that clients use their cycles
* Tomorrow we decide to make search an api call, this still works

And a personal, not a valid reason: I find that translations are easier (cleaner) in js
Comment 35 David Cook 2016-05-05 02:38:29 UTC
(In reply to Srdjan Jankovic from comment #34)
> I could have done it both ways, but these days I prefer client side.
> * As a sever side dude, I prefer that clients use their cycles

I think client capabilities really range a lot, so as a server side dude I'd rather present as consistent a baseline as possible. 

> * Tomorrow we decide to make search an api call, this still works
> 

Your API call would still need to return the text for you to replace, so that doesn't really make sense. The API call could return translated text. You'd just need a locale parameter. 

> And a personal, not a valid reason: I find that translations are easier
> (cleaner) in js

I haven't done much with translations, but I believe you with that one. That said, if we used resource files for certain strings as I've suggested elsewhere, translations would be simple. In a persistent environment, you only load the file once, so there's not much overhead. 

But that's just my 2 cents. I'm not offering up any alternative patches, so I'm mostly just curious about the rationale. Thanks for taking the time to respond :).
Comment 36 Katrin Fischer 2016-09-25 20:20:09 UTC
I've added this bug to the next dev meeting's agenda.
Comment 37 Jonathan Druart 2016-09-29 13:08:15 UTC
Note: This does not work with Elastic.

The most I think about it, the most I am convinced it's not a good idea to make it client side.
We will need to provide tests to unsure future patches won't break this enhancement.
Comment 38 Katrin Fischer 2016-10-16 13:31:08 UTC
I think there is a majority here disagreeing with the way this patch was done and Jonathan has a good point about Elastic Search and testability. Marking this Failed QA, although I'd really like to have the feature.