TERMINOLOGY: "Child record" or "Component part": A Bibliographic record connected to it's host record via $773w -> 001 or via other Record-control-number-index relation. "Host record": A bibliographic record which contains child records. Host record doesn't have a link to the child record. USEFUL: For music cataloguers whose jobs depend on these. For browsing the contents of a musical record. Provides the basis for a high quality music library service. Because Koha has the building blocks to create these child records out of box, it is odd that they can't be displayed out of box. This patch adds a system preference AddChildRecordsToDetailedViews. This controls if child records data is appended after the bibliographic data for XSLT parsing. Also makes sure that child records are not pulled needlessly for non-detailed views as this would cripple performance. Modifies the default XSLT templates: MARC21slim2intranetDetail.xsl and MARC21slim2OPACDetail.xsl to display the child record data as HTML. Modifies .css files as well.
Sponsored by the Joensuu Regional Library
Created attachment 22612 [details] [review] Bug 11175 - Show the parent records child records (aka. component parts) in the detailed views.
Created attachment 22649 [details] [review] Bug 11175 - Show the parent records child records (aka. component parts) in the detailed views. TERMINOLOGY: "Child record" or "Component part": A Bibliographic record connected to it's host record via $773w -> 001 or via other Record-control-number-index relation. "Host record": A bibliographic record which contains child records. Host record doesn't have a link to the child record. USEFUL: For music cataloguers whose jobs depend on these. For browsing the contents of a musical record. Provides the basis for a high quality music library service. Because Koha has the building blocks to create these child records out of box, it is odd that they can't be displayed out of box. This patch adds a system preference AddChildRecordsToDetailedViews. This controls if child records data is appended after the bibliographic data for XSLT parsing. Also makes sure that child records are not pulled needlessly for non-detailed views as this would cripple performance. Modifies the default XSLT templates: MARC21slim2intranetDetail.xsl and MARC21slim2OPACDetail.xsl to display the child record data as HTML and link to the detailed views fopr the child records. Modifies .css files as well. An extra container div is added to opac-detail.tt, mimicking presentation in detail.tt, to prevent a possible long child records list from breaking the opac-details -view. -------------------------------------- TEST PLAN -------------------------------------- PREREQUISITES: -AddChildRecordsToDetailedViews system preference needs to be activated. -Catalog a bunch of child records for a host record. --Search a record and go to detailed view. --Use the existing functionality "+ New" -> "New Child Record" to catalog child records. ------------------------ TEST CASE1 - Staff client ------------------------ -Search for the record you just made and display it in the normal detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ TEST CASE2 - OPAC ------------------------ -Search for the record you just made and display it in the opac-detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ Thank you!
Created attachment 23049 [details] [review] [SIGNED-OFF] Bug 11175 - Show the parent records child records (aka. component parts) in the detailed views.
Quick note on this - the functionality is only available in the Prog theme - it will need to be added to the new default bootstrap theming. Cheers, Liz
Note : when it is pushed, a followup will be needed for unimarc M. Saby
Hi, starting with a code review: - Small thing: Please break your commit messages into shorter lines so they are easier to read in a terminal window. - Please provide a follow-up for the Bootstrap theme. As prog and CCSR are going to be deprecated this is mandatory now. - Database update adding the new system preference is missing. - The way this is coded it's a MARC21 specific feature. The rcn index and 001 > $w linking are not MARC agnostic. Please check for the marcflavor in your code and include a note about the limitation to MARC21 in the syspref description. - Some of the comments look like TODOs - maybe something you want to take another look at? - Creating our 'own' XML in a MARCXML record doesn't look right to me. I would much prefer if that could be expressed in normal MARC21 fields and subfields. - With the heading "Child records" you assume that it will be always a component part, but that's not true. 001 > $w linkings are used to express a lot of different relationships between records. I think putting a bit more thought into the various relationships could be good here. Also the QA script points out some issues: FAIL C4/XSLT.pm FAIL pod *** ERROR: Unknown command 'head' in file C4/XSLT.pm *** ERROR: Spurious text after =cut in file C4/XSLT.pm OK forbidden patterns OK valid FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
Thanks for the QA everyone, thanks for the uncovered bugs! I'll try to get back at this sometime next year :)
Created attachment 24788 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Business layer. TERMINOLOGY: "Component part": A Bibliographic record connected to it's host record via $773w -> 001 or via other Record-control-number-index relation. "Host record": A bibliographic record which contains child records. Host record doesn't need to have a link to the child record. USEFUL: For music cataloguers whose jobs depend on these. For browsing the contents of a musical record. Provides the basis for a high quality music library service. Because Koha has the building blocks to create these component part records out of box, it is odd that they can't be displayed out of box. This patch adds a system preference AddComponentPartRecordsToDetailedViews. This controls if component part records data is appended after the bibliographic data for XSLT parsing. Also makes sure that component part records are not pulled needlessly for non-detailed views as this would cripple performance.
Created attachment 24789 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Presentation for prog-theme. This displays the injected component part records in the prog theme. -------------------------------------- TEST PLAN -------------------------------------- PREREQUISITES: -AddChildRecordsToDetailedViews system preference needs to be activated. -opacthemes system preference needs to be 'prog'. -template system preference needs to be 'prog'. -Catalog a bunch of child records for a host record. --Search a record and go to detailed view. --Use the existing functionality "+ New" -> "New Child Record" to catalog child records. ------------------------ TEST CASE1 - Staff client ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the normal detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ TEST CASE2 - OPAC ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the opac-detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ Thank you!
Created attachment 24790 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Presentation for bootstrap-theme. This displays the injected component part records in the bootstrap theme. -------------------------------------- TEST PLAN -------------------------------------- PREREQUISITES: -AddChildRecordsToDetailedViews system preference needs to be activated. -opacthemes system preference needs to be 'bootstrap'. -Catalog a bunch of child records for a host record. --Search a record and go to detailed view. --Use the existing functionality "+ New" -> "New Child Record" to catalog child records. ------------------------ TEST CASE1 - OPAC ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the opac-detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ Thank you!
(In reply to Katrin Fischer from comment #7) > Hi, > > starting with a code review: Thank you for the effort Katrin! It is much appreciated! > > - Small thing: Please break your commit messages into shorter lines so > they are easier to read in a terminal window. I hope it's better. I also hope with shorter lines you don't mean the dreaded 80 character maximum. > > - Please provide a follow-up for the Bootstrap theme. As prog and > CCSR are going to be deprecated this is mandatory now. > Follow up provided! > - Database update adding the new system preference is missing. > Provided! > - The way this is coded it's a MARC21 specific feature. The rcn index > and 001 > $w linking are not MARC agnostic. Please check for the > marcflavor in your code and include a note > about the limitation to MARC21 in the syspref description. > Added a note to the system preference. Didn't add any syspref checks to the business layer. Uncertain if rcn is unused in UNIMARC. If rcn-index is unpopulated, then should cause zero extra overhead. If rcn is populated in UNIMARC, then this feature should work? > - Some of the comments look like TODOs - maybe something you want to > take another look at? > Apologies for that. I think it was a classical mistake. > - Creating our 'own' XML in a MARCXML record doesn't look right to me. I > would much prefer if that could be expressed in normal MARC21 fields > and subfields. > I guess I could differentiate from the searched component part records the marc field they use to make the linking. All the data is available since the found records are converted into MARC::Record objects. Then this data could be injected to appropriate MARC::Fields to construct a proper marc record. If this behaviour is wanted maybe it is better to create them when these component part relations are defined? Thus these links would be preserved during MARC-export function. The effort however doesn't justify the perceived benefits in my opinion. If you can demonstrate the benefits (like displaying different kinds of component relations under different heading) a follow-up can be prepared and designed. Also a "own" XML in MARCXML is already there inside the <sysprefs>-tag. > - With the heading "Child records" you assume that it will be always > a component part, but that's not true. 001 > $w linkings are used > to express a lot of different relationships between records. I think > putting a bit more thought into the various relationships could be > good here. > You are very correct. I went to the MARC21 manual and looked into the various aspects of linkings. I changed the terminology to "child" -> "component part". > Also the QA script points out some issues: > > FAIL C4/XSLT.pm > FAIL pod > *** ERROR: Unknown command 'head' in file C4/XSLT.pm > *** ERROR: Spurious text after =cut in file C4/XSLT.pm I don't know from where these are coming from? > OK forbidden patterns > OK valid > FAIL critic > # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). Fixed this issue. perlcritic++ Happy to deliver!
Created attachment 24791 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Presentation for bootstrap-theme. This displays the injected component part records in the bootstrap theme. -------------------------------------- TEST PLAN -------------------------------------- PREREQUISITES: -AddChildRecordsToDetailedViews system preference needs to be activated. -opacthemes system preference needs to be 'bootstrap'. -Catalog a bunch of child records for a host record. --Search a record and go to detailed view. --Use the existing functionality "+ New" -> "New Child Record" to catalog child records. ------------------------ TEST CASE1 - OPAC ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the opac-detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ Thank you!
Would you kindly help me help you get this feature to master?
Created attachment 26402 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Business layer. TERMINOLOGY: "Component part": A Bibliographic record connected to it's host record via $773w -> 001 or via other Record-control-number-index relation. "Host record": A bibliographic record which contains child records. Host record doesn't need to have a link to the child record. USEFUL: For music cataloguers whose jobs depend on these. For browsing the contents of a musical record. Provides the basis for a high quality music library service. Because Koha has the building blocks to create these component part records out of box, it is odd that they can't be displayed out of box. This patch adds a system preference AddComponentPartRecordsToDetailedViews. This controls if component part records data is appended after the bibliographic data for XSLT parsing. Also makes sure that component part records are not pulled needlessly for non-detailed views as this would cripple performance.
Created attachment 26403 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Presentation for prog-theme. This displays the injected component part records in the prog theme. -------------------------------------- TEST PLAN -------------------------------------- PREREQUISITES: -AddChildRecordsToDetailedViews system preference needs to be activated. -opacthemes system preference needs to be 'prog'. -template system preference needs to be 'prog'. -Catalog a bunch of child records for a host record. --Search a record and go to detailed view. --Use the existing functionality "+ New" -> "New Child Record" to catalog child records. ------------------------ TEST CASE1 - Staff client ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the normal detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ TEST CASE2 - OPAC ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the opac-detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ Thank you!
Created attachment 26404 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Presentation for bootstrap-theme. This displays the injected component part records in the bootstrap theme. -------------------------------------- TEST PLAN -------------------------------------- PREREQUISITES: -AddChildRecordsToDetailedViews system preference needs to be activated. -opacthemes system preference needs to be 'bootstrap'. -Catalog a bunch of child records for a host record. --Search a record and go to detailed view. --Use the existing functionality "+ New" -> "New Child Record" to catalog child records. ------------------------ TEST CASE1 - OPAC ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the opac-detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ Thank you!
Created attachment 26405 [details] [review] Bug 11175 - Fixing component part injection for non-default XSL-templates Changed the Detailed-view detection to include also custom XSLTs'. Removed unnecessary checks.
I'm quite interested in linking records, so I might have to take a look at this one. I haven't reviewed the code or applied the patch yet, but how might this fare with a high number of child records? In the case of a journal with article component parts, you might have dozens, hundreds, or more child records for each parent. I wonder if this would be handled better by an AJAX call and using the relational database instead of XML records. In any case, I'll take a look later, but just a thought...
Thank you for your comment! AJAX might be a better option. You are free to submit any changes. The complexity of such goes way over what we can develop given our timeframe. For us this feature works relatively well since we only have a couple of hundred records in our database with over 100 component parts. I can totally see the caching issues, but returning only a subset of a search query from Zebra is beyond my competence.
Created attachment 26766 [details] [review] Bug 11175 - Rebased to parse XML result from SimpleSearch instead of the ISO2709-version which it used to be.
Created attachment 26767 [details] [review] Bug 11175 - Show the parent record's component parts in the detailed views. Presentation for bootstrap-theme. This displays the injected component part records in the bootstrap theme. -------------------------------------- TEST PLAN -------------------------------------- PREREQUISITES: -AddChildRecordsToDetailedViews system preference needs to be activated. -opacthemes system preference needs to be 'bootstrap'. -Catalog a bunch of child records for a host record. --Search a record and go to detailed view. --Use the existing functionality "+ New" -> "New Child Record" to catalog child records. ------------------------ TEST CASE1 - OPAC ------------------------ -Reindex Zebra -Search for the record you just made childs for and display it in the opac-detail.pl -view. -Verify that a list of child records are displayed on the right side of the browser window. ------------------------ Thank you!
Nice feature to test, could you rebase it?
I'd be interested in this as well :)
Created attachment 78022 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host on the host intranet detail view, with clickable links to the component records. Host has no linking entries to the components, but the components link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001'. Does not use easyanalytics. Only for MARC21 biblios - MARC21 holdings and UNIMARC indexdefs haven't been updated. Only for the intranet detail page. Staff-client.css has not been recreated, so you need to sass that
Created attachment 78023 [details] Example marc data with host record and component parts
Created attachment 78024 [details] Screenshot
I cannot test this: Using index info to reconstruct a base tree... M C4/Search.pm M koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss M koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl Auto-merging koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/css/src/staff-global.scss Auto-merging C4/Search.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 11175: Show record component parts in the detail view The copy of the patch that failed is found in: .git/rebase-apply/patch Also would it be better to make adding Control-number-identifier to it's own patch and how is this handled with Elasticsearch now that it is also supported by koha?
I really like the idea of this, but it won't work for some of our libraries with hundreds of analytics on serial records. Could we truncate the list at some (configurable) point with a 'see more' link at the end?
(In reply to Katrin Fischer from comment #29) > I really like the idea of this, but it won't work for some of our libraries > with hundreds of analytics on serial records. Could we truncate the list at > some (configurable) point with a 'see more' link at the end? Your serial parts are component records? So there's a serial "host record", of which each published magazine (or whatever) is a child record?
Worse. The serial is the host, the children are the articles. The issue level is missing. This is because of the structure in our union catalog, but also fits Koha's subscription management. So this feature would have to be at least optional, nicer if we could use it of course.
Created attachment 79687 [details] [review] Bug 11175: Show record component parts in the detail view
After talking with Cait, I'll make the feature depend on a syspref.
Created attachment 79803 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the host detail view in staff client or OPAC, with clickable links to the component records. Host has no linking entries to the components, but the components link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds a Yet Another System Preference, ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001'. Except if UseControlNumber syspref is on, then it looks only for the Host field 001. Does not use easyanalytics. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records.
Created attachment 79809 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the host detail view in staff client or OPAC, with clickable links to the component records. Host has no linking entries to the components, but the components link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds a Yet Another System Preference, ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001'. Does not use easyanalytics. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records.
Created attachment 79810 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Signed-off-by: Pasi Kallinen <pasi.kallinen@joensuu.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected for me and it looks like all QA comments to date have been resolved. Adding my SO as a second signoff.
Comment on attachment 79810 [details] [review] Bug 11175: Show record component parts in the detail view Review of attachment 79810 [details] [review]: ----------------------------------------------------------------- ::: Koha/Biblio.pm @@ +388,4 @@ > return $self->{_biblioitem}; > } > > +=head3 components Please, provide tests for this method @@ +399,5 @@ > + > +sub components { > + my ($self) = @_; > + > + return undef if (C4::Context->preference('marcflavour') ne 'MARC21'); Do not return explicit undef
And please add support for elasticsearch.
Someone else can take this bug now, I have no time to work on it.
(In reply to Josef Moravec from comment #38) > Do not return explicit undef Could you elaborate a bit why returning undef is bad?
(In reply to Olli-Antti Kivilahti from comment #41) > (In reply to Josef Moravec from comment #38) > > Do not return explicit undef > > Could you elaborate a bit why returning undef is bad? It could lead to unexpected results, see: https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef https://perlmaven.com/how-to-return-undef-from-a-function We even have it in our guidelines (this policy is part of perlcritic severity 5): https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL4:_Perlcritic
(In reply to Olli-Antti Kivilahti from comment #41) Are you going to adopt this? It would be great!
(In reply to Josef Moravec from comment #39) > And please add support for elasticsearch. I don't think we should make this a requirement at the moment until Elasticsearch has left experimental status and this has been added to the coding guidelines as a requirement. For now, I think, not breaking Elasticsearch functionality with new code would be enough. Other things can be dealt with later. This is something we have been asked about for an awful long time, it would be sad to see the first steps abandoned.
I think this feature should be generalized to work with all the 77X linking fields as instructed by the MARC21 manual. That might not be such a big step up. Adding pagination is more complicated and requires this feature to be ported to the REST API, which would be superb anyway.
I think making it work with other 7xx could be a next step on a separate bug report. Also for the other 7xx there is more information in the record already about the linked items. From a set you often don't see much if anything about the children, so this appears to be the most urgent.
(In reply to Katrin Fischer from comment #44) > (In reply to Josef Moravec from comment #39) > > And please add support for elasticsearch. > > I don't think we should make this a requirement at the moment until > Elasticsearch has left experimental status and this has been added to the > coding guidelines as a requirement. For now, I think, not breaking > Elasticsearch functionality with new code would be enough. Other things can > be dealt with later. Ok, I could live without elastic for now, but I have filled a follow-up bug report - bug 21484 to address this. > > This is something we have been asked about for an awful long time, it would > be sad to see the first steps abandoned.
As we have used this in Finland for some years now, my opinion is that using the index for linking the records is not very efficient. You are always depended on it and if you want to make REST endpoint for the host and its component parts the callback probably isn't always solid. Also we have had issues with removing records. If you remove the host record the children are not going to be removed at the same time. And finding which children can be removed afterwards is difficult. On the other hand cleaning the database is getting very annoying and slow. My proposal is to move this linking to database level. Having a linking table which has host's biblionumber and child's biblionumber would make it solid and easier to handle. This could be a solution that we don't have to implement always to new search index platform.
DB link table is a good idea. Zebra searches for the link relations were blazingly fast tho.
Add DB link tables moves this bug into entirely different territory in my opinion... lets get the basics working well with zebra first as we're very nearly there here already.
So I think the only thing left fixing are these 2 points right now, correct? BTW I am not sure about a linking table for links between records, but I would love to see a linking table for the EasyAnalytics feature that happens on item level and 'spills over' into the record with non-standard 77x entries. In reply to Josef Moravec from comment #38) > Comment on attachment 79810 [details] [review] [review] > Bug 11175: Show record component parts in the detail view > > Review of attachment 79810 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Biblio.pm > @@ +388,4 @@ > > return $self->{_biblioitem}; > > } > > > > +=head3 components > > Please, provide tests for this method > > @@ +399,5 @@ > > + > > +sub components { > > + my ($self) = @_; > > + > > + return undef if (C4::Context->preference('marcflavour') ne 'MARC21'); > > Do not return explicit undef
We are currently discussing how we solve this problem properly for both the Finnish academic and public library sectors. This includes more than 773 linking. I have 0% interest to keep fiddling with this miniscule minimum product which no longer satisfies our extended requirements. I understand the need for small steps, but thanks to the small steps, we still have to restart the SIP2-server due to memcached.
So you are going to let this die here?
We can always open a new bug ticket :)
BTW Zebra doesn't always return the correct search results. For ex we have a REST API endpoint which returns all component parts for a biblio. It is awkward that we know some biblios have component parts, and still the Zebra searches don't return those results. That makes for rather bad API service. Using a DB link-table adds reliability.
BTW. First Finnish academic library HAMK went live last tuesday. Using ES in production. What we are trying to decide is, should these links be only in the ES index, only in the DB as a link-table with different link types separated, or should we implement both.
Maybe we should open new ticket for it. The linking table could be generic and used more versatile for different types of links.
Created attachment 105631 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
I rebased the earlier patch and will now start working on follow-ups for the two problems raised by Josef in comment #38: 1. Please, provide tests for this method 2. Do not return explicit undef I think it's is not bad idea to finish this implementation, the refactoring to DB table based implementation can be done later, this already adds a nice UI.
Created attachment 105641 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components.
Created attachment 105703 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components.
Created attachment 105706 [details] [review] Bug 11175: (follow-up) Return empty array if no components
Created attachment 105707 [details] [review] Bug 11175: (follow-up) Add tests
I added the missing tests and addressed the undef problem. In my kohadevbox setup however the test works only on the first run after running "reset_all". For some reason the Koha::Biblio->find accessor stops working, it could be something related to my setup, or there might be a bug in the test?
I will do some more testing with the unit test issue to make sure its not a bug in the test code, so changing the status to "In discussion" until that is resolved.
Created attachment 106692 [details] [review] Bug 11175: (follow-up) Add tests
(In reply to Joonas Kylmälä from comment #65) > I will do some more testing with the unit test issue to make sure its not a > bug in the test code, so changing the status to "In discussion" until that > is resolved. The issue was with the test's handling of C4::AddBiblio() results: it was missing parentheses around $host_bibnum so it was treated as an array. I have fixed this now. The patches are ready for someone to sign-off!
I have concerns about these patches. Firstly, they're only targeting Zebra and not Elasticsearch. While I'm not sure if there is any formal rules about that yet, I think we want to be including Elasticsearch for all search related changes. Secondly, I don't really like the idea of stuffing more XML into the search result XSLT. I think that's already one of the slowest parts of Koha, and this is likely to just slow it down further. It would be nicer to load related data asynchronously using AJAX. But the more I read and re-read the code... maybe it's not a big drama. I feel like this is adding to technical debt, but I'm not going to block it.
I've poked Nick C to ask how easy it might be to add a followup for ES support here. Otherwise, I'm generally happy with the code as it stands.. yes, it might be nice to eventually make it asynchronous, and perhaps we could make it clearer in the various sysprefs how EasyAnalytics and UseControlNumber are not involved in this one. But generally I think the approach is sound and doesn't add too much additional maintenance burden at this time. Awaiting Nicks reply before a final SO/QA run.
Hi Joonas, finally found some time to take a closer look: 1) System preference - I think staff interface is not a good tab choise, I'd put it under "Cataloguing" or "Search". - As this currently doesn't work for UNIMARC or Elasticsearch, we should add a hint about this in the pref so people know what to expect. - And maybe "does not require UseControlNumber".? - Change staff client to staff interface (see terminology list) 2) Use of 003 Looking at the patch and the test plan I am quite happy with the proposed search strategy. This should work really well for our libraries! 3) I am still slightly worried about what will happen when there are several hundred of analytical records found. This would be very commonly the case for our law and special libraries that do extensive analytical cataloguing for serials. And all the analyticals are linked to the serials (there are no volume or issue records as per the rules of our union catalog) so it gets quite a lot. Could you think of a strategy for those? Maybe we could define a number for a "cutoff" and display a link for "Show more". Ideal would be to show the newest x linked records. I am going to sign-off as this is an optional feature that works well as is. I hope we can figure out something for 1) (maybe we will add Elasticsearch support after consulting Nick?) and possibly 3) in the QA step.
Hm, sorry, found a bigger issue last minute: In the OPAC the display is not nice, probably due to the bootstrap patch being pushed in between. All the components show above the title of the record now.
Created attachment 112838 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 112839 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components.
Created attachment 112840 [details] [review] Bug 11175: (follow-up) Return empty array if no components
Created attachment 112841 [details] [review] Bug 11175: (follow-up) Add tests
Rebased.
Created attachment 112846 [details] [review] Bug 11175: (follow-up) Use uppercase AND and OR words in search query Elasticsearch only understands uppercase query operators.
Created attachment 112847 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported.
(In reply to Katrin Fischer from comment #71) > Hm, sorry, found a bigger issue last minute: In the OPAC the display is not > nice, probably due to the bootstrap patch being pushed in between. All the > components show above the title of the record now. To have it show correctly one needs to rebuild the CSS files: https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_interface#Compiling_Sass_files
(In reply to Katrin Fischer from comment #70) > Hi Joonas, > > finally found some time to take a closer look: > > 1) System preference > - I think staff interface is not a good tab choise, I'd put it under > "Cataloguing" or "Search". I went with Cataloguing. > - As this currently doesn't work for UNIMARC or Elasticsearch, we should add > a hint about this in the pref so people know what to expect. Added this info to description. > - And maybe "does not require UseControlNumber".? I don't understand what you mean here. > - Change staff client to staff interface (see terminology list) Done. > 3) I am still slightly worried about what will happen when there are several > hundred of analytical records found. This would be very commonly the case > for our law and special libraries that do extensive analytical cataloguing > for serials. And all the analyticals are linked to the serials (there are no > volume or issue records as per the rules of our union catalog) so it gets > quite a lot. Could you think of a strategy for those? > > Maybe we could define a number for a "cutoff" and display a link for "Show > more". Ideal would be to show the newest x linked records. I added 325 component part records and it loaded as far as I could tell the same speed as without any component part records, so there is no noticeable slow down here, the algorithm for this looks linear. The current approach seems OK to me, no need for paging because the HTML links generated here are very light weight. I'm thinking this is again ready for sign-off.
Created attachment 112851 [details] [review] Bug 11175: Add Elasticsearch support
(In reply to Joonas Kylmälä from comment #77) > Created attachment 112846 [details] [review] [review] > Bug 11175: (follow-up) Use uppercase AND and OR words in search query > > Elasticsearch only understands uppercase query operators. Hi Joonas, I obsoleted yours and added a new patch to fully add ES support here, let me know if that works for you
Created attachment 112852 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 112853 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 112854 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 112855 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 112856 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 112857 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Nick Clemens from comment #82) > Hi Joonas, > > I obsoleted yours and added a new patch to fully add ES support here, let me > know if that works for you Of course it works for me! You're awesome, thanks :) I tested the Elasticsearch implementation and I noticed slight difference there: with Elasticsearch only the first 100 component part records are returned meanwhile with Zebra all the component part records are returned. Maybe a follow-up for this would be needed?
(In reply to Joonas Kylmälä from comment #89) > (In reply to Nick Clemens from comment #82) > > Hi Joonas, > > > > I obsoleted yours and added a new patch to fully add ES support here, let me > > know if that works for you > > Of course it works for me! You're awesome, thanks :) > > I tested the Elasticsearch implementation and I noticed slight difference > there: with Elasticsearch only the first 100 component part records are > returned meanwhile with Zebra all the component part records are returned. > Maybe a follow-up for this would be needed? Koha/SearchEngine/Elasticsearch/Search.pm 298 =item C<$max_results> 299 300 The max number of results to return. The default is 100 (because unlimited 301 is a pretty terrible thing to do.) Probably this should be a syspref? For Zebra we don't hardcode a limit, but maybe we should not return everything and it would be better to have one syspref to control both>
(In reply to Nick Clemens from comment #90) > Koha/SearchEngine/Elasticsearch/Search.pm > 298 =item C<$max_results> > 299 > 300 The max number of results to return. The default is 100 (because > unlimited > 301 is a pretty terrible thing to do.) > > Probably this should be a syspref? For Zebra we don't hardcode a limit, but > maybe we should not return everything and it would be better to have one > syspref to control both> I don't think a global max results syspref would be a good idea. We could probably identify all calls of simple_search_compat() and then those that require more than 100 results, like this feature, should handle this case by making more calls using offset or different max results value. So it seems like we are now back to the question whether implement AJAX as was discussed earlier but it would be useful to have a rest api endpoint for component parts in that case. Or we could also say we are fine with 100 results at this point in time. What are your opinions on this?
Just as an example, from one of our larger consortiums, the single biblio with the most components they have is 1000 - from "THE FOLKSONG FAKE BOOK : A COLLECTION OF OVER 1000 FOLKSONGS FROM AROUND THE WORLD" Average number of components is 18.5, and there are about 56000 component parts in total. If the max number of components is set to something like 300, it would cover more than 99% of the cases, which IMO would be good enough for now - this could be changed after the feature is in.
If the performance is ok, I would be happy with 100. Maybe we could add a note that it will show up to 100 if 100 results are returned or so? Just a hint that there might be more.
(In reply to Katrin Fischer from comment #93) > If the performance is ok, I would be happy with 100. Maybe we could add a > note that it will show up to 100 if 100 results are returned or so? Just a > hint that there might be more. As the page here uses a search to find these, why not just add a link to perform that search as a regular search and provide the results to the user?
(In reply to Nick Clemens from comment #94) > (In reply to Katrin Fischer from comment #93) > > If the performance is ok, I would be happy with 100. Maybe we could add a > > note that it will show up to 100 if 100 results are returned or so? Just a > > hint that there might be more. > > As the page here uses a search to find these, why not just add a link to > perform that search as a regular search and provide the results to the user? Great idea! I will start working on a follow-up. I will also fix the wording in the syspref page regarding ES not being supported.
Created attachment 113649 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown
Created attachment 113650 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes
Created attachment 113962 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234')
If anybody knows why the double quotes are needed with Elasticsearch I would be happy to know :)
Created attachment 113964 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234')
Created attachment 114326 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114327 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114328 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114329 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114330 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114331 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114332 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114333 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 114334 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Elasticsearch from the syspref text "Elasticsearch and UNIMARC are not supported" needs to be removed.
Created attachment 116819 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116820 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116821 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116822 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116823 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116824 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116825 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116826 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116827 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Created attachment 116828 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
There was a minor rebase conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/staff_interface.pref because a new syspref was introduced so I rebased that. I removed now from the syspref text in "Bug 11175: Remove outdated info about Elasticsearch not being suppported" the info that elasticsearch is not supported. These were super minor changes so I'm keeping the Signed off state. Ready for QA.
Created attachment 119499 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
rebased one conflicting patch
(sorry, undoed rebasing, wrong contents)
Created attachment 119506 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
I am not able to download the records from Bugzilla. This might be a side effect of some spam protection measure :( Could you make them available somewhere else maybe? I've also got a question about this: >When looking up the component part records, the code searches for >records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' >or, if the 003 is not defined in the Host, 773$w=Host001. I am in a hard spot here. Because Koha didn't care about 003 so far, our $w never have the 003, but it's defined in the records. Does this mean this won't work until we would change data or is there a way this could still work? This does not only apply for our use case. When a library has used the "New child record" option from the detail page for cataloguing, 003 won't have been added to their $w.
(In reply to Katrin Fischer from comment #126) > I am not able to download the records from Bugzilla. This might be a side > effect of some spam protection measure :( Could you make them available > somewhere else maybe? https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78023&action=diff&collapsed=&headers=1&format=raw > > I've also got a question about this: > > >When looking up the component part records, the code searches for > >records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' > >or, if the 003 is not defined in the Host, 773$w=Host001. > > I am in a hard spot here. Because Koha didn't care about 003 so far, our $w > never have the 003, but it's defined in the records. Does this mean this > won't work until we would change data or is there a way this could still > work? It will work, it supports both, the 003 will be used only for further disambiguation in case the records have it. See also bug 27507.
(In reply to Joonas Kylmälä from comment #127) > It will work, it supports both, the 003 will be used only for further > disambiguation in case the records have it. See also bug 27507. Also to further elaborate, the adding of 003 is to fully support the marc format, see https://www.loc.gov/marc/bibliographic/bd773.html for examples of 773$w.
(In reply to Joonas Kylmälä from comment #128) > (In reply to Joonas Kylmälä from comment #127) > > It will work, it supports both, the 003 will be used only for further > > disambiguation in case the records have it. See also bug 27507. > > Also to further elaborate, the adding of 003 is to fully support the marc > format, see https://www.loc.gov/marc/bibliographic/bd773.html for examples > of 773$w. I've wanted to add the use of 003 for ages but never got around to it.. Nice one!
As an asside, once we push this, it would be great to have another look at the relevant wiki pages an make sure they reflect these updates: https://wiki.koha-community.org/wiki/Multipart_Bibliographics
> It will work, it supports both, the 003 will be used only for further > disambiguation in case the records have it. See also bug 27507. We do have the 003 set, but not using prefixes in $w (to clarify). Does that change your reply? I am aware 003 is standard, but as Koha does enforce 003 (the advanced editor at least does), but the plugins we have to crate analytics do not, there is some mismatch I want to check for. We should definitely move to be more standard compliant, but maybe we can have fallbacks to make this useful for people with the existing data meanwhile.
(In reply to Katrin Fischer from comment #131) > > It will work, it supports both, the 003 will be used only for further > > disambiguation in case the records have it. See also bug 27507. > > We do have the 003 set, but not using prefixes in $w (to clarify). Does that > change your reply? No, it still works.
> > We do have the 003 set, but not using prefixes in $w (to clarify). Does that > > change your reply? > > No, it still works. Awesome :)
Created attachment 120206 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent.
Created attachment 122707 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122708 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122709 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122710 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122711 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122712 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122713 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122714 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122715 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122716 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122717 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122718 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This seems pretty solid to me.. remember to build the CSS on push ;). Also, there's a weird QA script fail for me on POD encoding.. but I can't spot any weird characters to I'm a bit confused by it. Leaving to the RM for a final check there. Passing QA
Koha::Util::Search feels wrong, why not under Koha::SearchEngine?
(In reply to Martin Renvoize from comment #147) > Also, there's a weird QA script fail for me on POD encoding.. but I can't > spot any weird characters to I'm a bit confused by it. There is a "–" vs "-".
Created attachment 123081 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123082 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123083 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123084 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123085 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123086 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123087 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123088 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123089 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123090 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123091 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123092 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123093 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
(In reply to Jonathan Druart from comment #149) > (In reply to Martin Renvoize from comment #147) > > Also, there's a weird QA script fail for me on POD encoding.. but I can't > > spot any weird characters to I'm a bit confused by it. > > There is a "–" vs "-". Fixed this in the latest patch, thanks for spotting.
(In reply to Jonathan Druart from comment #148) > Koha::Util::Search feels wrong, why not under Koha::SearchEngine? Because Koha::SearchEngine felt wrong too, it has functions to interact with the Zebra and Elasticsearch APIs, not to create specific queries. And then another question: why not have Koha::Search::something, well, I think Koha::Util::Search being under Koha::Util already helps to tell it is a collection of helper functions, and also then all our helper functions are in the same place to be found.
Created attachment 123406 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section.
Created attachment 123410 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'.
QA Question Should 'Analytics: Show analytics' still display even when the syspref is enabled to display them inline?
OK, having dug a bit further I'm a little less sure about this patch set.. I love it in theory.. but I feel we're perhaps duplicating code a bit.. The code that governs the 'show analytics' link display already does a similar search but does it within the two -details' controller files.. I feel we should move to using the Util function and XSLT method from here for both cases.. and likely take some notice of UseControlNumber.
Comment on attachment 123087 [details] [review] Bug 11175: Limit the amount of component parts returned Review of attachment 123087 [details] [review]: ----------------------------------------------------------------- ::: Koha/Util/Search.pm @@ +34,5 @@ > +=cut > + > +sub get_component_part_query { > + my ($biblionumber) = @_; > + I think perhaps we should be taking UseControlNumber into account here... See opac-details.pl and details.pl controllers for a near the same case. my $cleaned_title = $biblio->title; $cleaned_title =~ tr|/||; my $query = ( C4::Context->preference('UseControlNumber') and $record->field('001') ) ? 'rcn:'. $record->field('001')->data . ' AND (bib-level:a OR bib-level:b)' : "Host-item:($cleaned_title)"; We should really utilise something similar here I feel.. and even better.. start using this in those two locations too.. or rather.. move that handling into XSLT.pm as you have done for the overall show parts code.
Created attachment 123508 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123509 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123510 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123511 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123512 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123513 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123514 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123515 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123516 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123517 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123518 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123519 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123520 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123521 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123522 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123523 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123524 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
t/Koha/Util/Search.t needs updating for that final patch.. one to attack tomorrow.
Created attachment 123541 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123542 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123543 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123544 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123545 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123546 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123547 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123548 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123549 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123550 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123551 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123552 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123553 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123554 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123555 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123556 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123557 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123562 [details] [review] Bug 11175: (follow-up) Missed line in previous commit
1. It needs more work, there is something broken (search does not return anything) 2. The UI is not the same as what is advertised in the screenshot, the component part records are display at the top (not on the right) 3. I don't think we need the overhead we introduced with the "show all" link. We call get_component_part_query from C4::XSLT (for that link) and Koha::Biblio (for the list we display). I would suggest to retrieve them all and have a expand/collapse link if we have "too many" element to display. We could then move the get_component_part_query into Koha::Biblio->get_marc_components (Koha::Biblio->get_marc_host is doing that already).
Created attachment 123564 [details] [review] Bug 11175: (follow-up) More Fixes * Default to an empty arrayref from get_marc_components * Standardise search syntax in Koha::Util::Search
(In reply to Jonathan Druart from comment #206) > 1. It needs more work, there is something broken (search does not return > anything) OK, I've done more digging here.. in short, I think we need to make a distinction between "Analytics" and "Volumes of sets and traced series" akin to the change bug 7609 made way back. At least for the UseControlNumber case.. that's what the search results end up empty as I moved the query without fully understanding it. > 2. The UI is not the same as what is advertised in the screenshot, the > component part records are display at the top (not on the right) This is just a missing CSS build.. I feel down that hole too ;) > 3. I don't think we need the overhead we introduced with the "show all" > link. We call get_component_part_query from C4::XSLT (for that link) and > Koha::Biblio (for the list we display). I would suggest retrieving them all > and have a expand/collapse link if we have "too many" element to display. > We could then move the get_component_part_query into > Koha::Biblio->get_marc_components (Koha::Biblio->get_marc_host is doing that > already). I think in theory the reason wasn't to reduce display size but to reduce search return scope for performance. In reality, I'm not sure this is a real issue.. need to read the rest of the comments that asked for the reduction to be introduced.. it all feels very artificial.
(In reply to Martin Renvoize from comment #208) > (In reply to Jonathan Druart from comment #206) > > 1. It needs more work, there is something broken (search does not return > > anything) > > OK, I've done more digging here.. in short, I think we need to make a > distinction between "Analytics" and "Volumes of sets and traced series" akin > to the change bug 7609 made way back. At least for the UseControlNumber > case.. that's what the search results end up empty as I moved the query > without fully understanding it. Actually.. I think this is working as expected.. I just had other issues to resolve. > > 3. I don't think we need the overhead we introduced with the "show all" > > link. We call get_component_part_query from C4::XSLT (for that link) and > > Koha::Biblio (for the list we display). I would suggest retrieving them all > > and have a expand/collapse link if we have "too many" element to display. > > We could then move the get_component_part_query into > > Koha::Biblio->get_marc_components (Koha::Biblio->get_marc_host is doing that > > already). > > I think in theory the reason wasn't to reduce display size but to reduce > search return scope for performance. In reality, I'm not sure this is a real > issue.. need to read the rest of the comments that asked for the reduction > to be introduced.. it all feels very artificial. See comment 90. ES is hard coded to return 100 max unless passed a new max.. there's not option to pass 'all'.. Hense where we're all with passing a search string to get all.
Created attachment 123569 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123570 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query.
Created attachment 123571 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123572 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123573 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123574 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
-sub get_marc_components { +sub get_marc_analytics { Do we really want to use "analytics" here? It's not related to the EasyAnalytics feature, and "components" is used all over the place (perl and xslt variables, css classes, syspref, etc.)
On the other hand, "component" is confusing as well (at least for me). Doesn't an official wording (in the MARC world) exist?
(In reply to Jonathan Druart from comment #217) > On the other hand, "component" is confusing as well (at least for me). > Doesn't an official wording (in the MARC world) exist? Component/component part appears in the LOC documentation, while analytics doesn't: LDR, pos. 7 https://www.loc.gov/marc/bibliographic/bdleader.html 773 https://www.loc.gov/marc/bibliographic/bd773.html
Fair enough.. I renamed it to analytics as the query limits to bib-level:a + bib-level:b now which corresponds to analytics in my understanding.. if we want all components then we should probably drop that filtering. Katrin, I'd love your thoughts on that.
right.. back to FQA.. been testing with Katrin. We found the searches don't work fully yet. * rcn:003 001 - Works as expected * rcn:(003)001 - Is needed in addition to above * (rcn:001 AND cni:003) - Doesn't appear to be working
Created attachment 123582 [details] [review] Bug 11175: (QA follow-up) Fix queries
OK, that fixes the query issue.. for Zebra at least.. we'll test Elastic next. However.. another issue we've spotted. This does not work well with RLT languages.. If you have a record with both LTR and RTL in then the component parts box will stick itself between the two.
Created attachment 123583 [details] Detail view of record with RTL and LTR script Adding a screenshot to explain the issue a little better. I believe usability of the scroll box might not be too well if you have really a lot of records, because it limits you to a relatively small "view". Also slightly worried about handling on mobile interfaces as it sticks to the 2 column layout and won't stack as other elements on smaller sizes. I haven't checked all the comments yet, but could we imagine a different placing? Maybe below or above the items?
I'm starting to ask myself if doing this within the XSLT is right at all... It feels like perhaps we should be placing these details inside a tab alongside holdings, description and friends. Right now, we're passing allot of data to the front end and then displaying very minimal details from it via XSLT. I'm thinking that perhaps we should either get selective about what fields of the component part records we pass and just display a nice table or we can continue to pass whole records, but split the xslt into it's own file (or re-use the results xslt perhaps) and use that for more of an embedded results display in this tab. Think I'm going to have to tag out of this for now, I need to get back to other things.
Created attachment 123608 [details] [review] WIP: Display components in own tab
Created attachment 123609 [details] [review] Bug 11175: (WIP) Display components in own tab
Created attachment 123611 [details] [review] Bug 11175: (WIP) Display components in own tab
Created attachment 123612 [details] [review] Bug 11175: (WIP) Remove special XSLT handling for parts With the work to move this into it's own tab in the details display, we don't need the special handling in the XSLT display templates, utils or module.
Created attachment 123613 [details] Alternative display option
All patches can be applied now, the final two are an alternative implementation based on the work that preceded. Test plan is as before, grab the test mrc records and import them, build the indexes then search for 'Easy piano'. Navigate to the host record and see what it looks like.. I new 'Components (200)' tab should appear and clicking on it should display a list of all the attached records. The big difference with this implementation is that we no longer embed the analytics in the records passed to the XSLT parser. Instead, we pass each record in an array to the template and them use the Results XSLT to format their output. It might be nice to have a simplified template for this display instead of using Results, but as a proof of concept I felt this was enough to showcase the alternative method.
I'm also wondering about the need for 'ShowComponentParts' pref at all now.. We already do the search anyway to ascertain whether we should display the 'Show analytics' link or not so we're not really saving anything performance-wise by disabling the display.
Created attachment 123614 [details] [review] Bug 11175: (WIP) OPAC
*** Bug 21484 has been marked as a duplicate of this bug. ***
Created attachment 123617 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123618 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123619 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123620 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123621 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123622 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123623 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123624 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123625 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123626 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123627 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123628 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123629 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123630 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123631 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123632 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123633 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123634 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123635 [details] [review] Bug 11175: (QA follow-up) Fix queries
Created attachment 123636 [details] [review] Bug 11175: (WIP) Display components in own tab
Created attachment 123637 [details] [review] Bug 11175: (WIP) Remove special XSLT handling for parts With the work to move this into it's own tab in the details display, we don't need the special handling in the XSLT display templates, utils or module.
Created attachment 123638 [details] [review] Bug 11175: (WIP) OPAC
Created attachment 123736 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123737 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123738 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123739 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123740 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123741 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123742 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123743 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123744 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123745 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123746 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123747 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123748 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123749 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123750 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123751 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123752 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123753 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123754 [details] [review] Bug 11175: (QA follow-up) Fix queries
Created attachment 123755 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details.
Created attachment 123756 [details] [review] Bug 11175: (QA follow-up) Rename back to get_marc_components
Can't apply on current master
Created attachment 124134 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124135 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124136 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124137 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124138 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124139 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124140 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124141 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124142 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124143 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124144 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124145 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124146 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124147 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124148 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124149 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124150 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124151 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124152 [details] [review] Bug 11175: (QA follow-up) Fix queries
Created attachment 124153 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details.
Created attachment 124154 [details] [review] Bug 11175: (QA follow-up) Rename back to get_marc_components
Created attachment 124155 [details] [review] Bug 11175: (QA follow-up) Fixes for bug 12561 Bug 12561 changed the prototype for XSLTParse4Display so this patcha accounts for the additional calls to that method introduced in this patchset.
Rebased on master :)
Created attachment 124382 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124383 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124384 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124385 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124386 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124387 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124388 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124389 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124390 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124391 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124392 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124393 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124394 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124395 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124396 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124397 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124398 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124399 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124400 [details] [review] Bug 11175: (QA follow-up) Fix queries
Created attachment 124401 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details.
Created attachment 124402 [details] [review] Bug 11175: (QA follow-up) Rename back to get_marc_components
Created attachment 124403 [details] [review] Bug 11175: (QA follow-up) Fixes for bug 12561 Bug 12561 changed the prototype for XSLTParse4Display so this patcha accounts for the additional calls to that method introduced in this patchset.
Created attachment 124404 [details] [review] Bug 11175: (follow-up) Use clean_search_term This patch uses the now public clean_search_term from bug 28316 to sanitize the title string and prevent crashes in elastic search when title contain reserved characters.
Another rebase and update of dependencies to account for issue found during QA and testing.
Martin, your rebase applies on master silently, but conflicts (catalogue/detail.pl) when to apply over dependant (as in ticket Bug 28484). Ah, now I figured more: patch 124397 removes most of Bug 28484 changed code, ... so it looks like we just need to remove dependency reference from this BZ ticket?
Created attachment 124534 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124535 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124536 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124537 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124538 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124539 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124540 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124541 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124542 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124544 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124545 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124546 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124547 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124548 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124549 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124550 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124551 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124552 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124553 [details] [review] Bug 11175: (QA follow-up) Fix queries
Created attachment 124554 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details.
Created attachment 124555 [details] [review] Bug 11175: (QA follow-up) Rename back to get_marc_components
Created attachment 124556 [details] [review] Bug 11175: (QA follow-up) Fixes for bug 12561 Bug 12561 changed the prototype for XSLTParse4Display so this patcha accounts for the additional calls to that method introduced in this patchset.
Created attachment 124557 [details] [review] Bug 11175: (follow-up) Use clean_search_term This patch uses the now public clean_search_term from bug 28316 to sanitize the title string and prevent crashes in elastic search when title contain reserved characters.
(In reply to Andrew Nugged from comment #325) > Martin, your rebase applies on master silently, > but conflicts (catalogue/detail.pl) when to apply over dependant (as in > ticket Bug 28484). > > Ah, now I figured more: patch 124397 removes most of Bug 28484 changed code, > > > ... so it looks like we just need to remove dependency reference from this > BZ ticket? There was a BZ dependency chain issue, your right. All sorted now and rebased.
The search changes introduced in bug 20277 will also need to be applied here.
bug and hotfix: we testing this on pre-prod for our libraries (we already using this feature from this ticket for a year), I found that we already have MARC::Record object in $part variable so no need to have: $part = MARC::Record->new_from_xml( $part, 'UTF-8' ); so just commenting out this line makes it work. I did this half-blindly, Martin, you might re-check this proposal if this is the real fix.
Interesting.. this appears to be part of bug 19893.. what's interesting it that as far as I can tell it changes the return value for ES searches but doesn't do the same for Zebra.. so unless I'm mistaken there's a bigger bug here.
I did such patch for our "test production" code which now serves lively for one library and works for them (and I needed this urgently because 11175 and 20447 already heavily used by our customer libraries): - $part = MARC::Record->new_from_xml( $part, 'UTF-8' ); + unless (ref $part eq 'MARC::Record') { + $part = MARC::Record->new_from_xml( $part, 'UTF-8' ); + } this is mimicking how it was before in other place. but that also might not be THAT solution, yet it passed over the "unless" for us because it already has MARC::Record in $part for us (because of ES as you said).
Created attachment 126057 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126058 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126059 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126060 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126061 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126062 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126063 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126064 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126065 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126066 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126067 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126068 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126069 [details] [review] Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126070 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126071 [details] [review] Bug 11175: (QA follow-up) We lost Encode somewhere This adds back `use Encode qw( decode )` into C4::XSLT.pm. We lost it somewhere along the way and broke the feature as it uses 'decode'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126072 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126073 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126074 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126075 [details] [review] Bug 11175: (QA follow-up) Fix queries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126076 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126077 [details] [review] Bug 11175: (QA follow-up) Rename back to get_marc_components Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126078 [details] [review] Bug 11175: (QA follow-up) Fixes for bug 12561 Bug 12561 changed the prototype for XSLTParse4Display so this patcha accounts for the additional calls to that method introduced in this patchset. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126079 [details] [review] Bug 11175: (follow-up) Use clean_search_term This patch uses the now public clean_search_term from bug 28316 to sanitize the title string and prevent crashes in elastic search when title contain reserved characters. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126080 [details] [review] Bug 11175: (QA follow-up) Account for difference in simple_search return Thanks to a comment from Andrew, I discovered that the return from simple_search_compat actually differs depending on if you are using ElasticSearch or Zebra. The 'results' arrayref contains MARC::Record objects for ES and raw marc strings if you are using Zebra. This is actually already dealt with in the misnamed new_record_from_zebra function inside C4::Search.. so this patch simply replaces our MARC::Record instantiation code with a call to that function. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126081 [details] [review] Bug 11175: (QA follow-up) Display components tab if no holdings This patch updates the default details tab selection to components for the case when there are no holdings attached to the record and there is no HTML5 content to display in preference.
Created attachment 126084 [details] [review] Bug 11175: (QA follow-up) OPAC - Display components tab if no holdings As for the staff client, this selects the default tab to be 'Components' for the case where no holdings are found.
Review, all patches squashed: 1. +use Encode qw(decode); Not used in C4/XSLT.pm One patch is saying that we need it, but the squashed version does not show that. 2. ES index added to the default mappings but to the upgrade process. Is that expected? 3. +[% IF ( ComponentParts ) %]<li id="components_tab"><a href="#components">Components ([% ( ComponentParts.size ) %])</a></li>[% END %] + <a href="#components">Components ([% ( ComponentParts.size || 1 ) %])</a> => Missing filter AND not consistent (the '|| 1')
4. Shouldn't the limit (300) be configurable?
Cannot get anything on the detail pages, the search does not return anything (tested with both ES and zebra, reset_all + import of the mrc file + full reindex) 2 debug statements in get_marc_components: warn $searchstr; warn $total_hits; output: [WARN] Host-item:(Easy piano pieces for children / compiled and edited by Lisa Cox, Sam Lung and Andrew Skirrow.) at /kohadevbox/koha/Koha/Biblio.pm line 497. [WARN] 0 at /kohadevbox/koha/Koha/Biblio.pm line 502.
(In reply to Jonathan Druart from comment #382) > Cannot get anything on the detail pages, the search does not return anything > (tested with both ES and zebra, reset_all + import of the mrc file + full > reindex) > > 2 debug statements in get_marc_components: > warn $searchstr; > warn $total_hits; > > output: > [WARN] Host-item:(Easy piano pieces for children / compiled and edited by > Lisa Cox, Sam Lung and Andrew Skirrow.) at /kohadevbox/koha/Koha/Biblio.pm > line 497. > [WARN] 0 at /kohadevbox/koha/Koha/Biblio.pm line 502. Caused by UseControlNumber=Don't use. It works when switched to "Use" (Thanks Martin!)
(In reply to Jonathan Druart from comment #383) > Caused by UseControlNumber=Don't use. > It works when switched to "Use" (Thanks Martin!) This, I think, highlights some flaws in our Host-item index.. I would have expected the search to work.. but I think that can be saved for another bug.
Created attachment 126441 [details] [review] Bug 11175: (QA follow-up) Be more specific with title fields We were using the MARC::Record title accessor, but that just stringifies all the subfields of field 245. The indexer, and the XSLT, specifically use 245a so we need to do the same.
Created attachment 126442 [details] [review] Bug 11175: (QA follow-up) Drop useless use We no longer need Encode in he XSLT module
Created attachment 126443 [details] [review] Bug 11175: (QA follow-up) Restore link to more results This patch restores the link to 'more results' should you exceed the hard coded limit of 300 component parts.
(In reply to Olli-Antti Kivilahti from comment #8) > Thanks for the QA everyone, thanks for the uncovered bugs! > I'll try to get back at this sometime next year :) Hahaha
QAing
14 files changed, 322 insertions(+), 55 deletions(-) $showcomp variable naming; show_components better Changes XSLTParse4Display; no tests. (Yes, there is no, but there should be.) Code block if ($xslsyspref =~ m/Details/) { We are fetching components while we may not use them. Have the impression that the if inside the block should be incorporated into the main condition. installer/data/mysql/atomicupdate/bug_11175.perl Needs to be converted to new style +('ShowComponentRecords', 'nowhere', 'nowhere|staff|opac|both','In which record detail pages to show list of the component records, as linked via 773','Choice'), Should nowhere just be empty?
[% IF count == 0 %] [% IF ( Koha.Preference('HTML5MediaEnabled') == 'staff' or Koha.Preference('HTML5MediaEnabled') == 'both' ) && HTML5MediaSets.size %] $('#bibliodetails').tabs("option", "active", $('#media_tab').index() ); [% ELSIF ComponentParts %] $('#bibliodetails').tabs("option", "active", $('#components_tab').index() ); [% END %] [% END %] What about indentation?
koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slimUtils.xsl koha-tmpl/opac-tmpl/bootstrap/css/src/opac.scss Seems you only remove 2 newlines, please remove these changes. + [% IF defaulttab == 'components' %] + <li id="tab_components" class="ui-tabs-active"> + [% ELSE %] + <li id="tab_components"> + [% END %] + <a href="#components">Components ([% ( ComponentParts.size ) %])</a> + </li> Indentation. diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl b/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl index 105da508c7..2df0e1b3b4 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl +++ b/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl @@ -541,7 +541,6 @@ <xsl:value-of select="$partName" /> </xsl:if> </xsl:template> - </xsl:stylesheet> <!-- Stylus Studio meta-information - (c)1998-2002 eXcelon Corp. Dont do this please
my $components = $biblio->get_marc_components(300) This 300 is hardcoded. At least a constant? Or some other configuration option? Probably the latter.
Not from this report, but very related: $self->{_components} = $results if ( defined($results) && scalar(@$results) ); } return $self->{_components} || []; Why do we save these component parts in the object and not use them later (if we would call again)?
Looks quite good to me, but still needs attention for minor details. Perfection is in the details?
Created attachment 126687 [details] [review] Bug 11175: Add syspref MaxComponentRecords to remove hardcoded limit
Created attachment 126688 [details] [review] Bug 11175: Add syspref MaxComponentRecords to remove hardcoded limit
Created attachment 126689 [details] [review] Bug 11175: Add syspref MaxComponentRecords to remove hardcoded limit
Created attachment 126690 [details] [review] Bug 11175: Uniformize ComponentParts in template More robust as it takes empty array into account
Created attachment 126692 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> JD amended path - if ($xslsyspref =~ m/Details/) { + if ( $xslsyspref eq "OPACXSLTDetailsDisplay" || $xslsyspref eq "XSLTDetailsDisplay" ) {
Created attachment 126693 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126694 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126695 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126696 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126697 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126698 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126699 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126700 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126701 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126702 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126703 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126704 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126705 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126706 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Rename back to get_marc_components Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126707 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126708 [details] [review] Bug 11175: (QA follow-up) Fix queries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126709 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Fixes for bug 12561 Bug 12561 changed the prototype for XSLTParse4Display so this patcha accounts for the additional calls to that method introduced in this patchset. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126710 [details] [review] Bug 11175: (follow-up) Use clean_search_term This patch uses the now public clean_search_term from bug 28316 to sanitize the title string and prevent crashes in elastic search when title contain reserved characters. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126711 [details] [review] Bug 11175: (QA follow-up) Account for difference in simple_search return Thanks to a comment from Andrew, I discovered that the return from simple_search_compat actually differs depending on if you are using ElasticSearch or Zebra. The 'results' arrayref contains MARC::Record objects for ES and raw marc strings if you are using Zebra. This is actually already dealt with in the misnamed new_record_from_zebra function inside C4::Search.. so this patch simply replaces our MARC::Record instantiation code with a call to that function. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com>
Created attachment 126712 [details] [review] Bug 11175: (QA follow-up) Display components tab if no holdings This patch updates the default details tab selection to components for the case when there are no holdings attached to the record and there is no HTML5 content to display in preference. Bug 11175: (QA follow-up) OPAC - Display components tab if no holdings As for the staff client, this selects the default tab to be 'Components' for the case where no holdings are found.
Created attachment 126713 [details] [review] Bug 11175: (QA follow-up) Be more specific with title fields We were using the MARC::Record title accessor, but that just stringifies all the subfields of field 245. The indexer, and the XSLT, specifically use 245a so we need to do the same.
Created attachment 126714 [details] [review] Bug 11175: (QA follow-up) Restore link to more results This patch restores the link to 'more results' should you exceed the hard coded limit of 300 component parts.
Created attachment 126715 [details] [review] Bug 11175: Add syspref MaxComponentRecords to remove hardcoded limit
Created attachment 126716 [details] [review] Bug 11175: Uniformize ComponentParts in template More robust as it takes empty array into account
Some patches have been squashed for readability. (In reply to Marcel de Rooy from comment #390) > 14 files changed, 322 insertions(+), 55 deletions(-) > > $showcomp variable naming; show_components better Too late, too complicated or we add a separate commit for that. Not a big deal IMO. > Changes XSLTParse4Display; no tests. (Yes, there is no, but there should be.) Still missing, I will try and provide some a bit later today. > Code block if ($xslsyspref =~ m/Details/) { > We are fetching components while we may not use them. > Have the impression that the if inside the block should be incorporated into > the main condition. Done and squashed with the first patch. > installer/data/mysql/atomicupdate/bug_11175.perl > Needs to be converted to new style I will do it when pushing it. > +('ShowComponentRecords', 'nowhere', 'nowhere|staff|opac|both','In which > record detail pages to show list of the component records, as linked via > 773','Choice'), > Should nowhere just be empty? empty or 'nowhere' is fine. (In reply to Marcel de Rooy from comment #391) > What about indentation? Done and squashed. (In reply to Marcel de Rooy from comment #392) > koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slimUtils.xsl > koha-tmpl/opac-tmpl/bootstrap/css/src/opac.scss > > Seems you only remove 2 newlines, please remove these changes. Done, squashed. > + [% IF defaulttab == 'components' %] > + <li id="tab_components" class="ui-tabs-active"> > + [% ELSE %] > + <li id="tab_components"> > + [% END %] > + <a href="#components">Components ([% ( > ComponentParts.size ) %])</a> > + </li> > > Indentation. Done, squashed. > diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl > b/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl > index 105da508c7..2df0e1b3b4 100644 > --- a/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl > +++ b/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl > @@ -541,7 +541,6 @@ > <xsl:value-of select="$partName" /> > </xsl:if> > </xsl:template> > - > </xsl:stylesheet> > > <!-- Stylus Studio meta-information - (c)1998-2002 eXcelon Corp. > > Dont do this please Done, squashed. (In reply to Marcel de Rooy from comment #393) > my $components = $biblio->get_marc_components(300) > > This 300 is hardcoded. At least a constant? Or some other configuration > option? > Probably the latter. Done, see follow-up. (In reply to Marcel de Rooy from comment #394) > Not from this report, but very related: > > $self->{_components} = $results if ( defined($results) && > scalar(@$results) ); > } > > return $self->{_components} || []; > > Why do we save these component parts in the object and not use them later > (if we would call again)? Yes, that's a bad pattern. We discussed that recently but cannot remember where. We shouldn't cache that in the object IMO.
(In reply to Jonathan Druart from comment #425) > Some patches have been squashed for readability. > > (In reply to Marcel de Rooy from comment #390) > > 14 files changed, 322 insertions(+), 55 deletions(-) > > > > $showcomp variable naming; show_components better > > Too late, too complicated or we add a separate commit for that. Not a big > deal IMO. I was trying to be light-touch when I first adopted this bug.. but happy to change this too. > > > Changes XSLTParse4Display; no tests. (Yes, there is no, but there should be.) > > Still missing, I will try and provide some a bit later today. Damn, I meant to add these tests. > > > Code block if ($xslsyspref =~ m/Details/) { > > We are fetching components while we may not use them. > > Have the impression that the if inside the block should be incorporated into > > the main condition. We use them for the 'Show analytics' link handling stuff.. I was also not a fan of always firing this query.. but that predates this patchset. > (In reply to Marcel de Rooy from comment #394) > > Not from this report, but very related: > > > > $self->{_components} = $results if ( defined($results) && > > scalar(@$results) ); > > } > > > > return $self->{_components} || []; > > > > Why do we save these component parts in the object and not use them later > > (if we would call again)? > > Yes, that's a bad pattern. We discussed that recently but cannot remember > where. > We shouldn't cache that in the object IMO. The caching here appears to be completely broken to me.. likely by one of the many many rebases/follow-ups. The idea was that we execute this search from both C4::XSLT (for the 'Show analytics' link handling) AND each of the details controllers.. caching at the object level would mean we could re-use the results from the controller level at the XSLT call. I'm not sure how else you could cache this if not at the object level.
Yup.. blames shows me that 'Bug 11175: (follow-up) Return empty array if no components' drops the `if (!defined($self->{_components})` which is actually what does the caching.
But.. as we don't actually pass the Biblio object around.. it's kinda pointless anyway.. feel free to just drop the whole _components internal stuff.. at this point it serves no purpose. Good catch.. I've been too close to this and too along to spot everything all these months.
Created attachment 126717 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> JD amended path - if ($xslsyspref =~ m/Details/) { + if ( $xslsyspref eq "OPACXSLTDetailsDisplay" || $xslsyspref eq "XSLTDetailsDisplay" ) { Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126718 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126719 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126720 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126721 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126722 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126723 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126724 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126725 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126726 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126727 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126728 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126729 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126730 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126731 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Rename back to get_marc_components Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126732 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126733 [details] [review] Bug 11175: (QA follow-up) Fix queries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126734 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Fixes for bug 12561 Bug 12561 changed the prototype for XSLTParse4Display so this patcha accounts for the additional calls to that method introduced in this patchset. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126735 [details] [review] Bug 11175: (follow-up) Use clean_search_term This patch uses the now public clean_search_term from bug 28316 to sanitize the title string and prevent crashes in elastic search when title contain reserved characters. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126736 [details] [review] Bug 11175: (QA follow-up) Account for difference in simple_search return Thanks to a comment from Andrew, I discovered that the return from simple_search_compat actually differs depending on if you are using ElasticSearch or Zebra. The 'results' arrayref contains MARC::Record objects for ES and raw marc strings if you are using Zebra. This is actually already dealt with in the misnamed new_record_from_zebra function inside C4::Search.. so this patch simply replaces our MARC::Record instantiation code with a call to that function. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126737 [details] [review] Bug 11175: (QA follow-up) Display components tab if no holdings This patch updates the default details tab selection to components for the case when there are no holdings attached to the record and there is no HTML5 content to display in preference. Bug 11175: (QA follow-up) OPAC - Display components tab if no holdings As for the staff client, this selects the default tab to be 'Components' for the case where no holdings are found. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126738 [details] [review] Bug 11175: (QA follow-up) Be more specific with title fields We were using the MARC::Record title accessor, but that just stringifies all the subfields of field 245. The indexer, and the XSLT, specifically use 245a so we need to do the same. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126739 [details] [review] Bug 11175: (QA follow-up) Restore link to more results This patch restores the link to 'more results' should you exceed the hard coded limit of 300 component parts. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126740 [details] [review] Bug 11175: Add syspref MaxComponentRecords to remove hardcoded limit Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126741 [details] [review] Bug 11175: Uniformize ComponentParts in template More robust as it takes empty array into account Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126742 [details] [review] Bug 11175: (QA follow-up) Simplify block in XSLT Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126743 [details] [review] Bug 11175: (QA follow-up) Remove caching _components Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment on attachment 126742 [details] [review] Bug 11175: (QA follow-up) Simplify block in XSLT Tests are not passing with this patch, keeping my version.
Created attachment 126744 [details] [review] Bug 11175: Add tests and correct show_analytics logic
Created attachment 126745 [details] [review] Bug 11175: Add tests and correct show_analytics logic
(In reply to Jonathan Druart from comment #458) > Created attachment 126745 [details] [review] [review] > Bug 11175: Add tests and correct show_analytics logic Somehow it still feels a bit strange to do this in the XSLT module.
And what about showing a link without fetching all components? In terms of performance?
(In reply to Marcel de Rooy from comment #459) > (In reply to Jonathan Druart from comment #458) > > Created attachment 126745 [details] [review] [review] [review] > > Bug 11175: Add tests and correct show_analytics logic > > Somehow it still feels a bit strange to do this in the XSLT module. It was moved to the XSLT module for two reasons.. 1. DRY the code out.. it was sat in the controllers before and was slightly different in each.. so had bugs. 2. We were already passing a variable through to XSLT so we could use it in the stylesheets to work out whether to show the link or not.. thus it made sense to deal with it in the XSLT parser rather than passing it around at multiple levels.
(In reply to Marcel de Rooy from comment #460) > And what about showing a link without fetching all components? In terms of > performance? You mean undoing bug 15851 effectively reverting it and dropping the requested bugfix/feature.?
(In reply to Martin Renvoize from comment #462) > (In reply to Marcel de Rooy from comment #460) > > And what about showing a link without fetching all components? In terms of > > performance? > > You mean undoing bug 15851 effectively reverting it and dropping the > requested bugfix/feature.? This has an on/off switch now, so I think if you are worried about performance, you can not use the feature for now and still implement something later on if needed.
(In reply to Katrin Fischer from comment #463) > This has an on/off switch now, so I think if you are worried about > performance, you can not use the feature for now and still implement > something later on if needed. I think you're misunderstanding.. regardless of the preference the search will take place.. and has been taking place since bug 15851... I just make sure we don't execute it twice.
Note: Bug 26314 will do the same for volumes.. the only way we can selectively show/now show the 'Show volumes' link is to pre-execute the search to see if it will get results ;)
The performance you DO save by disabling this one is an XSLT parse of each of the resultant linked records.
(In reply to Martin Renvoize from comment #466) > The performance you DO save by disabling this one is an XSLT parse of each > of the resultant linked records. Thx for clarifying!
Well, just asking. Dont worry. Not saying that we should revert things, people. But the fact that something has been pushed does certainly not always mean that it cannot be done better, different, etc. Step back and look again.
(In reply to Martin Renvoize from comment #464) > I think you're misunderstanding.. regardless of the preference the search > will take place.. and has been taking place since bug 15851... I just make > sure we don't execute it twice. I am not sure if this is correct btw. Where does that happen?
Note that I am referring to XSLTParse4Display and not a detail script. I can understand this better going on in detail but not over a page of search results.
Still thinking about this a bit, and looking here: for my $part ( @{$components} ) { $part = C4::Search::new_record_from_zebra( 'biblioserver', $part ); push @{$parts}, XSLTParse4Display( { biblionumber => $biblionumber, Why pass the same biblionumber here when running through all component records?
(In reply to Marcel de Rooy from comment #471) > Why pass the same biblionumber here when running through all component > records? Beginning to understand some of what happened here ;) Finally :) We still need a bit of fixing things that may have been broken during the process. Changing status. Will be submitting a follow-up. Dont touch please (today or tomorrow).
I wrote two follow-ups now. WIll attach them tomorrow. But did look a bit closer to analytical records in Koha and they are quite different from what is done here. They use 773 but thats about it. So there seems to be no reason to braid(?) them together here. Making the analytics disappear when there are components. Note: the components here work with 773 and record control numbers looked up in biblio records. Analytics works different, storing item numbers(!) in 773$9. This seems to be something that we add on top of MARC21, if I understand correctly (so no default MARC21). Conclusion: We should keep them really separate from each other. Will have to add that to my follow-up still. Which should be very trivial. Katrin: Could you confirm?
Created attachment 126816 [details] [review] Bug 11175: (QA follow-up) Remove caching _components Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126817 [details] [review] Bug 11175: (QA follow-up) Do not fetch twice We were fetching components in the first call of XSLTParse4Display (opac-detail around L220). And again (opac-detail around L660). Same for catalogue/detail.pl. Moving the XSLT block in both scripts and removing the code from XSLT.pm which again makes the tests obsolete. Not hiding the link when there are components. Might be helpful too. Also fixing biblionumber in the XSLTParse4Display call for parts. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested this and feeling more comfortable now. The value of the MaxComponents syspref is 300 now. Which feels a bit high. But no problem. The EasyAnalytics stuff in Koha made this development a bit harder to understand. That area needs probably a bit more attention. Switching back to SO. If Martin agrees, we can move it back to PQA imho.
+ [% IF ComponentParts.size == Koha.Preference('MaxComponentRecords')%] Might be better btw to check with >.
(In reply to Marcel de Rooy from comment #473) > I wrote two follow-ups now. WIll attach them tomorrow. > > But did look a bit closer to analytical records in Koha and they are quite > different from what is done here. They use 773 but thats about it. So there > seems to be no reason to braid(?) them together here. Making the analytics > disappear when there are components. > > Note: the components here work with 773 and record control numbers looked up > in biblio records. Analytics works different, storing item numbers(!) in > 773$9. This seems to be something that we add on top of MARC21, if I > understand correctly (so no default MARC21). > > Conclusion: We should keep them really separate from each other. Will have > to add that to my follow-up still. Which should be very trivial. > > Katrin: Could you confirm? I believe there are some major differences between the features - so fully agree. What we are looking for here is linking between bibliographic records. We link a record to another record, using either $w or a combination of title and author from 773 for search. So this is about hierarchies on the bibliographic record level. EasyAnalytics as I understand it, is linking an item to multiple records using non-standard MARC subfields of 773. It's a useful feature, but a different use case. For example it could probably be used very well for "bound-togethers", where different works where combined into one bound unit (often found in historical holdings). We could probably make both work together by checking for the prefs and maybe existence of certain subfields in 773, but as UseControlnumber and Easyanalytics are already mutually exclusive right now, I think it's outside of scope here.
(In reply to Katrin Fischer from comment #478) > I believe there are some major differences between the features - so fully > agree. > > What we are looking for here is linking between bibliographic records. We > link a record to another record, using either $w or a combination of title > and author from 773 for search. So this is about hierarchies on the > bibliographic record level. > > EasyAnalytics as I understand it, is linking an item to multiple records > using non-standard MARC subfields of 773. It's a useful feature, but a > different use case. For example it could probably be used very well for > "bound-togethers", where different works where combined into one bound unit > (often found in historical holdings). > > We could probably make both work together by checking for the prefs and > maybe existence of certain subfields in 773, but as UseControlnumber and > Easyanalytics are already mutually exclusive right now, I think it's outside > of scope here. Thx for replying. We are on the same track.
Fair enough, if you feel we should move the check back to the two controllers I'm OK with that.. I've DRY'd the code in other regards now so that bit of refactoring can get undone. You're incorrect in saying that the search query would have ever been executed twice though.. we do not use the 'Details' templates for the display of the component parts when inlined, so the block with the query within it would not have been executed. Regarding having the 'Show analytics' link regardless of the preference.. that's fine too; Though personally, I feel it perhaps confuses the end-user with two links to display the same information.. one without a re-fetch and one with. I feel this bug has gone on long enough and any further refinements should be in their own bugs.. it worries me that so much is still going on at the end of the development cycle of the bug personally. Don't you also need to remove some XSLT tests if you're moving the code back out into the Controllers?
Now things have changed again I'm not confident in rebasing the set again.. no longer applies.
Needs rebasing for bug 29284 conflicts.. this will probably be easier with a significant squash and loss of history.
(In reply to Martin Renvoize from comment #480) > Fair enough, if you feel we should move the check back to the two > controllers I'm OK with that.. I've DRY'd the code in other regards now so > that bit of refactoring can get undone. You're incorrect in saying that the > search query would have ever been executed twice though.. we do not use the > 'Details' templates for the display of the component parts when inlined, so > the block with the query within it would not have been executed. Never say never: [2021/10/26 07:31:06] [WARN] components: C4::XSLT at /usr/share/koha/Koha/Biblio.pm line 503. [2021/10/26 07:31:06] [WARN] components: CGI::Compile::ROOT::usr_share_koha_opac_opac_2ddetail_2epl at /usr/share/koha/Koha/Biblio.pm line 503. Without the last follow-up. One view of opac-detail. First warn comes from XSLT as expected. Second call from opac-detail line 664 (really, as expected too). Just read the code. Ha, who really DRYed here? :) > Regarding having the 'Show analytics' link regardless of the preference.. > that's fine too; Though personally, I feel it perhaps confuses the end-user > with two links to display the same information.. one without a re-fetch and > one with. No time left for a debate here ;) If this would really be so confusing, we can remove it later or just css it away. Dont think so. > I feel this bug has gone on long enough and any further refinements should > be in their own bugs.. it worries me that so much is still going on at the > end of the development cycle of the bug personally. > > Don't you also need to remove some XSLT tests if you're moving the code back > out into the Controllers? Was already done.
(In reply to Martin Renvoize from comment #482) > Needs rebasing for bug 29284 conflicts.. this will probably be easier with a > significant squash and loss of history. I'll do it.
Created attachment 126869 [details] [review] Bug 11175: Show record component parts in the detail view Shows the component records of a host, on the hosts detail view in staff client or OPAC, with clickable links to the component records. The host does not require linking entries to the components, but components do require a link to the host record via 773$w. Adds a new search index, Control-number-identifier (aka cni), which indexes the 003 controlfield. Adds 'Yet Another System Preference', ShowComponentRecords, which can be used to turn this feature on or off in staff client and/or OPAC, and defaults to off. When looking up the component part records, the code searches for records with (773$w=Host001 and 003=Host003) or 773$w='Host003 Host001' or, if the 003 is not defined in the Host, 773$w=Host001. Does not use easyanalytics or useControlNumber. Only for MARC21 biblios - UNIMARC has not been updated. staff-global.css and opac.css have not been recreated, so you need to use sass to recreate those from staff-global.scss and opac.scss Test plan: 0) Apply patch 1) perl bulkmarcimport -file /tmp/easypiano.mrc -m MARCXML (This file is an attachment on the bug) 2) rebuild the zebra biblio index 3) Search for "easy piano" in staff client, and go to the biblio detail page. You should not see anything different in the record detail page. 4) Do the same on OPAC. 5) Change the ShowComponentRecords syspref appropriately and check the record detail page in staff client and OPAC. You should see a list of component part records. Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> JD amended path - if ($xslsyspref =~ m/Details/) { + if ( $xslsyspref eq "OPACXSLTDetailsDisplay" || $xslsyspref eq "XSLTDetailsDisplay" ) { Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126870 [details] [review] Bug 11175: (follow-up) Don't return explicitly undef This would create in list context an list of one element that is undef, which we don't want, we want empty list if there are no components. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126871 [details] [review] Bug 11175: (follow-up) Return empty array if no components Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126872 [details] [review] Bug 11175: (follow-up) Relocate and reword ShowComponentRecords syspref description The syspref is not only affecting staff interface so moving it to Cataloguing section. Added also notes about UNIMARC and Elasticsearch not being supported. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126873 [details] [review] Bug 11175: Add Elasticsearch support Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126874 [details] [review] Bug 11175: Keep component part record list items inside the box Without this the number denoting the component part item in the list will overflow and is partly hidden. To test: 1) Create 100 component parts 2) Notice without this patch the number next to the component part is not fully shown Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126875 [details] [review] Bug 11175: Limit the amount of component parts returned There was already before this a limit of 100 results when using Elasticsearch but then the list of component parts was truncated silently. This change now limits the amount to hard coded limit of 300 which is still fast to render. Also when the 300 component part record limit is reached there is a link in the list now to list all the records via the cataloging search. To test: 1) Create 300+ component part records and see if the link to list all the component part records shows up 2) Make sure prove t/Koha/Util/Search.t passes Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Replace en dash with hyphen in perldoc This fixes the QA tool failure. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126876 [details] [review] Bug 11175: Search using double quotes to support Elasticsearch With Elasticsearch if you would use the search string here without the change to double quotes, i.e. (rcn='1234' AND cni='FI-XXX') OR rcn='FI-XXX 1234' then the search would somehow manage to return a record with 001 = 1234 and 003 = FI-XXX. Using double quotes prevents that. Also using parenthesis around the latter part of the OR seems to work: (rcn='1234' AND cni='FI-XXX') OR (rcn='FI-XXX 1234') Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126877 [details] [review] Bug 11175: Remove outdated info about Elasticsearch not being suppported Support for Elasticsearch was added in commit "Bug 11175: Add Elasticsearch support". Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126878 [details] [review] Bug 11175: (follow-up) Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126879 [details] [review] Bug 11175: (follow-up) make atomic update idempotent Atomic update tries to insert line into system preferences even if it's already there. This patch fixes that by adding 'IGNORE' to the query, making it idempotent. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126880 [details] [review] Bug 11175: (follow-up) Rename routine get_marc_components For consistency with other marc data accessor methods rename the new 'components' method to 'get_marc_components'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126881 [details] [review] Bug 11175: (QA follow-up) Move preference to 'Display' The 'ShowComponentRecords' preference was displaying under the 'Export' section in the cataloging system preferences tab. As a display related feature, I felt it was more appropriate to put it into the 'Display' section. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126882 [details] [review] Bug 11175: (QA follow-up) Take account of bug 15851 We can simplify the code introduced by bug 15851 by moving the 'show_analytics_link' variable assignment into C4::XSLT and thus making the code more DRY. Taking the code in bug 15851 as inspiration this patch also adds proper handling for UseControlNumber vs EasyAnalytics style 773 linking and ensures we only return analytic component parts and no other records containing 773's. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126883 [details] [review] Bug 11175: (QA follow-up) Move get_component_part_query This patch removes Koha::Util::Search in preference to embedding the search query builder in Koha::Biblio as get_analytics_query. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Rename back to get_marc_components Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126884 [details] [review] Bug 11175: (QA follow-up) Only get biblio once Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126885 [details] [review] Bug 11175: (QA follow-up) Fix queries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126886 [details] [review] Bug 11175: Display components in own tab This patch updates the display so that rather than displaying the components using the main XSLT and then using CSS to move them into a box on the right side for display, which causes issues with RTL records; We instead add them in a new tab beneath the rest of the record details alongside holdings and other details. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Bug 11175: (QA follow-up) Fixes for bug 12561 Bug 12561 changed the prototype for XSLTParse4Display so this patcha accounts for the additional calls to that method introduced in this patchset. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126887 [details] [review] Bug 11175: (follow-up) Use clean_search_term This patch uses the now public clean_search_term from bug 28316 to sanitize the title string and prevent crashes in elastic search when title contain reserved characters. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126888 [details] [review] Bug 11175: (QA follow-up) Account for difference in simple_search return Thanks to a comment from Andrew, I discovered that the return from simple_search_compat actually differs depending on if you are using ElasticSearch or Zebra. The 'results' arrayref contains MARC::Record objects for ES and raw marc strings if you are using Zebra. This is actually already dealt with in the misnamed new_record_from_zebra function inside C4::Search.. so this patch simply replaces our MARC::Record instantiation code with a call to that function. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Andrew Nugged <nugged@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126889 [details] [review] Bug 11175: (QA follow-up) Display components tab if no holdings This patch updates the default details tab selection to components for the case when there are no holdings attached to the record and there is no HTML5 content to display in preference. Bug 11175: (QA follow-up) OPAC - Display components tab if no holdings As for the staff client, this selects the default tab to be 'Components' for the case where no holdings are found. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126890 [details] [review] Bug 11175: (QA follow-up) Be more specific with title fields We were using the MARC::Record title accessor, but that just stringifies all the subfields of field 245. The indexer, and the XSLT, specifically use 245a so we need to do the same. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126891 [details] [review] Bug 11175: (QA follow-up) Restore link to more results This patch restores the link to 'more results' should you exceed the hard coded limit of 300 component parts. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126892 [details] [review] Bug 11175: Add syspref MaxComponentRecords to remove hardcoded limit Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126893 [details] [review] Bug 11175: Uniformize ComponentParts in template More robust as it takes empty array into account Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126894 [details] [review] Bug 11175: (QA follow-up) Remove caching _components Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126895 [details] [review] Bug 11175: (QA follow-up) Do not fetch twice We were fetching components in the first call of XSLTParse4Display (opac-detail around L220). And again (opac-detail around L660). Same for catalogue/detail.pl. Moving the XSLT block in both scripts and removing the code from XSLT.pm which again makes the tests obsolete. Not hiding the link when there are components. Might be helpful too. Also fixing biblionumber in the XSLTParse4Display call for parts. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Latest rebase effectively reverts bug 29284, failing QA
Created attachment 126897 [details] [review] Bug 11175: (QA follow-up) Restore bug 29284 This patch restores the functional fixes introduced in bug 29284 and also prevents the 'Show analytics' link from displaying when no component parts are found and inline display is enabled.
Created attachment 126898 [details] [review] Bug 11175: (QA follow-up) Restore bug 29284 This patch restores the functional fixes introduced in bug 29284 and also prevents the 'Show analytics' link from displaying when no component parts are found and inline display is enabled.
Created attachment 126904 [details] [review] Bug 11175: (QA follow-up) Restore bug 29284 This patch restores the functional fixes introduced in bug 29284 and also prevents the 'Show analytics' link from displaying when no component parts are found and inline display is enabled. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: Fixed error with $err vs $error(s) :)
Created attachment 126911 [details] [review] Bug 11175: (QA follow-up) Test message contents for analytics error Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126912 [details] [review] Bug 11175: (QA follow-up) Test message contents for analytics error Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 126913 [details] [review] Bug 11175: Add tests Add coverage for the message set by get_marc_components when the search fails. Also cleans a bit the weird return comparisons.
Pushed to master for 21.11, thanks to everybody involved!
Nice one.. faith in community restored.. sorry for being grumpy
Bug 11175, we have gone such a long way together. I am so proud of you, our bug.
(In reply to Martin Renvoize from comment #520) > Nice one.. faith in community restored.. sorry for being grumpy Now I'm feeling grumpy reading through these patches again [U+1F605]. (In reply to Marcel de Rooy from comment #499) > Created attachment 126883 [details] [review] [review] > Bug 11175: (QA follow-up) Move get_component_part_query This patch removed the double-quotes from the search terms which causes this to break when using Zebra if you're using a title like "MyTitle: MySubtitle" since the following query is a syntax error: Host-item=(MyTitle= MySubtitle) It should be: Host-item=("MyTitle= MySubtitle") According to https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html#query-string-syntax, it looks like Elasticsearch works the same way. Single words don't need to be quoted but terms consisting of more than one word do. Regarding Zebra, we actually have the tools to easily catch this in our unit tests: #!/usr/bin/perl use strict; use warnings; use C4::Context; use ZOOM; my $Zconn = C4::Context->Zconn; eval { my $bad = ZOOM::Query::CCL2RPN->new("Host-item=(MyTitle= MySubtitle)",$Zconn); }; warn $@ if $@; my $good = ZOOM::Query::CCL2RPN->new('Host-item=("MyTitle= MySubtitle")',$Zconn); That code will output the following error: ZOOM error 10014 "CCL parsing error" (addinfo: "Unknown qualifier") from diag-set 'ZOOM' at test.pl line 10. It looks like https://metacpan.org/pod/Search::Elasticsearch::Compat::QueryParser#check() could provide similar error checking for Elasticsearch. If we just syntax checked our query strings, we'd run into far fewer problems... Anyway, I'll open a new bug report for this one. I just figured it would be good for folk to hear about it.
(In reply to David Cook from comment #522) > (In reply to Martin Renvoize from comment #520) > > Nice one.. faith in community restored.. sorry for being grumpy > > Now I'm feeling grumpy reading through these patches again [U+1F605]. > Actually now that I think about it... I feel like I've already grumped about this code recently... Ah yes: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29418#c16
(In reply to David Cook from comment #522) > > Regarding Zebra, we actually have the tools to easily catch this in our unit > tests: > Except of course we're feeding the output of "get_components_query" into $searcher->simple_search_compat or into search.pl/opac-search.pl "q" parameter which both transform the query differently (both for Zebra and for Elasticsearch so we get at least 4x more things to check). Well... if it were easy it would already be done [U+1F923]. Someday...