|
Description
Ivan Dziuba
2021-01-13 18:17:46 UTC
Created attachment 115159 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Created attachment 115206 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Bug 27428: Show the number of records in the bibliographic. Update code. I have applied the patch but I can't get it to work. Maybe I misunderstand how it is supposed to work. Can you clarify what it should look like, maybe post a screen shots? I see an error in the console: Could not compile /kohadevbox/koha/opac/svc/elasticsearch/opac-autocomplete.pl: Can't locate Switch.pm in @INC (you may need to install the Switch module) I see this line in the patch for opac/svc/elasticsearch/opac-autocomplete.pl: 8 use Switch; But it doesn't seem to be used in the code? There seems to be other modules used which aren't used either. Sorry, the Switch module is the only one which isn't actually used in the code. There are no switch or case statements. I recall dependency on Text::Unaccent was recently removed by Bug 14759, replaced by Unicode::Normalize. We shouldn't ad it back in, perhaps. Created attachment 117950 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Remove Switch. (In reply to Frank Hansen from comment #3) > I have applied the patch but I can't get it to work. Maybe I misunderstand > how it is supposed to work. Can you clarify what it should look like, maybe > post a screen shots? Same as Franck, I don't see differents in bib record page when I enable the syspref. It seems I misunderstood something too. Created attachment 119980 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Bug 27113: ElasticSearch: Autocomplete in input search. Use Unicode::Normalize instead Text::Unaccent. This patch includes elasticsearch/opac-autocomplete.pl. Does that change belong here? That shoudl be opac-elasticsearch.pl (or similar generic name) in here and Bug 27113 Created attachment 120034 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Created attachment 120633 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> The number of records did show, but some of the search words got the number 0, and that is obviously wrong. See: Subject(s): Clans -- Scotland -- Great Glen -- History -- 17th century [1] | Clans -- Scotland -- Great Glen -- History -- 18th century [1] | Jacobites [2] | Great Glen (Scotland) -- History [1] | Highlands (Scotland) -- History [0] | Scotland -- History -- 17th century [1] | Scotland -- History -- 18th century [0] From opac-detail.pl?biblionumber=60. (In reply to Stina Hallin from comment #15) > The number of records did show, but some of the search words got the number > 0, and that is obviously wrong. See: > Subject(s): Clans -- Scotland -- Great Glen -- History -- 17th century [1] | > Clans -- Scotland -- Great Glen -- History -- 18th century [1] | Jacobites > [2] | Great Glen (Scotland) -- History [1] | Highlands (Scotland) -- History > [0] | Scotland -- History -- 17th century [1] | Scotland -- History -- 18th > century [0] > From opac-detail.pl?biblionumber=60. Stina, if you find things that are obviously wrong, you may change the status of the patch to Failed QA. +('OPACDisplayRecordCount','0',NULL,NULL,'YesNo')
Please add description here
+ default: no + choices: + 1: Show + 0: "Don't show" You probably mean default 0 ? Created attachment 132068 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Ok, I got it now ! It works nice :) I've got few questions though before signing it. I wonder what is the perimeter of the application : does the feature applied for every search link available in the record ? I saw that some special signs are not taken into account in the search but some are kept such like ';' or ':'. Is it a specific reason for that, for I don't get the logic ? This special signs can lead to unsuccessful searches so, for some records with a lot of informations, I wonder if, in the case result is [0], it's useful to display the information (see screenshot https://snipboard.io/RvlewS.jpg) Created attachment 139625 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Created attachment 139626 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Created attachment 139681 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Created attachment 145222 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> FAIL Koha/SearchEngine/Elasticsearch/Browse.pm
OK critic
OK forbidden patterns
OK git manipulation
OK pod
FAIL pod coverage
POD is missing for 'count_record'
SKIP spelling
OK valid
OK installer/data/mysql/mandatory/sysprefs.sql
OK git manipulation
OK semicolon
OK sysprefs_order
FAIL koha-tmpl/opac-tmpl/bootstrap/en/includes/doc-head-close.inc
FAIL filters
asset_must_be_raw at line 52 ( [% Asset.js("js/opac-display-record-count.js") %])
OK forbidden patterns
OK git manipulation
OK js_in_body
SKIP spelling
OK tt_valid
OK valid_template
OK koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
OK filters
OK forbidden patterns
OK git manipulation
OK js_in_body
SKIP spelling
OK tt_valid
OK valid_template
FAIL koha-tmpl/opac-tmpl/bootstrap/js/opac-display-record-count.js
FAIL ES template literals
ES template literals found ( var regex = `a[href*="${regex_es_link}"]`;) - See bug 24725
Created attachment 148842 [details] [review] Bug 27428: (QA follow-up) Add missing POD, remove ES template literals in opac-display-record-count.js Created attachment 154509 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 154510 [details] [review] Bug 27428: (QA follow-up) Add missing POD, remove ES template literals in opac-display-record-count.js Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Hi, this is a really interesting feature! Some notes: 1) The QA test tools are currently still failing, but it's only a perltidy that is needed. 2) The database update still has the old format and should be moved to the newer atomic update template. 3) The system preference is added correctly, but should a) Include a note that this feature is "Elasticsearch only" b) Be moved to the "Features" section instead of the "Did you mean" where it appears now. 4) It didn't work in my testing :( Maybe something broke with changes made to the OPAC since this was submitted the first time? * I used ktd --es7 up * Made sure SearchEngine was Elasticsearch and search worked * Applied patch * restart_all * Turned on OPACDisplayRecordCount system preference I don't see any errors in the console, but the JSON responses come back empty, possibly because the q param is empty:: http://localhost:8080/cgi-bin/koha/svc/elasticsearch/opac-elasticsearch.pl?q=[]&key=nb_notice ? I searched for "perl" in a MARC21 sample database and paged through a lot of the results with no counts showing. 5) Code review * Please translate code comments if you want to keep them: +/* Regex pour tous les URL avec un recherche */ This uses the older /svc pattern, we might want to get a second more technical review on this one. Created attachment 158639 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 158640 [details] [review] Bug 27428: (QA follow-up) Add missing POD, remove ES template literals in opac-display-record-count.js Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 158641 [details] [review] Bug 27428: (follow-up) perltidy, change the atomic file to the newer format, moving system preference to Features section and Fixing opac-display-record-count.js Created attachment 161349 [details] [review] Bug 27428: (follow-up) Fix Session Created attachment 164557 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 164558 [details] [review] Bug 27428: (QA follow-up) Add missing POD, remove ES template literals in opac-display-record-count.js Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Created attachment 164559 [details] [review] Bug 27428: (follow-up) perltidy, change the atomic file to the newer format, moving system preference to Features section and Fixing opac-display-record-count.js Created attachment 164560 [details] [review] Bug 27428: (follow-up) Fix Session Created attachment 167328 [details]
Screenshot - display for record detail page - [[object Object]]
For me, [[object Object]] is being displayed instead of the number of records.
I've attached a screenshot of what I'm seeing.
I also did a complete reindex (not sure whether this is necessary).
Other things I noted:
1. OPACDisplayRecordCount is still listed under the "Did you mean/spell checking" section, rather than "Features"
2. Should the description have a note saying only for Elasticsearch/Open Search?
3. If search engine is Zebra, is currently showing the [[object Object]]
Testing notes (using KTD):
1. Start up KTD with Elasticsearch: ktd --es8 up or ktd --es7 up
2. Apply the patch
3. Update the database: updatedatabase
4. Restart everything: flush_memcached and restart_all
5. Reindex: koha-elasticsearch --rebuild -d -b -a kohadev
Created attachment 167677 [details] [review] Bug 27428: (follow-up) show the number instead [[object Object]], move OPACDisplayRecordCount to the 'Features' section Created attachment 167767 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 167768 [details] [review] Bug 27428: (QA follow-up) Add missing POD, remove ES template literals in opac-display-record-count.js Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 167769 [details] [review] Bug 27428: (follow-up) perltidy, change the atomic file to the newer format, moving system preference to Features section and Fixing opac-display-record-count.js Signed-off-by: David Nind <david@davidnind.com> Created attachment 167770 [details] [review] Bug 27428: (follow-up) Fix Session Signed-off-by: David Nind <david@davidnind.com> Created attachment 167771 [details] [review] Bug 27428: (follow-up) show the number instead [[object Object]], move OPACDisplayRecordCount to the 'Features' section Signed-off-by: David Nind <david@davidnind.com> Created attachment 167774 [details] [review] Bug 27428: (follow-up) Update system preference description Add Elasticsearch only to OPACDisplayRecordCount description. I added a follow-up to update the new system preference description to indicate that it is Elasticsearch only. I got a bit confused when signing off - if I started up KTD with Elasticsearch, it worked as expected. If I switched the search engine to Zebra, rebuilt the index, and restarted everything, it was showing the counts in the OPAC. However, if I started up KTD without Elasticsearch it did not display the counts. Some more comments, but not something I'm sure of any more - so will leave to the QA team: 1. The change using the <b> tag for the count, I think we should be using <strong> to make the count bold. 2. Maybe a class should be used for the count style, so that libraries can customise the CSS. For some, the count in black and bold may stand out a little too much on the page (it does for me). Can someone explain the bug title? "Show the number of records in the bibliographic record" doesn't make sense to me. Is that supposed to be "Show the number of item records"? Is this a brand new development or is this something inLibro has had around for a while? I haven't read through everything here, but noticing some issues. While we don't explicitly have a coding guideline for this, we shouldn't be adding new "svc" scripts. API calls should be added via the REST API. I see that "opac/svc/elasticsearch/opac-elasticsearch.pl" doesn't have any authentication or authorization (even if it's a public endpoint, this is needed for appropriate session handling at the very least), and has completely non-standard output. It's also not clear to me why you're using Unicode::Normalize here. The "not_symbols" bit in "koha-tmpl/opac-tmpl/bootstrap/js/opac-display-record-count.js" is also weird. I could be missing something but I have a feeling that something isn't right here. Comment on attachment 167770 [details] [review] Bug 27428: (follow-up) Fix Session Review of attachment 167770 [details] [review]: ----------------------------------------------------------------- ::: opac/svc/elasticsearch/opac-elasticsearch.pl @@ +10,4 @@ > > my $browser = Koha::SearchEngine::Elasticsearch::Browse->new( { index => 'biblios' } ); > my $cgi = CGI->new; > +my $session = CGI::Session->load() or die CGI::Session->errstr(); This isn't how we do this in Koha. This is also why you'd use something like check_api_auth() (not that you should be doing a "svc" script at all), as you'd get a $sessionID that you could then use with C4::Auth::get_session($sessionID). But really this should be a REST API endpoint. Apologies if I sound grumpy. It's not my intention. It's first thing Monday morning [U+1F605]. (In reply to David Cook from comment #48) > Can someone explain the bug title? "Show the number of records in the > bibliographic record" doesn't make sense to me. Is that supposed to be "Show > the number of item records"? I think it's probably a hard one to explain in a short phrase. What it means is: Show the number of records you will find, when clicking on that search link in the record. I know libraries would love this especially for the 7xx/8xx linkings (how many linked volumes? how many analytics?) but also cool for subjects to see which search route is the most promising. You do sound grumpy :) but I can justify the current state of affair. Katryn already explained the functionality. We coded that many years ago along Bug 27113. We asked the community HOW to do it to avoid exactly this type of comment (which I don't mind in essence, but the timing is off). As usual, the target kept moving. We tried to share code between the two bz, or at least architecture. But Bug 27113 got blocked by another one. Your points are probably all valid, but until Bug 27113 (autocomplete) gets moving, were not going to rearchitecture this one as we don't want this little feature complicating our future development of the big one. We rebase to keep it valid for the libraries who want it, and to give it to our users. (In reply to Katrin Fischer from comment #52) > (In reply to David Cook from comment #48) > > Can someone explain the bug title? "Show the number of records in the > > bibliographic record" doesn't make sense to me. Is that supposed to be "Show > > the number of item records"? > > I think it's probably a hard one to explain in a short phrase. What it means > is: > Show the number of records you will find, when clicking on that search link > in the record. > > I know libraries would love this especially for the 7xx/8xx linkings (how > many linked volumes? how many analytics?) but also cool for subjects to see > which search route is the most promising. I wouldn't have guessed that, so thanks for the explanation! That's interesting! Seems like it could be difficult to do in a high performance way... (In reply to Blou from comment #53) > You do sound grumpy :) Haha thanks for the confirmation :) > We coded that many years ago along Bug 27113. We asked the community HOW to > do it to avoid exactly this type of comment (which I don't mind in essence, > but the timing is off). As usual, the target kept moving. Thanks for mentioning that. I had a feeling that was probably the case, but wanted to check. > We tried to share code between the two bz, or at least architecture. But > Bug 27113 got blocked by another one. > > Your points are probably all valid, but until Bug 27113 (autocomplete) gets > moving, were not going to rearchitecture this one as we don't want this > little feature complicating our future development of the big one. Fair enough. I am still curious about the use of Unicode::Normalize here though. I didn't code it, but my first instinct would be that it's related to Bug 27113, again. You don't want to bother with accents when comparing for an autocomplete. I read in another comment you suggest using Unicode::Collate::Locale instead of Unicode::Normalize, and hopefully it'll be caught whenever a "real" release is done for either patch. (In reply to Blou from comment #56) > I didn't code it, but my first instinct would be that it's related to Bug > 27113, again. You don't want to bother with accents when comparing for an > autocomplete. > > I read in another comment you suggest using Unicode::Collate::Locale instead > of Unicode::Normalize, and hopefully it'll be caught whenever a "real" > release is done for either patch. Ah right the autocomplete. Makes sense I think. Overall, I think that you've answered most of my questions. I think the SVC vs REST API question needs to be answered and it looks like that question is mostly being asked in bug 27113, so I think BLOCKED makes sense for a status here. Also adding a dependency on bug 27113. Created attachment 170274 [details] [review] Bug 27428: Show the number of records in the bibliographic record detail page with ElasticSearch. Tests: 1. Apply this patch; 2. In the Preference system - OPACDisplayRecordCount -> Show; 3. We can see the number of records next to search links in the bibliographic record detail page in the OPAC. Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se> Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 170275 [details] [review] Bug 27428: (QA follow-up) Add missing POD, remove ES template literals in opac-display-record-count.js Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 170276 [details] [review] Bug 27428: (follow-up) perltidy, change the atomic file to the newer format, moving system preference to Features section and Fixing opac-display-record-count.js Signed-off-by: David Nind <david@davidnind.com> Created attachment 170277 [details] [review] Bug 27428: (follow-up) Fix Session Signed-off-by: David Nind <david@davidnind.com> Created attachment 170278 [details] [review] Bug 27428: (follow-up) show the number instead [[object Object]], move OPACDisplayRecordCount to the 'Features' section Signed-off-by: David Nind <david@davidnind.com> Created attachment 170279 [details] [review] Bug 27428: (follow-up) Update system preference description Add Elasticsearch only to OPACDisplayRecordCount description. |
