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

Adds info:islandora/islandora-system:def/scholar# predicates to both … #5

Closed
wants to merge 1 commit into from

Conversation

bryjbrown
Copy link
Member

JIRA Ticket: https://jira.duraspace.org/browse/ISLANDORA-1727

What does this Pull Request do?

This PR adds the 'embargo-until' and 'embargo-expiry-notification-date' predicates from 'info:islandora/islandora-system:def/scholar#' to the relsext.rdf and relsint.rdf ontologies. Both are predicates from the Islandora Scholar Embargoes module.

Additional Notes

Someone should probably review this to make sure its syntactically/semantically correct.

Interested parties

@ruebot @DiegoPino

@ruebot
Copy link
Member

ruebot commented Jun 7, 2016

@bryjbrown do you have a sample RELS-EXT or RELS-INT I can see? I want see how they're actually implemented in there.

@ruebot ruebot self-assigned this Jun 7, 2016
@DiegoPino
Copy link
Contributor

I second that request @ruebot, if possible @bryjbrown a few samples with different usage of those predicates we can look at? thanks 👍

@bryjbrown
Copy link
Member Author

@ruebot @DiegoPino Sure, no problem.

So some context. Embargoes can either be on the object itself (in which case it shows up in RELS-EXT) or on a datastream (in which case it shows up in the RELS-INT).

RELS-EXT of an indefinitely embargoed object:

<rdf:RDF xmlns:fedora="info:fedora/fedora-system:def/relations-external#" xmlns:fedora-model="info:fedora/fedora-system:def/model#" xmlns:islandora="http://islandora.ca/ontology/relsext#" xmlns:islandora-embargo="info:islandora/islandora-system:def/scholar#" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
  <rdf:Description rdf:about="info:fedora/fsu:210502">                             
    <fedora-model:hasModel rdf:resource="info:fedora/ir:thesisCModel"></fedora-model:hasModel>
    <islandora:inheritXacmlFrom rdf:resource="info:fedora/fsu:research_repository_tmp"></islandora:inheritXacmlFrom>
    <fedora:isMemberOfCollection rdf:resource="info:fedora/fsu:honors_theses"></fedora:isMemberOfCollection>
    <islandora-embargo:embargo-until>indefinite</islandora-embargo:embargo-until>
    <islandora:isViewableByUser>drupal-admin</islandora:isViewableByUser>          
    <islandora:isViewableByUser>Bryan Brown</islandora:isViewableByUser>           
    <islandora:isViewableByRole>administrator</islandora:isViewableByRole>         
    <islandora:isViewableByRole>fsu_embargo_admin</islandora:isViewableByRole>  
  </rdf:Description>                                                               
</rdf:RDF>  

RELS-INT of an object with a temporarily embargoed PDF datastream:

<rdf:RDF xmlns:islandora="http://islandora.ca/ontology/relsint#" xmlns:islandora-embargo="info:islandora/islandora-system:def/scholar#" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
  <rdf:Description rdf:about="info:fedora/fsu:182710/PDF">                         
    <islandora-embargo:embargo-until rdf:datatype="http://www.w3.org/2001/XMLSchema#dateTime">2020-01-22T05:00:00Z</islandora-embargo:embargo-until>
    <islandora-embargo:embargo-expiry-notification-date rdf:datatype="http://www.w3.org/2001/XMLSchema#dateTime">2020-01-12T05:00:00Z</islandora-embargo:embargo-expiry-notification-date>
    <islandora:isViewableByUser>drupal-admin</islandora:isViewableByUser>          
    <islandora:isViewableByUser>Bryan Brown</islandora:isViewableByUser>           
    <islandora:isViewableByRole>administrator</islandora:isViewableByRole>         
    <islandora:isViewableByRole>fsu_embargo_admin</islandora:isViewableByRole>  
  </rdf:Description>                                                               
</rdf:RDF>   

RELS-EXT of an object with a temporarily embargoed PDF datastream:
(nothing out of the ordinary, just included for completeness)

<rdf:RDF xmlns:fedora="info:fedora/fedora-system:def/relations-external#" xmlns:fedora-model="info:fedora/fedora-system:def/model#" xmlns:islandora="http://islandora.ca/ontology/
relsext#" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">             
  <rdf:Description rdf:about="info:fedora/fsu:182710">                          
    <fedora:isMemberOfCollection rdf:resource="info:fedora/fsu:bepress_modern_etds"></fedora:isMemberOfCollection>
    <fedora-model:hasModel rdf:resource="info:fedora/ir:thesisCModel"></fedora-model:hasModel>
    <islandora:inheritXacmlFrom rdf:resource="info:fedora/fsu:bepress_modern_etds"></islandora:inheritXacmlFrom>
  </rdf:Description>                                                               
</rdf:RDF>  

@@ -168,4 +168,19 @@
<rdf:Property rdf:about="http://islandora.ca/ontology/relsext#isSequenceNumberOf$escaped_pid">
<rdfs:label xml:lang="en">is sequence number of escaped PID</rdfs:label>
</rdf:Property>

<!-- info:islandora/islandora-system:def/scholar#embargo-until -->
Copy link
Member

Choose a reason for hiding this comment

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

<!-- http://islandora.ca/ontology/relsext#embargo-until -->

Copy link
Member

Choose a reason for hiding this comment

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

@DiegoPino I'm concerned about the rdf:Property being having the other namespace info:islandora/islandora-system:def/scholar#

Can we have both in here? Is that legal?

We'll also need to figure out how to compensate for it in rdfs2html.xsl. If I do a build now on @bryjbrown's fork, they don't show up.

@DiegoPino
Copy link
Contributor

Ok, just a question(don't hate me), but how big a change would be to normalise this namespace stuff.
basically the predicates involved are not in the islandora ontology namespace at all. islandora-embargo is not defined as being part of REL-EXT nor RELS-INT ontologies, it has this fictional namespace which if used, should be here: http://info-uri.info/registry/OAIHandler?verb=ListRecords&metadataPrefix=oai_dc

Fedora has it right in that registry.

Sadly this applies to everywhere in islandora 😢
So the problem is that even when you define this predicates as part of the ontology, they are not used here as part of the ontology, they reference the non existing info namespace.

@ruebot
Copy link
Member

ruebot commented Jun 7, 2016

So the problem is that even when you define this predicates as part of the ontology, they are not used here as part of the ontology, they reference the non existing info namespace.

That's exactly what I'm struggling with.

Don't worry @bryjbrown, this isn't an issue with your pull request. It's a larger issue with the design and implementation of Islandora Scholar in the larger Islandora ecosystem.

@bryjbrown
Copy link
Member Author

bryjbrown commented Jun 7, 2016

As @ruebot pointed on on IRC earlier, the reason for implementing these predicates as part of a different ontology can be found in the embargo module itself

// Doesn't make sense to use either of the Islandora RELS-EXT or RELS-INT
// namespaces, as our predicates can occur in both.
define('ISLANDORA_SCHOLAR_EMBARGO_RELS_URI', 'info:islandora/islandora-system:def/scholar#');
define('ISLANDORA_SCHOLAR_EMBARGO_NOTIFICATION_PRED', 'embargo-expiry-notification-date');
define('ISLANDORA_SCHOLAR_EMBARGO_EXPIRY_PRED', 'embargo-until');
define('ISLANDORA_SCHOLAR_EMBARGO_CAN_EMBARGO_OWNED', 'can embargo owned objects');
define('ISLANDORA_SCHOLAR_EMBARGO_CAN_EMBARGO_ANY', 'can embargo any object');

So there's a reason, but obviously the solution of creating another ontology is problematic. Is this more problematic than just having the predicates exist in both ontologies though?

If thats the route we want to take, then I could create a pull against Scholar to update the URI and create duplicate predicate constants (one for RELS-EXT, one for RELS-INT) and then change the embargo application code to put the correct one on depending on whether its an object or datastream embargo.

The only problem with this would be the obsolete metadata in currently existing embargoes, but the recent PR to fix the way embargoes are implemented in Scholar includes a batch process to reapply embargoes to all currently embargoed objects. Perhaps that batch process could wipe away the old URIs and apply the new ones?

@ruebot
Copy link
Member

ruebot commented Jun 7, 2016

@qadan do you have any thoughts on this given Islandora/islandora_scholar#232?

@ruebot
Copy link
Member

ruebot commented Jun 7, 2016

...and I'm not sure the rationale makes sense in the code we highlighted, because we have some predicates that are in both.

@ruebot
Copy link
Member

ruebot commented Jun 16, 2016

@qadan bump

@qadan
Copy link

qadan commented Jun 16, 2016

Whoop, missed this, sorry.

I think the trouble we would run into is that in order for the batch script made in #232 to find objects to update, it iterates over them via a SPARQL query, and that SPARQL query uses the old URI to find them. We couldn't quite just ask people to run that script outright; we'd have to include a modified version of it in an update hook.

I don't quite have strong feelings on which way to go with this; typically I'd be on the side of not changing what currently works, but the nitty-gritty of RDF is somewhat outside of my wheelhouse, so I would defer to the experts on it.

@ruebot
Copy link
Member

ruebot commented Jun 16, 2016

@qadan so, since these predicates are written to the RELS-EXT or RELS-INT datastream, we will need get this URI updated, as well as the namespace because right now, these two predicates are kinda off in their own little world. Is that solved by the drush script you are hesitant on?

@qadan
Copy link

qadan commented Jun 16, 2016

@ruebot As I see it, we'd have to:

  • Modify existing queries that look for embargoes to search in both the RELS-EXT and RELS-INT
  • Update the embargo applicator so that it assigns the correct predicate based on whether what's being updated is a datastream or a full object
  • Create an update hook that searches for embargoes using the old info:islandora predicate and runs the updated embargo applicator on them

That's at a cursory glance; some more digging could be appropriate to determine whether or not there's anything else in there that'll get borked.

@ruebot
Copy link
Member

ruebot commented Jun 16, 2016

@bryjbrown how's that sound? @qadan can you start crafting a JIRA ticket?

@qadan
Copy link

qadan commented Jun 16, 2016

@ruebot sure; I'll reference it in https://jira.duraspace.org/browse/ISLANDORA-1727 since we'd be creating a ticket dependency

@ruebot
Copy link
Member

ruebot commented Jun 16, 2016

@qadan thank you!!

@qadan
Copy link

qadan commented Jun 16, 2016

https://jira.duraspace.org/browse/ISLANDORA-1738 to address the above-noted issue

@bryjbrown
Copy link
Member Author

@ruebot The ticket and steps for moving forward mentioned by @qadan look good to me.

Since the embargo batch applicator is @qadan's baby, he get's first dibs on creating a PR for this. I'd happily collaborate and/or implement the fixes myself, too. Since Scholar Embargoes is still fully functional in its current state even with the wrong URIs, I'm assuming this is relatively low priority, right?

@DonRichards
Copy link
Member

Was there a ticket create for @qadan to look into modifying the embargo batch applicator? I know it's low priority, just following up.

@DiegoPino
Copy link
Contributor

@bryjbrown do you remember what the last known status of this is? Trying to clean open pulls so doing a general checkup on frozen threads. Thanks!

@bryjbrown
Copy link
Member Author

@DiegoPino @DonRichards I think that the final status of this was that the Scholar embargo predicates as implemented are incorrect, and we have ISLANDORA-1738 advising we fix them. You can delete this PR and I'll create another one at a later date with the updated predicates.

@DiegoPino
Copy link
Contributor

@bryjbrown thanks, you are great! Yeah, reading that one now, It does not seem to be "minor" but a good way to go. Feel free to close this pull if you feel so. 👍

@bryjbrown bryjbrown closed this Feb 27, 2017
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.

5 participants