Remediate fileset duplication in items list #371
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member_ids will no longer include the lineage_service method.
Search was broken in cases where there is no json file to search (i.e. the pdf file didn't have the json file). There may have been a change in a blacklight_iiif_search version update, which made the INVALID text that we used in this situation lock up the search. This change returns the normal search behavior.
laritakr
changed the title
We added descendent_member_ids_ssim to the index, but want member_ids_ssim as a fallback. This use was missed previously.
laritakr
commented
Sep 25, 2024
| end | ||
| # We must convert these to strings as Valkyrie's identifiers will be cast to hashes when we | ||
| # attempt to write the SolrDocument. | ||
| file_set_ids.flatten.uniq.compact.map(&:to_s) | ||
| child_ids.flatten.compact.map(&:to_s).uniq |
There was a problem hiding this comment.
.uniq was moved, because this can contain a combination of valkyrie ids and string ids before the mapping to_s and some duplications could have been missed.
ShanaLMoore
reviewed
Sep 25, 2024
| Array.wrap(all_member_ids).each do |id| | ||
| return true if file_type_and_permissions_valid?(member_presenters_for([id]).first) | ||
| end | ||
| false | ||
| end | ||
|
|
||
| def load_file_set_ids(solr_doc) |
There was a problem hiding this comment.
not blocking, but we could extract this method into a module since I think it's the same one declared in app/presenters/iiif_print/iiif_manifest_presenter_factory_decorator.rb#L25
There was a problem hiding this comment.
Agreed... I changed an existing method which was also duplicated. To save time waiting for another build now, maybe we can do it when we do the other ticket about going directly to the first page found?
ShanaLMoore
approved these changes
Sep 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Story
Refs scientist-softserv/adventist_knapsack#817
IiifPrint previously updated
member_idsto include the child work's filesets. After Valkyrization, this resulted in the inclusion of both the pages & child works in the items list. By removing the override ofmember_idsand indexingdescendent_member_ids_ssim, we now separated the inclusion of the descendent's filesets to only those situations where the behavior is desired.Additionally, searching from the parent work was broken because the PDF files do not have text extraction and were returning invalid results.
Expected Behavior Before Changes
Expected Behavior After Changes
Screenshots / Video
Before
Items List
After
Items List
Search for word that exists
Search for word that doesn't exist
Notes
Fix requires a reindex of works to redefine the indexed value of
member_ids_ssimand to indexdescendent_member_ids_ssiminstead. Prior to a reindex, the items list will continue to contain both child works and their filesets.