Bug 5337 - In Acquisition, EAN search will return results
Summary: In Acquisition, EAN search will return results
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Deadline: 2011-05-01
Assignee: Matthias Meusburger
QA Contact: Marcel de Rooy
URL: http://wiki.koha-community.org/wiki/E...
Keywords:
: 5384 (view as bug list)
Depends on:
Blocks: 7163 5357
  Show dependency treegraph
 
Reported: 2010-11-02 02:35 UTC by Paul Poulain
Modified: 2015-03-05 14:36 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
proposed patch (28.45 KB, patch)
2012-01-16 14:08 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Proposed patch (36.05 KB, patch)
2012-02-16 16:31 UTC, Matthias Meusburger
Details | Diff | Splinter Review
proposed patch (34.86 KB, patch)
2012-03-20 13:07 UTC, Matthias Meusburger
Details | Diff | Splinter Review
proposed patch (35.37 KB, patch)
2012-03-21 08:09 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 5337: EAN management : Adds ean for various searches (36.09 KB, patch)
2012-03-29 15:04 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 5337: EAN management : Adds ean for various searches (36.47 KB, patch)
2012-05-24 13:32 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 5337: EAN management : Adds ean for various searches (38.84 KB, patch)
2012-06-09 10:48 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Patch for EAN (38.69 KB, patch)
2012-06-09 11:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 5337 : Upping the count of the unit tests (634 bytes, patch)
2012-06-12 06:39 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2010-11-02 02:35:14 UTC
In acquisition module, scanning an EAN will result in a correct search
Comment 1 Paul Poulain 2011-11-05 10:56:58 UTC
If you want to participate to the upstreaming:
* assign the bug to yourself
* retrieve the patch(es) on git.biblibre.com
* the branch name can be found using
git branch -a|grep NNNN
Usually, it should be 
  biblibre/customer/ft/MTNNNN
For example:
   remotes/origin/stetienne/ft/MT6522

This patch is in branch number 5558
Comment 2 Matthias Meusburger 2012-01-16 14:08:23 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2012-01-17 07:20:03 UTC
Hi Matthias,

some first notes about this:

1) Patch includes a change to kohaversion.pl but should better not change this file.  ( our $VERSION = '3.02.00.059'; )

2) The new mapping is only introduced to en and fr-FR UNIMARC frameworks. 
I think it should include all UNIMARC frameworks (it-IT,...?) for consistency.

I think for MARC21 the EAN would go here:
http://www.loc.gov/marc/bibliographic/bd024.html
Display Constants - EAN is carried in the MARC record without the internal hyphens or spaces. Hyphens, spaces, and introductory words may be system generated as display constants associated with first indicator value 3 and the content of subfields $a, $d, and $z.
Content designated field:
024 	3#$a9780838934326$d90000 

The value is depending on an indicator which certainly complicates things. We currently don't support indicators in our mappings - so I am not sure we will be able to make use of this feature for MARC21. 

I think for documentation we should note that this feature is currently limited to UNIMARC?

3) There is a (left over?) warn in Serials.pm
+    warn Data::Dumper::Dumper(@bind_params);

I am not sure how to continue testing here, because I currently have no UNIMARC database and data.
Comment 4 Katrin Fischer 2012-01-17 07:22:35 UTC
Oh, if it's UNIMARC only, the display should act accordingly. We should not offer search options that simply can't work out of the box. 
I am failing QA for this reason.
Comment 5 Katrin Fischer 2012-01-17 07:23:03 UTC
Had to set "Needs sign-off" first, before I could fail.
Comment 6 Paul Poulain 2012-01-27 10:11:08 UTC
Katrin, it seems you raised a good point. Switching to "in discussion" to see how we could deal with this.
Comment 7 Colin Campbell 2012-01-27 18:38:49 UTC
In MARC21 the EAN appears in 020 as the ISBN is either a valid EAN or (for 10-digit versions) convertable to one
Comment 8 Katrin Fischer 2012-01-30 08:43:43 UTC
Hi Colin - thx for your comment. I looked it up on loc.gov but was not totally sure. Does this also work that way for other media (DVDs, CDs, language courses, video games etc.)?
Comment 9 Colin Campbell 2012-01-30 10:42:25 UTC
I assume for non-book materials (or rather no ISBN materials) the ean gets relegated to 024. Not conducive to standardizing practice
Comment 10 Paul Poulain 2012-02-10 13:16:17 UTC
reading this thread again, i'm don't know what to say in the starting discussion... Is it needed ?
Comment 11 Katrin Fischer 2012-02-10 13:22:13 UTC
I think the feature can not go in as it is now - because it will never work on MARC21 installations. We should find a way to make it work or hide it from the templates accordingly.
Comment 12 Paul Poulain 2012-02-10 13:45:49 UTC
My question was unclear it seems: When the bug has a status "in discussion", we're supposed to start a discussion
In the case of this bug, what the discussion should be about ? What is the question.
If it's just "can't be accepted as is", then:
* specify what would make the patch acceptable
* switch to failed QA

As I understand it, the only problem on this patch is that the "EAN" column always appear on serial search, and is meaningless for MARC21 libraries.
In this case, a trivial solution to this could be to remove it from the screen.

The other option would be to map EAN to a MARC field, but it seems in MARC21, the correct one is not easy to find.
Do I understand well ?
Comment 13 Jared Camins-Esakov 2012-02-10 14:08:51 UTC
I think the best thing to do here is not display the EAN search for MARC21, as it's not really usable for MARC21 (thanks to the indicator issue with field 024).
Comment 14 Matthias Meusburger 2012-02-16 16:31:59 UTC Comment hidden (obsolete)
Comment 15 jmbroust 2012-03-19 10:50:46 UTC
Applying: Bug 5337: EAN management : Adds ean for various searches
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Acquisition.pm
Auto-merging C4/Biblio.pm
Auto-merging C4/Serials.pm
Auto-merging acqui/addorder.pl
Auto-merging acqui/histsearch.pl
Auto-merging acqui/neworderempty.pl
Auto-merging acqui/parcel.pl
CONFLICT (content): Merge conflict in acqui/parcel.pl
Auto-merging catalogue/detail.pl
Auto-merging installer/data/mysql/en/marcflavour/unimarc/mandatory/unimarc_framework_DEFAULT.sql
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/histsearch.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Auto-merging kohaversion.pl
CONFLICT (content): Merge conflict in kohaversion.pl
Auto-merging opac/opac-ISBDdetail.pl
Auto-merging opac/opac-detail.pl
Auto-merging serials/routing-preview.pl
Auto-merging serials/routing.pl
Auto-merging serials/serials-collection.pl
Auto-merging serials/serials-edit.pl
Auto-merging serials/subscription-detail.pl
Failed to merge in the changes.
Comment 16 Matthias Meusburger 2012-03-20 13:07:02 UTC Comment hidden (obsolete)
Comment 17 Alexandre Lengereau 2012-03-20 13:44:49 UTC
<h1>Something went wrong !</h1>Applying: Bug 5337: EAN management : Adds ean for various searches<br/>
Repository lacks necessary blobs to fall back on 3-way merge.<br/>
Cannot fall back to three-way merge.<br/>
Patch failed at 0001 Bug 5337: EAN management : Adds ean for various searches<br/>
When you have resolved this problem run git am --resolved.<br/>
If you would prefer to skip this patch, instead run git am --skip.<br/>
To restore the original branch and stop patching run git am --abort.<br/>
Bug 5337 - acq EAN search<br/>
<br/>
proposed patch<br/>
Apply? [yn] <br/>
Patch left in /tmp/proposed-patch-F6QPRX.patch<br/>
<br/>=== translation installation ===<br/>Install templates
  Install templates 'opac
    From: /home/koha/src/koha-tmpl/opac-tmpl/prog/en/
    To  : /home/koha/src/koha-tmpl/opac-tmpl/prog/fr-FR
    With: /home/koha/src/misc/translator/po/fr-FR-i-opac-t-prog-v-3006000.po
The install seems to be successful.
  Install templates 'intranet
    From: /home/koha/src/koha-tmpl/intranet-tmpl/prog/en/
    To  : /home/koha/src/koha-tmpl/intranet-tmpl/prog/fr-FR
    With: /home/koha/src/misc/translator/po/fr-FR-i-staff-t-prog-v-3006000.po
The install seems to be successful.
Write opac.pref
Write cataloguing.pref
Write staff_client.pref
Write admin.pref
Write serials.pref
Write i18n_l10n.pref
Write searching.pref
Write web_services.pref
Write creators.pref
Write circulation.pref
Write enhanced_content.pref
Write authorities.pref
Write local_use.pref
Write acquisitions.pref
Write logs.pref
Write patrons.pref
Comment 18 Alexandre Lengereau 2012-03-20 15:03:01 UTC
In this url it's work /cgi-bin/koha/acqui/histsearch.pl search by EAN

But the filter rules by "EAN :" in this url cgi-bin/koha/acqui/parcel.pl?booksellerid=2&op=new&invoice=fgf&datereceived=20%2F03%2F2012
seem not working properly.

Is it a zebra conf ?
Comment 19 Matthias Meusburger 2012-03-21 08:09:40 UTC Comment hidden (obsolete)
Comment 20 jmbroust 2012-03-21 09:35:36 UTC
Doesn't seem to work in neworderbiblio.pl when searching for an existing record
works in histsearch.pl and parcel.pl
Comment 21 Matthias Meusburger 2012-03-22 15:14:59 UTC
Search in histsearch.pl and parcel.pl is mysql based, so it works out of the box.

Search in neworderempty.pl is zebra-based, so you need an up-to-date zebra index.

Is my answer satisfying enough for signoff ?
Comment 22 jmbroust 2012-03-22 15:53:35 UTC
ok for zebra search in neworderempty.pl I switch to signed off. Thanks :-)
Comment 23 Paul Poulain 2012-03-23 11:14:36 UTC
QA comment:

* this patch removes EAN search for MARC21, good point
* it uses [% UNLESS (MARC21) %] => bad point. If you set NORMARC, you'll get the ean option. The general rule is, when something is specific to UNIMARC, to do "IF UNIMARC" and not "UNLESS MARC21" !
* it would be good to add a marc_subfield_structure when updating:
+    if (C4::Context->preference("marcflavour") eq 'UNIMARC') {
+        $dbh->do("UPDATE marc_subfield_structure SET kohafield='biblioitems.ean' WHERE tagfield='073' and tagsubfield='a'");
+    }
(just after the add column and index)

* in opac-detail, there is a repeated my $marcflavour = ...;
remove the one at line 390 I think
-my $marcflavour  = C4::Context->preference("marcflavour");
(this makes perl -wc complain)

failed QA, once it's fixed, i'll mark passed QA I think, everything else is OK
Comment 24 Matthias Meusburger 2012-03-29 15:04:42 UTC Comment hidden (obsolete)
Comment 25 Ian Walls 2012-03-29 19:08:53 UTC
This patch does some pretty heavy-weight changes:  adds a new field to biblioitems, modifies the parameters sent to a subroutine, and exports the MARC flavour as a template variable on some pages.  All this is done for one of the benefit of one of the three potential MARC flavours.  This doesn't mean we shouldn't implement this, but it does mean we need to do so carefully, as there are lots of places where new bugs could be introduced.


In terms of discussion, I contribute the following (though it may be a little late in the game):

I think we may be better served by taking a wider view of the issue.  We've got lots of kinds of identifiers floating around:  ISBN, ISSN, EAN, LCCN, UPC, OCLC number, etc.  Depending on when they were invented, and what MARC flavour you're working in, they can be mapped all kinds of different ways.  It would be good to have an extensible framework where we can associate any given biblio with a set of keys, of any kind.  Each type of key could be associated with a normalization routine to weed out the formatting that is applied to the value (added bonus: we could have just one key for ISBN, and have both 10 and 13 expressions connect to it).

We'd probably need to rely on Zebra with DOM indexing to get these values properly extracted from our records, but once that's done, they could be stashed in the database.
Comment 26 Paul Poulain 2012-04-20 12:26:10 UTC
(In reply to comment #25)
> In terms of discussion, I contribute the following (though it may be a
> little late in the game):

Hi Ian,

Your suggestion is good, but addresses a much much larger scope than what this enh was for.
This patch was specifically made for acquisition, and I think it should be included.

And, probably, another topic be opened for identifiers. In french universities, we also have the SUDOC with a unique number called PPN. atm, we store it randomly in a biblio. field that is not used elsewhere. That's very handfull for finding the biblionumber when you have a PPN, but not very clean. (A NoSql database would be more efficient in this matter)
Comment 27 Paul Poulain 2012-05-14 16:30:26 UTC
This patch does not apply anymore.
it has been broken by the new staff interface look, and the conflict should not be too hard to solve, but Matthias I let you do it
(I think the problem is only <div class="gradient">)

About QA = the patch has been signed-off almost 2 months ago, i've requested for QA by email because it's been written by BibLibre.
Overall, we should improve our organisation to avoid having patches staying for a long time in the QA queue.
Comment 28 Matthias Meusburger 2012-05-24 13:32:06 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2012-06-08 15:29:29 UTC
Still applies. Will take a look.
Comment 30 Marcel de Rooy 2012-06-08 16:36:56 UTC
QA Comment:
Although I agree with Ian that a more general solution would be welcome, I do not oppose this patch. But this problem will come up again. And if we continuously would say "Next time we must solve it better..", we will never come there. So, it would be nice to trigger that discussion now. But who will take care of the generic housekeeping solution..

Specific comments on this patch below. Note that Paul wanted to pass qa already on it, but I unfortunately disagree. Note that it was signed-off by jean-manuel.broust@univ-lyon2.fr. The patch commit does not show that however. Please fix that too.

The atomicupdate file could be removed (adding the field is done in updatedatabase)?
The version number there should be filled with XXX instead of 005. This is a BLOCKER.
Why do you need a index in deletedbiblioitems on ean? Please clarify. 

The commit reads: However, if you already have records with ean, you will have to run misc/batchRebuildBiblioTables.pl to populate biblioitems.ean. Should this be published too in the update print statement or anywhere else too? 

Routine GetSubscriptions: You add line:
$sqlwhere .= ( $sqlwhere ? " AND " : " WHERE " ) . "(" . join( ") OR (", @sqlstrings ) . ")";
Please add an additional ( and ) around this construct, just as is done a few lines above it. This prevents wrong interpretation of the whole expression.

Since you are adding parameters to existing routines: Did you check if you did not miss any function call for the two routines changed? You should grep on the name only. Do not assume that the name always is immediately followed by parenthesis. Could be whitespace also. Note that I find instances of test scripts referring to the function that you do not change (e.g. t/db_dependent/Serials.t). Please verify. BLOCKER too.

Please clarify: Why a span if biblionumber passed and otherwise a label in neworderempty.tt change?

Changing status to reflect need for corrections and clarification.
Comment 31 Matthias Meusburger 2012-06-09 10:42:40 UTC
(En réponse au commentaire 30)

>  Note that it was signed-off by jean-manuel.broust@univ-lyon2.fr. The patch commit does not show that however. Please fix that too.

Done

 
> The atomicupdate file could be removed (adding the field is done in
> updatedatabase)?

Ok, removed.

The version number there should be filled with XXX instead of 005.

Fixed

> Why do you need a index in deletedbiblioitems on ean? Please clarify.

You're right, indeed. Nothing really justify the use of and index on this field. Removed.

> 
> The commit reads: However, if you already have records with ean, you will
> have to run misc/batchRebuildBiblioTables.pl to populate biblioitems.ean.
> Should this be published too in the update print statement or anywhere else
> too? 

I've added the message in the updatedatabase (in case the marcflavor is unimarc, otherwise the mapping is not automatically added, and there's no need for rebuilding biblio tables).
Do you think of another place where this message should be added?

> 
> Routine GetSubscriptions: You add line:
> $sqlwhere .= ( $sqlwhere ? " AND " : " WHERE " ) . "(" . join( ") OR (",
> @sqlstrings ) . ")";
> Please add an additional ( and ) around this construct, just as is done a
> few lines above it. This prevents wrong interpretation of the whole
> expression.

Done

> 
> Since you are adding parameters to existing routines: Did you check if you
> did not miss any function call for the two routines changed? You should grep
> on the name only. Do not assume that the name always is immediately followed
> by parenthesis. Could be whitespace also. Note that I find instances of test
> scripts referring to the function that you do not change (e.g.
> t/db_dependent/Serials.t). Please verify. BLOCKER too.

I've done the grep with only the function name and it seems that every call in the source code has been changed. But yes, the tests should have been changed too. Now it's done. I added ean test to t/db_dependent/Serials.t. I added ean (and isbn, which was missing) test to t/db_dependent/lib/KohaTest/Acquisition/GetHistory.pm.

> 
> Please clarify: Why a span if biblionumber passed and otherwise a label in
> neworderempty.tt change?

The neworderempty page allows to add catalogue informations (title, author, ean...) to an order while creating a new order. However, those informations cannot be directly modified when editing an existing order (there's a link for editing the record). This is why it's only a label when there is a biblionumber.
Comment 32 Matthias Meusburger 2012-06-09 10:48:00 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2012-06-09 11:39:47 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2012-06-09 11:46:03 UTC
QA Comment:
All points have got attention. Thanks for the quick fix.
Note that the test t/db_dependent/lib/KohaTest like others in that directory seems to be obsolete.

Passed QA
We saved a kitten!
Comment 35 Chris Cormack 2012-06-12 06:39:49 UTC
Created attachment 10243 [details] [review]
Bug 5337 : Upping the count of the unit tests
Comment 36 Chris Cormack 2012-06-12 06:40:14 UTC
Just a follow up to make jenkins happy
Comment 37 Paul Poulain 2012-06-14 13:44:21 UTC
trivial patch, pushing immediately
Comment 38 Katrin Fischer 2013-04-13 23:22:52 UTC
*** Bug 5384 has been marked as a duplicate of this bug. ***