FOLIO: more flexible hold exclusion configuration

[meganschanz]

This adds a new config in the FOLIO driver to optionally exclude hold location matches by substrings instead of full string matches. This config is set to false by default for backwards compatibility.

This was useful for us because we have a dozens of locations we need to exclude that either start with or contain a string (such as "Reserve" or "Special Collection") but the full location have more specific qualifiers in them like the floor and the list would be quite long and difficult to maintain anytime a new location was added or changed.

[meganschanz]

Rookie mistake! I'm working on cleaning up the format issues and I'll make another commit tomorrow.

[demiankatz]

Thanks, @meganschanz -- I'm happy to merge this as is, but a suggestion for your consideration first: what if, instead of a binary excludeHoldLocationsBySubString setting, we had a excludeHoldLocationsCompareMode setting, with "exact" or "substring" options? This would open the way to adding additional options (like a "regex" mode) if we need them, without having to further redesign the configuration. Or we could implement regex mode now, and make "substring" a special case of regex (we could just add /.* ... .*/ around each value. Case sensitivity is another question: do we want to offer case-sensitive and case-insensitive versions? Using regular expressions makes it easier to configure this without having to add lots of different modes, since you can control it with regex switches.

[meganschanz]

Good idea. I much prefer that. I took your suggested and made a commit that has a setting with excludeHoldLocationsCompareMode with the default value of exact but allowing for regex.

[demiankatz]

Looking great, @meganschanz! A few minor suggestions for finishing touches. It might also be nice to add some test coverage for the new behavior. If you'd like to try that and are not yet familiar with VuFind's test suite, I can point you to some resources and make some suggestions. If you don't have time, I can probably add a quick test myself before merging this.

[meganschanz]

Those suggestions look good. I just started in on writing the unit tests and ran into a problem, the isHoldable function is protected. Is there a preferred way for tested those. I could have it call the public getHolding function and then just check the is_holdable attribute of item, but that seems like more overhead than necessary.

Error: Call to protected method VuFind\ILS\Driver\Folio::isHoldable() from scope VuFindTest\ILS\Driver\FolioTest

[demiankatz]

Thanks, @meganschanz! If you need to test a protected method, and there's not a straightforward way to test it via a public method, you can mix in the ReflectionTrait and use $this->callMethod().

[meganschanz]

I learned so much about the testing today... that vendor/bin/phing qa-php command that the CI runs is handy!

Let me know if you have suggestions for any rework that could be done to the tests. When working on the tests, I also added in a small change to the isHoldable function to better handle the scenario when a bad regex pattern is set in the config.

[demiankatz]

Thanks for the ongoing progress, @meganschanz -- this is looking great, and could nearly be merged as it is... but since you asked about test style, I provided some suggestions that I think will make this even better. I also had one small stylistic comment which you can use or ignore as you see fit. The only thing that I think we absolutely must do is fix the order of parameters in the assertions; the rest is a matter of taste.

One other tool in the PHPUnit test kit that I find very useful is the @dataProvider annotation, which lets you separate your test logic from your test data in a useful way. You could use that for some of the tests here, though I think it's probably overkill. If you're interested in learning more about it, though, feel free to experiment.

Let me know if there's anything more I can do to help!

[meganschanz]

I spent some time playing around with the @dataProvider syntax this morning, which was really neat! I did think it probably would be overkill to do in this case since I couldn't use the same data provider for each of the separate test functions (due to the expected output changing based on the config setting of that particular test function). So for simplicity and readability I kept it as-is for now. But for future tests I will definitely keep that in mind!

I made the other corrections and suggestions to the tests and removed the unnecessary else statement in the isHoldable function. Let me know if you see anything else!

[demiankatz]

This looks great, @meganschanz -- thanks for taking the time to put it all together!