Skip to content

Conversation

@mwarin
Copy link
Contributor

@mwarin mwarin commented Jun 10, 2025

Fix for https://hathitrust.atlassian.net/browse/GS-13230

... in which some of bc's IA items were failing because they fall back on an invalid resolution value from the $volume.
Single line fix that removes ppi from resolution values.

This should fail on the main branch:

$ git checkout main
$ docker compose run --rm test perl bin/ingest_ia_dev.pl realceduladesuma00spai_0 test ark:/13960/s25zng884bb
[...]
Error setting new tag Resolution => 400ppi: Not a floating point number for MIE-Image:Resolution

... but should succeed on this PR branch:

$ git checkout GS-13230-ppi-resolution-fix
$ docker compose run --rm test perl bin/ingest_ia_dev.pl realceduladesuma00spai_0 test ark:/13960/s25zng884bb

@mwarin
Copy link
Contributor Author

mwarin commented Jun 10, 2025

@aelkiss I would like a chance to run this through with you once or twice. I think there are dueling red herrings:

  • can't run it as namespace:test because it's going to look for things in zephir and fail
  • can't run it as namespace:bc in dev because I don't know how to make real namespaces work in dev
  • once i manage to get it in zephir, the arkid on https://zephir.cdlib.org/api/item/bc.ark:/13960/s25zng884bb does not match the arkid realceduladesuma00spai_0

@mwarin mwarin marked this pull request as ready for review June 10, 2025 17:29
@mwarin mwarin requested a review from aelkiss June 10, 2025 17:29
Copy link
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

After testing with the correct IA ID matching this arkid, everything looks good. We added ETT-340 as a potential future step to make testing this kind of thing easier.

@mwarin mwarin merged commit 528ab55 into main Jun 11, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants