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)

QA: Looking here

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.

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.

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.

Another thing outside the strict scope of this report: add_xslt is ignored in authority search. See bug 20431.

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.

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 ?

(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.

> 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?

(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.

Ok, noted. Thank you Marcel for the QA, thank you François for the Sign-Off.

Jonathan: What happens here and why (not)?

I do not know this code but seems quite old school and error prone.

It seemed safer to not include it into 18.05.

(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 ;)

Changes from 15360 cause conflicts, can you rebase/add a follow up?

(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 ?

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>

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.

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>

(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.

Yes, sorry about that. Thank you for the rebase!

I think we can also remove those lines in z3950servers.tt :
    // enable recordtype to include field in post
    $('#recordtype').prop('disabled',false);

This was QAed in March, guys. We are in August now! Is this part of the "scare the last developers away" project or so ?

(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.

Awesome work all!

Pushed to master for 18.11

Enhancement, will not be backported to 18.05.x series.