Skip to content
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

Display loan types from FOLIO #4183

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

bbusenius
Copy link
Contributor

@bbusenius bbusenius commented Jan 9, 2025

This is a simple prototype of how we're displaying loan types from Folio. There's a getLoanTypeData function that retrieves the data by loanTypeId and returns it with items in formatHoldingItem. Then we just display it in the templates.

TODO

  • Update ILS driver spec with new getHolding return values when merging

@bbusenius bbusenius requested a review from demiankatz January 9, 2025 19:56
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @bbusenius! This is off to a great start. See below for a couple of thoughts/comments. Regarding configurability, I'm interested in your opinion, but I don't mind helping with the actual implementation part if you're short on time.

@@ -30,6 +30,9 @@
</span>
<?php if ($availabilityStatus->isAvailable()): ?>
<?php /* Begin Available Items (Holds) */ ?>
<?php if (isset($holding['loan_type_id']) && !empty($holding['loan_type_name'])): ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to check loan_type_id? If loan_type_name is the only thing you're displaying, it's probably fair to say it's the only thing that needs checking.

However, we should probably come up with a mechanism for enabling or disabling this display, since not everyone is going to want it. Any thoughts/preferences on whether it would make more sense to put something in Folio.ini or in a more general place like config.ini? (I see pros/cons to both -- if we put the setting in Folio.ini, then we can totally bypass the extra data lookup when it is not wanted and save some time/cycles... but if we put the setting in config.ini, then it's a more universal setting and will carry across all ILS drivers without having to implement things independently in all of them).

@demiankatz demiankatz changed the title Display loan types from Folio Display loan types from FOLIO Jan 9, 2025
@bbusenius
Copy link
Contributor Author

No problem @demiankatz! Ah yes, I should have caught that, regarding loan_type_id. I'll take that out. I still think it's a good idea to return it though. That way if someone needs to add custom logic for a particular case, they won't have to peg it to the string.

Regarding where to put the config, I normally look to the ILS config for something like that, however, I think you raise a good point about having it in a general place. My problem with that is that I sometimes can't find the setting. It might make it easier to decide if there was already a logical place for it in config.ini. I just browsed quickly and thought it could almost go in the Item_Status section but something about that doesn't feel quite right. Long-term it might be nice to have a "General ILS settings" section or something like that, where it's clear that the settings are related to the ILS but applied more generally. I think organizing it by "functionality" in config.ini might make it harder to locate, if that makes sense. But honestly, that's all just an initial reaction. I don't have a strong opinion about it.

@demiankatz
Copy link
Member

Thanks, @bbusenius -- I agree that it makes sense to return the loan type ID; we just don't need to use it for anything in the default template.

The [Catalog] section of config.ini is where we put ILS-specific settings... adding a display_loan_type_in_holdings setting might make sense. (Like you, I don't have a strong opinion... but that's a brainstormed idea I could live with). :-)

@bbusenius
Copy link
Contributor Author

That sounds OK to me @demiankatz. I'll go with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants