Bug 19436 - Add SRU support for authorities
Summary: Add SRU support for authorities
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Z39.50 / SRU / OpenSearch Servers (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Matthias Meusburger
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed
Depends on:
Blocks: 21151 21318 21647
  Show dependency treegraph
 
Reported: 2017-10-09 06:03 UTC by Katrin Fischer
Modified: 2020-01-06 20:15 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Some record sources only offer SRU connections (and not Z39.50) this update allows Koha to utilize these resources for authority records as we can for bibliographic records
Version(s) released in:


Attachments
Bug 19436: Add SRU support for authorities (25.46 KB, patch)
2018-02-27 10:23 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 19436: Add SRU support for authorities (25.58 KB, patch)
2018-03-14 11:49 UTC, François Pichenot
Details | Diff | Splinter Review
Bug 19436: Add SRU support for authorities (25.68 KB, patch)
2018-03-16 10:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: (QA follow-up) Revert change in _handle_one_result (1.43 KB, patch)
2018-03-16 10:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: (QA follow-up; see 19608) Fix use of intranet-bottom in template (1.55 KB, patch)
2018-03-16 10:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: Add SRU support for authorities (25.68 KB, patch)
2018-03-21 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: (QA follow-up) Revert change in _handle_one_result (2.49 KB, patch)
2018-03-21 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: (QA follow-up; see 19608) Fix use of intranet-bottom in template (1.55 KB, patch)
2018-03-21 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: Add SRU support for authorities (25.70 KB, patch)
2018-07-20 10:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: (QA follow-up) Revert change in _handle_one_result (2.49 KB, patch)
2018-07-20 10:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19436: (QA follow-up; see 19608) Fix use of intranet-bottom in template (1.55 KB, patch)
2018-07-20 10:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2017-10-09 06:03:10 UTC
It would be great if Koha also supported SRU for authorities, as libraries like the National Library of Germany only support SRU and no longer offer Z39.50 access.
Comment 1 Gaetan Boisson 2017-11-07 10:16:56 UTC
This would indeed be great. The French national library is also offering SRU for authorities  now : http://catalogue.bnf.fr/api/test.do

See and example here : http://catalogue.bnf.fr/api/SRU?version=1.2&operation=searchRetrieve&query=(aut.type%20any%20%22pep%20org%22)%20%20and%20(aut.accesspoint%20all%20%22victor%20hugo%22)
Comment 2 Matthias Meusburger 2018-02-27 10:23:29 UTC Comment hidden (obsolete)
Comment 3 François Pichenot 2018-03-14 11:49:27 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2018-03-16 07:32:47 UTC
QA: Looking here
Comment 5 Marcel de Rooy 2018-03-16 08:28:16 UTC
koha-tmpl/intranet-tmpl/prog/en/includes/authorities-toolbar.inc:            <a class="btn btn-default btn-sm" id="z3950submit" href="#"><i class="fa fa-search"></i> New from Z39.50/SRU</a>

servers.count > 0
Should depend on authority servers, not biblio servers.

Fix submitted on bug 20430, not caused by this report.
Comment 6 Marcel de Rooy 2018-03-16 08:29:09 UTC
Search/Cancel buttons on cataloguing/z3950_auth_search.pl should be moved upwards.
Note that cataloguing/z3950_auth_search.pl  should have been in authorities dir.

Same thing: caused earlier, out of scope.
Comment 7 Marcel de Rooy 2018-03-16 08:53:09 UTC
Sub _handle_one_result is changed here, but not used in the scope of this report.

     if( $servhref->{servertype} eq 'sru' ) {
-        $marcrecord= MARC::Record->new_from_xml( $raw, 'UTF-8', $servhref->{syntax} );
-    } else {
-        ($marcrecord) = MarcToUTF8Record($raw, C4::Context->preference('marcflavour'), $servhref->{encoding} // "iso-5426" ); #ignores charset return values
+        $raw= MARC::Record->new_from_xml( $raw, $servhref->{encoding}, $servhref->{syntax} );
     }
+    ($marcrecord) = MarcToUTF8Record($raw, C4::Context->preference('marcflavour'), $servhref->{encoding} // "iso-5426" ); #ignores charset return values

OLD:
    if( $servhref->{servertype} eq 'sru' ) {
        $marcrecord= MARC::Record->new_from_xml( $raw, 'UTF-8',
            $servhref->{syntax} );
    } else {
        ($marcrecord) = MarcToUTF8Record($raw, C4::Context->preference('marcflavour'), $servhref->{encoding} // "iso-5426" ); #ignores charset return values
    }

NEW:
    if( $servhref->{servertype} eq 'sru' ) {
        $raw= MARC::Record->new_from_xml( $raw, $servhref->{encoding}, $servhref->{syntax} );
    }
    ($marcrecord) = MarcToUTF8Record($raw, C4::Context->preference('marcflavour'), $servhref->{encoding} // "iso-5426" ); #ignores charset return values

You might be solving another bug here (??), but it should be moved to another report. I plan to revert this change in a follow-up.
Please explain the change too. The old situation looks better to me.
Note also that the encoding variable may contain 'utf8' instead of 'UTF-8'; might be wrong for other encodings as well.
Comment 8 Marcel de Rooy 2018-03-16 09:51:52 UTC
Another thing outside the strict scope of this report: add_xslt is ignored in authority search. See bug 20431.
Comment 9 Marcel de Rooy 2018-03-16 10:49:32 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2018-03-16 10:49:36 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2018-03-16 10:49:40 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2018-03-16 10:54:20 UTC
Looks quite good to me, only lots of distraction by earlier reports :)

Small design issue, no blocker: Text of Author meeting/conference behind input on sru_modmapping.

One final question remains however:
+                            if( $servers[$k]->{servertype} eq 'sru' ) {
+                                $marcdata = MARC::Record->new_from_xml( $marcdata, $encoding[$k], $servers[$k]->{syntax} );
+                            }
                             ($marcrecord, $charset_result, $charset_errors)= MarcToUTF8Record($marcdata, $marc_type, $encoding[$k]);

With reference to comment7, I would prefer to adjust this change in Z3950SearchAuth in line with _handle_one_result.
Please give feedback.
Will pass QA after your response.
Comment 13 Matthias Meusburger 2018-03-19 17:15:17 UTC
Okay, I had some trouble seeing what was going on here, but here we go:

Both parts (in handle_one_result and Z3950SearchAuth) were made to fix an encoding problem, but I may have not made the best of fixes.

The problem comes from the fact that I tried to use the encoding string from the Z3950 profiles for the MARC::Record->new_from_xml calls, just like it is used for MarcToUTF8Record.

However, if MarcToUTF8Record recognizes 'UTF-8' as well as 'utf8', MARC::Record->new_from_xml doesn't.

So I'm perfectly fine with your revert of _handle_one_result, and making the calls in Z3950SearchAuth in line with _handle_one_result (with the use of the 'UTF-8' string in plain text in the code).

However, that would be assuming that all SRU servers are serving UTF-8 (are they?).
In that case, we should disable the "Encoding" dropdown list in SRU servers configuration.

What do you think ?
Comment 14 Marcel de Rooy 2018-03-20 13:20:22 UTC
(In reply to Matthias Meusburger from comment #13)

> Both parts (in handle_one_result and Z3950SearchAuth) were made to fix an
> encoding problem, but I may have not made the best of fixes.

Please specify what the problem was. What settings/encodings ?

> The problem comes from the fact that I tried to use the encoding string from
> the Z3950 profiles for the MARC::Record->new_from_xml calls, just like it is
> used for MarcToUTF8Record.
> However, if MarcToUTF8Record recognizes 'UTF-8' as well as 'utf8',
> MARC::Record->new_from_xml doesn't.

The second encoding parameter of new_from_xml is the desired result encoding. Since we want UTF-8, there is no need to pass utf8. The third parameter is the source format (UNIMARC, USMARC, MARC21), not the encoding. The xml itself should contain an encoding too.

The optimization in handle_one_result eliminated the need for calling new_from_xml and MarcToUTF8Record in a row, doing more or less the same.

> So I'm perfectly fine with your revert of _handle_one_result, and making the
> calls in Z3950SearchAuth in line with _handle_one_result (with the use of
> the 'UTF-8' string in plain text in the code).
> However, that would be assuming that all SRU servers are serving UTF-8 (are
> they?).
> In that case, we should disable the "Encoding" dropdown list in SRU servers
> configuration.

No, that is a misunderstanding. We are converting to UTF-8 as an internal format for Koha, not as a source encoding.
Comment 15 Matthias Meusburger 2018-03-20 13:55:29 UTC
> The second encoding parameter of new_from_xml is the desired result
> encoding. Since we want UTF-8, there is no need to pass utf8. The third
> parameter is the source format (UNIMARC, USMARC, MARC21), not the encoding.

Yes, my bad.


> Please specify what the problem was. What settings/encodings ?

Nothing to see here. In the end, it was a problem I created myself. Doh.


> The xml itself should contain an encoding too.
 
> No, that is a misunderstanding. We are converting to UTF-8 as an internal
> format for Koha, not as a source encoding.

Yes, indeed. 

However, if new_from_xml can manage the encoding of the xml by itself to produce valid UTF-8, and we always use UTF-8 in Koha, this dropdown list "Encoding (z3950 can send records in various encodings. Choose one):" is completely useless when editing an SRU server, right?
If it is the case, shouldn't we disable it or hide it?
Comment 16 Marcel de Rooy 2018-03-21 12:42:22 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2018-03-21 12:42:26 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2018-03-21 12:42:30 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2018-03-21 12:44:08 UTC
(In reply to Matthias Meusburger from comment #15)

> However, if new_from_xml can manage the encoding of the xml by itself to
> produce valid UTF-8, and we always use UTF-8 in Koha, this dropdown list
> "Encoding (z3950 can send records in various encodings. Choose one):" is
> completely useless when editing an SRU server, right?
> If it is the case, shouldn't we disable it or hide it?

OK Got your point. But note: A few lines after the call to new_from_xml versus MarcToUTF8Record, $encoding[$k] is used in call ImportBreedingAuth -> AddAuthToBatch -> _create_import_record -> table import_records. (So it seems that we only save it in this table..) This could get attention on another report. 

Amended the second patch (first follow-up) in favor of the new_from_xml approach in handle_one_result.
Comment 20 Matthias Meusburger 2018-03-22 07:38:08 UTC
Ok, noted. Thank you Marcel for the QA, thank you François for the Sign-Off.
Comment 21 Marcel de Rooy 2018-05-11 06:19:24 UTC
Jonathan: What happens here and why (not)?
Comment 22 Jonathan Druart 2018-06-01 13:14:10 UTC
I do not know this code but seems quite old school and error prone.

It seemed safer to not include it into 18.05.
Comment 23 Marcel de Rooy 2018-06-04 07:09:56 UTC
(In reply to Jonathan Druart from comment #22)
> I do not know this code but seems quite old school and error prone.
> 
> It seemed safer to not include it into 18.05.

Thx for responding now. Too bad that there was no chance for any participant on this report to address such concerns.

@Nick: Could you push this one soon now? Let's catch these errors ;)
Comment 24 Nick Clemens 2018-07-02 13:10:26 UTC
Changes from 15360 cause conflicts, can you rebase/add a follow up?
Comment 25 Marcel de Rooy 2018-07-02 13:18:51 UTC
(In reply to Nick Clemens from comment #24)
> Changes from 15360 cause conflicts, can you rebase/add a follow up?

You should have waited a bit with that one, huh ?
Comment 26 Marcel de Rooy 2018-07-20 10:24:47 UTC
Created attachment 77158 [details] [review]
Bug 19436: Add SRU support for authorities

Test plan:
 - Apply the patch
 - Add an SRU authority server in admininistration -> Z39.50/SRU servers
   You can try with the French national library, configured as such:
   Hostname: catalogue.bnf.fr
   Port: 80
   Database: api/SRU
   Syntax: Unimarc
   Record type: authority
   Additional SRU options: version=1.2,sru=get
   SRU Search fields mapping example:
	Keyword (any): aut.anywhere
	Name (any): aut.anywhere
	Author (any): (aut.type any "pep org") and aut.accesspoint
	Author (personal): aut.type=pep and aut.accesspoint
	Author (corporate): aut.type=org and aut.accesspoint
	Author (meeting/conference): aut.type=org and aut.accesspoint
	Subject heading: (aut.type any "geo ram_nc ram_ge ram_pe ram_co") and aut.accesspoint
	Subject sub-division: aut.type=ram_pe and aut.accesspoint
	Title (any): (aut.type any "tic tut tum ram_tp ram_tu") and aut.accesspoint
	Title (uniform):(aut.type any "tut tum ram_tu") and aut.accesspoint

 - Try a search from Authorities -> New from Z39.50/SRU
 - Check that the authority is correctly displayed in "Show Marc"
 - Check that the authority is correclty added to koha in "Import"
 - prove t/db_dependent/Breeding.t

Signed-off-by: François Pichenot <fpichenot@ville-roubaix.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2018-07-20 10:24:52 UTC
Created attachment 77159 [details] [review]
Bug 19436: (QA follow-up) Revert change in _handle_one_result

See Bugzilla comment 7. This change does not belong here and is
dubious on its own. Needs further attention on another report.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Amended: In consultation with the author the same change is applied to the
corresponding lines in Z3950SearchAuth.
Comment 28 Marcel de Rooy 2018-07-20 10:24:56 UTC
Created attachment 77160 [details] [review]
Bug 19436: (QA follow-up; see 19608) Fix use of intranet-bottom in template

Bug 19608 incorrectly replaced intranet-bottom by popup-bottom.
Adjusting the new template in the same way.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2018-07-20 10:26:02 UTC
(In reply to Nick Clemens from comment #24)
> Changes from 15360 cause conflicts, can you rebase/add a follow up?

Actually quite trivial rebase ;)
Appears that Matthias missed your request?

Verified that I still get results from LOC SRU auth server.
Comment 30 Matthias Meusburger 2018-07-20 12:56:06 UTC
Yes, sorry about that. Thank you for the rebase!
Comment 31 Fridolin SOMERS 2018-08-01 06:13:49 UTC
I think we can also remove those lines in z3950servers.tt :
    // enable recordtype to include field in post
    $('#recordtype').prop('disabled',false);
Comment 32 Marcel de Rooy 2018-08-03 12:35:13 UTC
This was QAed in March, guys. We are in August now! Is this part of the "scare the last developers away" project or so ?
Comment 33 Jonathan Druart 2018-08-03 16:04:43 UTC
(In reply to Marcel de Rooy from comment #32)
> This was QAed in March, guys. We are in August now! Is this part of the
> "scare the last developers away" project or so ?

I am not aware of such project, where is the code?

RM is on holiday and Tomas is processing bug fixes only.

About the non-inclusion into 18.05 I have answered already.
Comment 34 Nick Clemens 2018-08-08 21:32:40 UTC
Awesome work all!

Pushed to master for 18.11
Comment 35 Martin Renvoize 2018-08-09 10:11:35 UTC
Enhancement, will not be backported to 18.05.x series.