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

fix getDepositionByConceptDOI method #59

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Conversation

mpadge
Copy link
Contributor

@mpadge mpadge commented Oct 11, 2021

@eblondel The current code has the following lines:

zen4R/R/ZenodoManager.R

Lines 593 to 597 in 969053a

getDepositionByConceptDOI = function(conceptdoi){
query <- sprintf("conceptdoi:%s", gsub("/", "//", conceptdoi))
result <- self$getDepositions(q = query, exact = TRUE)
if(length(result)>0){
result <- result[[1]]

which presumes that the requested conceptdoi is the first element of the returned result, yet this is not necessarily so. In particular, personal releases associated with a token are delivered in chronological order. This PR rectifies by matching conceptdoi values of all returned results to find the desired one.

@eblondel eblondel merged commit a774850 into eblondel:master Oct 11, 2021
@eblondel
Copy link
Owner

Thanks @mpadge
PS: tests might fail as they rely on sandbox and given the nb of requests, my sandbox account gets blocked at some point which makes fail tests. I still have to find a workaround or reduce nb of tests.

@maelle
Copy link

maelle commented Oct 11, 2021

@eblondel might I suggest https://books.ropensci.org/http-testing/ 😁

@eblondel
Copy link
Owner

for the timebeing it's fine, it depends on the nb of requests per hour/day, as you can see tests passed. I know that mocks should be an alternate, it was also suggested by Zenodo team while interacting with them on the matter... but I didn't have time to work on it, as i'm busy on other deliverables in production.

@eblondel
Copy link
Owner

hum, no it failed as well. I will try to find some time for mocking, meanwhile i'm also interacting with Zenodo to find a solution, as i'd like to have real integration tests as well based on the sandbox.

@mpadge
Copy link
Contributor Author

mpadge commented Oct 11, 2021

Great, thanks @eblondel. This definitely should not affect code, as "old" behaviour will either correctly be restored for any and all calls which associate a token with a single release; otherwise it will fix any for this this might have previously failed. No idea how you could even test any of that, and I don't envy your position of having to figure that out. I merely remain grateful for the package!

@eblondel
Copy link
Owner

@mpadge you're welcome! FYI, I will push soon a revision to CRAN (in case you have few additional ajustements for zen4R to submit). I still have to perform further testing through integrated tool geoflow for Zenodo records versioning support, afterwhat it will go on CRAN.
Best

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.

3 participants