Bug 13937

Summary: Add a Z39.50 daemon that can inject item status MARC subfields
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens <nick>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: abl, amy, chris.kirby, chris, gmcharlt, gwilliams, jonathan.druart, josef.moravec, katrin.fischer, m.de.rooy, mirko, nicole, olli-antti.kivilahti, tomascohen
Version: masterKeywords: dependency
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
Whiteboard:
Change sponsored?: Sponsored Patch complexity: Medium patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
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

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?