DAIA: make accepted service list configurable #2060
Conversation
…uration is in DAIA.ini, default loan and presentation (interloan can be added). Limitations are only regarded if the belong to an accepted service. There is as well a bit of code restructured in order to be able to include electronic holdings. Holdings without holding services are skipped.
|
There is a failure of the CI-tests I don't understand; can anybody give me a hint? |
|
Thanks, @hajoseng. I have requested a review from @olli-gold since I think he is better qualified to comment on this than I am... but if either of you need my input, just let me know and I'll take a closer look. I will certainly follow your conversation as it unfolds. Thanks also for fixing the CI failure. Sorry I didn't get a chance to look before you solved it. Was it just a php-cs-fixer failure, or something different? Let me know if you had to spend a lot of time on the solution, and perhaps I can offer you a shortcut for future reference. |
|
@demiankatz: Thank you; it was a php-cs-fixer failure. I was a bit irritated because it only showed a diff-output instead of pointing on errors or warnings und - moreover - it showed these diffs at placed where I haven't changed anything. I guess the rules have become more strict. |
|
@hajoseng, when you get php-cs-fixer diff output, you can just run (And it's likely that the new problems were caused by the recent upgrade to php-cs-fixer 3, which is more strict about argument lists than php-cs-fixer 2 was). |
|
@demiankatz, thank you for your hint on php-cs-fixer as command tool. Just tried it. |
… connected to remote service.
| 'storageid' => '', | ||
| 'storagehref' => '', | ||
| 'item_notes' => ['mit Zustimmung'], | ||
| 'services' => ['presentation', 'loan'], |
There was a problem hiding this comment.
It's not clear, to which service the item_note is related. Maybe we need generic item_notes and should add a new field for service-related item_notes?
There was a problem hiding this comment.
Could this also be handled by appending/prepending service-specific text to the notes in the general field? (I don't mind adding something new, but if we can keep the ILS driver spec simpler and less specific without losing functionality, that's probably preferable).
There was a problem hiding this comment.
Yes, I thought about that as well and it may be a workaround if we do not want to introduce a new field .
There was a problem hiding this comment.
I'll be interested to hear what @hajoseng has to say! The other consideration with either solution is that, since translation for 8.0 is already underway, we don't have the ability to add new translation strings right now... (Though in this case, if it's absolutely necessary, I could live with adding new strings in just English and German for now, since I don't think DAIA is widely used in other language communities yet... but correct me if I'm wrong about that!)
There was a problem hiding this comment.
Thank you @hajoseng! Overall I am happy with that PR, although it breaks with some of our customizations in our VuFind instance (which are expecting to have electronic holdings in the DAIA response, if electronic holdings are applicable at all). But I guess, those customizations can easily deal with the new array key for electronic holdings instead of the holdings key.
Before eventually approving this, I would like to discuss the effects of the introduction of the new array key, though. The DAIA driver would be the only driver handling it this way so far and I guess this might be a reason to postpone these changes to VuFind 9, as it might affect some display logics and templates as well...
I do also not see, that this PR really solves the problem of mixed limitations for different services, which was the original problem in #2026 - it just skips limitations, if they are related to services, which are not in the onSiteServices, as I noted here: https://github.com/vufind-org/vufind/pull/2060/files#r691956985... For instance, it's still not clear, to which service an item_note is related. Maybe we need a new item_note field to indicate relations between services and notes (ie. limitations)?
|
Just did another commit to include electronic holdings as well. Those holdings can be connected to several remote services and several remote-links for each item - each with different limitations (e.g. one for within university scope and one for externals provided with a login). I just put those links and limitation remarks into item_notes; maybe - or hopefully - there show up nicer solutions. Showing remote services is configurable as well; it is skipped by leaving the daiaRemoteServices list empty. |
|
@olli-gold, thank you for your answer. Important issues which we have to work out. Maybe you have a look at the way the driver handles remote services after my last commit. |
|
@olli-gold, the |
|
Ah, ok, thanks for that notice - I did not know, that this electronic_holdings key already exists. That definitely changes my mind about approving this PR :) However, I'll test the latest development again. |
…lled als locations (just like the related template expects them to be). As there might be several links per item, several locations are displayed if there are; otherwise everything is kept as is in order not to break backwards compatibility.
|
@demiankatz: It doesn't look like a real merge conflict; or have I missed something? |
There was a problem hiding this comment.
@hajoseng, there were some merge conflicts related to @EreMaijala's recent work to improve consistency of HTML escaping. I think I've resolved the direct conflicts, but there also seem to be some other related issues in the template, which I've commented on below.
I also suspect it may be possible to simplify this code (perhaps by casting the data structure to an array) but I didn't analyze it in depth to confirm this suspicion.
| @@ -23,7 +40,7 @@ | |||
| <b><?=$this->transEsc("Item Notes")?>:</b> | |||
| <ul> | |||
| <?php foreach ($item['item_notes'] as $item_note): ?> | |||
| <li><?=$this->escapeHtml($item_note) ?></li> | |||
| <li><?=$item_note ?></li> | |||
There was a problem hiding this comment.
Was this reverted intentionally, or just a merge error?
There was a problem hiding this comment.
This is a merge error.
| <?php if ($item['locationhref'][$index] ?? false): ?> | ||
| <p> | ||
| <a href="<?=$this->escapeHtmlAttr($item['locationhref'][$index])?>" target="_blank"><?=$locationTextEsc?></a> | ||
| <?=$item['locationnote'] ?? ''?> |
There was a problem hiding this comment.
This probably needs HTML escaping.
| <?php else: ?> | ||
| <p> | ||
| <?=$locationTextEsc?> | ||
| <?=$item['locationnote'] ?? ''?> |
There was a problem hiding this comment.
This probably needs HTML escaping.
| 'storage' => 'Abteilung III', | ||
| 'storageid' => '', | ||
| 'storagehref' => '', | ||
| 'item_notes' => ['mit Zustimmung'], |
There was a problem hiding this comment.
I just notice, that this might be a limitation_type rather than an item_note...
There was a problem hiding this comment.
@olli-gold: a limitation_type contains the id of a limitation, which is in any case should be a purl. Only the contents of limitations are mapped to item_notes. In fact I've never seen a limitation id in real DAIA; limitations seem to be announced only in a free-text style.
There was a problem hiding this comment.
I see the problem, that we do not get an ID from our DAIA provider, but the specification recommends to set IDs (cf. https://gbv.github.io/daia/daia.html#limitations). Maybe we should ask for IDs at our DAIA provider, but it will certainly take much time until we really have them. In the meantime we could choose to either add IDs in the driver (at least for some known free text limitations) or to keep the status of just showing the limitations as item_notes (if this is common practice for the most drivers in VuFind)...
There was a problem hiding this comment.
But maybe we should open a different PR for that development, as it really does not need to be addressed here.
…of magazines; about: sometimes legal informations)
|
Now on-site and remote services are no longer distiguished. Instead electronic holdings are identified by a method using service-related criteria. As well there are some more information gathered as item_notes (e.g. about issues of newspapers or magazines and licence notes). Additionally some minor changes (empty notes shouldn't be shown as well as holding informations lacking basic items). See comments. |
|
Changes so far are:
But anyhow there's maybe more work to do:
@lahmann @olli-gold: I would be glad, if you have another look at this PR and leave some comments. |
|
@lahmann / @olli-gold, I just happened to notice that this PR has been inactive for several months. Did either of you have a chance to look at it? I realize that @hajoseng is not available right now, but I just wanted to see if we can do anything to keep this moving forward. |
| ); | ||
| } else { | ||
| $this->debug( | ||
| 'Accepting loan, presentation, remote and openaccess as services.' |
There was a problem hiding this comment.
Improved debugging message: "No services configured - accepting loan, presentation, remote and openaccess as services by default."
| $isElectronic = $this->serviceIsElectronic($status); | ||
| // add result_item to the result array: either to electronic_ | ||
| // holdings if it is electronic or to holdings otherwise | ||
| if ($this->serviceIsElectronic($status)) { |
There was a problem hiding this comment.
Check can be done on $isElectronic.
| $item_notes[] = $item['about']; | ||
| } | ||
|
|
||
| // If no useful availability service is found, return only the |
There was a problem hiding this comment.
The added logic block below will introduce very strict requirements for DAIA items in terms of availablity modelling: according to this if-conditional the unavailability information of an unavailable item will be shown to the user only if either a link, a duedate or an available service exists. Otherwise this item will simply be shown as unavailable. This is too strict as according to DAIA specification (http://gbv.github.io/daia/daia.html#availability) the only required field for defining an unavailable service is service. E.g. the availability of an unavailable item could be further specified by a given limitation, but any limitation will be ignored if the above mentioned conditions are not met.
|
I finished my review and would not recommend merging this pull request in its current state as the introduced changes will require a very specific and strict way of DAIA availability modelling which is not defined by the DAIA specification (see #2060 (review)). My suggestion is to postpone this pull request to the VuFind 10 release. It would be interesting to hear if @olli-gold has some thoughts on this. |
|
Thanks, @lahmann, I have changed the milestone to 10.0 for now. We can, of course, always merge this early if things get resolved sooner/more easily than anticipated. |
|
@hajoseng, @olli-gold and @lahmann: I see that this PR now has some conflicts that need to be resolved. Is there still a need for this work, and does anyone have time/interest to move it forward? Unfortunately, I am not in a good position to help resolve this one, but of course I'm always happy to support progress however I can! |
|
I still have objections (see above) which I would like to discuss before merging this PR - feedback by @olli-gold and @hajoseng would be very much appreciated. |
|
Since both @olli-gold and @hajoseng may have limited availability right now, I'm going to push this forward to the 10.1 milestone to give us more time for this conversation. Thanks, @lahmann! (Of course, I'd still be happy to merge this in time for the 10.0 release if the work gets done faster than expected). |
demiankatz
changed the title
|
I'm moving this to the 11.0 milestone since significant work is still needed, and the 10.1 release is getting close. |
…uration is in DAIA.ini, default loan and presentation (interloan can be added). Limitations are only regarded if the belong to an accepted service. There is as well a bit of code restructured in order to be able to include electronic holdings. Holdings without holding services are skipped.
This is the first part which should solve pr #2026. I'd like to add the electronic holdings functionality as well, but I have to do some more considerations on this topic.