Hi Marcel,

On searching isbn=2070518426 on the BNF Z3950 server, I got
on master:
  Harry Potter à l'école des sorciers # J. K. Rowling #      # 2070518426 | 9782070612369
and, on master + your patch:
  Harry Potter à l'école des sorciers # J. K. Rowling # 1998 # 2070518426

I separate fields with '#'.

With your patch, the date is get back but the isbn field seems truncated: on master I got 2 isbn separated with a pipe, now I get only one isbn.

Looking at the code, I cannot explain this change.

(In reply to Jonathan Druart from comment #3)
> Hi Marcel,
> 
> On searching isbn=2070518426 on the BNF Z3950 server, I got
> on master:
>   Harry Potter à l'école des sorciers # J. K. Rowling #      # 2070518426 |
> 9782070612369
> and, on master + your patch:
>   Harry Potter à l'école des sorciers # J. K. Rowling # 1998 # 2070518426
> 
> I separate fields with '#'.
> 
> With your patch, the date is get back but the isbn field seems truncated: on
> master I got 2 isbn separated with a pipe, now I get only one isbn.
> 
> Looking at the code, I cannot explain this change.

Will soon have a look at this. Thanks.

Looking at this now.

Jonathan,
Thanks for testing. Good catch for the isbn numbers.
Note that it only was a display problem. As explained on the followup commit message, this has been resolved now.
You are more than welcome to check again :)

QA comment:

Looking at the code, nothing to say.
It is not perfect, but it has the merit of being better than before (using  refactoring).
I did not find any regression with the last patch.

Marked as Passed QA.

Created attachment 19374 [details] [review]
Bug 10462: Some optimizations in Z3950 search paving the way for enhancements

Refactors Z3950Search.
Disable batch record counts for z3950 records.

Test plan:
Do various Z3950 searches on multiple targets from Cataloging and Acquisition.
Behavior should not have changed.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 19375 [details] [review]
Bug 10462: Followup for showing multiple ISBNs in Z3950 response

As Jonathan correctly noted, the new Z3950 response only showed one isbn
although more isbn numbers could be in the record and would be imported.
To resolve this display problem, I traverse them all now in the updated
routine _isbn_show. There is no change in the imported records.
Note that before this patch TransformMarcToKoha did put all isbn numbers in
one field, separated by pipes (for display only). This behavior is restored
now. The three regexes on the individual isbn numbers now seem to be
overkill, but I left them there for completeness.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested this on a fresh French install under UNIMARC with BNF server.
Tested it too for MARC21.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

This patch changes what gets returned as the LCCN.  The acronym "LCCN" is ambiguous, unfortunately, as it can refer to LC Control Number or LC Call Number.  The current behavior of the Z39.50 search is to return the former (i.e., the control number or LC-assigned identification number).

Here is a diff (before and after applying the patch) of the output of a little test script I wrote that searches LC's Z39.50 server for ISBN 9780415964845.

                  'total_pages' => 1,
                  'breeding_loop' => [
                                       {
-                                        'lccn' => '  2008005439',
-                                        'breedingid' => '91',
-                                        'date' => '2008',
-                                        'author' => undef,
+                                        'lccn' => 'PR6068.O93',
+                                        'date' => '2008.',
+                                        'breedingid' => '92',
+                                        'author' => '',
                                         'biblionumber' => undef,
                                         'edition' => '2nd ed.',
                                         'server' => 'LIBRARY OF CONGRESS',

I'm marking this as failed QA for the change in visible behavior, but this issue speaks to a broader concern I have with this patch: why replace the call to TransformMarcToKoha, which uses the installed default bibliographic framework to extract strings from the MARC record, with hardcoding the tags and subfields for each MARC flavor?

> This patch changes what gets returned as the LCCN.  The acronym "LCCN" is 
> ambiguous, unfortunately, as it can refer to LC Control Number or LC Call 
> Number.  The current behavior of the Z39.50 search is to return the former 
> (i.e., the control number or LC-assigned identification number).

Good catch. Apparently mixed up these two tags with same acronym. The small followup corrects that.

> Why replace the call to TransformMarcToKoha, which uses the installed default
> bibliographic framework to extract strings from the MARC record, with 
> hardcoding the tags and subfields for each MARC flavor?

Good question. The main reason was just optimization. I saw that we needed only six fields from the MARC record for a simple display, and TransformMarcToKoha is doing a transformation of the whole MARC record.
The original code contains a call to TransformMarcToKoha followed by a call to ImportBreeding that again calls TransformMarcToKoha. Both calls have been optimized. 'Downside' is indeed a few lines hardcoding tags in the sub _add_rowdata.
Let me still see a moment if we can still improve some there in order to gain more confidence :)

(In reply to M. de Rooy from comment #13)
> Good question. The main reason was just optimization. I saw that we needed
> only six fields from the MARC record for a simple display, and
> TransformMarcToKoha is doing a transformation of the whole MARC record.
> The original code contains a call to TransformMarcToKoha followed by a call
> to ImportBreeding that again calls TransformMarcToKoha. Both calls have been
> optimized. 'Downside' is indeed a few lines hardcoding tags in the sub
> _add_rowdata.
> Let me still see a moment if we can still improve some there in order to
> gain more confidence :)

I suspected it would be something like that.  Arguably that's at the level of micro-optimization that could be dispensed with in the interests of keeping the code general, but a possible middle ground that might speed things up would be writing a routine like this:

$hashref = ExtractFieldsPerFrameWork($marcrecord, $arrayref_of_fields, $frameworkcode);

e.g.

ExtractFieldsPerFrameWork($marcrecord, ['biblio.title', 'bibliitems.issn' ], '');

returning

{
  'biblio.title' => 'Journal of irreproducible results',
  'biblioitems.issn' => '0022-2038',
}

Created attachment 19928 [details] [review]
Bug 10462: QA Followup to resolve LCCN mixup and remove hardcoded marc tags

This patch corrects the mixup for LC call number and control number.

Further, as suggested by Galen, it would be better to not introduce hardcoded
tags in the Z3950Search subs in Breeding.pm.
This patch resolves that by calling TransformMarcToKohaOneField.
Note that this only involves changes to _addrowdata and _isbn_show. These
subs are only used in building the displayed results table.

Additionally, for French UNIMARC installs publicationyear is used to fill
the Date column (copyrightdate is not used in those installs). The edition
statement is only used in unimarc_lecture_pub not in unimarc_complet.

Test plan:
Do some Z3950 searches and look for values in all result columns.
For MARC21 on LOC (and/or others):
  Look for isbn 9780415964845 (check LCCN).
  Look for author Rowling.
For UNIMARC on BNF2 (and/or others):
  On BNF2 look for isbn 2070518426: result contains date and multiple isbn's.
  Look for title: Guide des candidats aux emplois de commissaire de police.
  Third result show edition statement (if you use 205$a with pub install).
  Note that there are no results with LCCN here (just as before).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested for MARC21 and UNIMARC (French lecture_pub install).

Galen,
Saw your comment just after uploading the latest followup that hopefully covers your suggestions as well.

Resetting the status.
If you need additional signoff, please let me know. In that case Kyle or Jonathan may be willing to repeat it..

Pushed to master, along with a follow-up to get rid of some undefined variable warnings.  Thanks, Marcel!