Description
Zachary Spalding
2011-04-08 17:39:22 UTC
Currently using Koha version: 3.04.01.000, also I think it is affecting search results. There are records being return with only the operator highlighted. Is this something that can be fixed in ccl.properties file? The highlighting is done by css/jquery client side. So it cant be fixed/changed server side, and indeed does not affect anything in the search. If your search is returning bad results, that is unrelated to highlighting, and would need its own bug I believe this is still a valid bug. The highlighter needs to be more intelligent about what it highlights. Perhaps we can just tell it to never highlight the boolean operators, or pass the keywords only to it? Created attachment 20240 [details] [review] Patch to add syspref controlled stopwords to highlighting. This is an initial patch to implement some additional control upon the search result term highlighting features. It add a stopwords system preference where a pipe delimited list of words can be added which will prevent these words from being highlighted in search results. To Test 1. Enable OpacHighlightWords in the staff client 2. Run a search where results will be obtained 3. Note that all serach terms (including operators like 'and') are highlighted 4. Click through to the detailed display for one of the results 5. Note that all search terms are highlighted. 6. Apply patch 7. Add some 'stopwords' to new OpacHighlightStopWords system preference. 8. Run another search that includes some of the words you've just added to stopwords 9. Note that the stopwords are no longer highlighted, all other search terms should still appear highlighted. 10. Click through to the detailed display for a record 11. Note that the stopwrods are no longer highlighted, all other search terms should still be highlighted. Note for QA.. I'm sure there's a follow-up required for this patch with the correct DB additions for adding a syspref, but I'm reminding myself on the etiquette for that before submitting the follow-up. (In reply to Martin Renvoize from comment #4) > Note for QA.. I'm sure there's a follow-up required for this patch with the > correct DB additions for adding a syspref, but I'm reminding myself on the > etiquette for that before submitting the follow-up. Martin, take a look at the installer/data/mysql/updatedatabase.pl file. You should add a new entry (as the 3.13.00.015 entry). You have to add the new syspref to the installer/data/mysql/sysprefs.sql file too. This patch does not apply anymore. Created attachment 20469 [details] [review] Patch to add syspref controlled stopwords to highlighting Updated to apply to current master and also added db updates as suggested by Jonathan (thanks for the reminder). 1) OpacHighlightStopWords is used the database update and sysprefs sql file, but OpacHighlightedStopWords is used everywhere else. 2) Please use the TT plugin function Koha.Preference() to get the preference, rather than passing it it from the perl script 3) It's not working for me! Created attachment 20540 [details] [review] Patch to add syspref controlled stopwords to highlighting Patch updated to reflect Kyles comments: 1. OpacHightlightedStopWords used consistently throughout files 2. TT Plugin used for sysprefs inside the template instead of adding logic to koha pl files where not required. 3. See follow-up comment. Patch also rebased against master Thanks for the comments Kyle, I've done as asked with regard to silly mistakes in the sql update files and using the TT Plugin to fetch the preference now. (I didn't realise we had that yet.. oops) As for 'It's not working!', could you elaborate at all? What browser are you using, is javascript enabled (without it highlighting won't work at all)? Cheers Martin I'm using chrome. Enabling the feature and setting it to "OR|or|Or" did not stop the "or"s in my results from being highlighted. I'll retest and let you know if I find any errors. Martin, I've retested and discovered my problem. I had assumed this would affect the staff intranet! It works great in the OPAC. Do you think you could provide a followup to add this functionality to the staff intranet as well? That's a relief, I hadn't got a clue why it wasn't working otherwise. Patch coming up to also add the stopwords to the staff client.. to be honest, I'de completely forgotten the staff client also does the highlighting.. looking through it, the code is very inconsistent with the opac method and doesn't apply to the detailed result view, only the search results page. Anywho.. I'll patch the results page as is. Martin Created attachment 20543 [details] [review] Patch to add syspref controlled stopwords to highlighting Followup patch: Adds same stopwords to Staff Client Search Results page. I don't think this actually got to QA.. so resetting to Needs Signoff, as I think the 'Failed QA' status is holding it back from any further comment or testing. Created attachment 20715 [details] [review] Bug 6149 - Stopwords for Result Highlighting This patch adds a system preference to control stopwords used to prevent operators and other 'minor' words from being highlighted in search results when the OpacHighlightWords syspref is enabled Signed-off-by: Campbell Reid-Tait <campbellreidtait@gmail.com> Works on OPAC. Followup patch to add same stopwords to Staff Client Search Results page, still needed. Hi Campbell, It should all be in the one patch, including the staff client piece... I can see it's still in the patch you signed off from the diffs.. Could you give any errors you see if it's really not working? Martin (In reply to Martin Renvoize from comment #17) > Could you give any errors you see if it's really not working? Hi Martin, It does not work neither for me on the staff interface. Try to use the Koha plugin at the top of the results.tt file ([% USE Koha %]). Moreover, I think it would be great to have the list of stopwords case insensitive, in order to avoid to list all variants (The, the, THE). Marked as Failed QA. Hi Jonathan (In reply to Jonathan Druart from comment #18) > It does not work neither for me on the staff interface. Try to use the Koha > plugin at the top of the results.tt file ([% USE Koha %]). Thanks for the heads up regarding calling the Koha plugin in results.tt, I've now added this and tested. Seems to be working as expected to me. > Moreover, I think it would be great to have the list of stopwords case > insensitive, in order to avoid to list all variants (The, the, THE). I've now taken this into account too, making it case insensitive was a great idea! Patch to follow. . . Created attachment 20802 [details] [review] Bug 6149 - Stopwords for Result Highlighting Rebased again, and taken Jonathans comments into account. Also noted that these highlighting systems are out of sync between opac and staff client, including the lack of staff client code for Bug 10686. Once I've got this bit out of the way, I'm tempted to submit that as a speerate bug and patch. Applying: Bug 6149 - Stopwords for Result Highlighting Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M installer/data/mysql/updatedatabase.pl M koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt M koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt M koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Patch failed at 0001 Bug 6149 - Stopwords for Result Highlighting The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Created attachment 21168 [details] [review] Bug 6149 - Stopwords for result highlighting Rebased upon master Applying: Bug 6149 - Stopwords for Result Highlighting Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M installer/data/mysql/updatedatabase.pl Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Patch failed at 0001 Bug 6149 - Stopwords for Result Highlighting The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Created attachment 21582 [details] [review] Bug 6149 - Stopwords for Result Highlighting This patch adds a system preference to control stopwords used to prevent operators and other 'minor' words from being highlighted in search results when the OpacHighlightWords syspref is enabled Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Works as advertised, I fixed the updatedatabase to use XXX so it won't clash as much Hm, bit confused about the example values: and|And|or|Or
1) Do we need 'And' and 'and' when the values are now case insensitive?
>I've now taken this into account too, making it case insensitive was a great >idea!
2) Why not list 'not'?
I will continue to test, this could go into a follow up.
Created attachment 21831 [details] [review] [PASSED QA] Bug 6149 - Stopwords for Result Highlighting This patch adds a system preference to control stopwords used to prevent operators and other 'minor' words from being highlighted in search results when the OpacHighlightWords syspref is enabled Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Works as advertised, I fixed the updatedatabase to use XXX so it won't clash as much Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed a conflict in updatedatabase.pl. Also fixed tabs in opac-detail.tt Feature passes all tests and QA script now. Tested in intranet and OPAC with different words and searches. Works on results pages and OPAC detail page. Highlighting and unhighlighting still works correctly. (In reply to Katrin Fischer from comment #26) > Hm, bit confused about the example values: and|And|or|Or > > 1) Do we need 'And' and 'and' when the values are now case insensitive? > >I've now taken this into account too, making it case insensitive was a great >idea! > > 2) Why not list 'not'? > > I will continue to test, this could go into a follow up. Hmmm, it's passed QA now, but I was intending on posting a followup for Katrin's comments. The sample that should go into the sql would probably be best as 'and|or|not' now that I've made it case insensative and the 'not' is a good addition that I just hadn't thought of before ;) Either way, that's very trivial and not worth losing QA for :) Created attachment 22448 [details] [review] Bug 6149: Follow-up - Operator highlighted in search results - changed sample words to and|or|not in sysprefs.sql and updatedatabase - changed systempreference name from OpacHighlightedStopwords to HighlightedStopwords, as it also works for the staff results page Hi Martin, could you take a look at my follow-up and sign-off on it if it's ok? Hm, I think we will also need a follow up for the Bootstrap OPAC. Created attachment 22463 [details] [review] [SIGNED OFF] Bug 6149: Follow-up - Operator highlighted in search results Signed off on Katrin's followup, works perfectly and both changes make great sense. Created attachment 22466 [details] [review] Bug 6149 - Follow-up - Stopwords for Result Highlighting This follow-up adds the feature to the bootstrap theme as requested by Katrin ;) Created attachment 22505 [details] [review] [SIGNED OFF] Bug 6149: Follow-up - Stopwords for Result Highlighting (Bootstrap) Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Adds fix to the Bootstrap OPAC theme, works nicely. QA comment: The highlight does not work at all on the detail page with the bootstrap theme (caused by a JS error: q_array[x] = q_array[x]toLowerCase();). I think the change should be done in the opac-results-grouped template file too. Maybe should you include this function into a js file in order to avoid a duplication of code. Marked as Failed QA. Hi Jonathan, it worked for me yesterday - but i was not using the grouped template and was not aware it's currently used. (In reply to Katrin Fischer from comment #36) > Hi Jonathan, it worked for me yesterday - but i was not using the grouped > template and was not aware it's currently used. The opac-detail cannot work, a dot is missing in the JS code: + q_array[x] = q_array[x]toLowerCase(); The template seems to be used if the pref OpacGroupResults is enabled. Ah you are right - I forgot to test the highlighting on the detail page :( Just tested results - thx Jonathan! Pants, there's always a typo.. I must have been in ccsr instead of bootstrap. Now I feel silly! Created attachment 22511 [details] [review] Bug 6149: Follow-up - Stopwords for Result Highlighting (Bootstrap) QA Followup to fix the stupid typo... ack, double checked it this time! Created attachment 23005 [details] [review] [SIGNED-OFF] Bug 6149: Follow-up - Stopwords for Result Highlighting (Bootstrap) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 23025 [details] [review] [NOT PASSED QA] Bug 6149: Follow-up - Stopwords for Result Highlighting (opac-results-grouped) Added a follow up patch to add the feature to opac-results-grouped templates as requested. I'm working on a further follow-up to tidy the code out into a manageable file now. Jonathan, could you test that last patch for me? I'm confident it should work, but I don't have any systems with a working pazpar config to test upon. Martin Created attachment 23027 [details] [review] Bug 6149: Follow-up - Stopwords for Result Highlighting Final follow-up: Refactors code to includes to simpify future maintainability. (In reply to Martin Renvoize from comment #44) > Jonathan, could you test that last patch for me? I'm confident it should > work, but I don't have any systems with a working pazpar config to test upon. Sorry Martin but I don't have pazpar2 configured. I tested all pages (except opac-results-grouped.tt), and all works great. Maybe the refactoring could be done in another report in order not to block this one. Comment on attachment 23027 [details] [review] Bug 6149: Follow-up - Stopwords for Result Highlighting Moved last patch (re-factoring) into separate bug. (bug 11270) Thanks Martin. The last patch modifies a page I cannot test (opac-results-grouped), so I cannot pass QA on it. Given that neither you nor me are tested this patch, I propose either someone (with pazpar2 configured) signs off this last patch or someone else does qa on it. I am not sure pazpar2 is actually functional at this point in time, so I am not sure the change should block this from going in. If I just turn on OpacGroupResults I get a perl error in OPAC: Fehler: Could not find opac-search.xml in /usr/share/koha/opac/cgi-bin/opac/ at /usr/share/koha/lib/C4/Search.pm line 689 Maybe we could agree on having the first 3 patches go in and leaving the third on the bug? OpacGroupResults Agreed! Note the highlight does not work on the catalogue detail page (not a regression). 3 first patches marked as Passed QA. Created attachment 23064 [details] [review] Bug 6149 - Stopwords for Result Highlighting This patch adds a system preference to control stopwords used to prevent operators and other 'minor' words from being highlighted in search results when the OpacHighlightWords syspref is enabled Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Works as advertised, I fixed the updatedatabase to use XXX so it won't clash as much Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed a conflict in updatedatabase.pl. Also fixed tabs in opac-detail.tt Feature passes all tests and QA script now. Tested in intranet and OPAC with different words and searches. Works on results pages and OPAC detail page. Highlighting and unhighlighting still works correctly. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 23065 [details] [review] Bug 6149: Follow-up - Operator highlighted in search results - changed sample words to and|or|not in sysprefs.sql and updatedatabase - changed systempreference name from OpacHighlightedStopwords to HighlightedStopwords, as it also works for the staff results page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 23066 [details] [review] Bug 6149: Follow-up - Stopwords for Result Highlighting (Bootstrap) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Think on the name of the sysprefs: HighlightedStopWords. A user could be excused for thinking that it was the name of stopwords that /should/ be highlighted. So could a developer maintaining this in the future. WordsNotToHighlightInSearchResults is closer to the mark, although I'm open to a less verbose way of saying it. In any event, please resubmit with a better name. I have no other serious objection to the patch, though if you think of searching for titles like "Harry potter and the goblet of fire", I don't really consider it much of a win. Created attachment 26051 [details] [review] Bug 6149 - RM followup. Gloabal replaced HightedStopWords syspref with NotHightedWords for clarification purposes. Hopefully this is a bit better and still not too verbose. Submitted followup as requested. I think the name is suitable descriptive whilst not being overly verbose now, though am happy to change if advised. I agree, it's a pretty trivial solution, and could probabylbe implimetned more thoroughly given a rewrite of the highlighter itself... but it does solves the imediate problem for a number of our customers. This is not working for me, I think because there is some mixup with the preference name. It appears that updatedatabase.pl at the very least suffers from this. Hmm, that's interesting.. The only follow-up I did was a mas find/replace on that syspref name as per Galen request. I can't find any mention of the wrong name in the codebase now? I'll dig a little further Created attachment 30136 [details] [review] Bug 6149: Follow-up - Stopwords for Result Highlighting (opac-results-grouped) Created attachment 30137 [details] [review] Bug 6149 - Stopwords for Result Highlighting This patch adds a system preference to control stopwords used to prevent operators and other 'minor' words from being highlighted in search results when the OpacHighlightWords syspref is enabled Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Works as advertised, I fixed the updatedatabase to use XXX so it won't clash as much Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed a conflict in updatedatabase.pl. Also fixed tabs in opac-detail.tt Feature passes all tests and QA script now. Tested in intranet and OPAC with different words and searches. Works on results pages and OPAC detail page. Highlighting and unhighlighting still works correctly. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 30138 [details] [review] Bug 6149: Follow-up - Operator highlighted in search results - changed sample words to and|or|not in sysprefs.sql and updatedatabase - changed systempreference name from OpacHighlightedStopwords to HighlightedStopwords, as it also works for the staff results page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 30139 [details] [review] Bug 6149 - RM followup. Gloabal replaced HightedStopWords syspref with NotHightedWords for clarification purposes. Hopefully this is a bit better and still not too verbose. Re-based upon current master, though with the imminent removal of CCSR and prog I'm thinking I should factor those two bits of the patch out now. I still intend, but have no idea if I'll ever get around to it, to factor out the highlighting code so it's more easily applied across templates with just an include. I'd also like to update the jquery plugin (either to v4 from v3.. or to http://bartaz.github.io/sandbox.js/jquery.highlight.html fork), and add some ajax cleverness so that instead of having to create a list of stopwords (though that may still be useful for some other cases) we could get a more accurate set of search terms without operators back from the search script itself) I don't see why this one should get held up much further though.. it still works. Created attachment 30140 [details] [review] Bug 6149 - Stopwords for Result Highlighting This patch adds a system preference to control stopwords used to prevent operators and other 'minor' words from being highlighted in search results when the OpacHighlightWords syspref is enabled Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Works as advertised, I fixed the updatedatabase to use XXX so it won't clash as much Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed a conflict in updatedatabase.pl. Also fixed tabs in opac-detail.tt Feature passes all tests and QA script now. Tested in intranet and OPAC with different words and searches. Works on results pages and OPAC detail page. Highlighting and unhighlighting still works correctly. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 30141 [details] [review] Bug 6149: Follow-up [defaults] - Stopwords for Result Highlighting - changed sample words to and|or|not in sysprefs.sql and updatedatabase - changed systempreference name from OpacHighlightedStopwords to HighlightedStopwords, as it also works for the staff results page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 30142 [details] [review] Bug 6149: Follow-up [bootstrap] - Stopwords for Result Highlighting - Added functionality to new bootstrap theme Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 30143 [details] [review] Bug 6149: Follow-up [grouped] - Stopwords for Result Highlighting - Added functionaility to opac-results-grouped page Created attachment 30144 [details] [review] Bug 6149 - Follow-up [syspref] - Stopwords for Result Highlighting - global replaced HightedStopWords syspref with NotHightedWords for clarification purposes. Hopefully this is a bit better and still not too verbose. Sorry to have to add a comment 70 to this, but it doesn't work in Bootstrap for me. I tried with der|die|das and it still highlights them in the bootstrap theme. Created attachment 30835 [details] [review] Bug 6149 - Follow-up [syspref] - Stopwords for Result Highlighting - global replaced HightedStopWords syspref with NotHightedWords for clarification purposes. Hopefully this is a bit better and still not too verbose. Created attachment 30842 [details] [review] [PASSED QA] Bug 6149 - Stopwords for Result Highlighting This patch adds a system preference to control stopwords used to prevent operators and other 'minor' words from being highlighted in search results when the OpacHighlightWords syspref is enabled Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Works as advertised, I fixed the updatedatabase to use XXX so it won't clash as much Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed a conflict in updatedatabase.pl. Also fixed tabs in opac-detail.tt Feature passes all tests and QA script now. Tested in intranet and OPAC with different words and searches. Works on results pages and OPAC detail page. Highlighting and unhighlighting still works correctly. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script, works as expected. Created attachment 30843 [details] [review] [PASSED QA] Bug 6149: Follow-up [defaults] - Stopwords for Result Highlighting - changed sample words to and|or|not in sysprefs.sql and updatedatabase - changed systempreference name from OpacHighlightedStopwords to HighlightedStopwords, as it also works for the staff results page Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 30844 [details] [review] [PASSED QA] Bug 6149: Follow-up [bootstrap] - Stopwords for Result Highlighting - Added functionality to new bootstrap theme Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 30845 [details] [review] [PASSED QA] Bug 6149: Follow-up [grouped] - Stopwords for Result Highlighting - Added functionaility to opac-results-grouped page Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 30846 [details] [review] [PASSED QA] Bug 6149 - Follow-up [syspref] - Stopwords for Result Highlighting - global replaced HightedStopWords syspref with NotHightedWords for clarification purposes. Hopefully this is a bit better and still not too verbose. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works as described, fixed some tabs and a typo. Patches also change the old prog theme, but changes are small, bug report is old and has gone through a lot of iterations already, and we are going to remove the templates soon ...I am going to let it pass. Created attachment 31403 [details] [review] Bug 6149: (qa followup) sysprefs should be alphabetically sorted Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Patches pushed to master. Thanks Martin! |