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

[ESSI-2062] fix iiif urls unaware of external_storage #648

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

aploshay
Copy link
Contributor

@aploshay aploshay commented Jan 23, 2025

Refactors lookup of IIIF image server URLs (in a manner aware of external_storage settings), and applies that logic where not yet present.

New/modified infrastructure:

Updated cases, adding handling of external storage:

Refactoring cases:

@aploshay aploshay marked this pull request as draft January 27, 2025 15:59
@aploshay aploshay force-pushed the essi-2062_collection_thumbnails_in_s3 branch 5 times, most recently from e6666e6 to f4cf9b5 Compare January 29, 2025 17:15
@aploshay aploshay marked this pull request as ready for review January 30, 2025 18:41
Copy link
Contributor

Choose a reason for hiding this comment

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

Pulling these IIIF behaviors into this service presents an opportunity to make some spec tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlpierce spec tests added, which did indeed lead to some revisions!

@aploshay
Copy link
Contributor Author

aploshay commented Jan 31, 2025

@dlpierce for the refactoring pass, I didn't want to change any logic, but there's an opportunity to simplify/unify logic if it was unintentionally variable, originally:

  • if we get consistent (either way) about just checking that content_location has text (at all) vs starting with "s3", we can simplify #versioned_lookup_id to call #basic_lookup_id
  • if we get consistent about checking for file versioning if #basic_lookup_id fails -- originally done only in the DisplaysImage module -- we can do away with the #basic_lookup_id vs #versioned_lookup_id split, entirely

@aploshay aploshay force-pushed the essi-2062_collection_thumbnails_in_s3 branch 4 times, most recently from 2b0ecc7 to 2e3f17c Compare February 3, 2025 16:09
@aploshay aploshay changed the title Fix collection thumbnails being unaware of external_storage [ESSI-2062] Fix collection thumbnails being unaware of external_storage Feb 3, 2025
@aploshay aploshay changed the title [ESSI-2062] Fix collection thumbnails being unaware of external_storage [ESSI-2062] fix iiif urls unaware of external_storage Feb 3, 2025
@dlpierce dlpierce force-pushed the essi-2062_collection_thumbnails_in_s3 branch from 2e3f17c to 40c7980 Compare February 5, 2025 17:49
@aploshay aploshay force-pushed the essi-2062_collection_thumbnails_in_s3 branch from 40c7980 to d8d25c7 Compare February 5, 2025 22:24
@aploshay
Copy link
Contributor Author

aploshay commented Feb 5, 2025

@dlpierce so that for failing test, it's complaining that it's not expecting version information in the indexed path:
...%2Ffcr:versions%2Fversion1/full/250,/0/default.jpg
vs
.../full/250,/0/default.jpg (expected)

What are your thoughts how to approach a stable fix?

  • we could amend the test, so the matching pattern passes with or without version information
  • we could amend the test, to expect version information (and fail without)
  • we could amend FileSet#original_file_id so it doesn't do any version lookup -- the test should pass as-written, and that might be more in-line with expectations?

@aploshay aploshay force-pushed the essi-2062_collection_thumbnails_in_s3 branch from 443be66 to 40113a2 Compare February 6, 2025 01:23
@aploshay aploshay requested a review from dlpierce February 6, 2025 01:35
Copy link
Contributor

@dlpierce dlpierce left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for cleaning this up!

@dlpierce dlpierce merged commit 933cc84 into main Feb 6, 2025
1 check passed
@dlpierce dlpierce deleted the essi-2062_collection_thumbnails_in_s3 branch February 6, 2025 15:30
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.

2 participants