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

8210 importddi fix #8483

Merged
merged 10 commits into from
Mar 16, 2022
Merged

8210 importddi fix #8483

merged 10 commits into from
Mar 16, 2022

Conversation

lubitchv
Copy link
Contributor

@lubitchv lubitchv commented Mar 14, 2022

What this PR does / why we need it:
importddi is broken because ddi xml does not have corresponding subject field. The fix is by filling Subject field with "N/A" similar to Sword Api.

Which issue(s) this PR closes:

Suggestions on how to test this:
testImportDDI was added to DataversesIT.
One can also test by using curl:
curl -H X-Dataverse-key:$API_TOKEN -X POST $SERVER_URL/api/dataverses/$DATAVERSE_ID/datasets/:importddi --upload-file ddi_dataset.xml
Sample xml file is in doc/sphinx-guides/source/_static/api/ddi_dataset.xml

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

Is there a release notes update needed for this change?:
Release notes update is added to doc/release-notes/8210-importddi-fix.md

Additional documentation:
Documentation added to Native Api guide

@coveralls
Copy link

coveralls commented Mar 14, 2022

Coverage Status

Coverage increased (+0.0003%) to 18.864% when pulling f23809d on lubitchv:8210-importddi-fix into dbca4ab on IQSS:develop.

@pdurbin pdurbin self-assigned this Mar 15, 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 great.

I did suggest a doc change, so please take a look.

The test is failing but it's our fault. Please hold on until we resolve #8489.

@pdurbin
Copy link
Member

pdurbin commented Mar 15, 2022

@lubitchv @landreev @qqmyers @scolapasta we've been discussing in IQSS and community Slack but not all at once. Let me try to get us all on the same page.

@landreev made pull request #8491 to address the collectionMode change that affects this pull request about DDI import. As he notes in the description, it's hard to test it since DDI import is broken. So, I went ahead and merged it into this pull request. The new testImportDDI test runs fine on my laptop. I'll wait until Jenkins is happy with it before moving this pull request to QA. Obviously, if anyone else wants to do further review of this pull request, please go ahead.

Meanwhile, @qqmyers also has a change for scripts/api/data/dataset-create-new-all-default-fields.json in pull request #8493. I think we should merge this pull request (#8483) first and then merge develop into his.

As for this pull request, I think I'm done making changes. Thank you @lubitchv for committing my doc suggestion. That last thing I haven't mentioned is that I removed one of the test XML files since they're identical.

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.

The new "import DDI" test just passed on Jenkins. Moving to QA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment