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

add an access_panels component for linking to the same object in other user facing systems #4357

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmartin-sul
Copy link
Member

@jmartin-sul jmartin-sul commented Aug 1, 2024

remove existing PURL link from MODS bibliographic info, since it's moving to this new panel

Closes #4312

PURL in new section, no longer appears in MODS bibliographic section, Earthworks link present also since it's released to EW:
Screenshot 2024-08-02 at 12 26 08 PM

no link to either EW or PURL, but still shows a non-PURL location in the bibliographic section:
Screenshot 2024-08-02 at 12 27 24 PM

link to PURL now appears in "Also listed in", didn't appear in bibliographic section before anyway, since that section is rendered from MARC for this object, and the MARC biblio partial doesn't show the 856 field:
Screenshot 2024-08-02 at 12 28 39 PM

# This value should always be a single hash. It's in the required JSON-LD format for a value of type DataCatalog as
# specified by schema.org, even if there is a single value. It should be safe to assume the value won't be a list (despite
# the seemingly generalizable '@type' and 'name' fields in the hash).
data_catalog_name = document.as_schema_dot_org.dig('includedInDataCatalog', 'name')
Copy link
Member

@cbeer cbeer Aug 1, 2024

Choose a reason for hiding this comment

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

Pulling this out of as_schema_dot_org seems a little indirect (and surpising) to me. Can we just use the type geo like the indexer does (and I guess store the earthworks url in Settings like we do for a bunch of other services?

https://github.com/sul-dlss/searchworks_traject_indexer/blob/main/lib/traject/config/sdr_config.rb#L351

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks!

i only went this direction because it was the suggestion on the ticket, and because i'm pretty unfamiliar with searchworks and its indexing. but your suggestion worked in my local testing, definitely feels less circuitous, and i think alleviates a spec difficulty i was running into. (i was having trouble indexing a harvested fixture with a schema_dot_org_struct using bin/rails searchworks:fixtures, and i had to submit JSON directly through the solr web UI to get a document for local testing).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. I think either using type geo or explicitly indexing release tags (if there's some reason type geo is problematic) is probably the way to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

would the latter be a searchworks_traject_indexer change? any reason to investigate that and block merging this before figuring more out? i don't really have any sense one way or the other of whether or how using type geo could cause issues.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think this actually may be problematic. There are many thousands of items with content type map (not geo) that are released to EarthWorks, and we allow quite a few other types too. There's also no guarantee that any given geo item is actually in EarthWorks. We might need to change the implementation (both here and for schema.org metadata) to look at release tags.

Copy link
Member

@thatbudakguy thatbudakguy Aug 20, 2024

Choose a reason for hiding this comment

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

@cbeer is it a bad idea to ask purl or purl-fetcher at render time about release info?

@jmartin-sul jmartin-sul force-pushed the iss4312-add-earthworks-link branch 3 times, most recently from 4d126ce to bf11ebe Compare August 2, 2024 18:08
…r user facing systems

remove existing PURL link from MODS bibliographic info, since it's moving to this new panel
@jmartin-sul jmartin-sul marked this pull request as ready for review August 2, 2024 19:06
Copy link
Member Author

@jmartin-sul jmartin-sul left a comment

Choose a reason for hiding this comment

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

left a couple questions inline for reviewers with more SW knowledge than i have.

one other question i'd had at one point was whether i needed to do any filtering in app/views/catalog/record/_marc_bibliographic.html.erb. but i was reminded when looking at the fixtures that the PURL is in the 856, and i didn't see the 856 in that partial's field list. so i think nothing to do there?

# @return [Array<String>] a tags representing PURL links
def purl_links
@purl_links ||=
if purls.size > 1 # text of the link is the actual URL if there is more than one unique URL, so there aren't multiple links with the same title
Copy link
Member Author

Choose a reason for hiding this comment

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

no idea whether accounting for this possibility is overly complicated, i.e. i have no idea whether we'd ever see multiple PURLs in practice among the MARC and MODS metadata?


# @return [Array<String>] URLs representing links to other locations, extracted from MODS metadata, e.g. ["https://purl.stanford.edu/hj948rn6493"]
def mods_purls
Array(document['url_fulltext']).find_all { |str| str.start_with?(Settings.PURL_EMBED_RESOURCE) }
Copy link
Member Author

Choose a reason for hiding this comment

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

i wasn't sure if document['url_fulltext'] was the right field to use. other possible options i saw were:

  • document.as_schema_dot_org['identifier'] (which seems to have an array of PURLs, but i ended up moving away form using as_schema_dot_org at all, so guessing we don't want to reintroduce its usage here)
  • document.mods&.location&.map { |location| location.values }&.flatten (in testing, this got me an array of a tags for the PURLs, but the text of the tag was the URL, which didn't match the mockup's language, and parsing the PURL out of the tag seemed unnecessarily indirect)

@jmartin-sul
Copy link
Member Author

implemented all your feedback, @cbeer, thanks!

@jmartin-sul
Copy link
Member Author

i just realized i didn't add a test for the PURLs being filtered out of location display in the MODS bibliographic section, though i did test for it manually. happy to add that test in either this PR, or a follow on, if that seems worth it.


# @return [Array<String>] URLs representing links to other locations, extracted from MARC metadata, e.g. ["https://purl.stanford.edu/hj948rn6493"]
def marc_managed_purls
document.marc_links.managed_purls.filter_map(&:href) # filter_map because href can be nil, at least in fixture data
Copy link
Member Author

Choose a reason for hiding this comment

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

this seemed safe to me based on looking at similar calls elsewhere in the app, but i can add more safeguards if marc_links or managed_purls might sometimes be nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

mentioning this specifically because i would love for my first searchworks PR to not cause a bunch of unexpected 500 errors 😅

@jmartin-sul jmartin-sul requested a review from cbeer August 9, 2024 17:15
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.

Add link to view items listed in Earthworks
3 participants