Skip to content

Bug fix for multiple IIIF image property#1847

Merged
tdonohue merged 4 commits intoDSpace:mainfrom
mspalti:iiif-multi-fix
Sep 28, 2022
Merged

Bug fix for multiple IIIF image property#1847
tdonohue merged 4 commits intoDSpace:mainfrom
mspalti:iiif-multi-fix

Conversation

@mspalti
Copy link
Copy Markdown
Member

@mspalti mspalti commented Sep 21, 2022

Description

Fixes a bug in mirador-viewer.component that incorrectly sets the multi property when images are added to bundles other than ORIGINAL.

The PR modifies the Angular UI to look for images in ALL eligible Bundles, not just ORIGINAL.

Some quick background: the multi property is set by the mirador-viewer.component component and is passed to the Mirador viewer, which in turn uses the property to create internal configuration at startup. (The default Mirador behavior is to add right thumbnail navigation when multi is 'true.') multi should be true whenever the Item has multiple image bitstreams.

Sorry! I just realized I didn't submit this bug fix earlier!! It's a fairly minor fix so if it can make it into 7.4 that would be great.

Instructions for Reviewers

  • Create an IIIF-enabled Item with more than one image bitstream. Upload the images into a custom bundle (e.g. TestBundle).
  • View the Item and check to be sure the images are found in the custom bundle by checking for right navigation thumbnail images in the Mirador viewer (see below). Remember that (unfortunately) the viewer can only be tested in production mode.

Screen Shot 2022-09-21 at 1 00 26 PM

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@mspalti mspalti changed the title Bug fix for multiple image property Bug fix for multiple IIIF image property Sep 21, 2022
@tdonohue tdonohue added bug integration: IIIF Related to International Image Interoperability Framework (IIIF) support 1 APPROVAL pull request only requires a single approval to merge labels Sep 21, 2022
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@mspalti : I tried testing this today & I cannot get it to work as described.

  1. I setup an Item with an existing image in the ORIGINAL bundle. Verified it loads in Mirador & it does
  2. Then, I edited that Item, created a new bundle named "Test" and added a new image to it.
  3. Went back to the Item page & looked in Mirador. It still only displays the single image in the ORIGINAL bundle.
  4. Reloaded page in browser (in case there's caching). Still only displays the single image in the ORIGINAL bundle.
  5. Then tried to add a new image to the ORIGINAL bundle. It also doesn't show up in Mirador.

Is there some sort of caching here that I need to refresh? I cannot figure out how to get this to work at all.

UPDATE: Does this maybe require DSpace/DSpace#8402 ??

@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Sep 27, 2022

@tdonohue I will take a look. It could be a bug in the implementation. I think it should have worked given what you tried. There's no relationship to DSpace/DSpace#8402

@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Sep 28, 2022

@tdonohue , this should be working now. You 'll need to reload to clear the cache after adding the image to your Test bundle. Sorry I didn't catch this earlier. Use of switchMap was blocking evaluation of all bundles and it just happened that my tests didn't catch it.

@tdonohue tdonohue self-requested a review September 28, 2022 16:41
Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

thanks @mspalti

I made the following test :

  • tested successful with an item with bitstreams in the ORIGINAL bundle and in a custom one
  • tested successful with an item with bitstream with only a custom bundle
  • test failed with an item with an empty bundle ORIGINAL and bitstreams in a custom one. I found out this fails due to an issue on the rest side, indeed the request to the manifest fail with 500 error. we'll open an issue on rest side

@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Sep 28, 2022

Thanks @atarix83 , I see the problem on the backend! The problem should be addressed in DSpace/DSpace#8402 which currently is under review.

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @mspalti ! I've retested and verified everything is working perfectly now.

@tdonohue tdonohue added this to the 7.4 milestone Sep 28, 2022
@tdonohue tdonohue merged commit c7029ed into DSpace:main Sep 28, 2022
@mspalti mspalti deleted the iiif-multi-fix branch September 29, 2022 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug integration: IIIF Related to International Image Interoperability Framework (IIIF) support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants