Display status of item locations by status priority. #3758
Conversation
|
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! |
|
I refactored it a bit to @ThoWagen's approach. Based on the returned availability, all four statuses can be displayed now. 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 |
There was a problem hiding this comment.
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.
|
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. |
…djusted check_item_statuses.js.
|
Sorry, had to do something else first. I reverted the The return values would have to be changed in another PR. |
|
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. |
|
Sorry for the late fix. |
|
@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. :-) |
|
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". |
|
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. |
|
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 |
|
@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. :-) |
|
If it helps, with a wget on
|
|
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!). |
|
@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: 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. |
|
@demiankatz @GrothTobias I belive as I said here #3758 (comment) that the tests are failing because they are not correct anymore. The tests complain about the uncertain's status class "text-warning". It expects "text-succsess". But that is not correct. However, we can see another problem in Demian's screenshot. The icon is not displayed because it was not yet added to the list in Please let me know if I can help implementing some of the fixes. |
|
I have added the icons to @ThoWagen It would be great if you could change the tests. As I have said before, i can't get them running. As a side note, I would probably change the priorities to From my understanding |
"Uncertain" is meant for situations where the item is known to exist, but there's no information on whether the user can have it (could be something like user group restrictions, holdings missing specific item data or such). Unknown is the "more unavailable" one of the two. The current order is for back-compatibility, but isn't necessarily that important anymore if the class and constants are used everywhere. |
There was a problem hiding this comment.
Thanks, @GrothTobias and @ThoWagen -- I've adjusted the tests so they pass.
I also gave the code another review and had a couple of thoughts -- see below.
I also feel like this might be best moved to the 11.0 milestone since it does make some significant architectural changes. If you feel otherwise, I'm happy to discuss further, though! :-)
Finally, please let me know when you're satisfied with the order of precedence; I wasn't entirely sure from recent conversation whether that should or should not be adjusted further.
| + result.locationList[x].location | ||
| + '</span> '; | ||
| } | ||
| locationListHTML += '<span class="' + availabilityClasses[availability] + '">' |
There was a problem hiding this comment.
Should we have fallback behavior if availabilityClasses[availability] is undefined for some reason (either the AJAX handler returns an unexpected value, or the scripts setting the underlying data get broken)? It might be helpful to log something to the console, or at least avoid a Javascript error caused by an illegal array access.
There was a problem hiding this comment.
Or alternatively, would it make sense to just pass the class as part of the AJAX response, and move some of the logic currently added to results-scripts.phtml into GetItemStatuses.php instead?
There was a problem hiding this comment.
I think returning the class and icon in GetItemStatuses.php would be cleaner and is easier to customize. Also the whole availabilityClasses would be unnecessary (I wasn't really happy with how I added it).
We could also directly return the rendered HTML. We have customized the HTML a bit, so returning rendered HTML would be appreciated. :-)
@ThoWagen what do you think?
Regarding the priority: For now I would leave it as is. I think this is a somewhat individual desicion for the libraries, so maybe a little survey would be good to determine what should be used as default.
Also, if we move this to 11.0, I would change the true and false in Helper\Root\AvailabilityStatus:availabilityAsString() to more descriptive terms. Previously I reverted that change in case you wanted to include this PR in 10.1
There was a problem hiding this comment.
@GrothTobias I just remembered that we had this discussion already here #3585 (review) . So yes, I think moving the logic from JS to GetItemStatuses.php would make sense.
There was a problem hiding this comment.
I wasn't clear if server-side rendering of the location list is wanted, so the classes and icons for each location are now returned in GetItemStatuses.php. I added the icon names (not sure what to call it) to Helper\Root\AvailabilityStatus.php, similar to the classes.
With this the field availability is not needed for locations but I'm unsure if I should remove it - it could be useful if someone wants to customize this in the JS.
There was a problem hiding this comment.
I think at this point I've gone ahead and moved this to the 11.0 milestone, so you can feel free to make larger changes if you like. I don't mind doing more server-side rendering in the AJAX handler as long as it properly accounts for the theme system (e.g. use a template to render where appropriate). Less Javascript code constructing HTML is probably a good thing, for readability and customization. :-)
I'm going to hold off on further review/testing until you're done making the changes you wish to make, so please let me know when you are ready for me to take a closer look again!
There was a problem hiding this comment.
The GetItemStatuses handler now returns rendered HTML. As mentioned in the @TODO there is still something to do but I don't have time for it next week.
There was a problem hiding this comment.
Thanks for the progress; since this is now scheduled for release 11, there's no huge rush to finish -- just please let me know when you're ready for another review.
|
@demiankatz I have refactored the code so it does not need to create strings and explode the callnumbers and results from While I'm not quite satisfied due to serializing the callnumber arrays to use Do you have an idea to do this in a better way? |
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')