-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: main
Are you sure you want to change the base?
Conversation
# 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') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
4d126ce
to
bf11ebe
Compare
…r user facing systems remove existing PURL link from MODS bibliographic info, since it's moving to this new panel
bf11ebe
to
b1af387
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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) } |
There was a problem hiding this comment.
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 usingas_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 ofa
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)
implemented all your feedback, @cbeer, thanks! |
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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 😅
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:
no link to either EW or PURL, but still shows a non-PURL location in the bibliographic section:
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: