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>