Updated DAIA and new PAIA ILS-driver

[lahmann]

Finally - the fully functional DAIA-PAIA ILS-driver for VuFind. Credits to @olli-gold, @tillk, Magdalena Roos.

Changes in DAIA:

• added configurable caching
• added some more helper methods to be used as hooks for customization
• refactored getItemLocation to getItemDepartment to avoid confusion and match DAIA field names
• added getItemStorage methods to allow better customization of item location display (currently only getItemDepartment* methods are mapped to VuFind's return value 'location')
• getHolding will return some more DAIA specific values (doc_id, locationid, storage, storageid, storage_href, limitation_types)
• better handling of item availability (can be customized):
  • available item for loan with href is mapped to storageRetrievalRequest
  • unavailable item with duedate is mapped to being recallable

PAIA driver (based on GBV PAIA driver https://github.com/gbv/vufind/blob/master/module/VZG/src/VZG/ILS/Driver/PAIA.php):

• supports caching of items
• supported ILS functions:
  • recall
  • storageretrievalrequest
  • cancel/renew
  • changepassword
  • fees

TODO:

• adapt DAIATest to updated driver
• write unit test for PAIA driver
• refactor DAIA driver to reflect changes in caching after Fix for cache keys not matching Zend's default cache key naming pattern in ILS driver #698 got merged (formatCacheKey() to getCacheKey())

[demiankatz]

Thanks for making this available -- obviously we shouldn't merge until tests are passing at very least. Let me know if I can be of any assistance with any of that.

[demiankatz]

A few comments/questions:

1.) Travis was complaining about some php-cs-fixer issues; I've run the tool to fix them and pushed it up, so the build is now passing.

2.) I think we might want to consider some changes to the way cache enabling/disabling is handled in the DAIA driver. Right now, we check if cache is enabled every time we try to read from the cache, but we always write to the cache even if it is disabled. That seems undesirable. It seems like it would be cleaner to handle the enable check inside the getCachedData/putCachedData methods so that there's a single central check and less chance of accidental inconsistent behavior. That raises the question of whether Daia should simply wrap the parent class' methods with an extra check, or whether we should actually build in enable/disable functionality to the abstract base class. What do you think?

3.) Also related to caching -- there is an open conversation on #698 about how best to handle cache key naming. It might be a good idea to resolve that before we merge this, in case it simplifies anything. Do you mind taking another look and adding a comment there if you have an opinion?

4.) Assuming we get the cache stuff sorted out, should I also hold off on merging until a PAIA test is written, or would you prefer to get this out there as soon as possible?

Thanks!

[olli-gold]

I'm currently preparing some test cases, so I think there is a chance, that we have tests pretty soon. ;-)

[lahmann]

Thanks Demian for the comments and fixing the php-cs issues!

2.) Your suggestion to check whether caching is enabled for the current ILS driver in the get/putCachedData methods sound absolutely reasonable but especially in the case of the DAIA/PAIA driver this would not work as bot drivers use the same methods but it makes sense to disable/enable caching for each service (DAIA and/or PAIA). Enabling caching for DAIA will gain performance for the result list as availability information does not need to be retrieved from the DAIA service all the time whereas PAIA will only help with MyResearch views but might lead to some delay in providing changes in availability status of holds or requests for the logged in user.
At least for this scenario coping with enabled/disabled cache-settings in the get/putCachedData methods seems to imply new problems. Therefore I would vote for leaving the cache implementation in AbstractBase as it is as it allows most flexibility and leaves it to the ILS driver to care for appropriate implementation of checks whether caching is enabled or not.

3.) Agreed - see #698 .

4.) No let's make this a proper pull request - as @olli-gold said, we are working on it :)

5.) I just saw that I accidentally added removeCachedData() to DAIA -- I think this could be a useful addition to AbstractBase as it should be often the case for ILS-drivers to manually invalidate the cache in order to update the availability information for just requested items. Should I move this method to AbstractBase or do you prefer a seperate pull request?

[demiankatz]

2.) You make a good point -- best to leave global enabling/disabling to higher-level code.

4.) Sounds good -- just let me know when you feel things are ready to go, and I'll give this one more close look and merge if I see no obvious outstanding issues.

5.) This seems like a reasonable addition to AbstractBase. Might as well do a separate PR so we can get it in more quickly and keep the complexity of this PR as low as possible.

[lahmann]

Thanks to @olli-gold now we have very complete unit-tests for the PAIA driver. There is one remaining issue: as PAIA->checkRequestIsValid() depends on session data @olli-gold had to add PAIA->addToSession() in order to manually add the needed session value making the tests (especially testValidRequest()) run. Is there a better way to create a mock session object for the tests?

Above that I did some fixes in DAIA.php: the location_href array field got renamed to locationhref (according to the updated docs) and the href-field was wrongly addressed for DAIA responses (it is on the same level as the service field). The latter bugfix resulted in a different output therefore the unit-test had to be updated as well.
I also mapped the href field for unavailable services to the DAIA specific variable $serviceLink which can be used via getHoldLink() to replace the VuFind Hold/Recall actions (cf. 0d9e126#diff-d637617bdb310092d9a3d1a3e5dda5dfR853). As DAIA does not provide any ILS interaction functionalities (that's what PAIA is for) the DAIA driver (if used on its own) should not create holdings which VuFind interprets as being able to place holds etc. via its own actions. Therefore I added some documentation to the DAIA.ini which explains that by setting function = getHoldLink for Holds the DAIA driver will actually use the DAIA response's href-value for linking directly to the OPAC's recall action. Unfortunately this works only for holds/recalls but not for storage retrieval requests (SRR) as SRR do not support any further options than HMACKeys. So, to cover DAIA functionality this is as much as we can do with the current ILS logic.
This issue looks like something we might need to consider when rethinking the ILS integration in VuFind (having multiple actions - holds/recall/SRR/ILL but with the same capabilities) -- but this leads to off topic discussion for this pull request.

From my perspective we are closing into the final stretch - Demian, what do you think?

[olli-gold]

I have prepared a new PR against finc getting rid of PAIA->addToSession(): finc#15

[lahmann]

Thanks @olli-gold! I guess now we just have to wait for Demians review. :)

[olli-gold]

Sorry for spamming with these cross-references, obviously there was a collision with our internal bugtracker after I had pushed our local repository to github. Our local Ticket 729 has nothing to do with this PR...

[lahmann]

Finally some progress - I implemented helper functions to build DAIA and PAIA-specific cache keys as I could not implement getCacheKey for PAIA without affecting DAIAs use of getCacheKey. The committed should suffice to make sure DAIA and PAIA cache items will not collide with other DAIA/PAIA backends.

This should hopefully resolve the final issue - or did I overlook some pending issue concerning the DAIA/PAIA driver?

[demiankatz]

@olli-gold, it is unfortunate that your ticket numbers overlap with our PR numbers. I get a lot of notifications about comments from your team on my pull requests, but they're never actually related. Just a bad side effect of the way GitHub operates, but if you are able to designate ticket numbers in a different way, it would certainly be helpful. If you need to do it as you are doing it, though, I can certainly live with it -- I've just learned to ignore certain types of notices.

@lahmann, I'm not aware of any outstanding issues, but I'll give this another couple of days before merging in case somebody else thinks of something. I'll also try to give the code one more review a little later in case I have any other comments. Thanks for the progress!

[olli-gold]

Yes, I understand that this is really unfortunate and I'm really sorry about that. I will check, if we can use a namespace for our tickets; this should solve this issue and should avoid collisions.

[olli-gold]

Unfortunately our Bugtracker (Redmine) does not support namespaces for their IDs, so I have no idea how to change the situation. Perhaps the only way would be to decouple our repository tubhh/vufind from the official repository. But that would also not be nice.

[demiankatz]

Too bad. As I said, we can live with it for now... but something to think about when evaluating tools in future.

[demiankatz]

@lahmann, regarding the caching solution you've come up with, it should work fine, though it's a shame it can't be done more simply. Is there a reason you can't have the DAIA driver define the cache key as the hash of the DAIA URL and the PAIA driver define the cache key as the DAIA and PAIA URLs concatenated? I imagine that would be more likely to result in redundant cache entries under certain circumstances and it would reduce the potential for reuse in some cases... but it might also make the code simpler and more readable. I have no strong feelings here, and you know the use cases better than I do, but I just bring it up for your consideration. If you feel it's good to go as-is, I'll plan on merging this on Friday if nobody else offers any objections in the meantime.

[lahmann]

@olli-gold a plugin for Redmine might allow to add namespaces (http://www.redmine.org/plugins/issue_id and see the comments for some up-to-date github forks) although this will certainly not fix issue id spill-over for the already committed issue ids.

@demiankatz I agree that the double implementation for the getCacheKey is far from optimal and your suggestion might be a working alternative. Assuming this solution we would have in

DAIA.php

protected function getCacheKey($key)
{
    return parent::getCacheKey(md5($this->baseURL) . $key);
}

and in PAIA.php

protected function getCacheKey($key)
{
    return \VuFind\ILS\Driver\AbstractBase->getCacheKey(
        md5($this->baseURL . $this->paiaURL) . $key
    );
}

I'm a bit undecided - I do favor the current code as it allows DAIA and PAIA specific customizations to the cache-key-suffix, but it adds a little bit of function calls overhead (and somewhat reproduces the changes we introduced with #698 on driver level). Your suggestion would be more strict to the class architecture introduced with #698 and I don't think that it would cause redundant entries as the cache item specific key in DAIA would always be the item URI and in PAIA the patron id. So it's probably more a question of code aesthetics, explicity and/or sticking to agreed upon class architecture.

Maybe @olli-gold has some thoughts about that? :)

[olli-gold]

@lahmann Thanks for the hint about issue-id. Unfortunately we are using Redmine 3 and the plugin is for Redmine 2. Unfortunately both of the forks mentioned in the comments do not work for our Redmine as well. :-(

I don't have a preference about the caching question right now...

[demiankatz]

Since @olli-gold has no preference, I think I favor avoiding the double-implementation and extra function calls. There are certainly reasons that approach has advantages, but at the same time I think it makes the code less readable and more fragile, and it requires more specific knowledge to understand how things work. The simpler, more seamless approach is probably preferable, at least until we determine a more compelling argument to specialize.

You might also be able to implement the actual cache key generation in a more independent way, without relying on the KeyGeneratorTrait -- see, for example, how VoyagerRestful does it:

https://github.com/vufind-org/vufind/blob/master/module/VuFind/src/VuFind/ILS/Driver/VoyagerRestful.php#L325

You're certainly free to disagree with some or all of this assessment if you like -- but that's my gut feeling at the moment.

[lahmann]

Demian, alright - I'll finish this tomorrow morning to have the pull request ready for merge later in the afternoon.

[lahmann]

Demian, I decided to not re-implement getCacheKey from the KeyGeneratorTrait but to do a static call to the AbstractBase's getCacheKey with a modified suffix in order to stick to the standardized cache key pattern we established with the KeyGeneratorTrait.
This should finish this pull request - let me know if there are remaining issues.

[demiankatz]

Looking good. A few final comments:

1.) I've updated the config.ini comments to include PAIA as an option.

2.) I've merged the latest master to make sure there are no conflicts with #792 -- looks fine, but maybe worth one more real-life test.

3.) I notice that the daiaCacheLifetime setting in DAIA.ini is actually a global cache lifetime setting that will affect both DAIA and PAIA. Should we rename it to something more generic to prevent confusion?

Please let me know what you think about item 3 above. Once that is resolved (or you have decided it's better to leave it alone) and as long as nobody else reports problems, I'll merge this early next week. Thanks again for all of the work!

[lahmann]

Demian, thanks for your comments! After keeping the daiaCacheLifetime setting I finally decided to move it into a new [General] section in DAIA.ini which will hopefully be as clear as possible that this setting applies for both (and any deriving) drivers.

Luckily, today I discovered a small bug in PAIA->renewMyItems and fixed it - the time conversion was not in place and in some cases the driver would throw PHP errors. It's fixed now. I had to adjust the unit test output as the date conversion now results in properly formatted dates based on locale settings.

I also tested the current pull request branch locally and found no further issues (incl. #792) - so I think we are ready to merge. :)

[demiankatz]

Great, thanks. I made one last change -- updating paia.ini to reflect the configuration setting name change. I'll go ahead and merge. If you and/or @olli-gold can test master following the merge, just in case there are any remaining problems, I would of course be grateful!

[olli-gold]

I have tested the recent master after the merge and it seems to work fine on the first sight.

[lahmann]

I also tested the latest master and could not find any bugs. Thanks again @demiankatz @olli-gold !