Display status of item locations by status priority.

[GrothTobias]

Currently, if the item status for multiple locations is displayed in search results (Item_Status > multiple_locations = 'group'), locations with unknown items are always displayed as 'unknown' regardless if there are available items or not.

With this PR the displayed location status is based on the status priority configured in AvailabilityStatus:getPriority().

'Uncertain' and 'unavailable' are treated the same, as it is in the current implementation. (Not 'available' and not 'unknown')

TODO

• Update changelog when merging (changes to return values of \VuFind\ILS\Logic::availabilityAsString(); significant rewrite of \VuFind\AjaxHandler\GetItemStatuses; simplification of check_item_statuses.js to do less HTML generation).

[demiankatz]

Thanks, @GrothTobias! Since @ThoWagen designed this system, I'd be interested to hear his thoughts on these changes. Also, do we need to adjust integration tests to help cover this case?

At the moment I'm very busy finishing up some database-related refactoring for release 10, but if either of you think there's a strong value in getting this change included in release 10.0, I'll try to give it a closer look as soon as time permits!

[ThoWagen]

I think the general approach in this PR is very good. Only the "uncertain" case needs some kind of change. See the comment below.

[GrothTobias]

I refactored it a bit to @ThoWagen's approach.

Based on the returned availability, all four statuses can be displayed now.
For now the 'uncertain' icon is the same as 'unknown', if you have ideas or preferences as to which icon to use, let me know.

While I'd like to somewhat separate the classes from the JavaScript, so people can easily change them, I don't really like how I did it here.

As for the integration tests, I'm unsure as to how to change them to reflect the changes.

@ThoWagen, I changed the return values in AvailabilityStatus:availabilityAsString() from 'true'|'false'|'unknown'|'uncertain' to 'available'|'unavailable'|'unknown'|'uncertain'. This makes it simpler to handle the response and I think it is more consistent. From what I see the return value was only really used in the section I changed. If I overlooked some usages please let me know so I can change it.

[demiankatz]

See below for one small suggestion.

I like the idea of changing 'true' and 'false' to more descriptive terms, but that probably counts as a breaking change that might require us to reschedule this PR from 10.1 to 11.0. Is that a problem for you? Or maybe @ThoWagen feels differently... or maybe there's a way to incorporate some backward compatibility via a deprecated option.

Regarding integration tests, if things are failing after you and @ThoWagen agree on the final shape of the code, I don't mind helping to get the tests to pass.

[ThoWagen]

Regarding the classes in JS, I think it is good that you are importing them in JS, even though I understand that this does not look like the cleanest way. But currently I can not think of a better way to do this.

And I agree with @demiankatz on the change from 'true'|'false'|'unknown'|'uncertain' to 'available'|'unavailable'|'unknown'|'uncertain'. This should be part of a major release or done in a backward compatible way.

[GrothTobias]

Sorry, had to do something else first.

I reverted the AvailabilityStatus:availabilityAsString() to false | true | uncertain | unavailable and adjusted the JS since I couldn't think of a good way to make it backward compatible.

The return values would have to be changed in another PR.

[demiankatz]

Thanks, @GrothTobias! I've asked @ThoWagen to re-review when he has a moment. If he has no concerns with this revision, I'll give it a closer look myself and then (if ready) merge it.

[ThoWagen]

Sorry, I just realized that a review from me is still being requested here.

Unfortunately, something is not working properly. The Mink HoldingTest is failing. I think I found the problem. See the comments below.

[GrothTobias]

Sorry for the late fix.
I don't know why I committed it like that, it couldn't work...

[demiankatz]

@GrothTobias, thanks for the fix, and sorry for my own slow response. I think @ThoWagen will look at this soon, but he was out of office last week, and I haven't had a chance to look at this yet in his absence. :-)

[ThoWagen]

Unfortunately there are still two Mink tests failing. But the reason is not some wrong logic but that the tests are not correct anymore. The case when "multiple_locations" is "group" and the availability is "uncertain" or "unknown" needs to be modified. So basically the case "uncertain" needs to be handled somewhere in the lines 139-163 of the HoldingTest.php and if availability is null in line 174 the type is "muted" rather than "warning".

[demiankatz]

Thanks for taking another look, @ThoWagen. @GrothTobias, please let me know if you need help fixing the tests and/or getting a standard test environment set up. I'm happy to help if you need me to, but I'll wait to hear from you before taking action in case you prefer to work on this yourself.

[GrothTobias]

I can't get the vagrant testing environment running. At first I could not import the marc records, that seemed to be a problem with symlinks in a shared folder (sadly, my host system is windows).

Now I have created a new folder and did a git checkout and composer install. The httpd-vufind.conf and environment variabls were updated with the new vufind location.
With this I can import the marc data but have a new problem when running ./phing.sh startup:
Error connecting to solr at URL http://localhost:8983/solr/biblio/update : Error connecting to solr server for ping http://localhost:8983/solr/biblio

[demiankatz]

@GrothTobias, I confess that I have never tried using the test suite in the Vagrant environment; I typically set up an Ubuntu VM in VirtualBox for testing, as this makes everything a bit more accessible. If that's too much trouble, though, I'm willing to investigate this further in my existing environment and see if I can easily fix the tests. :-)

[GrothTobias]

If it helps, with a wget on http://localhost:8983/solr/admin/cores?action=STATUS I this response:

{ "responseHeader":{ "status":0, "QTime":1 }, "initFailures":{ }, "status":{ "authority":{ "name":"authority", "instanceDir":"/vufind_fork/solr/vufind/authority", "dataDir":"data/", "config":"solrconfig.xml", "schema":"schema.xml", "isLoaded":"false" }, "biblio":{ "name":"biblio", "instanceDir":"/vufind_fork/solr/vufind/biblio", "dataDir":"/vufind_fork/solr/vufind/biblio/", "config":"solrconfig.xml", "schema":"schema.xml", "startTime":"2024-09-04T13:46:38.632Z", "uptime":7709334, "index":{ "numDocs":10, "maxDoc":10, "deletedDocs":0, "version":6, "segmentCount":1, "current":true, "hasDeletions":false, "directory":"org.apache.lucene.store.NRTCachingDirectory:NRTCachingDirectory(MMapDirectory@/vufind_fork/solr/vufind/biblio/index lockFactory=org.apache.lucene.store.NativeFSLockFactory@20f0e4ce; maxCacheMB=48.0 maxMergeSizeMB=4.0)", "segmentsFile":"segments_2", "segmentsFileSizeInBytes":220, "userData":{ "commitCommandVer":"1809066741041463296", "commitTimeMSec":"1725260487596" }, "lastModified":"2024-09-02T07:01:27.596Z", "sizeInBytes":56004, "size":"54.69 KB" } }, "reserves":{ "name":"reserves", "instanceDir":"/vufind_fork/solr/vufind/reserves", "dataDir":"data/", "config":"solrconfig.xml", "schema":"schema.xml", "isLoaded":"false" }, "website":{ "name":"website", "instanceDir":"/vufind_fork/solr/vufind/website", "dataDir":"data/", "config":"solrconfig.xml", "schema":"schema.xml", "isLoaded":"false" } } }

[demiankatz]

That looks like a healthy Solr response. I suspect the problem is just that the automated testing code wasn't designed to run inside Vagrant. There's probably a way to make it work, but it may require a significant amount of configuration work. Before going down that road, I'll see if I can just solve the test problems in my own environment when time permits. (I unfortunately ran out of time today, but I might have better luck tomorrow!).

[demiankatz]

@GrothTobias, I finally found time to run tests and found that there are two failing tests. Here's what the browser looks like during the first failure:

That doesn't look quite right! Any idea what might be wrong?

The test that was running had set "multiple_locations" to "group" in config.ini, and had turned on the Demo driver with this configuration:

[StaticHoldings]
testsample1 = "[{\"barcode\":\"12345678\",\"availability\":0,\"status\":\"Foo\",\"location\":\"Test Location\",\"locationhref\":false,\"reserve\":\"N\",\"callnumber\":\"Test Call Number\",\"duedate\":\"\",\"is_holdable\":true,\"addLink\":true,\"addStorageRetrievalRequestLink\":\"check\",\"addILLRequestLink\":\"check\",\"__electronic__\":false},{\"barcode\":\"12345678\",\"availability\":0,\"status\":\"Foo\",\"location\":\"main\",\"locationhref\":false,\"reserve\":\"N\",\"callnumber\":\"Test Call Number\",\"duedate\":\"\",\"is_holdable\":true,\"addLink\":true,\"addStorageRetrievalRequestLink\":\"check\",\"addILLRequestLink\":\"check\",\"__electronic__\":false},{\"barcode\":\"12345678\",\"availability\":2,\"status\":\"Check with Staff\",\"location\":\"main\",\"locationhref\":false,\"reserve\":\"N\",\"callnumber\":\"Test Call Number\",\"duedate\":\"\",\"is_holdable\":true,\"addLink\":true,\"addStorageRetrievalRequestLink\":\"check\",\"addILLRequestLink\":\"check\",\"__electronic__\":false}]"

If you have a test instance that doesn't have a record with an ID of testsample1, you should still be able to reproduce this scenario -- just change testsample1 in your Demo.ini to a different record ID.

Is that any help? Please let me know if you need me to do further investigation on my end, or if you're having trouble reproducing the scenario.

[GrothTobias]

@demiankatz I have refactored the code so it does not need to create strings and explode the callnumbers and results from GetItemStatuses::pickValue().

While I'm not quite satisfied due to serializing the callnumber arrays to use array_unique in GetItemStatuses::155, I can't think of a good way to do this besides going back to creating a callnumber string and exploding it when needed.

Do you have an idea to do this in a better way?

[demiankatz]

@GrothTobias, thanks for the progress here. I've given this another review and have a few more comments.

I can't think of a better way to deduplicate the lists; I don't expect that these should ever be so unreasonably huge that this operation will be too expensive.

[demiankatz]

Thanks, @GrothTobias -- see below for a couple of follow-up suggestions.

It also looks like you need to run vendor/bin/phing update-bootstrap5 to get the themes back in sync.

[demiankatz]

@GrothTobias, the extra files were showing here because this branch was behind the current dev-11.0 branch. I've merged the latest changes in, and now the appropriate diffs seem to be showing again.

I'm interested in whether @ThoWagen has further comments on this latest version. I'll give him a chance to comment and will take another look myself after that.

[ThoWagen]

Besides the two comments below this is looking good to me!

[ThoWagen]

Besides that small suggestion this looks good to me :)

[ThoWagen]

Looking good to me now!

[demiankatz]

@GrothTobias, thanks for all the progress here (and thanks for taking time to review, @ThoWagen).

I think we're nearly done, but I had a few minor comments/suggestions related to the templates, mostly related to being paranoid about escaping things. I realize that some of the missing escaping has been faithfully ported from the previous Javascript, but we might as well fix them regardless if they need fixing. (And if any data is pre-escaped and I'm being overly paranoid, feel free to ignore any irrelevant suggestions).

[demiankatz]

Thanks again, @GrothTobias -- this looks good to me and all tests are passing!