-
Notifications
You must be signed in to change notification settings - Fork 492
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
make Alternative Title repeatable #9440
Conversation
Update from IQSS
Update from develop
I do not have access to continuous integration testing server. Please let me know which integration tests are failing. |
@lubitchv yeah, it would be very nice to give this insight on the outside somehow. Here are some screenshots at least! From https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9440/2/testReport/ |
The testImportDDI had error for productionPlace field (there was problem there with multiple). It was not related to this PR, but I fixed it. testArchivalStatusAPI and testLoadMetadataBlock_NoErrorPath I also fixed. But it seems that some integration tests are still failing and I do not know which. |
@lubitchv this time ( https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9440/5/ ) it wasn't API tests that failed. No API tests were run at all. I just clicked "re-run". |
API tests are now running fine. |
@lubitchv hi! Can you please resolve the merge conflicts in the PR? There were probably caused by this PR being merged yesterday: |
@pdurbin I updated the branch from develop |
@pdurbin It says that there is 1 errored check, but I cannot see it, or I do not have permissions to see it. |
@lubitchv hi! Sorry for the noise. The failure in Jenkins is unrelated to your changes. It's this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some quick feedback and a few questions:
Is it valid in DDI to have more than one alternate title?
This pull request was recently merged and now Dataverse produces valid DDI (as far as I know):
I see there's a test in that PR that calls isValid
. Does that test cover changes in this PR? If not, can we please have a test that shows that the DDI is still valid?
"value": "Alternative Title" | ||
"value": ["Alternative Title"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like API users will encounter backward incompatibility. Is that right? If so, can this please be added to the release note snippet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, repeatable altTitl is valid DDI. See https://ddialliance.org/Specification/DDI-Codebook/2.5/XMLSchema/field_level_documentation_files/schemas/codebook_xsd/elements/titlStmt.html#a19 It has maxOccurs="unbounded"
I updated dataset-finch xml and json and added multiple alternative titles such that validity test can test it.
I also added comment about incompatibility into release note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a second value here, just like in dataset-finch1.json -
["Alternative Title1", "Alternative Title2"]
and that will need a corresponding change in src/test/java/edu/harvard/iq/dataverse/export/ddi/exportfull.xml
that DdiExportUtilTest compares against.
(I'm putting together a list of all the changes that need to be made when fields are made repeatable, for other developers)
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@landreev I added ["Alternative Title1", "Alternative Title2"]
into dataset-create-new-all-ddi-fields.json and added another title in corresponding exportfull.xml
src/main/java/edu/harvard/iq/dataverse/export/openaire/OpenAireExportUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/export/openaire/OpenAireExportUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/export/openaire/OpenAireExportUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/export/ddi/DdiExportUtil.java
Outdated
Show resolved
Hide resolved
…l.java Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
…eExportUtil.java Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
…eExportUtil.java Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
@pdurbin updated from develop, I see that there is some error, is it integration tests? Compilation on my local machine and unit tests are fine. |
@lubitchv thanks for merging develop. I just kicked off https://github.com/gdcc/api-test-runner/actions/runs/5962550825 After it finishes, can you see the results? |
@poikilotherm when I open https://github.com/gdcc/api-test-runner/actions/runs/5962550825/job/16173949398 in a private window I can't see the details of the API test failure: Do you know if we can open this up somehow? Or maybe as a workaround for now we add @lubitchv to that test runner repo? @lubitchv I don't mean to keep you in suspense. Here's the failure: @lubitchv I'm happy to help you get set up to run API tests on your laptop. Do you use Docker? |
MetadataBlocksIT seems to work now. I am not sure what does 1 cancelled and 2 skipped checks mean. |
@lubitchv yes! All tests passing now! Please don't worry about the skips and cancels. The skips are related to this: The cancel was me, sorry, related to this: #9830 (comment) |
@lubitchv now that we've released 6.0, I'm moving waiting 6.1 issues back to the board. But could you first handle the 6.0 merge and address any EE10 issues. Thanks. |
a90aa22
to
62a9811
Compare
…e into 9428-alternative-title Merge
Some integration tests are failing again. can I see which ones? |
@lubitchv the failing tests might be due to Jenkins/Ansible not being updated to 6.0 yet: But I'm not sure. Here's the error I see at https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9440/20/consoleFull TASK [dataverse : fire off installer] ****************************************** Meanwhile, I tried a test run here: https://github.com/gdcc/api-test-runner/actions/runs/6150829101 |
All the tests passed: Tests run: 210, Failures: 0, Errors: 0, Skipped: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect, thank you again!
I'm merging this, thanks! I did find a minor search results display bug relating to fields that have changed from single to multiple in metadata but will open a separate issue since it exists already. Thanks for the help @landreev |
What this PR does / why we need it:
Make "Alternative Title" multiple (repeatable) field such that DDI export would not break.
Which issue(s) this PR closes:
Closes #9428
Special notes for your reviewer:
If one does not want "Alternative Title" to be repeatable it is enough to remove TRUE from citation.tsv and solr, but leave the changes in the code that will allow for those who wants repeatable.
Suggestions on how to test this:
Create a dataset with multiple alternative titles, publish it. Test DDI Exporter and OpenAire exporter. It should not break and should have multiple alternative titles.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Yes
Is there a release notes update needed for this change?:
Yes. 9428-alternative-title.md