Description
Jared Camins-Esakov
2012-01-07 22:55:19 UTC
Created attachment 11028 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Created attachment 11253 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 1 August 2012 I am very very excited about this feature, as it's something a lot of libraries have asked about. It already works, but we have to fix some rough edges: 1) Blocker: The detail page is all ok, but in the result list all the "see from" variations of the name show up in the results. 2) Blocker: There is a problem with the concatenation of strings for indexing: Authority record: used for/see from: Abraham, Barfüßer Result list (see 1) ): AbrahamBarfüßer; Search: Barfüßer does not give any results, but AbrahamBarfüßer does - so I think that display reflects the indexing here. 3) Tiny: You have to reindex _after_ activating the feature. I think it would be good to note that in the text of the system preference so people know that it won't work immediately and what to do about it. Created attachment 11319 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 1 August 2012 (In reply to comment #3) > I am very very excited about this feature, as it's something a lot of > libraries have asked about. It already works, but we have to fix some rough > edges: > > 1) Blocker: The detail page is all ok, but in the result list all the "see > from" variations of the name show up in the results. Fixed. > 2) Blocker: There is a problem with the concatenation of strings for > indexing: > > Authority record: used for/see from: Abraham, Barfüßer > > Result list (see 1) ): AbrahamBarfüßer; > > Search: Barfüßer does not give any results, but AbrahamBarfüßer does - so I > think that display reflects the indexing here. I was not able to replicate the problem, but I have an idea what it might have been, and fixed the issue I identified, so let me know if the updated patch works. > 3) Tiny: You have to reindex _after_ activating the feature. I think it > would be good to note that in the text of the system preference so people > know that it won't work immediately and what to do about it. Noted. Created attachment 11362 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 5 August 2012 Created attachment 11363 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 5 August 2012 Created attachment 11364 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 5 August 2012 Created attachment 11365 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 5 August 2012 Created attachment 11366 [details] [review] [SIGNED-OFF] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 5 August 2012 Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Also checked: - Verified database update works correctly - Checked system preference and its description - Checked staff/opac detail pages with feature on/off - Checked staff/opac search facets - Downloaded and tested records in various formats - Tried different searches for 'see from' entries of authorities - Ran all unit tests No problems found. QA Comments: This patch fixes a "Trailing comma of death" (see http://www.enterprisedojo.com/2010/12/19/beware-the-trailing-comma-of-death/) for IE Passed QA Oupps ! Wrong bug, sorry Hi Jared, For discussion: This patch is interesting. It introduces some new notions into Koha. For example, objects appear into Koha with an (not one of the least) Authority module \o/ But ... :) Why do you choose an old method to do OOP in Perl (bless, SUPER) ? I think you may have heard about Moose or Moo, why don't use one of them directly from the beginning of the Koha::* rewriting? It seems your naming into the Koha namespace is not in agreement with the wiki page: http://wiki.koha-community.org/wiki/Koha_Namespace_RFC For new files, I think a best practice is to use "use Modern::Perl" instead of "use strict; use warnings;" But nothing is written on the wiki. You are free to keep these lines. I really like your system of plugin. Our BibLibre dev/solr branch already have this kind of plugin for the routine index_record. This system is used for 1 index (for example we have a lot of plugins: Date (for index containing dates), Callnumber, isbn (for a dash management), etc.). Here we want to add a processing in 1 case (but other cases will certainly come). We can imagine to add a new part in the searchengine configuration page containing a list of features like this one. Thus, the routine will grow quickly and make it unmaintanable. In fact, we would have a pluggable system here too. But it is not for now... (In reply to comment #13) > Hi Jared, > > For discussion: > > This patch is interesting. It introduces some new notions into Koha. > For example, objects appear into Koha with an (not one of the least) > Authority module \o/ > But ... :) > > Why do you choose an old method to do OOP in Perl (bless, SUPER) ? I think > you may have heard about Moose or Moo, why don't use one of them directly > from the beginning of the Koha::* rewriting? > This is using Class::Accessor http://search.cpan.org/~rjbs/perl-5.16.0/pod/perlootut.pod#Class::Accessor Which introduces no new dependencies, and importantly it's fast Class::Accessor does not employ an autoloader, thus it is much faster than you'd think. Its generated methods incur no special penalty over ones you'd write yourself. And for those who want it, it has a moose like interface http://search.cpan.org/~kasei/Class-Accessor-0.34/lib/Class/Accessor.pm#Moose-like (In reply to comment #14) Chris, Thanks for your answer. > Which introduces no new dependencies Koha already use a lot of dependencies. For a dependency as much important part (OOP) I think it is not a good argument. Furthermore this dependency will be used elsewhere :) > and importantly it's fast Class::Accessor does not employ an autoloader Yes, I agree with you, fastness is one of the more important thing > thus it is much faster than you'd think. Its generated methods incur no > special penalty over ones you'd write yourself. Ok cool. But it is just for accessor isn't it ? I think we will quickly want to add relations between our classes (roles, superclasses, etc.). Before introducing new important classes (as Authority) in the Koha namespace, I think it is important to start a global discussion. What do we want for this new namespace and for this rewriting ? It would be great to have new classes written as we want from the beginning. This would show what are the best practices to follow for futures classes. To have a reflexion about how we want to rewrite the C4 modules seems to be the first step. How we want to order our classes ? What Koha::Authority must do ? We agree we don't want all C4::AuthoritiesMarc into Koha::Authority :) Mo[o][se] module[s] permits to create classes (with attr, method, etc.), inheritance, etc. In fact it allows to do OO, very simply if an application design has been made before. If we don't take the time to make cleanly things, my fear is to see a lot of extra developements inside the module not related to it. Hi Jonathan, I think I am more worried that we won't ever agree on something and we should start doing things. Also sometimes something in theory is good, but doesn't work really well in reality. Jared does a lot of work in authorities recently and has revamped a big part of it already, I would hate to see this work lost. Especially this feature is of big importance to libraries here. I think this could be seen as a working example of how we could make use of Class::Accessor, while I think the Solr code is using Moose? (In reply to comment #13) > Hi Jared, > > For discussion: > > This patch is interesting. It introduces some new notions into Koha. > For example, objects appear into Koha with an (not one of the least) > Authority module \o/ > But ... :) > > Why do you choose an old method to do OOP in Perl (bless, SUPER) ? I think > you may have heard about Moose or Moo, why don't use one of them directly > from the beginning of the Koha::* rewriting? My experience with Moose and CGI is that it's kind of slow. This seems like a huge problem on something as core as authority and bibliographic record objects. Also, after I tried to get the solr code working on my system Moose no longer functions. Choosing between core functionality (with a version of Moose that installs) and Solr (with a complex install procedure that will presumably leave Moose not working elsewhere) seems like a bad choice. > It seems your naming into the Koha namespace is not in agreement with the > wiki page: http://wiki.koha-community.org/wiki/Koha_Namespace_RFC It's easy to do a follow up to change the name. If you tell me what you'd like it called, I will do a follow-up that changes the name. I think it should probably be Koha::DataObject::Authority, based on the RFC, but I decided to just call it "Koha::Authority" until I had a second opinion. > For new files, I think a best practice is to use "use Modern::Perl" instead > of "use strict; use warnings;" > But nothing is written on the wiki. You are free to keep these lines. I'm not attached to them. :) > I really like your system of plugin. Our BibLibre dev/solr branch already > have this kind of plugin for the routine index_record. This system is used > for 1 index (for example we have a lot of plugins: Date (for index > containing dates), Callnumber, isbn (for a dash management), etc.). > Here we want to add a processing in 1 case (but other cases will certainly > come). We can imagine to add a new part in the searchengine configuration > page containing a list of features like this one. > Thus, the routine will grow quickly and make it unmaintanable. In fact, we > would have a pluggable system here too. But it is not for now... I wrote this with the idea that we should be able to use this filter system elsewhere in Koha, too... as a place to put the filters from dev/solr, as well as (conceivably) before display. This of course makes speed even more crucial. (In reply to comment #15) > (In reply to comment #14) > > Chris, > Thanks for your answer. > > > Which introduces no new dependencies > Koha already use a lot of dependencies. For a dependency as much important > part (OOP) I think it is not a good argument. > Furthermore this dependency will be used elsewhere :) > > > and importantly it's fast Class::Accessor does not employ an autoloader > Yes, I agree with you, fastness is one of the more important thing > > > thus it is much faster than you'd think. Its generated methods incur no > > special penalty over ones you'd write yourself. > Ok cool. > But it is just for accessor isn't it ? > I think we will quickly want to add relations between our classes (roles, > superclasses, etc.). > Before introducing new important classes (as Authority) in the Koha > namespace, I think it is important to start a global discussion. What do we > want for this new namespace and for this rewriting ? I think we want it to work, and provide much-needed features. > It would be great to have new classes written as we want from the beginning. > This would show what are the best practices to follow for futures classes. > To have a reflexion about how we want to rewrite the C4 modules seems to be > the first step. How we want to order our classes ? What Koha::Authority must > do ? We agree we don't want all C4::AuthoritiesMarc into Koha::Authority :) We have been talking about this for a year, and no one actually seems to have much of an opinion, other than the concepts set down in the namespace RFC. As for what Koha::Authority does, I propose the following list: 1. Retrieve an authority from the database. 2. Update/create an authority in the database. 3. Retrieve metadata about the authority. 4. Set metadata about the authority. 5. There is no five. > Mo[o][se] module[s] permits to create classes (with attr, method, etc.), > inheritance, etc. In fact it allows to do OO, very simply if an application > design has been made before. I don't think I see the benefit here. Don't get me wrong, I really like the idea of Moose, but the speed issue, and the errors every time I try to load it, make me think that we're better off using Class::Accessor, which we already use elsewhere in Koha to great success. (In reply to comment #17 and comment #13) > My experience with Moose and CGI is that it's kind of slow. This seems like > a huge problem on something as core as authority and bibliographic record > objects. Also, after I tried to get the solr code working on my system Moose > no longer functions. Choosing between core functionality (with a version of > Moose that installs) and Solr (with a complex install procedure that will > presumably leave Moose not working elsewhere) seems like a bad choice. Yes, it is a debian packages problem. Not easy to solve :-/ It for what I speak about Moo (a minimalist Moose, light-weight, less dependencies, etc.) In all cases it is a decision to have after a global discussion. > It's easy to do a follow up to change the name. If you tell me what you'd > like it called, I will do a follow-up that changes the name. I think it > should probably be Koha::DataObject::Authority, based on the RFC, but I > decided to just call it "Koha::Authority" until I had a second opinion. It seems this modification will be removing all ambiguity. > > For new files, I think a best practice is to use "use Modern::Perl" instead > > of "use strict; use warnings;" > > But nothing is written on the wiki. You are free to keep these lines. > I'm not attached to them. :) Great, no discussion :) > > Before introducing new important classes (as Authority) in the Koha > > namespace, I think it is important to start a global discussion. What do we > > want for this new namespace and for this rewriting ? > I think we want it to work, and provide much-needed features. Hum yes, but it is not precise. I didn't wait an exact answer :) (for now...) > I don't think I see the benefit here. Don't get me wrong, I really like the > idea of Moose, but the speed issue, and the errors every time I try to load > it, make me think that we're better off using Class::Accessor, which we > already use elsewhere in Koha to great success. If we have 2 modules to change when we will be choosing, it would be easy. If we have 15 modules using Moose, Class:Accessor and whatever, we will reproduce a new C4 shambles :-/ For me, this patch can passed QA if the module are moved (Koha::Authority => Koha::DataObject::Authority). But I think using Class::Accessor is not what we want. We should (re)rewrite modules using it. I hope you understand my goal is not to be annoy you or to waste your time ! ;-) (In reply to comment #19) > > But I think using Class::Accessor is not what we want. We should (re)rewrite > modules using it. > I disagree, I think we should rewrite modules using Moose. But mostly, I don't care, working code that is clean and passes tests is much more important, so far the code in Koha:: using Class::Accessor does this, the code using Moose ..... (In reply to comment #20) > (In reply to comment #19) > > > > > But I think using Class::Accessor is not what we want. We should (re)rewrite > > modules using it. > > > I disagree, I think we should rewrite modules using Moose. But mostly, I > don't care, working code that is clean and passes tests is much more > important, so far the code in Koha:: using Class::Accessor does this, the > code using Moose ..... Heh I realise now this isn't clear, I mean we shouldn't rewrite any modules, but if were to rewrite some. I would pick the ones already written in Moose as good candidates for a rewrite as those are the ones that are causing problems. If we were to rewrite anything, I agree Moo (at first glance) looks like an OK candidate. http://search.cpan.org/~mstrout/Moo-1.000002/lib/Moo.pm However it is not in Debian squeeze yet, so for 3.10 I think we should wait. http://packages.debian.org/search?keywords=libmoo-perl It is going to be in wheezy. (In reply to comment #22) > If we were to rewrite anything, I agree Moo (at first glance) looks like an > OK candidate. > > http://search.cpan.org/~mstrout/Moo-1.000002/lib/Moo.pm > > However it is not in Debian squeeze yet, so for 3.10 I think we should wait. > http://packages.debian.org/search?keywords=libmoo-perl > > It is going to be in wheezy. I don't know, I think Moo is a good candidate for packaging ourselves, and using that instead of Moose for Solr, etc. It would be easy enough to do the packaging. I just tried it, and unlike when I tried to get Moose to compile, it worked without any major hitches. I did some performance comparisons on Moo and Moose (and Class::Accessor). Here is my test script: #!/usr/bin/perl use [Moo|Moose|Class::Accessor]; print "Hi\n"; When running under NYTProf, I got the following results: Moo: Profile of test.pl for 140ms, executing 2273 statements and 823 subroutine calls in 36 source files and 7 string evals. Moose: Profile of test.pl for 3.34s, executing 112435 statements and 51928 subroutine calls in 112 source files and 153 string evals. Class::Accessor: Profile of test.pl for 42.5ms, executing 525 statements and 106 subroutine calls in 15 source files and 2 string evals. Class::Accessor is clearly way less overhead, and given how basic authority (and bibliographic) records are to Koha, I think that would be a powerful argument in favor of using it. However, Moo is only one order of magnitude slower to load, which I think probably isn't too bad. Moose, of course, is two orders of magnitude more expensive, making simply loading the file take an unacceptably long time. Since we don't support mod_perl, I think it is critical that we maintain CGI-compatibility for Koha. Setting up Plack is much more involved than pointing Apache at the appropriate directory and telling it to use CGI or mod_perl. All of which is a bit of a tangent. I would have no objection to rewriting Koha::Authority and the RecordProcessor stuff to use Moo, if there is a clear reason why it is better than using Class::Accessor. Created attachment 11393 [details] [review] Bug 7417 follow-up: incorporate QA comments This patch incorporates the two QA comments: * Renames Koha::Authority to Koha::DataObject::Authority * Uses Modern::Perl instead of 'use strict; use warnings;' This patch also improves test coverage, and eliminates the need for a database to test the EmbedSeeFromHeadings RecordProcessor plugin. (In reply to comment #23) > I don't know, I think Moo is a good candidate for packaging ourselves, and > using that instead of Moose for Solr, etc. It would be easy enough to do the > packaging. I just tried it, and unlike when I tried to get Moose to compile, > it worked without any major hitches. We can rewrite Koha::SearchEngine::Solr with Moo. But this module use a Data::SearchEngine::Solr module which use Moose :-/ Then we have to rewrite this external library too to drop the Moose dependency. > All of which is a bit of a tangent. I would have no objection to rewriting > Koha::Authority and the RecordProcessor stuff to use Moo, if there is a > clear reason why it is better than using Class::Accessor. I never used Moo and just used Moose for the Koha::SearchEngine::Solr. But I heard many good about it and it seems it is widely used now by Perl programmers communities. I am not attached or a fervent defender of any of them. However I'm convinced that Koha should have a powerfull and adapted object system. We want to write less code as possible then we have to choose a system with a minimum of code verbosity. In fact I think it will be difficult to have a consensus but all propositions must be considered in order to choose the most pertinent. My 1st comment there was not to troll or to impose my choices. I just gave my opinion about introducing a new auth class without rules However if each one of us keeps waiting for others not decision will be taken :-/ That is why I think that it would be worth having this discussion in a special topic in the koha-devel mailing list. Then everybody will participate and share about the subject, and then maybe we could reach a global consensus in agreement with the whole community (Maybe we should wait for the end of summer to launch such a discussion, as many people are currently on vacation :)). (In reply to comment #25) > (In reply to comment #23) > > I don't know, I think Moo is a good candidate for packaging ourselves, and > > using that instead of Moose for Solr, etc. It would be easy enough to do the > > packaging. I just tried it, and unlike when I tried to get Moose to compile, > > it worked without any major hitches. > > We can rewrite Koha::SearchEngine::Solr with Moo. But this module use a > Data::SearchEngine::Solr module which use Moose :-/ > Then we have to rewrite this external library too to drop the Moose > dependency. > > > All of which is a bit of a tangent. I would have no objection to rewriting > > Koha::Authority and the RecordProcessor stuff to use Moo, if there is a > > clear reason why it is better than using Class::Accessor. > > I never used Moo and just used Moose for the Koha::SearchEngine::Solr. But I > heard many good about it and it seems it is widely used now by Perl > programmers communities. > I am not attached or a fervent defender of any of them. However I'm > convinced that Koha should have a powerfull and adapted object system. > We want to write less code as possible then we have to choose a system with > a minimum of code verbosity. This isn't my goal, my goal is clean and tested code, verbosity has very little to do with that. Writing less code isn't inherently a good thing. So in order to have a consensus on how to achieve the goal, we need to have consensus on what that goal is. > In fact I think it will be difficult to have a consensus but all > propositions must be considered in order to choose the most pertinent. > > My 1st comment there was not to troll or to impose my choices. I just gave > my opinion about introducing a new auth class without rules > > However if each one of us keeps waiting for others not decision will be > taken :-/ > > That is why I think that it would be worth having this discussion in a > special topic in the koha-devel mailing list. > > Then everybody will participate and share about the subject, and then maybe > we could reach a global consensus in agreement with the whole community > (Maybe we should wait for the end of summer to launch such a discussion, as > many people are currently on vacation :)). End of summer isn't until april :) But seriously, if you mean the end of northern hemisphere summer, that is fine, but do we need to hold this patch until then? It is feature that a lot of people have been asking for, has close to 100% test coverage and works well. I am with Chris here, I don't think discussion should stop this from going in. It doesn't introduce any new dependency, we already have examples of both Moose and Class::Accessor in Koha... I don't see, why this should be held up. This is a much needed feature, that is programmed well. (In reply to comment #26) > End of summer isn't until april :) > > But seriously, if you mean the end of northern hemisphere summer, that is > fine, but do we need to hold this patch until then? > > It is feature that a lot of people have been asking for, has close to 100% > test coverage and works well. Yes, ~septembre, sorry. I never say I want to wait 2 months before adopting this patch. > For me, this patch can passed QA if the module are moved (Koha::Authority => Koha::DataObject::Authority). It's my quote ! :) I never failed QA this patch. Just launch a discussion. A new patch is proposed. It is a normal workflow. It's why I proposed to continue (or restart) this discusion elsewhere. I tried to do some efforts to be understood. Sorry if it is not the case. > Writing less code isn't inherently a good thing. Though the code of Koha should show you that not having a good design and having a lot of code dupplicated don't make it easy to maintain ;-) But it is not a high priority, you are right. Created attachment 11419 [details] [review] Bug 7417 follow-up: use Moose-like syntax for accessors In order to make the Koha codebase more consistent, this patch switches the RecordProcessor code to use the Moose-like syntax supported by Class::Accessor. Created attachment 11427 [details] [review] Bug 7417 follow-up: use Moose-like syntax for accessors In order to make the Koha codebase more consistent, this patch switches the RecordProcessor and Koha::DataObject::Authority code to use the Moose-like syntax supported by Class::Accessor. Jonathan, I think the thing to do here is move forward with the Class::Accessor-based code. While I am willing to rewrite this to use Moo, I think that it might be a bit premature to do that at this point. Better, I think, to make sure that there's consensus on using Moo as a compromise framework first, per comment 19. In the meantime, if you don't mind I would like to incorporate any further QA comments you might have so that this patch can move forward. Regarding what exactly goes in Koha::DataObject::Authority, I think we all agree that we can't just leave C4::AuthoritiesMarc as it is. I suggest that taking a minimalist approach to Koha::DataObject::Authority is the best we can do. My patch does that We can't possibly know what metadata we might have about authorities in the future, so any precise list of what can go in Koha::DataObject::Authority is guaranteed to be obsolete almost as soon as we have written it. (In reply to comment #31) > Jonathan, > > I think the thing to do here is move forward with the Class::Accessor-based > code. While I am willing to rewrite this to use Moo, I think that it might > be a bit premature to do that at this point. Better, I think, to make sure > that there's consensus on using Moo as a compromise framework first, per > comment 19. In the meantime, if you don't mind I would like to incorporate > any further QA comments you might have so that this patch can move forward. Jared, All seems good. To be perfect, the following should be made: - pass perltidy on new files - for discussion: Isn't it possible to have an "intelligent" constructor for Authorities ? I think we don't want a lot of get_from_*. Perhaps a "new" routine with a test on param: either: new( { record => $record} ), new ({authid => $authid}) or: new ($var). sub new {my $self = shift; if (ref $var eq 'MARC::Record') { print "it's a record" } else { print "it's an authid"} - You have to check the returned value of get_from_id in AuthoritiesMarc::GetAuthority: add a 'return unless $authority' seems to be sufficient - I don't understand what you do exactly with the indicator: Your comment: +In order to differentiate added headings from actual headings, a 'z' is +put in the first indicator. I am not an UNIMARC or *MARC* expert but isn't the content of this indicator normalized ? I don't manage to find if it is a work around or the best way to do that. (In reply to comment #32) > (In reply to comment #31) > > Jonathan, > > > > I think the thing to do here is move forward with the Class::Accessor-based > > code. While I am willing to rewrite this to use Moo, I think that it might > > be a bit premature to do that at this point. Better, I think, to make sure > > that there's consensus on using Moo as a compromise framework first, per > > comment 19. In the meantime, if you don't mind I would like to incorporate > > any further QA comments you might have so that this patch can move forward. > > Jared, > > All seems good. > To be perfect, the following should be made: > > - pass perltidy on new files Will do. > - for discussion: Isn't it possible to have an "intelligent" constructor for > Authorities ? I think we don't want a lot of get_from_*. Perhaps a "new" > routine with a test on param: > either: new( { record => $record} ), new ({authid => $authid}) > or: new ($var). sub new {my $self = shift; if (ref $var eq > 'MARC::Record') { print "it's a record" } else { print "it's an authid"} We could certainly do this. My original implementation used the former option, but for some reason I changed it. I don't remember quite why. > - You have to check the returned value of get_from_id in > AuthoritiesMarc::GetAuthority: > add a 'return unless $authority' seems to be sufficient Will do. > - I don't understand what you do exactly with the indicator: > Your comment: > +In order to differentiate added headings from actual headings, a 'z' is > +put in the first indicator. > I am not an UNIMARC or *MARC* expert but isn't the content of this > indicator normalized ? > I don't manage to find if it is a work around or the best way to do that. Yes. 'z' is always an invalid indicator value, because the standard calls for a numeric value. So any time we find a letter in an indicator, we can safely assume that the field is one that should not be displayed or used other than for searching. Created attachment 11441 [details] [review] Bug 7417 follow-up: additional QA fixes * perltidied all new files again * wrote an intelligent constructor for Koha::DataObject::Authority which takes a hash with the construction criteria * added rudimentary error handling in C4::AuthoritiesMarc::GetAuthority * added missing t/AuthoritiesMarc.t Followups need signoff Created attachment 11460 [details] [review] Bug 7417 follow-up: incorporate QA comments This patch incorporates the following QA comments: * Renames Koha::Authority to Koha::DataObject::Authority * Uses Modern::Perl instead of 'use strict; use warnings;' * perltidied all new files again * wrote an intelligent constructor for Koha::DataObject::Authority which takes a hash with the construction criteria * added rudimentary error handling in C4::AuthoritiesMarc::GetAuthority * added missing t/AuthoritiesMarc.t This patch also improves test coverage, and eliminates the need for a database to test the EmbedSeeFromHeadings RecordProcessor plugin. In order to make the Koha codebase more consistent, this patch switches the RecordProcessor and Koha::DataObject::Authority code to use the Moose-like syntax supported by Class::Accessor. Squashed follow-up. Created attachment 11465 [details] [review] [SIGNED-OFF] Bug 7417 follow-up: incorporate QA comments This patch incorporates the following QA comments: * Renames Koha::Authority to Koha::DataObject::Authority * Uses Modern::Perl instead of 'use strict; use warnings;' * perltidied all new files again * wrote an intelligent constructor for Koha::DataObject::Authority which takes a hash with the construction criteria * added rudimentary error handling in C4::AuthoritiesMarc::GetAuthority * added missing t/AuthoritiesMarc.t This patch also improves test coverage, and eliminates the need for a database to test the EmbedSeeFromHeadings RecordProcessor plugin. In order to make the Koha codebase more consistent, this patch switches the RecordProcessor and Koha::DataObject::Authority code to use the Moose-like syntax supported by Class::Accessor. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Follow up passes test plan. All QA remarks has been integrated in last patch. Marked as Passed QA Jared and Katrin, thank you for your reactivity :-) Question: I'm trying to find some things specific to UNIMARC, but can't. Does it mean it's MARC agnostic (/me doubt) ? shouldn't this patch be tested/signoff-ed with UNIMARC also ? (In reply to comment #39) > Question: I'm trying to find some things specific to UNIMARC, but can't. > Does it mean it's MARC agnostic (/me doubt) ? shouldn't this patch be > tested/signoff-ed with UNIMARC also ? It is MARC agnostic. I did not change the UNIMARC XSLT because as far as I could tell, the added headings don't show up with UNIMARC. The MARC21 and NORMARC stylesheets did not handle the data correctly, so I had to change them. Jared, this is a great and welcome initiative! Like Johnathan, I would have preferred another OO framework (Moose), but well... too early. In _processrecord, I can see you populate biblio record with see also forms, and you add 'z' as field first indicator, in order to distinguish, I suppose, authorized form and see also forms. Is it you own convention, or is it a norm? Are you sure nothing must be modified in UNIMARC XSL? You've edited MARC21 XSL in order to avoid displaying see also forms for authors, for example: <xsl:for-each select="marc:datafield[(@tag=100 or @tag=700) and @ind1!='z']"> Doesn't it mean that for UNIMARC, see also author forms will be displayed alongside with authorized form? And what about subjects? Are subjects see also forms displayed after authorized forms? It may be the case for 650 fields. Some libraries will want that, others won't. Otherwise, do you have any plan/idea to add other forms than see also? rejected forms, broader terms, narrower terms? (In reply to comment #41) > Jared, this is a great and welcome initiative! Like Johnathan, I would > have preferred another OO framework (Moose), but well... too early. My thought is once we agree on Moo or Mouse, it'll be easy to rewrite this. > In _processrecord, I can see you populate biblio record with see also > forms, and you add 'z' as field first indicator, in order to > distinguish, I suppose, authorized form and see also forms. Is it you > own convention, or is it a norm? I made up the ind1=z convention. Any letter would work, since the point is just to make sure that particular tag is invalid in all MARC flavours, and letters are not allowed in indicators by the ISO standard. > Are you sure nothing must be modified in UNIMARC XSL? You've edited > MARC21 XSL in order to avoid displaying see also forms for authors, for > example: > > <xsl:for-each select="marc:datafield[(@tag=100 or @tag=700) and @ind1!='z']"> > > Doesn't it mean that for UNIMARC, see also author forms will be > displayed alongside with authorized form? I am not 100% sure that the UNIMARC XSLT does not need to be modified. However, when I tested it, I didn't see the see-from headings. If they're showing up for you, I'm happy to do a follow-up modifying the UNIMARC XSLT. > And what about subjects? Are subjects see also forms displayed after > authorized forms? It may be the case for 650 fields. Some libraries will > want that, others won't. Subjects are not displayed in the results, so this is a non-issue. > Otherwise, do you have any plan/idea to add other forms than see also? > rejected forms, broader terms, narrower terms? It would be easy to do by adding additional filters. This patch, of course, adds rejected forms rather than see also forms to the record. I am currently working on a suggestion plugin for broader/narrower/related terms, the patch for which I will attach to bug 8211. UNIMARC QA comment: If I'm not mistaking, http://archive.ifla.org/VI/3/p1996-1/uniafull.htm#4-- says that 4xx are for "see references", it's the same as in marc21 (http://www.loc.gov/marc/authority/ad4xx.html) The 2nd digit is not always the same: UNIMARC = 420 See Reference Tracing - Family Name MARC21 = 420 does not exits The code says # for each field + foreach my $field ( $record->fields() ) { # if there is a link to authorities + my $authid = $field->subfield('9'); # retrieve the authority + my $authority = Koha::Authority->get_from_authid($authid); # if there are seefrom fields + my @seefrom = $auth_marc->field('4..'); # add them in the biblio record # find the biblio record tag + my $tag = substr($field->tag(), 0, 1) . substr($authfield->tag(), 1, 2); # create a new one, where you'll put datas coming from authorities + my $newfield = MARC::Field->new($tag, + 'z', + $authfield->indicator(2) || ' ', + '9' => '1'); # copy each subfield + foreach my $sub ($authfield->subfields()) { + my ($code,$val) = @$sub; + $newfield->add_subfields( $code => $val ); + } What I don't understand is the '9' => '1' You're connecting all biblios to the #1 authority ? why ? Another question = why do you set 1st indicator to 'z' ? Other question = do you have checked performances ? I think that it will be a *big* overhead for rebuild_zebra.pl -b -r (ie= rebuilding all the database) time rebuild_zebra.pl will give us numbers, that we need to have, at least to warn librarians ! Last question = do you also have checked/tested solr ? I see that you write + if ($record_type eq 'biblio' && C4::Context->preference('IncludeSeeFromInSearches')) { + my $normalizer = Koha::RecordProcessor->new( { filters => 'EmbedSeeFromHeadings' } ); + $record = $normalizer->process($record); + } in Koha/SearchEngine/Solr/Index.pm (asking Claire in // to get her feedback on this) (In reply to comment #43) > UNIMARC QA comment: > If I'm not mistaking, http://archive.ifla.org/VI/3/p1996-1/uniafull.htm#4-- > says that 4xx are for "see references", it's the same as in marc21 > (http://www.loc.gov/marc/authority/ad4xx.html) > > The 2nd digit is not always the same: > UNIMARC = 420 See Reference Tracing - Family Name > MARC21 = 420 does not exits As I understand it (I asked Francois a while ago), ALL 4xx fields work like this, so it doesn't matter what the second digit is. > The code says > # for each field > + foreach my $field ( $record->fields() ) { > > # if there is a link to authorities > + my $authid = $field->subfield('9'); > > # retrieve the authority > + my $authority = Koha::Authority->get_from_authid($authid); > > # if there are seefrom fields > + my @seefrom = $auth_marc->field('4..'); > > # add them in the biblio record > > # find the biblio record tag > + my $tag = substr($field->tag(), 0, 1) . > substr($authfield->tag(), 1, 2); > # create a new one, where you'll put datas coming from authorities > + my $newfield = MARC::Field->new($tag, > + 'z', > + $authfield->indicator(2) || ' ', > + '9' => '1'); > # copy each subfield > + foreach my $sub ($authfield->subfields()) { > + my ($code,$val) = @$sub; > + $newfield->add_subfields( $code => $val ); > + } > > > What I don't understand is the '9' => '1' > You're connecting all biblios to the #1 authority ? why ? This is a workaround for a peculiarity in MARC::Record. If you look a bit further down, you'll see I delete the subfield $9, once I've populated the other subfields. > > Another question = why do you set 1st indicator to 'z' ? I needed some way to CLEARLY identify the field as not being a real part of the record. Since ind1=z is not valid in MARC, that seemed like a good way. > > Other question = do you have checked performances ? I think that it will be > a *big* overhead for rebuild_zebra.pl -b -r (ie= rebuilding all the database) > time rebuild_zebra.pl will give us numbers, that we need to have, at least > to warn librarians ! I have. That's a good point. The syspref should note it takes twice as long to rebuild Zebra with this enabled. > Last question = do you also have checked/tested solr ? I see that you write > + if ($record_type eq 'biblio' && > C4::Context->preference('IncludeSeeFromInSearches')) { > + my $normalizer = Koha::RecordProcessor->new( { filters => > 'EmbedSeeFromHeadings' } ); > + $record = $normalizer->process($record); > + } > in Koha/SearchEngine/Solr/Index.pm (asking Claire in // to get her feedback > on this) I have not been able to test it. The Solr dependencies do not compile on Debian (nor, I think, would Solr run on my little laptop). I thought about not including the Solr bit, since Solr is still experimental, but decided I would provide it so that the RM could make that decision. Note that performance could be greatly improved by using Koha::Cache with an in-memory cache. This depends on the new CHI caching. I did not implement it yet because I prefer a more gradual approach to larger developments. It reduces the likelihood of bugs. Created attachment 12147 [details] [review] Bug 7417: Include see from references in bibliographic searches This patch adds the Koha::Indexer::RecordNormalizer and Koha::Indexer::MARC::RecordNormalizer::EmbedSeeFromHeadings packages to enable the inclusion of alternate forms of headings in bibliographic searches. When the new syspref IncludeSeeFromInSearches is turned on (default is off) rebuild_zebra.pl will insert see from headings from authority records into bibliographic records when indexing, so that a search on an obsolete term will turn up relevant records. To test: 1) Enable IncludeSeeFromInSearches 2) Add a heading that has an alternate form to a record (for example, "Cooking" has the alternate form "Cookery," if you have authority records from LC) 3) Index the zebraqueue (or reindex if you haven't indexed your system yet) 4) Confirm that if you search for "Cookery" you get the record you just modified Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 5 August 2012 Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com> Rebased on master 11 September 2012 Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Also checked: - Verified database update works correctly - Checked system preference and its description - Checked staff/opac detail pages with feature on/off - Checked staff/opac search facets - Downloaded and tested records in various formats - Tried different searches for 'see from' entries of authorities - Ran all unit tests No problems found. Created attachment 12148 [details] [review] Bug 7417 follow-up: incorporate QA comments This patch incorporates the following QA comments: * Renames Koha::Authority to Koha::DataObject::Authority * Uses Modern::Perl instead of 'use strict; use warnings;' * perltidied all new files again * wrote an intelligent constructor for Koha::DataObject::Authority which takes a hash with the construction criteria * added rudimentary error handling in C4::AuthoritiesMarc::GetAuthority * added missing t/AuthoritiesMarc.t This patch also improves test coverage, and eliminates the need for a database to test the EmbedSeeFromHeadings RecordProcessor plugin. In order to make the Koha codebase more consistent, this patch switches the RecordProcessor and Koha::DataObject::Authority code to use the Moose-like syntax supported by Class::Accessor. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Follow up passes test plan. Patch pushed to master Created attachment 12199 [details] [review] Bug 7417 follow-up: fix a perlcritic violation Created attachment 12200 [details] [review] Bug 7417 follow-up: fix a perlcritic violation Forgot to add one file to the patch. Created attachment 12204 [details] [review] Bug 7417 follow-up: fix a perlcritic violation Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> No longer fails perlcritic tests, signing off. Last patch passes QA. follow-up pushed to master This feature is available in Master. Tested this in our installation of 3.10.0 and 3.10.1 Three comments: 1) In both cases, despite having the box ticked for AuthorityFile in Intranet - (Home › Administration › Did you mean?) It does not work in the staff interface. Whereas in the OPAC there's a bar above the search results with suggestions, nothing appears in the staff interface. 1)Also in both cases, it appears to be pulling results from all fields in the authority records. For example the term Chanties - It suggests the correct term: Shanties, but also the name of an author who has a machine generated authority record with the title of the book: Old sea chanties. -Would it be possible to restrict the search to 4XX fields? 3) An unexpected behavior - If you click on a suggested term that does not have any biblios attached it runs a search on the authority number. This in turn returns suggested terms based on that authority number as a stem. So if I click on a suggestion with authority no. 271 (but there are no biblios linked to this authority) it comes back with suggestions for authority number 2715 and 2716 We uploaded all our authority files first and are adding the biblios in batches, which is why we have authority records not yet linked to biblios. (In reply to comment #56) > Tested this in our installation of 3.10.0 and 3.10.1 Bug 7417 (this bug) is for including see-from headings directly in search results, and has nothing to do with the "Did you mean?" suggestions, which are described in 8209. In order to use the feature described on this bug, you can enable the IncludeSeeFromInSearches system preference and reindex. > Three comments: > 1) In both cases, despite having the box ticked for AuthorityFile in > Intranet - (Home › Administration › Did you mean?) It does not work in the > staff interface. Whereas in the OPAC there's a bar above the search results > with suggestions, nothing appears in the staff interface. "Did you mean" is not yet implemented for the intranet (from the administration page: "NOTE: Did you mean? functionality is not yet enabled on the staff client"). > 1)Also in both cases, it appears to be pulling results from all fields in > the authority records. For example the term Chanties - It suggests the > correct term: Shanties, but also the name of an author who has a machine > generated authority record with the title of the book: Old sea chanties. > -Would it be possible to restrict the search to 4XX fields? It would be possible to write an additional "Did you mean" plugin to do this if you so desired. > 3) An unexpected behavior - > If you click on a suggested term that does not have any biblios attached it > runs a search on the authority number. This in turn returns suggested terms > based on that authority number as a stem. So if I click on a suggestion > with authority no. 271 (but there are no biblios linked to this authority) > it comes back with suggestions for authority number 2715 and 2716 > We uploaded all our authority files first and are adding the biblios in > batches, which is why we have authority records not yet linked to biblios. The problem here is that checking prior to the search whether a particular authority record is used would make searches so slow as to be unusable. |