Bug 7417

Summary: Include alternate forms from authorities in bibliographic searches
Product: Koha Reporter: Jared Camins-Esakov <jcamins>
Component: SearchingAssignee: Jared Camins-Esakov <jcamins>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P5 - low CC: bargioni, chris, eb, f.demians, jonathan.druart, katrin.fischer, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7284, 7818    
Bug Blocks: 8212    
Attachments: Bug 7417: Include see from references in bibliographic searches
Bug 7417: Include see from references in bibliographic searches
Bug 7417: Include see from references in bibliographic searches
Bug 7417: Include see from references in bibliographic searches
Bug 7417: Include see from references in bibliographic searches
Bug 7417: Include see from references in bibliographic searches
Bug 7417: Include see from references in bibliographic searches
[SIGNED-OFF] Bug 7417: Include see from references in bibliographic searches
Bug 7417 follow-up: incorporate QA comments
Bug 7417 follow-up: use Moose-like syntax for accessors
Bug 7417 follow-up: use Moose-like syntax for accessors
Bug 7417 follow-up: additional QA fixes
Bug 7417 follow-up: incorporate QA comments
[SIGNED-OFF] Bug 7417 follow-up: incorporate QA comments
Bug 7417: Include see from references in bibliographic searches
Bug 7417 follow-up: incorporate QA comments
Bug 7417 follow-up: fix a perlcritic violation
Bug 7417 follow-up: fix a perlcritic violation
Bug 7417 follow-up: fix a perlcritic violation

Description Jared Camins-Esakov 2012-01-07 22:55:19 UTC
Although authority records include references from unused terms, at present Koha does not take advantage of them when searching bibliographic records. It would be very useful if Koha could (optionally) include all the see-from terms in bibliographic records for indexing purposes. In order to do this, it will be necessary to inject see-from headings into bib records immediately before exporting them for indexing by Zebra (or solr, or any other indexing engine that Koha may use).
Comment 1 Jared Camins-Esakov 2012-07-19 19:59:06 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-08-01 13:04:13 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2012-08-02 16:40:03 UTC
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.
Comment 4 Jared Camins-Esakov 2012-08-02 20:11:30 UTC Comment hidden (obsolete)
Comment 5 Jared Camins-Esakov 2012-08-02 20:13:15 UTC
(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.
Comment 6 Jared Camins-Esakov 2012-08-05 19:58:20 UTC Comment hidden (obsolete)
Comment 7 Jared Camins-Esakov 2012-08-05 20:29:57 UTC Comment hidden (obsolete)
Comment 8 Jared Camins-Esakov 2012-08-05 20:52:33 UTC Comment hidden (obsolete)
Comment 9 Jared Camins-Esakov 2012-08-05 21:04:51 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2012-08-05 21:25:28 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-08-06 08:26:58 UTC
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
Comment 12 Jonathan Druart 2012-08-06 08:27:54 UTC
Oupps ! Wrong bug, sorry
Comment 13 Jonathan Druart 2012-08-06 09:52:49 UTC
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...
Comment 14 Chris Cormack 2012-08-06 10:07:03 UTC
(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
Comment 15 Jonathan Druart 2012-08-06 12:22:21 UTC
(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.
Comment 16 Katrin Fischer 2012-08-06 12:36:09 UTC
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?
Comment 17 Jared Camins-Esakov 2012-08-06 12:45:49 UTC
(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.
Comment 18 Jared Camins-Esakov 2012-08-06 12:51:36 UTC
(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.
Comment 19 Jonathan Druart 2012-08-06 15:21:12 UTC
(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 ! ;-)
Comment 20 Chris Cormack 2012-08-06 15:31:14 UTC
(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 .....
Comment 21 Chris Cormack 2012-08-06 15:39:40 UTC
(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.
Comment 22 Chris Cormack 2012-08-06 15:51:21 UTC
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.
Comment 23 Jared Camins-Esakov 2012-08-06 17:10:27 UTC
(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.
Comment 24 Jared Camins-Esakov 2012-08-06 20:46:28 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2012-08-07 09:44:38 UTC
(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 :)).
Comment 26 Chris Cormack 2012-08-07 09:54:03 UTC
(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.
Comment 27 Katrin Fischer 2012-08-07 09:59:20 UTC
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.
Comment 28 Jonathan Druart 2012-08-07 10:40:25 UTC
(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.
Comment 29 Jared Camins-Esakov 2012-08-07 14:25:45 UTC Comment hidden (obsolete)
Comment 30 Jared Camins-Esakov 2012-08-07 19:20:09 UTC Comment hidden (obsolete)
Comment 31 Jared Camins-Esakov 2012-08-07 19:33:36 UTC
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.
Comment 32 Jonathan Druart 2012-08-08 09:57:25 UTC
(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.
Comment 33 Jared Camins-Esakov 2012-08-08 11:36:27 UTC
(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.
Comment 34 Jared Camins-Esakov 2012-08-08 12:13:51 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2012-08-08 13:41:43 UTC
Followups need signoff
Comment 36 Jared Camins-Esakov 2012-08-08 22:20:35 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2012-08-09 04:46:33 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2012-08-09 07:56:26 UTC
All QA remarks has been integrated in last patch.

Marked as Passed QA

Jared and Katrin, thank you for your reactivity :-)
Comment 39 Paul Poulain 2012-09-03 14:25:45 UTC
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 ?
Comment 40 Jared Camins-Esakov 2012-09-03 14:31:39 UTC
(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.
Comment 41 Frédéric Demians 2012-09-04 08:16:34 UTC
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?
Comment 42 Jared Camins-Esakov 2012-09-04 10:52:03 UTC
(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.
Comment 43 Paul Poulain 2012-09-07 14:21:24 UTC
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)
Comment 44 Jared Camins-Esakov 2012-09-07 14:31:08 UTC
(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.
Comment 45 Jared Camins-Esakov 2012-09-07 14:39:41 UTC
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.
Comment 46 Jared Camins-Esakov 2012-09-11 18:56:03 UTC Comment hidden (obsolete)
Comment 47 Jared Camins-Esakov 2012-09-11 18:56:19 UTC Comment hidden (obsolete)
Comment 48 Paul Poulain 2012-09-13 12:30:08 UTC
Patch pushed to master
Comment 49 Jared Camins-Esakov 2012-09-13 17:38:36 UTC Comment hidden (obsolete)
Comment 50 Jared Camins-Esakov 2012-09-13 17:44:46 UTC Comment hidden (obsolete)
Comment 51 Chris Cormack 2012-09-14 02:05:06 UTC
Created attachment 12204 [details] [review]
Bug 7417 follow-up: fix a perlcritic violation

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 52 Chris Cormack 2012-09-14 02:07:56 UTC
No longer fails perlcritic tests, signing off.
Comment 53 Jonathan Druart 2012-09-14 12:04:49 UTC
Last patch passes QA.
Comment 54 Paul Poulain 2012-09-14 15:22:52 UTC
follow-up pushed to master
Comment 55 Jared Camins-Esakov 2012-12-03 13:32:39 UTC
This feature is available in Master.
Comment 56 Elaine Bradtke 2013-01-30 18:27:38 UTC
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.
Comment 57 Jared Camins-Esakov 2013-01-30 19:17:27 UTC
(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.