IQSS
In order to resolve #8838 in Add CSTR to Harvard Dataverse Related
Hi reviewer, In order to resolve #8838 in Add CSTR to Harvard Dataverse Related Publication ID Type list, we refer to the arXiv logo and Please review the code and make the changes.
@cheneyfeng3 hi! Can you please merge the latest from the "develop" branch into your branch and resolve the merge conflicts? Thanks!
FYI: I think you will need to change src/test/java/edu/harvard/iq/dataverse/api/AdminIT.java at some point as well. If you look at #8775, which also added a related publication id type (and is probably a source of merge issues for you), you'll see that test includes counting the total number of lines and vocab entries in the citation.tsv file which will change with your addition.
Just noting that #8775 adds an ID Type that is not all lowercase and is not in the ID Type list in alphabetical order. I didn't know this when I suggested to @cheneyfeng3 that the CSTR ID Type be made all lowercase and put in this ID Type list in alphabetical order.
Since #8775 has been reviewed and is already merged, @qqmyers could you say if this should be looked into later? If so I could open a GitHub issue about this.
@jggautier I don't know - the use of DASH-NRS in the display (and I think the idea of putting it last) was per design specification at Harvard. That said, the name of the entry in the tsv could be dash-nrs, with citation.properties having the upper case display (note 'arXiv' in there as well). Similarly, I think the tsv file entry could be made alphabetical while keeping the displayorder to have it last if desired. Or the display could be changed as well if that's acceptable w.r.t. the HDC effort. If it is going to change, it would probably be useful to have it in v5.12 - I don't think there's any breaking change if it came in later though.
Ah, thanks, yes arXiv does appear in the UI with the capital X. I missed that! The letters in everything else are lowercase. Hard to know sometimes if something is intentional (and why) or an oversight. I'll try learning more from the folks at Harvard who worked on #8775.
For now I'd be okay with the way things are (CSTR remaining lowercase and in alphabetical order)
We have modified the AdminIT.java file and submitted the code as per the example #8775 provided by @qqmyers , please check if the conflict has been resolved.
As we do not have access to the details of the conflict, if the modification does not resolve the conflict, please follow up with the conflicting file name and line number and we will follow up with the modification.
@pdurbin @qqmyers @jggautier We have modified the AdminIT.java file and submitted the code as per the example https://github.com/IQSS/dataverse/pull/8775 provided by @qqmyers on 29 August, could you please see if the conflict is resolved?
@xiaya2309 - Once you resolve the conflicts shown in the PR, you should be able to see whether the automated test run succeeds or not. If the tests pass, you're good. If not, we can send you info about what's failing and why. Looking at the code your changes to AdminIT.java look fine.
@qqmyers I combined these conflicts, but the automation failed. It can't be built. Please help me see the reasons for failure.
@cheneyfeng3 - It looks like something went wrong with the build run and the problem isn't something in your code. I made a few suggested fixes. If you make those updates we can see what the next build run does.
@qqmyers Thank you very much for your help. I have solved all the problems. Please review them again.
@cheneyfeng3 The changes look good to me.
FYI: I'm now seeing the following test error. I think you added RelatedMaterials3 and RelatedDatasets3 when you didn't need to.
You should be able to run the Unit Tests locally using mvn clean package which should let you see the errors directly.
[ERROR] edu.harvard.iq.dataverse.export.ddi.DdiExportUtilTest.testExportDDI Time elapsed: 0.078 s <<< FAILURE!
org.opentest4j.AssertionFailedError:
Expecting:
<control instance> and <test instance> to be similar
Expected child nodelist length '12' but was '9' - comparing <othrStdyMat...> at /codeBook[1]/stdyDscr[1]/othrStdyMat[1] to <othrStdyMat...> at /codeBook[1]/stdyDscr[1]/othrStdyMat[1]
expected:<<othrStdyMat xmlns="ddi:codebook:2_5">
<relMat>RelatedMaterial1</relMat>
<relMat>RelatedMaterial2</relMat>
<relMat>RelatedMaterial3</relMat>
<relStdy>RelatedDatasets1</relStdy>
<relStdy>RelatedDatasets2</relStdy>
<relStdy>RelatedDatasets3</relStdy>
<relPubl>
<citation>
<titlStmt>
<IDNo agency="ark">RelatedPublicationIDNumber1</IDNo>
</titlStmt>
<biblCit>RelatedPublicationCitation1</biblCit>
</citation>
<ExtLink URI="http://RelatedPublicationURL1.org"/>
</relPubl>
<relPubl>
<citation>
<titlStmt>
<IDNo agency="arXiv">RelatedPublicationIDNumber2</IDNo>
</titlStmt>
<biblCit>RelatedPublicationCitation2</biblCit>
</citation>
<ExtLink URI="http://RelatedPublicationURL2.org"/>
</relPubl>
<relPubl>
<citation>
<titlStmt>
<IDNo agency="CSTR">RelatedPublicationIDNumber3</IDNo>
</titlStmt>
<biblCit>RelatedPublicationCitation3</biblCit>
</citation>
<ExtLink URI="http://RelatedPublicationURL3.org"/>
</relPubl>
<othRefs>OtherReferences1</othRefs>
<othRefs>OtherReferences2</othRefs>
<othRefs>OtherReferences3</othRefs>
</othrStdyMat>> but was:<<othrStdyMat xmlns="ddi:codebook:2_5">
<relMat>RelatedMaterial1</relMat>
<relMat>RelatedMaterial2</relMat>
<relStdy>RelatedDatasets1</relStdy>
<relStdy>RelatedDatasets2</relStdy>
<relPubl>
<citation>
<titlStmt>
<IDNo agency="ark">RelatedPublicationIDNumber1</IDNo>
</titlStmt>
<biblCit>RelatedPublicationCitation1</biblCit>
</citation>
<ExtLink URI="http://RelatedPublicationURL1.org"/>
</relPubl>
<relPubl>
<citation>
<titlStmt>
<IDNo agency="arXiv">RelatedPublicationIDNumber2</IDNo>
</titlStmt>
<biblCit>RelatedPublicationCitation2</biblCit>
</citation>
<ExtLink URI="http://RelatedPublicationURL2.org"/>
</relPubl>
<relPubl>
<citation>
<titlStmt>
<IDNo agency="CSTR">RelatedPublicationIDNumber3</IDNo>
</titlStmt>
<biblCit>RelatedPublicationCitation3</biblCit>
</citation>
<ExtLink URI="http://RelatedPublicationURL3.org"/>
</relPubl>
<othRefs>OtherReferences1</othRefs>
<othRefs>OtherReferences2</othRefs>
</othrStdyMat>>>
at edu.harvard.iq.dataverse.export.ddi.DdiExportUtilTest.testExportDDI(DdiExportUtilTest.java:80)
Coverage increased (+0.09%) to 19.974% when pulling de6be210c10c491eeba8fb5dce9d74a3cff06926 on cheneyfeng3:develop into 7c1683b8eebb2065b548ce0274aa290bc51a1199 on IQSS:develop.
@cheneyfeng3 - it looks like the changes in the AdminIT test need to be made again - the two lines in https://github.com/IQSS/dataverse/pull/8913/commits/2f664d0886031feee1ea006d0a77b269303ccc1c have to be incremented again. (Once you merged with develop, you picked up the DASH-NRS idtype which increased those numbers by one, so with both DASH and CSTR, they need to be incremented again.
Hopefully that will fix the one test fail that is showing now.
@pdurbin @qqmyers @jggautier Hello, administrator. At present, this pr only has 'continuous-integration/jenkins/pr-merge', and the execution fails. I Click 'details' here, but the address can't be opened yet. Please help me see what the reason is, and I can adjust it here.
@cheneyfeng3 hi! It says this:
Stacktrace junit.framework.AssertionFailedError: expected:<322> but was:<323> at edu.harvard.iq.dataverse.api.AdminIT.testLoadMetadataBlock_NoErrorPath(AdminIT.java:765)
Here's a screenshot for more context:
@pdurbin @qqmyers @jggautier Hello, administrator. At present, this pr only has 'continuous-integration/jenkins/pr-merge', and the execution fails. I Click 'details' here, but the address can't be opened yet. Please help me see what the reason is, and I can adjust it here.
Sure, please see below. I wouldn't worry about the "edit DDI" one because we should have fixed #8992 already (if you merge from develop it should pass, I hope). I'm not sure about DatasetsIT.testAddUpdateDatasetViaNativeAPI. Maybe you can try to run this one locally.
@pdurbin Thanks for your advice I have merged the latest branch, and the problem has been solved. Please merge to the develop branch.
@pdurbin @qqmyers @jggautier Thank you for your assistance. This modification has passed all tests. Please merge it into the develop branch. At present, the technical level has been completed. Is there any other work we need to complete to promote the completion of this issue as soon as possible?
@cheneyfeng3 hi! Currently you are at this stage: https://guides.dataverse.org/en/5.11.1/developers/version-control.html#make-sure-your-pull-request-has-been-advanced-to-code-review
That is, you should continue doing what you're doing and advocate for this pull request to be reviewed.
After approval by a reviewer, it moves to QA.
After successful QA, it will be merged.
I hope this helps!
@jggautier @qqmyers Hello, thank you for your help in moving forward with "Adding CSTR to the list of relevant publication ID types". Our work on the code is now almost complete and the submission has passed the automated tests. However, it has been over a week since our work was completed but our code submissions have not been reviewed. Would you mind asking the reviewer to review the code to avoid errors when merging? Are there any other steps we need to go through between completing the branch merge and the user being able to use it(user can select the CSTR ID in the database)? Is there any way we can speed them up? Thank you very much for your help! Have a good day!
Hi @xiaya2309. I think that everyone who can review the code is finishing other work, but someone will re-review this PR as soon as possible.
About getting this change in Dataverse repositories so that depositors can choose the CSTR ID, I'd just like to clarify that after your pull request goes through the process that @pdurbin mentioned (code review and QA) and is ready to be merged, other pull requests that will be included in the next software release will also have to go through the process. Each software release usually includes multiple pull requests.
So right now we're not sure when the next software release will be. But the sooner this pull request gets through the process and it ready to be merged, the more likely it is that it will be included in the next software release.
I hope that helps clarifies the process.
For now I'd be okay with the way things are (CSTR remaining lowercase and in alphabetical order)
I'm glad @jggautier has already given his blessing.
https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-8913/28/ failed because of something unrelated so I just clicked "build now" to rerun the job.
FWIW: The branch now needs to be merged with develop to pick up the v5.12 tag - the build system expects to find a 5.12 war and this is still branch is still producing a v5.11.1 war.
@qqmyers yeah. Because @cheneyfeng3 's branch is also called "develop" I'm having trouble getting the git incantations right to merge origin/develop into cheneyfeng3/develop.
@cheneyfeng3 would you be able to do this? Or create a fresh pull request with a name like "8838-cstr"? (Anything but "develop", please).
I synchronized the latest code and resubmitted a PR. Is that so? @pdurbin https://github.com/IQSS/dataverse/pull/9064
@cheneyfeng3 yes, the new pull request seems to have the latest from develop and it has a branch name that should work if we need to update it. Thanks! I'll close this PR.