-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding configurable ignoreFilterReasons in Alma Resolver #2498
Adding configurable ignoreFilterReasons in Alma Resolver #2498
Conversation
@demiankatz could you help me with the UnitTest? I'm little lost as I set the parameter type to "\Laminas\Config\Config". |
Thanks, @he247! I'll give this a closer look tomorrow, but the test failure is because the test is constructing the Alma object with only two parameters, and your changes here add a third mandatory parameter. You'll either need to change the test to build the object differently, or else make the constructor parameter optional (e.g. by defaulting to null). A bigger question, though: is this something that belongs in config.ini, or should it really be part of Alma.ini? That might have some impact on how things are designed.... [and please forgive my earlier revision of this comment, which contained some inaccurate information, since I once again forgot that we were talking about resolver drivers and not ILS drivers]. |
Hi @demiankatz thank you for the hint about defaulting to null. One question would be if I use the entire config-object as parameter or just the string stored in "ignoredFilterReasons". I'd prefer keeping it in the general config.ini for OpenUrl, so it wouldn't be spread into several config-files. And I can imagine that it might also be useful for other Link Resolvers to not have static filter settings (even though it might not be implemented yet). [no worries, I always appreciate your input :) ] |
@he247 I agree the right place is config.ini since the resolver driver is not related to the ILS driver. |
@he247 and @EreMaijala, my concern about including the setting in config.ini is that if it only works for Alma and/or contains Alma-specific values, it will be confusing for users of other services. If we keep it there, I think we should either implement it universally or put "alma" in the setting name to indicate the relationship. A clear comment might also help, indicating context and legal values. |
@demiankatz A setting with "alma" in it, or an Alma resolver specific section would work. My comment was just meant to sat that this is unrelated to any of the settings in Alma.ini and it would be super-confusing to intertwine ILS driver and resolver driver settings in a single configuration file. |
@demiankatz and @EreMaijala , if you agree I'd just add a part to the comment in the config.ini stating that this is Alma-Resolver specific |
That sounds reasonable, @he247. If in future we decide to extend the setting to work with other drivers, then we can just update the comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, @he247, I think this is coming together now. I've done a more thorough review with some possible suggestions. As always, you can feel free to ignore any that are unhelpful... but hopefully at least some are relevant. (I was pretty far off-base yesterday ;-) ).
config/vufind/config.ini
Outdated
; List of filter reasons that are ignored (displayed regardless of filtering). | ||
; You may also use a comma-separated list of filter reasons. | ||
; (default is "" if commented out: non filter reason ignored) | ||
ignoredFilterReasons = "Date Filter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use an array-type setting here, rather than a delimited list? e.g.
ignoredFilterReasons[] = "reason 1"
ignoredFilterReasons[] = "reason 2"
rather than
ignoredFilterReasons = "reason 1,reason2"
?
This would require less parsing to deal with in the driver, and might avoid potentially problematic edge cases. You could support having the setting set to false (ignoredFilterReasons = false
) to disable filtering.
Also, is there a strong reason to change the default to "no filters" rather than allowing the driver to have default settings? Making this change could introduce unexpected changes in behavior if people fail to update their configurations when upgrading. I'm not sure how likely that is to be a problem but wanted to raise the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, a suggested rewording of the comment that might be more "future-proof":
; Some link resolver drivers can filter resource links based on specific criteria.
; This setting indicates which filter reasons should NOT be used to exclude
; resource links.
; [note about format, depending on discussion above]
; [note about defaults, depending on discussion above]
; This is currently ONLY supported by the Alma resolver driver.
; Legal values include: Date Filter (any others of note?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another Filter Reason I found "Available for - the ip groups are not part of the service AF groups" but unfortunately I wasn't able to find any documentation about those reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @he247, this is looking good to me. I've just left a few very small comments about comments.
Also, it would be nice if we could add a case to the test (https://github.com/vufind-org/vufind/blob/dev/module/VuFind/tests/unit-tests/src/VuFindTest/Resolver/Driver/AlmaTest.php) to cover this new functionality. Since the existing test fixture includes a date filter, I think it would be pretty simple to copy testParseLinks
to testParseLinksWithoutFiltering
, add a new parameter to createConnector
to accept options, and then set a custom filter option in the new test and adjust the expectations accordingly. I can help if you need me to!
Thanks, @demiankatz, I will apply the changes and add a test. :) |
…ys with empty strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @he247 -- there's one very small fix needed to the test, and then I believe this can be merged. If you're busy, let me know and I can fix this on my end if you prefer!
], | ||
]; | ||
|
||
$this->assertEquals($result, $testResult); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PHPUnit assertions, the first parameter is the "expected" value and the second is the "actual" value -- so I think these should be reversed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing, @demiankatz I corrected the parameter order in both assertions.
…" value and the second is the "actual" value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @he247 -- I see now that the test bug was copied and pasted from the existing test, so thanks for fixing that as well. I think we're all set now. I'll merge this right away!
* Move download functionality to a trait. * Adds caching for linked events and makes feed style properties parsing more robust.
This change was made based on the discussion in the mailing-list "[VuFind-Tech] [EXTERNAL] Alma Resolver - Date Filter".