Bug 13937

Summary: Add an Elasticsearch-compatible Z39.50/SRU daemon
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Z39.50 / SRU / OpenSearch ServersAssignee: Ere Maijala <ere.maijala>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: abl, alex.arnaud, amy, black23, bouzid.fergani, chris.kirby, chris, dcook, ere.maijala, fridolin.somers, gmcharlt, gwilliams, johanna.raisa, jonathan.druart, joonas.kylmala, josef.moravec, katrin.fischer, m.de.rooy, martin.renvoize, michaela.sieber, mirko, nick, nicole, olli-antti.kivilahti, olli-antti.kivilahti, stefan.berndtsson, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19360
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18760
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21210
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23887
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24445
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This development allows libraries wishing to run Elasticsearch but also serve as a public SRU/Z39.50 gateway to do so without running Zebra in parallel.
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 21210, 23949, 24022, 24445, 24508, 24905, 25277, 25278, 25292, 25618, 27149, 28364, 31964    
Attachments: Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 (Followup) - Remove reference to sub _prefetch_records
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 (Followup) - Remove reference to sub _prefetch_records
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 (Followup) - Remove reference to sub _prefetch_records
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 (Followup) - Remove reference to sub _prefetch_records
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 (Followup) - Remove reference to sub _prefetch_records
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937 (Followup) - Remove reference to sub _prefetch_records
Bug 13937 - (Followup) Correct error call to use self
Bug 13937 - Unit tests
Bug 13937: Followup - pass through yaz switches and don't ignore case
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Fix RPN conversion
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Fix RPN conversion
Bug 13937: Add support for Z39.50 responder to the Debian scripts
11 - Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields. Fix log path.
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Fix RPN conversion
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Bug 13937: Fix the daemon log path.
Bug 13937: Switch from GetItem() to Koha::Items->find()
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Fix RPN conversion
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Bug 13937: Fix the daemon log path.
Bug 13937: Switch from GetItem() to Koha::Items->find()
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields
Bug 13937: (follow-up) Remove reference to sub _prefetch_records
Bug 13937: (follow-up) Correct error call to use self
Bug 13937: Add unit tests
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Bug 13937: Fix issues found in QA
Bug 13937: Add support for SRU and Elasticsearch
Bug 13937: Add tests for search and retrieval
Bug 13937: Fix RPN conversion
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Bug 13937: Fix the daemon log path.
Bug 13937: Switch from GetItem() to Koha::Items->find()
Bug 13937: Add SRU tests
Bug 13937: (QA follow-up) Fix tests.
Bug 13937: (follow-up) Add missing entry to koha-common.install
Bug 13937: (QA follow-up) Make tests work with YAZ 4 and 5.
Bug 13937: (QA follow-up) Make tests work with YAZ 4 and 5.
Bug 13937: (RM follow-up) Set working directory for daemon mode.
Bug 13937: (follow-up) Fix searching with UTF-8 characters.
Bug 13937: (follow-up) sort SRU results by id asc

Description Kyle M Hall 2015-04-01 12:46:22 UTC
Some libraries have found that Koha doesn't work well with some external services like SHAREit that utilize z39.50 to get information about a record and its' items.

a) We propose to add field 'status' to items table ( this field can be mapped to whatever MARC tag you wish )

b) Engineer a subroutine that would generate the item's status

The precedence for status would be as follows:
1) Checked Out
2) Lost
3) Not for Loan
4) Damaged
5) On Hold ( In Transit )
6) In Transit
7) On Hold

This status could eventually be used within Koha to simplify our status display logic, but that is outside the immediate scope of this enhancement.
Comment 1 Katrin Fischer 2015-04-01 12:56:01 UTC
I am not sure about this idea - do we really need to store it in the database as it is a calculated value? Or could it be something that is added on export? 
Some libraries allow check-out of damaged items depending on severity - not sure if the sequence is something that should/could be configurable.
Comment 2 Kyle M Hall 2015-04-01 13:18:00 UTC
(In reply to Katrin Fischer from comment #1)
> I am not sure about this idea - do we really need to store it in the
> database as it is a calculated value? Or could it be something that is added
> on export? 
> Some libraries allow check-out of damaged items depending on severity - not
> sure if the sequence is something that should/could be configurable.

It must have the ability to be stored in the MARC, otherwise it won't be useful. The problem is that external software utilizing z39.50 cannot determine the true status of an item based on the fields returned.
Comment 3 Kyle M Hall 2015-04-01 13:21:36 UTC
I would also like the emphasize that this will be a completely optional component and will not be a required feature in Koha. As I wrote, we *could* use this feature within Koha, but I don't see that happening any time soon.
Comment 4 Galen Charlton 2015-04-01 14:41:34 UTC
(In reply to Kyle M Hall from comment #2)
> (In reply to Katrin Fischer from comment #1)
> > I am not sure about this idea - do we really need to store it in the
> > database as it is a calculated value? Or could it be something that is added
> > on export? 
> > Some libraries allow check-out of damaged items depending on severity - not
> > sure if the sequence is something that should/could be configurable.
> 
> It must have the ability to be stored in the MARC, otherwise it won't be
> useful. The problem is that external software utilizing z39.50 cannot
> determine the true status of an item based on the fields returned.

I'm +1 for the general idea of providing a unified item status value to consumers of Koha APIs, Z39.50 clients, and metadata exports.

But responding to your response to Katrin's point: for the purpose of a Z39.50 client or a consumer of a MARC export, it doesn't need to be _stored_ in the MARC record or in the items table; it only needs to be emitted when called for. This could, for example, be invoked by EmbedItemsInMarcBiblio().  Another option would be to have it be a derived DBIC virtual field.

Why not store it in an items.status column?  Because doing so automatically creates more work: it would HAVE to be kept up to date whenever an item gets modified or participates in a circulation transaction.  Why? You can't stop folks from writing SQL queries on it, and you can't stop developers who are not intimately familiar with Koha's notion of item status from trying to use it. To put it another way, *storing* the calculated unified item status value in the database means that you couldn't evade the scope creep of fixing all the item status things in Koha.
Comment 5 Katrin Fischer 2015-04-01 14:50:36 UTC
Hm, it would also be very annoying to correct in case of changing the logic or adding to it. Calculating on the fly might allow for more flexibility and configuration options here.
Comment 6 Chris Cormack 2015-04-01 16:52:03 UTC
To echo Galen, the items are not stored in MARC .. we got rid of that in Koha 3.4  it's only exported to MARC when we are generating the MARC for zebra or an export. So yeah, adding it to the EmbedItemsInMarcBiblio() subroutine would make a lot of sense to me. Since that is the single point in Koha where items data is converted to MARC
Comment 7 HB-NEKLS 2015-04-14 16:30:34 UTC
I'm not quite following the comments being made here -- forgive my ignorance on how MARC works with the Koha items table. 

TLDR version: Regardless of how this is developed and coded, will the final data field result be mappable to a MARC 952 subfield within Koha? It also is a status that needs to be constantly updated, in order for it to be used successfully by an external system.

Long version: 

Background: Right now the not for loan, the lost, the damaged, and the withdrawn fields are separate items table subfields and mapped to the 952 field within MARC. Additionally, the in transit/on hold, waiting for pickup status of an item is stored elsewhere. 

NEKLS, SEKLS, and CKLS would like to develop an additional Koha field that is then possible to be mapped to a MARC 952 subfield so external systems are able to tell if an item is TRULY available for ILL (in our specific case); I'm sure there are other uses we haven't thought about. This new field would be able to aggregate the separate onloan, notforloan, withdrawn, lost, and damaged item fields, as well as pull in the separate data from the branchtransfers and reserves tables that indicate if an item is in transit and/or on hold waiting for pickup; it would need to be updated automatically -- not only on exports or when scripts are run.

When we first looked into this last fall, we discovered that other ILSes typically have one MARC subfield where availability data is stored. Koha currently has the data that determines an item's availability stored all over the place. Right now, this external system profiles our systems using the 952$q field (items.onloan). That field isn't enough, obviously, to determine true availability; items that are on hold, in transit, lost, damaged, regularly appear on ILL request lists. Years ago, we asked about how to fix this problem, but were told a major rewrite of Koha would have to happen. Obviously, we've lived with the situation since. But Jason Robb at SEKLS started asking questions last fall, and we were told that this problem is possible to be fixed now.  

It was originally explained to us that this development could have greater impact on Koha in other places, too, but Kyle can explain that part much better. 

As long as we can map the outcome of where ever this data combining all of these statuses is stored to a MARC 952 item field, I don't care how it's developed :) But if the end result is that this new field is not going to be constantly updated and it can't be mapped to a MARC subfield, we may have to abandon this development.
Comment 8 Kyle M Hall 2015-04-15 14:07:54 UTC
Thanks Galen! That's a capital idea! I imagine then I would need to add a system preference to decide which field/subfield that status value should go into, right?

(In reply to Galen Charlton from comment #4)
> (In reply to Kyle M Hall from comment #2)
> > (In reply to Katrin Fischer from comment #1)
> > > I am not sure about this idea - do we really need to store it in the
> > > database as it is a calculated value? Or could it be something that is added
> > > on export? 
> > > Some libraries allow check-out of damaged items depending on severity - not
> > > sure if the sequence is something that should/could be configurable.
> > 
> > It must have the ability to be stored in the MARC, otherwise it won't be
> > useful. The problem is that external software utilizing z39.50 cannot
> > determine the true status of an item based on the fields returned.
> 
> I'm +1 for the general idea of providing a unified item status value to
> consumers of Koha APIs, Z39.50 clients, and metadata exports.
> 
> But responding to your response to Katrin's point: for the purpose of a
> Z39.50 client or a consumer of a MARC export, it doesn't need to be _stored_
> in the MARC record or in the items table; it only needs to be emitted when
> called for. This could, for example, be invoked by EmbedItemsInMarcBiblio().
> Another option would be to have it be a derived DBIC virtual field.
> 
> Why not store it in an items.status column?  Because doing so automatically
> creates more work: it would HAVE to be kept up to date whenever an item gets
> modified or participates in a circulation transaction.  Why? You can't stop
> folks from writing SQL queries on it, and you can't stop developers who are
> not intimately familiar with Koha's notion of item status from trying to use
> it. To put it another way, *storing* the calculated unified item status
> value in the database means that you couldn't evade the scope creep of
> fixing all the item status things in Koha.
Comment 9 Jesse Weaver 2015-12-11 16:52:21 UTC
I've stolen this development from Kyle, and I have an idea for implementing it that could get us other benefits down the road. Instead of shoving this new status subfield into the MARC upon export to Zebra, why not just make a simple Z39.50 proxy using https://metacpan.org/pod/Net::Z3950::SimpleServer that will query Zebra and insert the item status information on its way back? This means that the item status information will always be up to date and the export remains simple.

Also, once we bring in ElasticSearch, we won't have to keep Zebra around purely for Z39.50 (though we'll have to add some kind of PQF -> ElasticSearch translation layer to make this work).

Thoughts?
Comment 10 Kyle M Hall 2015-12-11 17:56:04 UTC
I'm for this idea. I think a path to z39.50 without Zebra is very good and useful, aside from the benefit of simplifying the feature in a way, and having it be always up to date.
Comment 11 HB-NEKLS 2015-12-11 18:01:50 UTC
I added a version of this comment privately where we're tracking the development, but I'll go ahead and add it here publicly. 

As one of the development sponsors, if this feature is not going to be coded so it can be profiled to a MARC 952 item field, I can no longer support this development. I can't speak for the development co-sponsors, but we're all in the same boat with why we pursued and are funding this idea in the first place.

We are specifically needing this new field to be able to be profiled to a MARC 952 item field to meet our needs (communicating accurate available/not available item status to an external ILL system that can only profile one MARC subfield using z3950 protocol). If that option is not feasible or supported, I'd consider this enhancement dead. 

If what Jesse is proposing still is able to be profiled to a MARC field in the end, then the above can be completely disregarded.
Comment 12 HB-NEKLS 2015-12-11 18:13:31 UTC
And all is good now -- Jesse has said that the MARC subfield creation is still possible with the procedure of what he's suggesting. So my concerns are satisfied.
Comment 13 Jesse Weaver 2016-01-06 22:12:17 UTC Comment hidden (obsolete)
Comment 14 Mirko Tietgen 2016-01-28 20:45:34 UTC
It works very well for me, except for step 8. I still get the default descriptions.
Comment 15 Jesse Weaver 2016-01-28 22:12:35 UTC Comment hidden (obsolete)
Comment 16 Jesse Weaver 2016-01-28 22:15:29 UTC Comment hidden (obsolete)
Comment 17 Jesse Weaver 2016-01-28 22:16:37 UTC
Sorry for the spam; needed to note a new option in the test plan and show our sponsors.

Mirko, could you try clearing memcached? Can't duplicate your issue.
Comment 18 Mirko Tietgen 2016-01-28 22:34:08 UTC Comment hidden (obsolete)
Comment 19 Mirko Tietgen 2016-01-28 22:35:18 UTC
There is no memcached running on the VM I am testing on. Anyway it worked now with the latest patch. I fixed a conflict in C4/Installer/PerlDependencies.pm.
Comment 20 Jonathan Druart 2016-01-29 10:34:38 UTC
Jesse,
I would love to see this code moved to a module covered by tests.
Do you think it's conceivable?
Comment 21 Jesse Weaver 2016-02-12 23:04:21 UTC
I think that's reasonable, depending on what you have in mind testing-wise. Full round-trip testing of Z39.50 could be difficult, but unit tests for the item-status-adding would be quite doable.
Comment 22 Jonathan Druart 2016-02-15 15:51:23 UTC
Something like start the server (make sure it starts correctly), request it, fetch records.
Comment 23 Jonathan Druart 2016-03-11 10:42:39 UTC
Waiting for test coverage.
Comment 24 Jesse Weaver 2016-07-19 21:36:15 UTC
Created attachment 53503 [details] [review]
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

This version of the patch moves the server out to a module but still needs unit tests.
Comment 25 Nick Clemens 2016-12-12 14:22:22 UTC
Created attachment 58122 [details] [review]
Bug 13937 (Followup) - Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this subis built into fetch and this call is a duplicate
Comment 26 Nick Clemens 2016-12-12 14:23:20 UTC
Created attachment 58125 [details] [review]
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.
Comment 27 Nick Clemens 2016-12-12 14:23:27 UTC
Created attachment 58126 [details] [review]
Bug 13937 (Followup) - Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this subis built into fetch and this call is a duplicate
Comment 28 Olli-Antti Kivilahti 2017-01-05 11:39:06 UTC
Maybe this Bug can help with availability calculations?

Bug 17712
Comment 29 Nick Clemens 2017-01-23 17:13:03 UTC
Created attachment 59443 [details] [review]
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.
Comment 30 Nick Clemens 2017-01-23 17:13:12 UTC
Created attachment 59444 [details] [review]
Bug 13937 (Followup) - Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this subis built into fetch and this call is a duplicate
Comment 31 Nick Clemens 2017-02-28 19:36:48 UTC
Created attachment 60735 [details] [review]
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.
Comment 32 Nick Clemens 2017-02-28 19:36:56 UTC
Created attachment 60736 [details] [review]
Bug 13937 (Followup) - Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this subis built into fetch and this call is a duplicate
Comment 33 Barton Chittenden 2017-02-28 20:01:50 UTC
Created attachment 60737 [details] [review]
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 34 Barton Chittenden 2017-02-28 20:02:04 UTC
Created attachment 60738 [details] [review]
Bug 13937 (Followup) - Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this subis built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 35 Jonathan Druart 2017-03-07 19:10:05 UTC
(In reply to Jonathan Druart from comment #23)
> Waiting for test coverage.

Hum?
Comment 36 Nick Clemens 2017-09-07 15:53:42 UTC
Created attachment 66943 [details] [review]
Bug 13937 - Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 37 Nick Clemens 2017-09-07 15:53:46 UTC
Created attachment 66944 [details] [review]
Bug 13937 (Followup) - Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this subis built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 38 Nick Clemens 2017-09-07 15:53:49 UTC
Created attachment 66945 [details] [review]
Bug 13937 - (Followup) Correct error call to use self
Comment 39 Nick Clemens 2017-09-07 15:53:53 UTC
Created attachment 66946 [details] [review]
Bug 13937 - Unit tests
Comment 40 Nick Clemens 2017-11-14 12:30:42 UTC
Created attachment 69139 [details] [review]
Bug 13937: Followup - pass through yaz switches and don't ignore case
Comment 41 Marcel de Rooy 2018-04-20 08:56:35 UTC
QA: Looking here
Comment 42 Marcel de Rooy 2018-04-20 10:29:22 UTC
=== General
The idea is interesting!

It works but I have the impression that it still needs some fine-tuning and further testing. Crashes on a show 0 for instance. Tests need attention.
As an optional feature, it will not harm people not using it. 

On the other hand, it adds 600 lines of code for an additional item status via a Z39.50 daemon (including overlap with C4/Search Zebra code). We need to maintain that. Food for discussion?

The feature does not support showing this new status via SRU/SRW in analogy with Zebra? (Only see YAZ responding with 404 on 9999 now in the browser.)

In terms of new development, I wonder if it would not be simpler to provide an item status via REST API, and not invest in Zebra/Z39.50 (not looking at specific sponsor specs here)? Further thoughts ?
Comment 43 Marcel de Rooy 2018-04-20 10:29:53 UTC
=== Details
Net::Z3950::SimpleServer
Just wondering why we need a new dep. SIPServer uses Net::Server::PreFork ?

t/Koha/Z3950responder.t
This test fails with me. Please check.
not ok 3 - We didn't start server , should fail
#   Failed test 'We didn't start server , should fail'
#   at t/Koha/Z3950responder.t line 24.
#          got: undef
#     expected: '2'
not ok 4 - We didn't start server, should fail because it cannot connect
#   Failed test 'We didn't start server, should fail because it cannot connect'
#   at t/Koha/Z3950responder.t line 25.
#          got: undef
#     expected: 'Cannot connect to upstream server'
not ok 5 - There is no handler as we aren't connected
#   Failed test 'There is no handler as we aren't connected'
#   at t/Koha/Z3950responder.t line 27.
#          got: '13'
#     expected: '30'
not ok 6 - We don't have a handler, should fail because we don't
#   Failed test 'We don't have a handler, should fail because we don't'
#   at t/Koha/Z3950responder.t line 28.
#          got: 'Fetch request out of range'
#     expected: 'No such resultset'
Use of uninitialized value $num_to_prefetch in division (/) at /usr/share/koha/devclone/Koha/Z3950Responder/Session.pm line 158.
Illegal division by zero at /usr/share/koha/devclone/Koha/Z3950Responder/Session.pm line 158.

t/db_dependent/Koha/Z3950Responder/Session.t
t/db_dependent/Koha/Z3950Responder/Session2.t
The test coverage only focuses on the add item status routine. We are not testing the server itself. Would it be too hard to try?

Commit message stuff from qa tools
        * Commit title does not contain 'follow-up' correctly spelt - 8e0da10
        * Commit title does not start with 'Bug XXXXX: ' - 31c0f7b
        * Commit title does not start with 'Bug XXXXX: ' - cf4f62b
        * Commit title does not contain 'follow-up' correctly spelt - cf4f62b
        * Commit title does not start with 'Bug XXXXX: ' - f592650
        * Commit title does not contain 'follow-up' correctly spelt - f592650
        * Commit title does not start with 'Bug XXXXX: ' - c24c232

Path::Tiny
Why is this dependency added? It is not used somewhere.

Log::Log4perl::Appender
Might be a new enhancement on itself?? 

my ($item_tag, $itemnumber_subfield) = GetMarcFromKohaField( "items.itemnumber", '' );
Does not need the default framework parameter anymore. See changed API.

Testing misc/z3950_responder.pl
Crashes on show 0
Can't call method "raw" on an undefined value at /usr/share/koha/devclone/Koha/Z3950Responder/Session.pm line 169.
Target closed connection

+    warn "name: $opt_name and value: $opt_value";
Debug leftover?

Configuration
Defaults to port 9999 (yaz default). Note that we may use port 9999 too for Zebra authorities.. Passing a listener-address worked though.
Should we add our own default?
Comment 44 Ere Maijala 2018-11-21 12:28:08 UTC
I'd suggest making the default port 210 since that's the registered port for Z39.50.
Comment 45 Ere Maijala 2018-11-21 13:14:46 UTC
Then again, 210 might be difficult since the user needs sufficient privileges to bind to it. Maybe 2100 is better.
Comment 46 Ere Maijala 2018-11-21 14:26:07 UTC
Created attachment 82562 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 47 Ere Maijala 2018-11-21 14:26:13 UTC
Created attachment 82563 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 48 Ere Maijala 2018-11-21 14:26:19 UTC
Created attachment 82564 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 49 Ere Maijala 2018-11-21 14:26:24 UTC
Created attachment 82565 [details] [review]
Bug 13937: Add unit tests
Comment 50 Ere Maijala 2018-11-21 14:26:29 UTC
Created attachment 82566 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 51 Ere Maijala 2018-11-21 14:26:34 UTC
Created attachment 82567 [details] [review]
Bug 13937: Fix issues found in QA
Comment 52 Ere Maijala 2018-11-21 14:28:18 UTC
I've addressed many of the QA issues, but I believe that it doesn't make much sense to push this forward until it works with both Zebra and Elasticsearch. That's what I'm going to work on next.
Comment 53 Marcel de Rooy 2018-11-21 14:32:28 UTC
(In reply to Ere Maijala from comment #52)
> I've addressed many of the QA issues, but I believe that it doesn't make
> much sense to push this forward until it works with both Zebra and
> Elasticsearch. That's what I'm going to work on next.

Great !
Comment 54 David Cook 2018-11-21 23:38:40 UTC
(In reply to Ere Maijala from comment #45)
> Then again, 210 might be difficult since the user needs sufficient
> privileges to bind to it. Maybe 2100 is better.

Another common Z39.50 port is 7090.
Comment 55 Ere Maijala 2018-11-22 08:11:08 UTC
It'll still be configurable.
Comment 56 Ere Maijala 2018-11-23 07:30:21 UTC
Created attachment 82589 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 57 Ere Maijala 2018-11-23 07:30:27 UTC
Created attachment 82590 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 58 Ere Maijala 2018-11-23 07:30:33 UTC
Created attachment 82591 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 59 Ere Maijala 2018-11-23 07:30:38 UTC
Created attachment 82592 [details] [review]
Bug 13937: Add unit tests
Comment 60 Ere Maijala 2018-11-23 07:30:43 UTC
Created attachment 82593 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 61 Ere Maijala 2018-11-23 07:30:49 UTC
Created attachment 82594 [details] [review]
Bug 13937: Fix issues found in QA
Comment 62 Ere Maijala 2018-11-23 07:30:54 UTC
Created attachment 82595 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Sponsored-by: National Library of Finland
Comment 63 Ere Maijala 2018-11-23 07:33:08 UTC
Ok, so, the latest version is pretty much feature-complete with support for Elasticsearch as well as Zebra. And SRU is working too. I still need to add more PODs and tests and verify that the config files work with packaging etc.
Comment 64 Ere Maijala 2018-11-23 07:52:07 UTC
*** Bug 21210 has been marked as a duplicate of this bug. ***
Comment 65 Ere Maijala 2018-11-27 11:45:06 UTC
Created attachment 82660 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 66 Ere Maijala 2018-11-27 11:45:13 UTC
Created attachment 82661 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 67 Ere Maijala 2018-11-27 11:45:19 UTC
Created attachment 82662 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 68 Ere Maijala 2018-11-27 11:45:26 UTC
Created attachment 82663 [details] [review]
Bug 13937: Add unit tests
Comment 69 Ere Maijala 2018-11-27 11:45:32 UTC
Created attachment 82664 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 70 Ere Maijala 2018-11-27 11:45:38 UTC
Created attachment 82665 [details] [review]
Bug 13937: Fix issues found in QA
Comment 71 Ere Maijala 2018-11-27 11:45:44 UTC
Created attachment 82666 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Comment 72 Ere Maijala 2018-11-27 11:45:50 UTC
Created attachment 82667 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Comment 73 Katrin Fischer 2018-12-15 09:13:21 UTC
I see that George Williams signed off the initial patches. 
George, could you retest with the new additions maybe?
Comment 74 Ere Maijala 2019-01-11 12:59:26 UTC
Created attachment 83780 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 75 Ere Maijala 2019-01-11 12:59:53 UTC
Created attachment 83781 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 76 Ere Maijala 2019-01-11 13:00:01 UTC
Created attachment 83782 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 77 Ere Maijala 2019-01-11 13:00:10 UTC
Created attachment 83783 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 78 Ere Maijala 2019-01-11 13:00:18 UTC
Created attachment 83784 [details] [review]
Bug 13937: Add unit tests
Comment 79 Ere Maijala 2019-01-11 13:00:26 UTC
Created attachment 83785 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 80 Ere Maijala 2019-01-11 13:00:32 UTC
Created attachment 83786 [details] [review]
Bug 13937: Fix issues found in QA
Comment 81 Ere Maijala 2019-01-11 13:00:38 UTC
Created attachment 83787 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Comment 82 Ere Maijala 2019-01-11 13:00:44 UTC
Created attachment 83788 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Comment 83 Ere Maijala 2019-01-11 13:00:50 UTC
Created attachment 83789 [details] [review]
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Comment 84 Ere Maijala 2019-01-11 13:02:32 UTC
Rebased and added support for managing the Z39.50 responder in the Debian init scripts. Not included in koha-create by default, but I reworded the current options in it to indicate that they apply to Zebra's Z39.50/SRU server only.
Comment 85 Joonas Kylmälä 2019-01-11 13:08:57 UTC
(In reply to Ere Maijala from comment #84)
> Rebased and added support for managing the Z39.50 responder in the Debian
> init scripts.

What did you rebase? The previous patches applied for me just fine with the current master.
Comment 86 Ere Maijala 2019-01-11 13:26:39 UTC
(In reply to Joonas Kylmälä from comment #85)
> (In reply to Ere Maijala from comment #84)
> > Rebased and added support for managing the Z39.50 responder in the Debian
> > init scripts.
> 
> What did you rebase? The previous patches applied for me just fine with the
> current master.

Nothing, actually. Sorry about that. Just verified that the patches still apply cleanly.
Comment 87 Ere Maijala 2019-01-14 15:33:13 UTC
Handling of quotes and parenthesis still needs some work.
Comment 88 Ere Maijala 2019-01-15 11:55:59 UTC
Created attachment 83951 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 89 Ere Maijala 2019-01-15 11:56:04 UTC
Created attachment 83952 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 90 Ere Maijala 2019-01-15 11:56:09 UTC
Created attachment 83953 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 91 Ere Maijala 2019-01-15 11:56:14 UTC
Created attachment 83954 [details] [review]
Bug 13937: Add unit tests
Comment 92 Ere Maijala 2019-01-15 11:56:19 UTC
Created attachment 83955 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 93 Ere Maijala 2019-01-15 11:56:24 UTC
Created attachment 83956 [details] [review]
Bug 13937: Fix issues found in QA
Comment 94 Ere Maijala 2019-01-15 11:56:29 UTC
Created attachment 83957 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Comment 95 Ere Maijala 2019-01-15 11:56:36 UTC
Created attachment 83958 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Comment 96 Ere Maijala 2019-01-15 11:56:41 UTC
Created attachment 83959 [details] [review]
Bug 13937: Fix RPN conversion

Fixes handling of quotes and parenthesis.
Comment 97 Ere Maijala 2019-01-16 12:05:32 UTC
Created attachment 84050 [details] [review]
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Comment 98 Ere Maijala 2019-01-16 12:08:10 UTC
Created attachment 84051 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 99 Ere Maijala 2019-01-16 12:08:32 UTC
Created attachment 84052 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 100 Ere Maijala 2019-01-16 12:08:56 UTC
Created attachment 84053 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 101 Ere Maijala 2019-01-16 12:09:19 UTC
Created attachment 84054 [details] [review]
Bug 13937: Add unit tests
Comment 102 Ere Maijala 2019-01-16 12:09:50 UTC
Created attachment 84055 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 103 Ere Maijala 2019-01-16 12:10:31 UTC
Created attachment 84056 [details] [review]
Bug 13937: Fix issues found in QA
Comment 104 Ere Maijala 2019-01-16 12:11:03 UTC
Created attachment 84057 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Comment 105 Ere Maijala 2019-01-16 12:11:39 UTC
Created attachment 84058 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Comment 106 Ere Maijala 2019-01-16 12:12:57 UTC
Created attachment 84059 [details] [review]
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Comment 107 Ere Maijala 2019-01-16 13:03:24 UTC
Comment on attachment 83959 [details] [review]
Bug 13937: Fix RPN conversion

I made a mess somehow, need to clean up the patches.
Comment 108 Ere Maijala 2019-01-16 13:21:40 UTC
Created attachment 84062 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 109 Ere Maijala 2019-01-16 13:22:00 UTC
Created attachment 84063 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 110 Ere Maijala 2019-01-16 13:22:28 UTC
Created attachment 84064 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 111 Ere Maijala 2019-01-16 13:22:50 UTC
Created attachment 84065 [details] [review]
Bug 13937: Add unit tests
Comment 112 Ere Maijala 2019-01-16 13:23:19 UTC
Created attachment 84066 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 113 Ere Maijala 2019-01-16 13:24:07 UTC
Created attachment 84067 [details] [review]
Bug 13937: Fix issues found in QA
Comment 114 Ere Maijala 2019-01-16 13:24:41 UTC
Created attachment 84068 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Comment 115 Ere Maijala 2019-01-16 13:25:09 UTC
Created attachment 84069 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Comment 116 Ere Maijala 2019-01-16 13:25:50 UTC
Created attachment 84072 [details] [review]
Bug 13937: Fix RPN conversion

Fixes handling of parenthesis and removes unused quote handling.
Comment 117 Ere Maijala 2019-01-16 13:26:21 UTC
Created attachment 84073 [details] [review]
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Comment 118 Katrin Fischer 2019-01-25 10:31:45 UTC
This seems to have moved beyond it's initial purpose with the Elasticsearch additions. Would it make sense to split it up or at least change the title? It might gain more interest that way too.
Comment 119 Olli-Antti Kivilahti 2019-02-14 13:17:50 UTC
Created attachment 85112 [details] [review]
11 - Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields. Fix log path.
Comment 120 Josef Moravec 2019-03-05 12:11:08 UTC
Just started to testing, but file Koha/Z3950Responder/Session.pm calls C4::Items::GetItem which was removed from master...

Could you add a follow-up for that?
Comment 121 Ere Maijala 2019-03-08 10:10:44 UTC
Created attachment 86353 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Comment 122 Ere Maijala 2019-03-08 10:10:50 UTC
Created attachment 86354 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Comment 123 Ere Maijala 2019-03-08 10:10:55 UTC
Created attachment 86355 [details] [review]
Bug 13937: (follow-up) Correct error call to use self
Comment 124 Ere Maijala 2019-03-08 10:11:01 UTC
Created attachment 86356 [details] [review]
Bug 13937: Add unit tests
Comment 125 Ere Maijala 2019-03-08 10:11:07 UTC
Created attachment 86357 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case
Comment 126 Ere Maijala 2019-03-08 10:11:12 UTC
Created attachment 86358 [details] [review]
Bug 13937: Fix issues found in QA
Comment 127 Ere Maijala 2019-03-08 10:11:18 UTC
Created attachment 86359 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Comment 128 Ere Maijala 2019-03-08 10:11:24 UTC
Created attachment 86360 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Comment 129 Ere Maijala 2019-03-08 10:11:30 UTC
Created attachment 86361 [details] [review]
Bug 13937: Fix RPN conversion

Fixes handling of parenthesis and removes unused quote handling.
Comment 130 Ere Maijala 2019-03-08 10:11:35 UTC
Created attachment 86362 [details] [review]
Bug 13937: Add support for Z39.50 responder to the Debian scripts
Comment 131 Ere Maijala 2019-03-08 10:11:41 UTC
Created attachment 86363 [details] [review]
Bug 13937: Fix the daemon log path.

directory _LOG_DIR_/logs does't exists.

All other log4perl -logs are out to _LOG_DIR_

Thank you for the good work Ere!
Comment 132 Ere Maijala 2019-03-08 10:11:47 UTC
Created attachment 86364 [details] [review]
Bug 13937: Switch from GetItem() to Koha::Items->find()
Comment 133 Ere Maijala 2019-03-08 10:12:34 UTC
Fixed the GetItem issue and a couple of pod errors.
Comment 134 Stefan Berndtsson 2019-03-11 10:21:27 UTC
This patch does not apply for me, with the following error:

Applying: Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields. Fix log path.
error: sha1 information is lacking or useless (etc/log4perl.conf).
error: could not build fake ancestor
Patch failed at 0001 Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields. Fix log path.
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/11---Bug-13937-Add-a-Z3950-daemon-that-can-inject--6jqZ05.patch



Why are both the first and the second to last patch doing the same change?
Comment 135 Ere Maijala 2019-03-11 10:52:09 UTC
Oops, the first patch was accidentally left over from previous iteration. Should be fixed now.
Comment 136 Stefan Berndtsson 2019-03-12 09:01:13 UTC
(In reply to Ere Maijala from comment #135)
> Oops, the first patch was accidentally left over from previous iteration.
> Should be fixed now.

Figured is was something like that. It applies fine now.

A couple of notes. The test plan says port 9999, but the config file has changed that to 2100.

Also, the test plan doesn't mention anything about the location of the config, nor the location of the pqf.properties (referred to relatively in the config.xml).

It wasn't hard to figure out what to do about it, so no worries there, just thought I'd mention it.
Comment 137 Katrin Fischer 2019-04-03 20:58:36 UTC
(In reply to Stefan Berndtsson from comment #136)
> (In reply to Ere Maijala from comment #135)
> > Oops, the first patch was accidentally left over from previous iteration.
> > Should be fixed now.
> 
> Figured is was something like that. It applies fine now.
> 
> A couple of notes. The test plan says port 9999, but the config file has
> changed that to 2100.
> 
> Also, the test plan doesn't mention anything about the location of the
> config, nor the location of the pqf.properties (referred to relatively in
> the config.xml).
> 
> It wasn't hard to figure out what to do about it, so no worries there, just
> thought I'd mention it.

Hi Stefan, any update on this one? I'd love to see it in 19.05
Comment 138 Michal Denar 2019-04-22 19:39:26 UTC
Ere,
I changed status. We're waiting for follow-up from Ere.
Comment 139 Ere Maijala 2019-04-23 08:37:25 UTC
It's applying fine for me. Where did it fail for you?
Comment 140 Ere Maijala 2019-04-25 12:02:05 UTC
I tested again, and applied cleanly on top of current master.
Comment 141 Stefan Berndtsson 2019-04-26 12:29:24 UTC
Created attachment 88907 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 142 Stefan Berndtsson 2019-04-26 12:29:34 UTC
Created attachment 88908 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 143 Stefan Berndtsson 2019-04-26 12:29:42 UTC
Created attachment 88909 [details] [review]
Bug 13937: (follow-up) Correct error call to use self

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 144 Stefan Berndtsson 2019-04-26 12:29:52 UTC
Created attachment 88910 [details] [review]
Bug 13937: Add unit tests

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 145 Stefan Berndtsson 2019-04-26 12:29:59 UTC
Created attachment 88911 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 146 Stefan Berndtsson 2019-04-26 12:30:08 UTC
Created attachment 88912 [details] [review]
Bug 13937: Fix issues found in QA

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 147 Stefan Berndtsson 2019-04-26 12:30:15 UTC
Created attachment 88913 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 148 Stefan Berndtsson 2019-04-26 12:30:22 UTC
Created attachment 88914 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 149 Stefan Berndtsson 2019-04-26 12:30:29 UTC
Created attachment 88915 [details] [review]
Bug 13937: Fix RPN conversion

Fixes handling of parenthesis and removes unused quote handling.

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 150 Stefan Berndtsson 2019-04-26 12:30:36 UTC
Created attachment 88916 [details] [review]
Bug 13937: Add support for Z39.50 responder to the Debian scripts

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 151 Stefan Berndtsson 2019-04-26 12:30:43 UTC
Created attachment 88917 [details] [review]
Bug 13937: Fix the daemon log path.

directory _LOG_DIR_/logs does't exists.

All other log4perl -logs are out to _LOG_DIR_

Thank you for the good work Ere!

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 152 Stefan Berndtsson 2019-04-26 12:34:14 UTC
Created attachment 88918 [details] [review]
Bug 13937: Switch from GetItem() to Koha::Items->find()

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 153 Katrin Fischer 2019-04-26 12:53:28 UTC
Woohoooo! Thx, Stefan! Next time icecream is on me :)
Comment 154 Fridolin Somers 2019-04-29 07:39:49 UTC
I see there is pubdate in etc/z3950/attribute_mappings.yaml but since Bug 19575 it is date-of-publication, isn't it ?
Maybe check all changes.
Comment 155 Ere Maijala 2019-04-29 07:43:28 UTC
Actually, it's a bit borked. Bug 19575 missed the change to default mappings file. That will be fixed in bug 22524. I'll check the mappings to match what should be correct.
Comment 156 Alex Arnaud 2019-05-10 14:30:41 UTC
It seems that something breaks the misc/z3950_responder.pl script after the first patch

 - apply only first patch (Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields),
 - run misc/z3950_responder.pl,
 - sudo netstat -tulpn | grep LISTEN
=> tcp6       0      0 :::9999                 :::*                    LISTEN      8918/perl

 - apply all patches,
 - run misc/z3950_responder.pl,
 - sudo netstat -tulpn | grep LISTEN
No listening on 9999
Comment 157 Alex Arnaud 2019-05-10 14:53:12 UTC
Also, a test plan for SRU would be great
Comment 158 Nick Clemens 2019-05-17 12:14:29 UTC
Excited about this, but too late for 19.05
Comment 159 Alex Arnaud 2019-05-17 14:17:56 UTC
(In reply to Nick Clemens from comment #158)
> Excited about this, but too late for 19.05

Nick,

Can you reproduce the issue on comment #156 ?
Comment 160 Ere Maijala 2019-07-23 07:41:36 UTC
Alex, please see comments #43 - #45. The listening port was changed due to conflict with Zebra. I'll provide a test plan for SRU.
Comment 161 Ere Maijala 2019-07-23 12:29:21 UTC
Created attachment 91728 [details] [review]
Bug 13937: Add a Z39.50 daemon that can inject item status MARC subfields

This creates a new daemon, misc/z3950_responder.pl, which can respond to
Z39.50 requests. By default, it just proxies searches to Zebra.

If desired, however, it can also add a subfield to the item tags on
outgoing records with a textual description of the item's status
(checked out, lost, etc.). This is useful for certain ILL systems. These
strings can be translated using the 'Z3950_STATUS' authorized value.

Test plan:
  1) Start the Z39.50 server using `perl misc/z3950_responder.pl`.
  2) Connect to the server using `yaz-client 127.0.0.1:9999/biblios`.
  3) Run a search, such as `find @attr 1=1016 book`.
  4) Fetch the results both one at a time with `show 1` and in a batch
     using `show 1+5`.
  5) Turn on MARCXML using `format xml` and `elements marcxml`, and
     verify that the records are still correctly fetched.
  6) Enable the item status subfield by restarting the server with the
     option `--add-item-status=k`.
  7) Search for and fetch records, and verify that a $k subfield is
     added to the item tags as appropriate. It should show some
     combination of "Checked Out", "Lost", "Not For Loan", "Damaged",
     "Withdrawn", "In Transit", or "On Hold" as appropriate, or
     "Available".
  8) Add an authorized value named "Z3950_STATUS" with any of the keys
     "AVAILABLE", "CHECKED_OUT", "LOST", "NOT_FOR_LOAN", "DAMAGED",
     "WITHDRAWN", "IN_TRANSIT" or "ON_HOLD", and verify that their
     descriptions are used instead of the default values above.

Signed-off-by: George Williams <george@nekls.org>
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 162 Ere Maijala 2019-07-23 12:29:28 UTC
Created attachment 91729 [details] [review]
Bug 13937: (follow-up) Remove reference to sub _prefetch_records

Remove reference to sub _prefetch_records

It appears this sub is built into fetch and this call is a duplicate

Signed-off-by: George Williams <george@nekls.org>
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 163 Ere Maijala 2019-07-23 12:29:35 UTC
Created attachment 91730 [details] [review]
Bug 13937: (follow-up) Correct error call to use self

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 164 Ere Maijala 2019-07-23 12:29:45 UTC
Created attachment 91731 [details] [review]
Bug 13937: Add unit tests

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 165 Ere Maijala 2019-07-23 12:29:53 UTC
Created attachment 91732 [details] [review]
Bug 13937: (follow-up) Pass through yaz switches and don't ignore case

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 166 Ere Maijala 2019-07-23 12:30:00 UTC
Created attachment 91733 [details] [review]
Bug 13937: Fix issues found in QA

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 167 Ere Maijala 2019-07-23 12:30:08 UTC
Created attachment 91734 [details] [review]
Bug 13937: Add support for SRU and Elasticsearch

Splits Session to GenericSession and ZebraSession where Generic supports any
search backend via the SearchEngine classes and Zebra maintains the direct
channel to the Zebra server.

Adds config files required for mapping BIB-1 attributes to Koha search fields
and SRU indexes to BIB-1 attributes.

Adds PODs.

Sponsored-by: National Library of Finland
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 168 Ere Maijala 2019-07-23 12:30:14 UTC
Created attachment 91735 [details] [review]
Bug 13937: Add tests for search and retrieval

Sponsored-by: National Library of Finland
Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 169 Ere Maijala 2019-07-23 12:30:20 UTC
Created attachment 91736 [details] [review]
Bug 13937: Fix RPN conversion

Fixes handling of parenthesis and removes unused quote handling.

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 170 Ere Maijala 2019-07-23 12:30:31 UTC
Created attachment 91737 [details] [review]
Bug 13937: Add support for Z39.50 responder to the Debian scripts

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 171 Ere Maijala 2019-07-23 12:30:40 UTC
Created attachment 91738 [details] [review]
Bug 13937: Fix the daemon log path.

directory _LOG_DIR_/logs does't exists.

All other log4perl -logs are out to _LOG_DIR_

Thank you for the good work Ere!

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 172 Ere Maijala 2019-07-23 12:30:46 UTC
Created attachment 91739 [details] [review]
Bug 13937: Switch from GetItem() to Koha::Items->find()

Signed-off-by: Stefan Berndtsson <stefan.berndtsson@ub.gu.se>
Comment 173 Ere Maijala 2019-07-23 12:30:53 UTC
Created attachment 91740 [details] [review]
Bug 13937: Add SRU tests
Comment 174 Ere Maijala 2019-07-23 12:31:31 UTC
Test plan for SRU:

1. Make sure tests in t/db_dependent/Koha/Z3950Responder/GenericSession.t pass.
2. Use Elasticsearch as the search engine
3. Start the server: perl misc/z3950_responder.pl --debug
4. Check that you get an explain response with browser from http://localhost:2100/
5. Try some searches to verify that you get results and good search queries in the output of the daemon . Examples:

http://localhost:2100/biblios?operation=searchRetrieve&maximumRecords=10&recordSchema=marcxml&query=dc.author%3dsmith
http://localhost:2100/biblios?operation=searchRetrieve&maximumRecords=10&recordSchema=marcxml&query=dc.title%3dnew AND dc.author%3dsmith
http://localhost:2100/biblios?operation=searchRetrieve&maximumRecords=10&recordSchema=marcxml&query=dc.title+cql.eq+"new world"+AND+dc.creator=smith
Comment 175 Katrin Fischer 2019-10-03 14:34:35 UTC
Patches still apply, what do we need to be able to continue here?
Comment 176 Ere Maijala 2019-10-04 07:23:37 UTC
QA, please :)
Comment 177 Katrin Fischer 2019-10-04 07:43:36 UTC
I think we are a little bit scared here about it breaking Zebra - it does a bit too many things in one patch right now, but I hope we'll find a brave soul.
Comment 178 Ere Maijala 2019-10-04 07:48:23 UTC
It's all pretty much standalone and shouldn't affect normal Zebra services in any way. Is there something that's particularly scary?
Comment 179 Martin Renvoize 2019-10-04 08:35:31 UTC
Right.. I've given this some thought, mulled it over with Nick and others at Hackfest and proved to myself that it's doesn't seem to break anything.

I'm going to PQA and push with the expectation that if we find issues down the line I can rely upon our growing team of ES competent people to dive in a fix things quickly. 

Thankyou everyone for your efforts on this one over the long years it's been developed.
Comment 180 Ere Maijala 2019-10-04 08:37:18 UTC
Awesome, thanks!
Comment 181 Marcel de Rooy 2019-10-04 08:42:30 UTC
(In reply to Martin Renvoize from comment #179)
> Right.. I've given this some thought, mulled it over with Nick and others at
> Hackfest and proved to myself that it's doesn't seem to break anything.
> 
> I'm going to PQA and push with the expectation that if we find issues down
> the line I can rely upon our growing team of ES competent people to dive in
> a fix things quickly. 
> 
> Thankyou everyone for your efforts on this one over the long years it's been
> developed.

A patch set of this size surely needs a sufficient amount of QA time. Hope you are not too fast?
Comment 182 David Cook 2019-10-08 00:09:23 UTC
Comment on attachment 91736 [details] [review]
Bug 13937: Fix RPN conversion

Review of attachment 91736 [details] [review]:
-----------------------------------------------------------------

Can this be explained a bit more? Why is this necessary?
Comment 183 David Cook 2019-10-08 00:13:49 UTC
(In reply to Ere Maijala from comment #178)
> It's all pretty much standalone and shouldn't affect normal Zebra services
> in any way. Is there something that's particularly scary?

At a glance, it does look pretty standalone. However, could a more normative example/documentation be provided?

I'm guessing that most people will never use this daemon so will never have to worry about it either? 

The number of patches makes it a bit challenging to see what's going on at a glance, but it looks fairly light. I don't understand some of what's going on here, but if I'm never going to use this feature, I'm probably not going to worry about looking at it.
Comment 184 Ere Maijala 2019-10-08 06:30:20 UTC
(In reply to David Cook from comment #182)
> Comment on attachment 91736 [details] [review] [review]
> Bug 13937: Fix RPN conversion
> 
> Review of attachment 91736 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Can this be explained a bit more? Why is this necessary?

That's because the escaped characters have a special meaning at least in Elasticsearch, and unless we escape these characters in ES query builder, they need to be accounted for here. Otherwise ES would balk e.g. trying to parse [something] as a range.

I'd be happy to provide some documentation on a wiki page, if that'd be useful. In the long run this may be used by quite many since it provides Z39.50 and SRU on top of Elasticsearch, which would otherwise require running Zebra.
Comment 185 Martin Renvoize 2019-10-08 13:38:29 UTC
(In reply to Marcel de Rooy from comment #181)
> A patch set of this size surely needs a sufficient amount of QA time. Hope
> you are not too fast?

You mean a few years isn't sufficient?  I've spent hours QAing this on a couple of occasions and spend a fair chunk of time discussing it with Nick and other ES experts at the Hackfest.. it has my full backing at this point and I also have assurances from ES users that we will get quick followups should there be issues.

(In reply to David Cook from comment #183)
>At a glance, it does look pretty standalone. However, could a more normative example/documentation be provided?

Agreed, I have requested such documentation and should Ere not get a chance to get to it quickly I will be asking Bywater for some assistance too.

So.. In short.. lets get this baby pushed :)
Comment 186 Martin Renvoize 2019-10-08 13:40:39 UTC
Nice work!

Pushed to master for 19.11.00
Comment 187 Marcel de Rooy 2019-10-08 13:42:36 UTC
(In reply to Martin Renvoize from comment #185)
> You mean a few years isn't sufficient?  I've spent hours QAing this on a
> couple of occasions and spend a fair chunk of time discussing it with Nick
> and other ES experts at the Hackfest.. it has my full backing at this point
> and I also have assurances from ES users that we will get quick followups
> should there be issues.

Ok ok
Comment 188 David Cook 2019-10-09 00:18:15 UTC
(In reply to Ere Maijala from comment #184)
> That's because the escaped characters have a special meaning at least in
> Elasticsearch, and unless we escape these characters in ES query builder,
> they need to be accounted for here. Otherwise ES would balk e.g. trying to
> parse [something] as a range.
> 

Ahhhh that's good to know. I thought it was passing the data straight to Zebra, so I was confused why we'd be mangling the data. What you say makes more sense.

> I'd be happy to provide some documentation on a wiki page, if that'd be
> useful. In the long run this may be used by quite many since it provides
> Z39.50 and SRU on top of Elasticsearch, which would otherwise require
> running Zebra.

Yes please :). 

I didn't see any references to Elasticsearch in the patches I viewed. I thought it required Zebra to be used. I must've missed the "Add support for SRU and Elasticsearch" patch now that I look again... hehe. It would be great to use this instead of Zebra in that context! I was groaning at the idea of storing MARCXML in MySQL and indexing it into *both* Elasticsearch and Zebra just to get Z39.50 functionality...

Nice one, Ere!
Comment 189 Martin Renvoize 2019-10-09 08:11:12 UTC
Hmm, this still needs a little more work to get tests passing inside koha-testing-docker.
Comment 190 Ere Maijala 2019-10-09 08:33:51 UTC
I'll try to set up koha-testing-docker to check it out.
Comment 191 Martin Renvoize 2019-10-09 08:36:30 UTC
It's something to do with the hard coded z connection port in the tests.. but there's also an obvious lack of /etc/koha/sites/kohadev/z3950 so the configs are missing and nothing starts the new daemon..

In short.. it's a few things together that cause the failures.. I don't think it's anything major to worry about with the actual bug itself.. just a matter of making Jenkins happy again.
Comment 192 Ere Maijala 2019-10-09 10:02:52 UTC
I'm having trouble even getting the docker to start, so if someone else knows what to do, I'm happy for any assistance. That said, the tests should fork their own server with the configs provided with the tests, so not having the generic config in place should not cause problems.
Comment 193 Ere Maijala 2019-10-09 10:45:14 UTC
Got the docker stuff running. It seems that there are at least some differences between YAZ versions. I've tested with YAZ 5, but the testing box has YAZ 4. Looks like v4 can't find the pqf.properties file with a relative path in t/db_dependent/Koha/Z3950Responder/config.xml. Upgrading YAZ helps a bit, but there are still failing tests I need to check. Would it be possible to upgrade the container to use YAZ 5?
Comment 194 Ere Maijala 2019-10-09 11:16:20 UTC
Created attachment 93913 [details] [review]
Bug 13937: (QA follow-up) Fix tests.

Checking result set size is enough to verify that proper results were returned. The check for non-existing record could have caused a range error depending of library versions.
Comment 195 Ere Maijala 2019-10-09 11:19:18 UTC
Comment on attachment 93913 [details] [review]
Bug 13937: (QA follow-up) Fix tests.

This patch would fix tests with YAZ 5. I tried to copy pqf.properties to the same directory, but YAZ 4 has other subtle differences making it difficult to make the tests work properly.
Comment 196 Martin Renvoize 2019-10-09 12:08:42 UTC
Thanks for taking the time to look Ere.

Hmm, so is it just the tests that require Yaz5, or does this patch inadvertently mean we need a version bump for Yaz5 in Koha.
Comment 197 Ere Maijala 2019-10-09 12:12:10 UTC
As far as I can tell YAZ 4 should work in normal use.
Comment 198 David Cook 2019-10-09 23:17:37 UTC
(In reply to Ere Maijala from comment #193)
> Got the docker stuff running. It seems that there are at least some
> differences between YAZ versions. I've tested with YAZ 5, but the testing
> box has YAZ 4. Looks like v4 can't find the pqf.properties file with a
> relative path in t/db_dependent/Koha/Z3950Responder/config.xml. Upgrading
> YAZ helps a bit, but there are still failing tests I need to check. Would it
> be possible to upgrade the container to use YAZ 5?

Do you have to use a relative path? Could the solution be to use an absolute path?

(I'm asking without looking at the patches at all. Just an idea.)
Comment 199 Ere Maijala 2019-10-14 06:23:09 UTC
The path is in a config file, so hard-coding it there there doesn't seem quite proper. It doesn't seem possible to specify it programmatically. Anyway, it wouldn't help since YAZ 4 and 5 handle some error conditions differently, unless I remove the offending tests. That's of course possible too, but I would rather not do that unless it's the last option.

Now, this strays from the main topic a bit, but as far as I can tell last YAZ 4 release was in 2013. That's pretty ancient. Could we decide not to fix the tests for such an old version?
Comment 200 Tomás Cohen Arazi 2019-10-14 12:36:35 UTC
Created attachment 94097 [details] [review]
Bug 13937: (follow-up) Add missing entry to koha-common.install

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 201 Martin Renvoize 2019-10-14 12:46:36 UTC
Nice work!

Pushed to master for 19.11.00
Comment 202 David Cook 2019-10-15 00:49:01 UTC
(In reply to Ere Maijala from comment #199)
> The path is in a config file, so hard-coding it there there doesn't seem
> quite proper. It doesn't seem possible to specify it programmatically.
> Anyway, it wouldn't help since YAZ 4 and 5 handle some error conditions
> differently, unless I remove the offending tests. That's of course possible
> too, but I would rather not do that unless it's the last option.
> 

I think I don't know enough about the particular test in question to really continue the conversation. I figured it was a case of providing a configuration value to a function in the test, but it sounds like perhaps it's a Zebra config file and that it's an integration test, so I see how that could be tricky. I withdraw my suggestion heh.

> Now, this strays from the main topic a bit, but as far as I can tell last
> YAZ 4 release was in 2013. That's pretty ancient. Could we decide not to fix
> the tests for such an old version?

That's an interesting question. Looking at https://packages.debian.org/search?keywords=yaz, I see YAZ 4 is used in Jessie and Stretch, but Buster and Bullseye use YAZ 5. 

Perhaps we could specify a YAZ 4 run time dependency and YAZ 5 as a build dependency in the Debian control file? 

But then we probably should be building on the same OSes that we're supporting at run time, so I think perhaps we do have to support YAZ 4 behaviour until we don't support Debian Stretch anymore?
Comment 203 Ere Maijala 2019-10-15 04:52:51 UTC
(In reply to David Cook from comment #202)

> I think I don't know enough about the particular test in question to really
> continue the conversation. I figured it was a case of providing a
> configuration value to a function in the test, but it sounds like perhaps
> it's a Zebra config file and that it's an integration test, so I see how
> that could be tricky. I withdraw my suggestion heh.

I value the suggestion, though it wouldn't solve the problem that YAZ 4 and 5 do diagnostics (error handling) differently. I'll see if I can modify the tests to pass on both.

> That's an interesting question. Looking at
> https://packages.debian.org/search?keywords=yaz, I see YAZ 4 is used in
> Jessie and Stretch, but Buster and Bullseye use YAZ 5. 

I see. I'll take another stab at trying to make the tests work with both versions.
Comment 204 David Cook 2019-10-15 05:58:51 UTC
(In reply to Ere Maijala from comment #203)
> I see. I'll take another stab at trying to make the tests work with both
> versions.

I can try to help. I'm pretty busy at the moment, so not too keen to spin up a koha-testing-docker and try it out, but if you point me at the test, I can have a look. 

Martin's already pushed the patches to master, so maybe it doesn't matter too much? Maybe he can clarify things.
Comment 205 Ere Maijala 2019-10-15 06:40:04 UTC
Created attachment 94109 [details] [review]
Bug 13937: (QA follow-up) Make tests work with YAZ 4 and 5.

Also makes add_item_status tests more robust by clearing the cache after modifying authorised values.
Comment 206 Ere Maijala 2019-10-15 06:42:59 UTC
The last patch should work with both YAZ versions. The real culprit was that the test requests did not include version parameter, and YAZ 5 defaulted to a newer SRU version that uses a different namespace. All tests now use version=1.1 parameter. Additionally, pqf.properties is copied to the test directory so that a relative path isn't needed in config.xml.
Comment 207 David Cook 2019-10-15 06:47:11 UTC
Comment on attachment 94109 [details] [review]
Bug 13937: (QA follow-up) Make tests work with YAZ 4 and 5.

Review of attachment 94109 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Koha/Z3950Responder/GenericSession.t
@@ +82,5 @@
>      $child = fork();
>      if ($child == 0) {
>          my $config_dir = dirname(__FILE__) . '/';
> +        # Copy pqf.properties from etc/z3950
> +        copy($config_dir . '../../../../etc/z3950/pqf.properties', $config_dir . 'pqf.properties') || die "Copying pqf.properties failed: $!";

This seems a little fragile to me. I don't know what we're doing in other scripts, but it might be worth using https://perldoc.perl.org/FindBin.html here?
Comment 208 David Cook 2019-10-15 06:47:28 UTC
(In reply to Ere Maijala from comment #206)
> The last patch should work with both YAZ versions. The real culprit was that
> the test requests did not include version parameter, and YAZ 5 defaulted to
> a newer SRU version that uses a different namespace. All tests now use
> version=1.1 parameter. Additionally, pqf.properties is copied to the test
> directory so that a relative path isn't needed in config.xml.

Nice one, Ere!
Comment 209 Ere Maijala 2019-10-15 06:52:01 UTC
(In reply to David Cook from comment #207)
> Comment on attachment 94109 [details] [review] [review]
> Bug 13937: (QA follow-up) Make tests work with YAZ 4 and 5.
> 
> Review of attachment 94109 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: t/db_dependent/Koha/Z3950Responder/GenericSession.t
> @@ +82,5 @@
> >      $child = fork();
> >      if ($child == 0) {
> >          my $config_dir = dirname(__FILE__) . '/';
> > +        # Copy pqf.properties from etc/z3950
> > +        copy($config_dir . '../../../../etc/z3950/pqf.properties', $config_dir . 'pqf.properties') || die "Copying pqf.properties failed: $!";
> 
> This seems a little fragile to me. I don't know what we're doing in other
> scripts, but it might be worth using https://perldoc.perl.org/FindBin.html
> here?

I'll revise. Actually, would it better to just include a copy of pqf.properties in the test dir? It would also make sure that any changes to the default pqf.properties don't break tests.
Comment 210 Ere Maijala 2019-10-15 07:16:49 UTC
Created attachment 94110 [details] [review]
Bug 13937: (QA follow-up) Make tests work with YAZ 4 and 5.

Also makes add_item_status tests more robust by clearing the cache after modifying authorised values.
Comment 211 Ere Maijala 2019-10-15 07:18:07 UTC
Ok, the latest patch includes a copy of pqf.properties so that no copying needs to be performed. This makes things more simple and avoids possible permission issues with copying.
Comment 212 Martin Renvoize 2019-10-15 08:53:13 UTC
Thanks so much for all your work here Ere,

It's looking great to me now, I've just pushed the final followup and am waiting on Jenkins to go green.. crossed fingers but it all passes locally for me now in the various versions so touch wood Jenkins will be happy too.

It's great to have this one in!

Martin
Comment 213 Ere Maijala 2019-10-15 10:14:39 UTC
Thanks, Martin!
Comment 214 Martin Renvoize 2019-10-15 12:48:56 UTC
Created attachment 94209 [details] [review]
Bug 13937: (RM follow-up) Set working directory for daemon mode.

When running as a daemon under Net::Z3950::SimpleServer YAZ requires a
little help to know where it's "working directory" is located so
relative paths in config files may resolve correctly.

This patch passes the working directory as prescribed on
https://software.indexdata.com/yaz/doc/server.invocation.html such that
the resolution of the cql2rpn filepath works as expected in tests.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 215 Martin Renvoize 2019-10-15 12:53:34 UTC
One last followup.

We were still referencing a relative path for the pqf.properties file from inside the config.xml and this was not resolving correctly.

After a little more digging I spotted that 'cql2rpn' should always be 'relative to the working directory', but under a daemon start the working directory was not being set correctly.  One can, however, pass a `-w` to the daemon to explicitly set working directory and as such make relative paths (from the config file directory) work as expected.

Ere, this gets the tests passing again in my koha-testing-docker instance and also in my local environment but it might be worth double-checking at your end whether things will continue to work in your live environments.. I have a hunch you're using full paths in your config files there (or perhaps YAZ5 just does a much better job of detecting working directory in the first place for you).

Pushed
Comment 216 Ere Maijala 2019-10-16 05:50:56 UTC
Thanks Martin! I found the YAZ documentation a bit scattered and didn't spot that bit of information. We're running YAZ 5, and it seems to do a better job. Or rather, it seems that YAZ 5 supports paths relative to the config file as well as current working dir.
Comment 217 Martin Renvoize 2019-10-16 19:01:23 UTC
No worries Ere, you've done a fantastic job getting us this far. As you say, the yaz and zebra docs are cryptic at best, it from me a fair while to find it and a few pointers to get the right search terms to finally spot it.

I'm very happy it all seems to be working now and am looking forward to seeing it running in production.  Hoping we can get it thoroughly documented before release.
Comment 218 Ere Maijala 2019-10-18 11:14:18 UTC
Not sure about thoroughness, but I've now added initial documentation on how to set this up: https://wiki.koha-community.org/wiki/Setting_up_the_Z39.50_and_SRU_Server. I also added notes and links to https://wiki.koha-community.org/wiki/APIs_and_protocols_supported_by_Koha.
Comment 219 Martin Renvoize 2019-10-23 10:55:18 UTC
Ere, you are a star!  Thanks :)
Comment 220 Ere Maijala 2019-10-23 13:32:23 UTC
Created attachment 94616 [details] [review]
Bug 13937: (follow-up) Fix searching with UTF-8 characters.
Comment 221 Ere Maijala 2019-10-23 13:33:45 UTC
Thanks! And sorry, one more issue was discovered: searching with UTF-8 characters like ä didn't work properly. The last patch fixes that.
Comment 222 Martin Renvoize 2019-10-23 14:02:16 UTC
Nice work!

Pushed to master for 19.11.00
Comment 223 Johanna Räisä 2019-10-24 07:19:04 UTC
Created attachment 94647 [details] [review]
Bug 13937: (follow-up) sort SRU results by id asc

SRU is giving results in random order. This patch fixes the order to be id asc.
Comment 224 Ere Maijala 2019-10-24 07:56:25 UTC
Thanks, Johanna, that's a good change. I'll file a follow-up issue about adding support for client-specified sort order.
Comment 225 Ere Maijala 2019-10-24 08:00:58 UTC
Actually, no. I'd say we shouldn't force sorting here. Relevance should be used by default. There are other bugs affecting the result with Elasticsearch. See bug 23875.
Comment 226 Johanna Räisä 2019-10-24 08:04:25 UTC
(In reply to Ere Maijala from comment #225)
> Actually, no. I'd say we shouldn't force sorting here. Relevance should be
> used by default. There are other bugs affecting the result with
> Elasticsearch. See bug 23875.

Is that fixing sorting component parts also? If there aren't any sorting then we are getting them in random order, not in the order they should be.
Comment 227 Ere Maijala 2019-10-24 08:06:22 UTC
Not really, unless you want them in reverse local-number order. But with bug 23887 you would be able to indicate the preferred order.
Comment 228 Ere Maijala 2019-10-24 11:14:58 UTC
Comment on attachment 94647 [details] [review]
Bug 13937: (follow-up) sort SRU results by id asc

See patch in bug 23887. This allows one to use e.g. local-number as the sort parameter, but it needs to be made sortable in search engine configuration.
Comment 229 Katrin Fischer 2019-10-24 17:26:15 UTC
(In reply to Ere Maijala from comment #228)
> Comment on attachment 94647 [details] [review] [review]
> Bug 13937: (follow-up) sort SRU results by id asc
> 
> See patch in bug 23887. This allows one to use e.g. local-number as the sort
> parameter, but it needs to be made sortable in search engine configuration.

With the patch now being pushed, maybe we should split these out into separate bugs? Just want to make sure nothing gets lost.
Comment 230 Martin Renvoize 2019-10-24 20:31:46 UTC
Agree with Katrin, we've exhausted this bug now, can followups go into their own bugs as I'm starting to loose track and wouldn't want to miss something.

Cheers
Comment 231 Ere Maijala 2019-10-25 06:05:15 UTC
Just to clarify: bug 23887 is a separate follow-up, and I obsoleted the patch attached here.