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

8290 harvest client api #9174

Merged
merged 19 commits into from
Dec 1, 2022
Merged

8290 harvest client api #9174

merged 19 commits into from
Dec 1, 2022

Conversation

landreev
Copy link
Contributor

@landreev landreev commented Nov 18, 2022

What this PR does / why we need it:

Our API for creating harvesting clients was an afterthought, we never got it fully to work, and never advertised or documented it. However a user discovered it and reported that it wasn't working. We will need this API to create functional restassured tests for harvesting clients (there's a dedicated issue that's been prioritized recently).

This PR provides a working API for creating a new harvesting client (with an -X POST method) and documents it in the guide. The documentation part of the PR should thus close the issue #8267 as well.

Which issue(s) this PR closes:

Closes #8290
Closes #8267

Special notes for your reviewer:

I'm not adding restassured tests for this API in this PR, because there's a dedicated issue for harvesting tests that's also been prioritized.

Suggestions on how to test this:

It should be possible to create a new client, with an -X POST method, as documented. The List, Edit and Delete, not specifically mentioned in the issue, should also be working as documented.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

@coveralls
Copy link

coveralls commented Nov 18, 2022

Coverage Status

Coverage decreased (-0.02%) to 20.003% when pulling 2710739 on 8290-harvest-client-api into bf2e426 on develop.

@pdurbin pdurbin self-assigned this Nov 22, 2022
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Overall this looks good but please take a look at my comments, @landreev. Happy to discuss. Thanks.

doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved

- archiveDescription: What the name suggests. If not supplied, will default to "This Dataset is harvested from our partners. Clicking the link will take you directly to the archival source of the data."
- set: The OAI set on the remote server. If not supplied, will default to none, i.e., "harvest everything".
- schedule: Harvesting schedule. Defaults to "none".
Copy link
Member

Choose a reason for hiding this comment

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

Can we have an example of a schedule? What does it look like?

On a related note, I see this comment in the code:

// TODO: Make schedule configurable via this API too.

Perhaps this means that schedule cannot be configured via API? If so, we should say so. Or if schedule can be set on create but not on modify we could document that under "Modify a Harvesting Client".

doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
@landreev
Copy link
Contributor Author

(I have added some simple restassured tests, as discussed)

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Looks good. Approving. I did notice that the last two Jenkins run failed so I pushed a tiny commit to kick off another run.

@kcondon kcondon self-assigned this Nov 30, 2022
@kcondon
Copy link
Contributor

kcondon commented Nov 30, 2022

Issues found:

  1. Can create a harvest client so long as the user has admin perms on the target dataverse collection, doc says super user only
    [Kevin] Correction: doc says super user only for delete, just assumed same for create since that is how it is in UI.
  2. Same with modify client, just need to be admin of target dv collection, not super user.
    Otherwise, pr looks good.

@mreekie
Copy link

mreekie commented Dec 1, 2022

Kevin is satisfied with everthing, but uncovered a missed use case.
This will go back to WIP and back to testing.
The amount of work does not look to be big.

@mreekie mreekie added the Size: 3 A percentage of a sprint. 2.1 hours. label Dec 1, 2022
@landreev landreev removed their assignment Dec 1, 2022
@kcondon kcondon merged commit bc02f37 into develop Dec 1, 2022
@kcondon kcondon deleted the 8290-harvest-client-api branch December 1, 2022 19:46
@pdurbin pdurbin added this to the 5.13 milestone Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: No status
5 participants