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.'
Created attachment 46017 [details] [review] Bug 15434: search-xlate.inc - search criteria definition table Collected search criteria and limit labels in search-xlate.inc, so it can be used gor both making search criteria selects and displaying back selections on the result screen.
Hi Srdjan, I am a bit worried about translatability - did you check by chance if the moved strings show up in updated po files?
Right, I can see a problem with separating "phrase". Will rework.
Thx a lot!
Created attachment 46103 [details] [review] 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
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.
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.
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.
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
Created attachment 46674 [details] [review] 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
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=)$
(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...
> 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.
(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.
(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.
Created attachment 46887 [details] [review] 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>
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"
(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
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.
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>
Created attachment 49075 [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.
Follow-up attached - Jonathan and Srjdan, can you check?
Created attachment 49077 [details] [review] [SIGNED-OFF] 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>
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.
Select Language= French: Results of search with limit(s): 'Language *:fr' The * does not seem to be expected.
(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.
(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?
Ah right, that's what Katrin wanted to replace rtrn. Katrin, can you please defend your choice of "*".
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.
Created attachment 50963 [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>
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>
Found a loose end here... removed the * that caused the problems from my follow-up. Please reconsider!
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?
> 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
(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 :).
I've added this bug to the next dev meeting's agenda.
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.
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.