Summary: | In Acquisition, EAN search will return results | ||
---|---|---|---|
Product: | Koha | Reporter: | Paul Poulain <paul.poulain> |
Component: | Acquisitions | Assignee: | Matthias Meusburger <matthias.meusburger> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | alexandre.lengereau, chris, claire.hernandez, colin.campbell, jcamins, jean-manuel.broust, jonathan.druart, koha.sekjal, m.de.rooy, matthias.meusburger |
Version: | 3.10 | ||
Hardware: | All | ||
OS: | All | ||
URL: | http://wiki.koha-community.org/wiki/EAN_reading_RFC | ||
See Also: | http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13766 | ||
Change sponsored?: | Sponsored | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 5357, 7163 | ||
Deadline: | 2011-05-01 | ||
Attachments: |
proposed patch
Proposed patch proposed patch proposed patch Bug 5337: EAN management : Adds ean for various searches Bug 5337: EAN management : Adds ean for various searches Bug 5337: EAN management : Adds ean for various searches Patch for EAN Bug 5337 : Upping the count of the unit tests |
Description
Paul Poulain
2010-11-02 02:35:14 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 Created attachment 7172 [details] [review] proposed patch 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. 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. Had to set "Needs sign-off" first, before I could fail. Katrin, it seems you raised a good point. Switching to "in discussion" to see how we could deal with this. In MARC21 the EAN appears in 020 as the ISBN is either a valid EAN or (for 10-digit versions) convertable to one 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.)? I assume for non-book materials (or rather no ISBN materials) the ean gets relegated to 024. Not conducive to standardizing practice reading this thread again, i'm don't know what to say in the starting discussion... Is it needed ? 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. 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 ? 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). Created attachment 7694 [details] [review] Proposed patch This patch disable ean search for MARC21 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. Created attachment 8349 [details] [review] proposed patch conflicts fixed. <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 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 ? Created attachment 8380 [details] [review] proposed patch This patch adds comments in the commit message on how to configure ean search on existing installations. It also fixes search in histsearch.pl Doesn't seem to work in neworderbiblio.pl when searching for an existing record works in histsearch.pl and parcel.pl 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 ? ok for zebra search in neworderempty.pl I switch to signed off. Thanks :-) 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 Created attachment 8692 [details] [review] Bug 5337: EAN management : Adds ean for various searches - in various acquisition pages and serials home - in database : biblioitems.ean - adds ean and its mapping in default english bibliographic framework - adds ean mapping in default french bibliographic framework - ean search is not enabled for MARC21 The required mapping between the ean marc field and the biblioitems.ean database field will be automatically added on an existing unimarc installation. However, if you already have records with ean, you will have to run misc/batchRebuildBiblioTables.pl to populate biblioitems.ean 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. (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) 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. Created attachment 9759 [details] [review] Bug 5337: EAN management : Adds ean for various searches - in various acquisition pages and serials home - in database : biblioitems.ean - adds ean and its mapping in default english bibliographic framework - adds ean mapping in default french bibliographic framework - ean search is not enabled for MARC21 The required mapping between the ean marc field and the biblioitems.ean database field will be automatically added on an existing unimarc installation. However, if you already have records with ean, you will have to run misc/batchRebuildBiblioTables.pl to populate biblioitems.ean Still applies. Will take a look. 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. (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. Created attachment 10009 [details] [review] Bug 5337: EAN management : Adds ean for various searches - in various acquisition pages and serials home - in database : biblioitems.ean - adds ean and its mapping in default english bibliographic framework - adds ean mapping in default french bibliographic framework - ean search is not enabled for MARC21 The required mapping between the ean marc field and the biblioitems.ean database field will be automatically added on an existing unimarc installation. However, if you already have records with ean, you will have to run misc/batchRebuildBiblioTables.pl to populate biblioitems.ean Signed-off-by: jmbroust <jean-manuel.broust@univ-lyon2.fr> Created attachment 10016 [details] [review] Patch for EAN Amended for removing merge marker 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! Created attachment 10243 [details] [review] Bug 5337 : Upping the count of the unit tests Just a follow up to make jenkins happy trivial patch, pushing immediately *** Bug 5384 has been marked as a duplicate of this bug. *** |