Publish update checks

[dmullen17]

Added publish_update argument checks + unit tests to verify that the resource_map_pid and metadata_pid are current. Resolve #99

[jeanetteclark]

this looks good to me @dmullen17. @amoeba do you want to have a look as well?

[amoeba]

Other checks in this function are guarded by the value fo the check_first arg. Is this one and, if not, could it be?

These two calls alone could slow down an publish_update by 2-10 seconds depending on server load.

[dmullen17]

check_first just checks if the objects exist, not if they're already obsoleted. It certainly could be put into the check_first block though.

I would argue against that (at least for the resource map check) because you can set to check_first = FALSE and using an obsoleted pid by mistake is one of our most common errors. It's harmless to use an incorrect metadata pid, but using an incorrect resource map pid will actually obsolete the metadata first before it fails, leaving the package as stranded metadata. This isn't a problem for the experienced user to fix, but new users typically make matters worse before asking for help, and we spend lots of time figuring out what happened.

In short: moving the metadata pid check into the check_first block is ok, but an incorrect resource map pid causes problems that I'd rather avoid. What do you think?

MRE:

## Incorrect metadata pid 
pkg1 <- create_dummy_package(adc_test)
pkg2 <- publish_update(adc_test, pkg1$metadata, pkg1$resource_map, pkg1$data)

# update package with incorrect metadata pid: 
pkg3 <- publish_update(adc_test, pkg1$metadata, pkg2$resource_map, pkg2$data) # this call errors but is harmless
# We can still publish successfully 
pkg3 <- publish_update(adc_test, pkg2$metadata, pkg2$resource_map, pkg2$data)


## Incorrect resource map 
pkg1 <- create_dummy_package(adc_test)
pkg2 <- publish_update(adc_test, pkg1$metadata, pkg1$resource_map, pkg1$data)

# update package with incorrect resource map pid:  
pkg3 <- publish_update(adc_test, pkg2$metadata, pkg1$resource_map, pkg2$data) 
# this call errors but obsoletes the metadata beforehand 
browseURL(paste0("https://test.arcticdata.io/#view/", pkg2$metadata))
# we also can't publish correctly now: 
pkg3 <- publish_update(adc_test, pkg2$metadata, pkg2$resource_map, pkg2$data)

[amoeba]

Excellent points! I think I could go either way on this. Always doing these two checks each time the function is run is fine w/ me if you prefer that.

[amoeba]

Can these error messages by a touch more helpful? Perhaps:

stop("The value passed in for the argument 'resource_map_pid' of '", resource_map_pid, "' is already obsoleted by a newer version with PID '", rm_obsoletedBy, "'. All PID arguments to publish_update should be the latest versions of each Object series.")

[dmullen17]

love this error message!

[amoeba]

nice and tidy test!

[amoeba]

Looks good! I didn't look at the full source code, just the patch: Are these checks guarded behind the value of the check_first arg so a user can opt not to run them? Otherwise looks good, sans my minor comments.