Remove automatic adding of EML otherEntity elements

I took this out because the data team no longer needs otherEntity
elements automatically added to the metadata because they are
hand-editing the metadata for other reasons which makes the metadata
non-editable in the registry which is the main reason we added this in
the first place.

I also factored out the creation of the physical sub-tree

Author: amoeba

NAMESPACE

@@ -67,7 +67,7 @@ export(new_uuid)
 export(object_exists)
 export(parse_resource_map)
 export(path_join)
-export(pid_to_entity)
+export(pid_to_other_entity)
 export(publish_object)
 export(publish_update)
 export(remove_public_read)
@@ -81,7 +81,8 @@ export(set_rights_and_access)
 export(set_rights_holder)
 export(show_random_dataset)
 export(substitute_eml_party)
-export(sysmeta_to_entity)
+export(sysmeta_to_eml_physical)
+export(sysmeta_to_other_entity)
 export(theme_packages)
 export(update_object)
 export(update_package)

R/editing.R

@@ -182,16 +182,16 @@ update_object <- function(mn, pid, path, format_id=NULL, new_pid=NULL, sid=NULL)
 #' @param identifier (character) Manually specify the identifier for the new metadata object.
 #' @param use_doi (logical) Generate and use a DOI as the identifier for the updated metadata object.
 #' @param parent_resmap_pid  (character)  Optional. PID of a parent package to be updated.
-#' @param parent_metadata_pid (character)  Optional. identifier for the metadata document of the parent package.
+#' @param parent_metadata_pid (character)  Optional. Identifier for the metadata document of the parent package.
+#' @param parent_data_pids (character)  Optional. Identifier for the data objects of the parent package.
 #' @param parent_child_pids (character) Optional. Resource map identifier(s) of child packages in the parent package.
 #' @param child_pids (character) Optional. Child packages resource map PIDs.
 #' @param metadata_path (character) Optional. Path to a metadata file to update with. If this is not set, the existing metadata document will be used.
 #' @param public (logical) Optional. Make the update public. If FALSE, will set the metadata and resource map to private (but not the data objects).
 #' This applies to the new metadata PID and its resource map and data object.
 #' access policies are not affected.
-#' @param check_first (logical) Optional. Whether to check the PIDs passed in as aruments exist on the MN before continuing. This speeds up the function, especially when `data_pids` has many elements.
+#' @param check_first (logical) Optional. Whether to check the PIDs passed in as aruments exist on the MN before continuing. Checks that objects exist and are of the right format type. This speeds up the function, especially when `data_pids` has many elements.
 #' @param parent_data_pids
-#' @param skip_other_entities (logical) Default FALSE Whether to create all EML otherEntity elements.
 #'
 #' @import dataone
 #' @import datapack
@@ -211,8 +211,7 @@ publish_update <- function(mn,
                            parent_data_pids=NULL,
                            parent_child_pids=NULL,
                            public=TRUE,
-                           check_first=TRUE,
-                           skip_other_entities=FALSE) {
+                           check_first=TRUE) {
 
   # Don't allow setting a dataset to private when it uses a DOI
   if (use_doi && !public) {
@@ -279,17 +278,6 @@ publish_update <- function(mn,
 
   # get the metadata sysmeta from the node
   metadata_sysmeta <- dataone::getSystemMetadata(mn, metadata_pid)
-  #eml_acl <- sysmeta_orig@accessPolicy
-  # TODO: error check: md and sm existence
-
-  # get the resource_map (not used for now, could be used to get the list of data pids)
-  # resmap <- rawToChar(dataone::getObject(mn, resource_map_pid))
-  # resmap_sysmeta <- dataone::getSystemMetadata(mn, resource_map_pid)
-  # TODO: error check: resmap existence, and ensure we don't fail hard
-
-  # Get the list of data files from the resource map
-  # TODO: parse these from the resource map, rather than taking them as input
-  # TODO: data_pids <- as.vector(c("pid1", "pid2", "pid3"))
 
   log_message("Downloaded EML and sysmeta...")
 
@@ -311,7 +299,7 @@ publish_update <- function(mn,
   # Generate a resource map PID from the new metadata PID
   resmap_updated_pid <- paste0("resource_map_",  metadata_updated_pid)
 
-  # Update the metadata object
+  # Update the metadata
 
   # Replace packageId
   eml@packageId <- new("xml_attribute", metadata_updated_pid)
@@ -326,15 +314,6 @@ publish_update <- function(mn,
   eml_path <- tempfile()
   EML::write_eml(eml, eml_path)
 
-  # Add other entity fields (if appropriate)
-  if (!is.null(data_pids) && !skip_other_entities) {
-    eml <- set_other_entities(mn, eml_path, data_pids)
-  }
-
-  if (skip_other_entities) {
-    log_message("Skipping modifying EML otherEntity elements. This may result a data package that is not editable using the registry!")
-  }
-
   # Create System Metadata for the updated EML file
   metadata_updated_sysmeta <- new("SystemMetadata",
                                   identifier = metadata_updated_pid,

R/eml.R

@@ -5,7 +5,7 @@
 
 #' Create an EML otherEntity sub-tree for the given PID.
 #'
-#' Note this is a wrapper around sysmeta_to_entity which handles the task of
+#' Note this is a wrapper around sysmeta_to_other_entity which handles the task of
 #' creating the EML otherEntity sub-ree.
 #'
 #' @param mn (MNode) Member Node where the PID is associated with an object.
@@ -16,7 +16,7 @@
 #' @export
 #'
 #' @examples
-pid_to_entity <- function(mn, pid, sysmeta=NULL) {
+pid_to_other_entity <- function(mn, pid, sysmeta=NULL) {
   stopifnot(class(mn) == "MNode")
   stopifnot(is.character(pid),
             nchar(pid) > 0)
@@ -35,18 +35,19 @@ pid_to_entity <- function(mn, pid, sysmeta=NULL) {
     stop(paste0("System Metadata for object with PID '", pid, "' did not have its fileName property set. This will result in 'NA' being set for the EML entityName and objectName (which we don't want). You need to give each data object a fileName property in its System Metadata. You can use the arcticdatautils::set_file_name() function to do this or you can use dataone::getSystemMetadata(), change the fileName property, and update it with dataone::updateSystemMetadata()"))
   }
 
-  sysmeta_to_entity(sysmeta)
+  sysmeta_to_other_entity(mn, sysmeta)
 }
 
 #' Create an EML otherEntity sub-tree for the given object.
 #'
-#' @param sysmeta (SystemMetadata) A SystemMedata instance for the object.
+#' @param mn (MemberNode) The Member Node the object lives on.
+#' @param sysmeta (SystemMetadata) The System Metadata of the object.
 #'
 #' @return (otherEntity) The XML sub-tree.
 #' @export
 #'
 #' @examples
-sysmeta_to_entity <- function(sysmeta) {
+sysmeta_to_other_entity <- function(mn, sysmeta) {
   stopifnot(class(sysmeta) == "SystemMetadata")
 
   # otherEntity
@@ -64,6 +65,28 @@ sysmeta_to_entity <- function(sysmeta) {
   other_entity@entityType <- "Other"
 
   # otherEntity/physical
+  phys <- sysmeta_to_eml_physical(mn, sysmeta)
+  other_entity@physical <- new("ListOfphysical", list(phys))
+
+  other_entity
+}
+
+#' Create an EML physical subtree from a System Metadata instance
+#'
+#' This function creates a pre-canned EML physical subtree from what's in the
+#' System Metadata of an Object. Note that it sets an Online Distrubtion URL
+#' of the DataONE v2 resolve service for the PID.
+#'
+#' @param mn (MemberNode) The Member Node the object lives on.
+#' @param sysmeta (SystemMetadata) The System Metadata of the object.
+#'
+#' @return
+#' @export
+#'
+#' @examples
+sysmeta_to_eml_physical <- function(sysmeta) {
+  stopifnot(class(sysmeta) == "SystemMetadata")
+
   phys <- new("physical")
   phys@scope <- new("xml_attribute", "document")
 
@@ -84,16 +107,13 @@ sysmeta_to_entity <- function(sysmeta) {
   phys@distribution <- new("ListOfdistribution", list(new("distribution")))
   phys@distribution[[1]]@scope  <- new("xml_attribute", "document")
   phys@distribution[[1]]@online <- new("online")
-  phys@distribution[[1]]@online@url <- new("url", paste0("ecogrid://knb/", get_doc_id(sysmeta)))
+  phys@distribution[[1]]@online@url <- new("url", paste0("https://cn.dataone.org/cn/v2/resolve/", sysmeta@identifier))
 
   slot(phys@distribution[[1]]@online@url, "function") <- new("xml_attribute", "download")
 
-  other_entity@physical <- new("ListOfphysical", list(phys))
-
-  other_entity
+  phys
 }
 
-
 #' Creates and sets EML otherEntity elements to an existing EML document.
 #'
 #' This function isn't that smart. It will remove existing otherEntity elements
@@ -137,7 +157,7 @@ set_other_entities <- function(mn, path, pids) {
 
   # Generate otherEntity elements for any new otherEntity elements that weren't
   # already in the EML
-  new_entities <- lapply(pids[!(pids %in% current_entity_pids)], function(pid) pid_to_entity(mn, pid))
+  new_entities <- lapply(pids[!(pids %in% current_entity_pids)], function(pid) pid_to_other_entity(mn, pid))
 
   # Concatenate the existing and new otherEntity elements and put back in the
   # EML

R/helpers.R

@@ -19,11 +19,6 @@ create_dummy_metadata <- function(mn, data_pids=NULL) {
   metadata_file <- tempfile()
   file.copy(original_file, metadata_file)
 
-  # Add otherEntity elements if needed
-  if (!is.null(data_pids)) {
-    metadata_file <- set_other_entities(mn, metadata_file, data_pids)
-  }
-
   sysmeta <- new("SystemMetadata",
                  id = pid,
                  formatId = "eml://ecoinformatics.org/eml-2.1.1",

man/pid_to_entity.Rd man/pid_to_other_entity.Rdman/pid_to_entity.Rd renamed to man/pid_to_other_entity.Rd

@@ -77,48 +77,6 @@ test_that("we can update a resource map", {
   expect_equal(updated, get_package(mn, response$metadata)$resource_map)
 })
 
-test_that("otherEntity elements are set when publishing an update", {
-  if (!is_token_set(mn)) {
-    skip("No token set. Skipping test.")
-  }
-
-  # Setup
-  library(dataone)
-  library(EML)
-  library(stringr)
-
-  # Create an initial package
-  response <- create_dummy_package(mn)
-
-  # Create a new dummy object
-  tmp <- tempfile()
-  dummy_df <- data.frame(x = 1:100, y = 1:100)
-  write.csv(dummy_df, file = tmp)
-  object <- publish_object(mn, tmp, "text/csv")
-  file.remove(tmp) # Remove dummy data file
-
-  # Note I use the wrong data pid argument here. I send the new data pid instead
-  # which lets me update the data in a package when doing a publish_update()
-  # call
-  update <- publish_update(mn,
-                           response$metadata,
-                           response$resource_map,
-                           object)
-
-
-  tmp <- tempfile()
-  writeLines(rawToChar(getObject(mn, update$metadata)), con = tmp)
-  doc <- read_eml(tmp)
-
-  expect_true(length(doc@dataset@otherEntity) == 1)
-
-  sysmeta <- getSystemMetadata(mn, object)
-  doc_id <- get_doc_id(sysmeta)
-  expect_true(str_detect(doc@dataset@otherEntity[[1]]@physical[[1]]@distribution[[1]]@online@url@.Data, doc_id))
-
-  file.remove(tmp)
-})
-
 test_that("an object can be published with a SID", {
   if (!is_token_set(mn)) {
     skip("No token set. Skipping test.")
@@ -274,6 +232,10 @@ test_that("replication policies are honored when updating packages", {
 
 
 test_that("extra statements are maintained between updates", {
+  if (!is_token_set(mn)) {
+    skip("No token set. Skipping test.")
+  }
+
   pkg <- create_dummy_package(mn, 3)
 
   # Add some PROV triples to the Resource Map

man/publish_update.Rd

@@ -8,7 +8,7 @@ test_that("an EML otherEntity subtree can be created when the sysmeta has a file
   sysmeta <- new("SystemMetadata")
   sysmeta <- datapack::parseSystemMetadata(sysmeta, XML::xmlRoot(doc))
 
-  other_entity <- sysmeta_to_entity(sysmeta)
+  other_entity <- sysmeta_to_other_entity(sysmeta)
 
   # Check some rough properties of the subtree
   expect_is(other_entity, "otherEntity")
@@ -24,7 +24,7 @@ test_that("an EML otherEntity subtree can be created when the sysmeta doesn't ha
   sysmeta <- new("SystemMetadata")
   sysmeta <- datapack::parseSystemMetadata(sysmeta, XML::xmlRoot(doc))
 
-  other_entity <- sysmeta_to_entity(sysmeta)
+  other_entity <- sysmeta_to_other_entity(sysmeta)
 
   # Check some rough properties of the subtree
   expect_is(other_entity, "otherEntity")