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

FOLIO: Fix grouping holdings by holdings_id #3101

Merged
merged 2 commits into from Sep 19, 2023

Conversation

maccabeelevine
Copy link
Member

@maccabeelevine maccabeelevine commented Sep 19, 2023

Per VUFIND-1641

The default holdings_grouping defined in config.ini (and if unspecified there, in Holds.php->getHoldingsGroupKey) is holdings_id,location. The holdings_id key is matched against keys in $copy, which ultimately come from the individual holding/item data supplied by the ILS driver in getHolding(). In the FOLIO driver, getHolding() calls formatHoldingItem() to generate this data. formatHoldingItem() creates a key called holding_id (singular "holding"), which then fails to match "holdings_id" in the grouping logic. That would be easy to fix, but the singular "holding_id" is also used in Folio.ini for something called HMACKeys which I don't understand. ... Of course an easy workaround is to set config.ini to "holding_id,location" instead, but no one would know to do that.

This PR fixes the typo in FOLIO.php->formatHoldingItem() and in FOLIO.ini->HMACKeys.

TODO

  • Update changelog when merging

@maccabeelevine maccabeelevine added this to the 10.0 milestone Sep 19, 2023
@maccabeelevine maccabeelevine marked this pull request as ready for review September 19, 2023 14:32
@demiankatz demiankatz changed the base branch from dev to dev-10.0 September 19, 2023 15:41
@demiankatz demiankatz changed the base branch from dev-10.0 to dev September 19, 2023 15:44
@demiankatz
Copy link
Member

Thanks, @maccabeelevine -- I tried to rebase this against the dev-10.0 branch, but that branch is currently a bit behind dev and it made things confusing, so I've changed it back for now. :-)

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.

I just tested, and this does not negatively impact my ability to place holds. I think this probably qualifies as a bug fix that could be merged into release 9.1, but let me know if you'd prefer for me to wait and merge this to dev-10.0 instead once conflicts there are resolved.

@maccabeelevine
Copy link
Member Author

maccabeelevine commented Sep 19, 2023

I just tested, and this does not negatively impact my ability to place holds. I think this probably qualifies as a bug fix that could be merged into release 9.1, but let me know if you'd prefer for me to wait and merge this to dev-10.0 instead once conflicts there are resolved.

@demiankatz I am fine either way, this particular issue is irrelevant to Lehigh, I just shared it on Jira so figured I should fix it.

@demiankatz demiankatz merged commit e076caf into vufind-org:dev Sep 19, 2023
7 checks passed
@maccabeelevine maccabeelevine deleted the folio-sort-by-holdings-id branch September 19, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants