GetThis feature + Regex config file support#4359
Conversation
demiankatz
added
new feature
new language strings
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @rominail. I have not had a chance to try this hands-on yet, or to review every file carefully, but see below for a few thoughts based on a relatively quick look at the code.
…s, move the getThisAction from trait to the controller, add a supportGetThis method instead of a hardcoded class, switcyh getThis constructor params from yamlReader to config, change priority of js file
…ng from previous commit (oversight)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @rominail, I've resolved all of the open conversation threads that have been fully addressed, and have opened a few new ones below (along with contributing to ongoing ones).
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…x.yaml, renaming to microform_viewing_request_html
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…in GetThis config
… + use of it, switch to non-positional "and" operator in GetThis config
Co-authored-by: Demian Katz <demian.katz@villanova.edu> Co-authored-by: Stefan Weil <sw@weilnetz.de>
EreMaijala
left a comment
EreMaijala
left a comment
There was a problem hiding this comment.
I'm sorry if I'm commenting on something that's already been addressed. There was just so much going on here that I may have missed a bunch. And I didn't even try to grasp everything here, so apologies if I'm missing the mark.
My thoughts:
- This seems quite a complex feature to set up, and would benefit from a wiki page that would describe the feature and possible uses (including examples).
- Translations should have their own text domain.
- Since
getThisURIis always a URL, could it be calledgetThisURL? - I'd prefer explicit comparison with null instead of
isset(). E.g. in GetThisLoaderisset($this->items)=>null !== $this->items. - I'd also prefer to have public methods first and protected ones after them, but that's just my preference.
|
Thanks for the review, @EreMaijala, and for the progress, @rominail. One point I'd like to highlight: Ere suggests moving the new text strings to a separate text domain. This is probably a good idea, but since Susan has already begun processing translation requests for translators, it will create extra work if we change it now. So I think there's a question of how strongly we feel. For purely practical reasons, I'm not inclined to fight too hard for change, but I do think it might be the right choice. We can potentially plan to adjust the text AFTER the translation work is completed but before the release of 11.1, if that turns out to be the easiest path. |
|
I'm not familiar with translations much, I'm not for extra work, doing it when the translations come back before the release sounds good; but again you are more aware than me |
|
Thanks, @rominail, I'll merge the translations as-is this week to facilitate the translation process while we finish up the final touches on this PR. If we decide to move them, we can do so in a follow-up shortly before the 11.1 release. I think that's the simplest approach under the circumstances. |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks once again, @rominail!
I took a closer look at @EreMaijala's review and your response to it. I also started building a Get This wiki page. It's bare minimum right now, but please let me know what you think of it, and if you'd like wiki access so you can expand upon it.
While reviewing recent changes and studying code to ensure the accuracy of my wiki page, I noticed a few more things -- see below:
| <?php | ||
| $selectedItem = $this->getThis->getItem(); | ||
| $useDropdown = $this->getThis->makeHoldingsDropdown(); | ||
| $holdingsList = $this->render('record/get-this/holdings-list.phtml'); |
There was a problem hiding this comment.
Would it make sense to use renderInContext here, and pass in $selectedItem and $useDropdown? I realize it's not expensive to retrieve the values again inside holdings-list.phtml, but passing the values explicitly might make it more clear that these settings impact the behavior of the template.
(I'm not insisting on this change, just suggesting it for your consideration).
There was a problem hiding this comment.
I was considering simpler code to read and easier to understand where the variables come from
But I don't have a strong opinion
Let me know what you prefer and I'll do the code change
There was a problem hiding this comment.
We can see if @EreMaijala has a strong opinion when he reviews the refactoring...
There was a problem hiding this comment.
Since the values come from same place I don't really mind it being as is. What I find a bit confusing though is having GetThisLoader as $this->getThis in the template.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the adjustments -- see below for some further simplifications that I think should work (but I apologize if I've made any mistakes).
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @rominail -- see below for one slight simplification.
After that, I'll try to follow up with others who currently have "request changes" reviews here so we can wrap this up!
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@rominail, regarding your latest changes, do you think it would be more clear to rename setRecord to setRecordDriver rather than just setDriver to make it clear what kind of driver is being set? I can live with it as is, but since this class also interacts with the ILS driver, more specific naming might be clearer.
Other than that, I think I'm finished commenting -- I'll re-request reviews from everyone with a pending "request changes" review so we can get those cleared out.
| } | ||
|
|
||
| if (isset($this->getThis)) { | ||
| $urlHelper = $this->renderer->plugin('url'); |
There was a problem hiding this comment.
Is there any reason to get url helper already here? As far as I can see it's only used once below conditionally.
There was a problem hiding this comment.
It is to prevent having it in the foreach loop, but I have no problem moving it
There was a problem hiding this comment.
So it depends on the typical number of loop iterations whether this optimisation should be done or not. For only a few iterations, I'd suggest to move it.
EreMaijala
left a comment
EreMaijala
left a comment
There was a problem hiding this comment.
I left a comment about the use of url helper as it seems a bit out of place and fragile, but other than that I've no further issues, so I'll approve now.
8 participants
This is a rewrite of this feature we call "Get this" which allows having a button next to the holding to open a lightbox showing the option the patron has to "get this" record. The options include placing a request, request for a in-person delivery, inter library loan, ...
It's made to be flexible and easy to extend, in order to add other sections to the lightbox; ie : in our case some logic specific to our collections.
It relies on Regex for some logic. We didn't have the proposed logic implemented hence the code is new and it's a feature on it's own which is completely reusable for other components.
I plan to write the tests once most of the logic is agreed upon.
Please, propose new names (for variables, functions, ...) when they don't make sense or are not clear.
Line 13 of
themes/bootstrap5/templates/record/get-this/holdings.phtmlWe were not sure how to name the item when the call number is not present to be displayed in the dropdownTODO: