In order to resolve #8838 in Add CSTR to Harvard Dataverse Related #8913

cheneyfeng3 · 2022-08-11T05:37:45Z

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.

Publication ID Type list, we refer to the arXiv logo and Please review the code and make the changes.

We have fixed the lowercase and sequential issues. Please review the code and make the changes.

pdurbin · 2022-08-23T23:50:04Z

@cheneyfeng3 hi! Can you please merge the latest from the "develop" branch into your branch and resolve the merge conflicts? Thanks!

qqmyers · 2022-08-24T11:36:37Z

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.

jggautier · 2022-08-24T15:35:38Z

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.

qqmyers · 2022-08-24T15:58:13Z

@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.

jggautier · 2022-08-24T16:24:31Z

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)

…ded by qqmyers, please check if the conflict has been resolved. As we do not have access to the conflict details, if the modification does not resolve the conflict, please follow up with the file name and line number of the conflict and we will follow up with the modification.

xiaya2309 · 2022-08-29T07:47:10Z

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.

xiaya2309 · 2022-09-09T06:22:57Z

@pdurbin @qqmyers @jggautier
We have modified the AdminIT.java file and submitted the code as per the example #8775 provided by @qqmyers on 29 August, could you please see if the conflict is resolved?

qqmyers · 2022-09-09T13:05:10Z

@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.

cheneyfeng3 · 2022-09-13T01:36:59Z

@qqmyers I combined these conflicts, but the automation failed. It can't be built. Please help me see the reasons for failure.

src/test/java/edu/harvard/iq/dataverse/export/SchemaDotOrgExporterTest.java

scripts/issues/2102/ready-state.sql

scripts/api/data/metadatablocks/citation.tsv

qqmyers · 2022-09-13T10:35:21Z

@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.

src/test/java/edu/harvard/iq/dataverse/export/OpenAireExportUtilTest.java

Override

Adjusted sequence number

cheneyfeng3 · 2022-09-14T01:27:15Z

@qqmyers Thank you very much for your help. I have solved all the problems. Please review them again.

qqmyers · 2022-09-14T02:24:16Z

@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)

coveralls · 2022-09-21T10:36:48Z

Coverage increased (+0.09%) to 19.974% when pulling de6be21 on cheneyfeng3:develop into 7c1683b on IQSS:develop.

qqmyers · 2022-09-21T11:19:24Z

@cheneyfeng3 - it looks like the changes in the AdminIT test need to be made again - the two lines in 2f664d0 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.

cheneyfeng3 · 2022-09-22T01:47:16Z

@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.

pdurbin · 2022-09-22T11:07:37Z

@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:

resolve AdminIT junit.framework.AssertionFailedError: expected:<322> but was:<323>

cheneyfeng3 · 2022-09-23T04:30:52Z

@pdurbin I have solved this problem. Please review again.

I should find the reason for the failure, please rebuild

cheneyfeng3 · 2022-09-28T01:38:18Z

@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.

pdurbin · 2022-09-28T11:03:26Z

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.

update

cheneyfeng3 · 2022-09-29T01:45:17Z

@pdurbin Thanks for your advice
I have merged the latest branch, and the problem has been solved. Please merge to the develop branch.

cheneyfeng3 · 2022-09-30T00:28:51Z

@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?

pdurbin · 2022-09-30T02:29:12Z

@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!

xiaya2309 · 2022-10-08T08:37:57Z

@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!

jggautier · 2022-10-12T15:32:09Z

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.

src/main/java/propertyFiles/citation.properties

pdurbin · 2022-10-14T12:33:10Z

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.

qqmyers · 2022-10-14T13:23:34Z

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.

pdurbin · 2022-10-14T13:38:48Z

@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).

cheneyfeng3 · 2022-10-15T22:11:30Z

I synchronized the latest code and resubmitted a PR.
Is that so? @pdurbin
#9064

pdurbin · 2022-10-21T21:29:32Z

@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.

In order to resolve IQSS#8838 in Add CSTR to Harvard Dataverse Related

1aa61ea

Publication ID Type list, we refer to the arXiv logo and Please review the code and make the changes.

cheneyfeng3 marked this pull request as draft August 11, 2022 05:45

cheneyfeng3 marked this pull request as ready for review August 11, 2022 05:45

jggautier mentioned this pull request Aug 11, 2022

Feature Request/Idea: Add CSTR to Related Publication ID Type list #8838

Closed

Hi reviewer,

8948faa

We have fixed the lowercase and sequential issues. Please review the code and make the changes.

Merge branch 'develop' into develop

87d7a81

qqmyers reviewed Sep 13, 2022

View reviewed changes

src/test/java/edu/harvard/iq/dataverse/export/SchemaDotOrgExporterTest.java Outdated Show resolved Hide resolved

qqmyers reviewed Sep 13, 2022

View reviewed changes

scripts/issues/2102/ready-state.sql Outdated Show resolved Hide resolved

qqmyers reviewed Sep 13, 2022

View reviewed changes

scripts/api/data/metadatablocks/citation.tsv Outdated Show resolved Hide resolved

qqmyers reviewed Sep 13, 2022

View reviewed changes

src/test/java/edu/harvard/iq/dataverse/export/OpenAireExportUtilTest.java Outdated Show resolved Hide resolved

cheneyfeng3 added 4 commits September 14, 2022 08:46

Update OpenAireExportUtilTest.java

0bd6842

Update SchemaDotOrgExporterTest.java

561d8b7

Update ready-state.sql

39fdcab

Override

Update citation.tsv

ffde80e

Adjusted sequence number

cheneyfeng3 requested a review from qqmyers September 14, 2022 01:40

Update OpenAireExportUtil.java

c715bb8

cheneyfeng3 added 2 commits September 23, 2022 11:25

Merge branch 'IQSS:develop' into develop

4b985c3

Update AdminIT.java

5c79de8

resolve AdminIT junit.framework.AssertionFailedError: expected:<322> but was:<323>

Update AdminIT.java

4bfca4a

I should find the reason for the failure, please rebuild

Merge pull request #1 from IQSS/develop

bf9f039

update

qqmyers reviewed Oct 12, 2022

View reviewed changes

src/main/java/propertyFiles/citation.properties Outdated Show resolved Hide resolved

cheneyfeng3 and others added 2 commits October 14, 2022 11:13

encoding change.

614c33d

update encoding change.

de6be21

pdurbin assigned cheneyfeng3 Oct 14, 2022

pdurbin closed this Oct 21, 2022

jggautier mentioned this pull request Oct 31, 2022

8838-cstr #9064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In order to resolve #8838 in Add CSTR to Harvard Dataverse Related #8913

In order to resolve #8838 in Add CSTR to Harvard Dataverse Related #8913

cheneyfeng3 commented Aug 11, 2022

pdurbin commented Aug 23, 2022

qqmyers commented Aug 24, 2022

jggautier commented Aug 24, 2022 •

edited

Loading

qqmyers commented Aug 24, 2022

jggautier commented Aug 24, 2022

xiaya2309 commented Aug 29, 2022 •

edited

Loading

xiaya2309 commented Sep 9, 2022

qqmyers commented Sep 9, 2022

cheneyfeng3 commented Sep 13, 2022

qqmyers commented Sep 13, 2022

cheneyfeng3 commented Sep 14, 2022

qqmyers commented Sep 14, 2022

coveralls commented Sep 21, 2022 •

edited

Loading

qqmyers commented Sep 21, 2022

cheneyfeng3 commented Sep 22, 2022 •

edited

Loading

pdurbin commented Sep 22, 2022

cheneyfeng3 commented Sep 23, 2022

cheneyfeng3 commented Sep 28, 2022

pdurbin commented Sep 28, 2022

cheneyfeng3 commented Sep 29, 2022

cheneyfeng3 commented Sep 30, 2022

pdurbin commented Sep 30, 2022

xiaya2309 commented Oct 8, 2022

jggautier commented Oct 12, 2022

pdurbin commented Oct 14, 2022

qqmyers commented Oct 14, 2022

pdurbin commented Oct 14, 2022

cheneyfeng3 commented Oct 15, 2022

pdurbin commented Oct 21, 2022

In order to resolve #8838 in Add CSTR to Harvard Dataverse Related #8913

In order to resolve #8838 in Add CSTR to Harvard Dataverse Related #8913

Conversation

cheneyfeng3 commented Aug 11, 2022

pdurbin commented Aug 23, 2022

qqmyers commented Aug 24, 2022

jggautier commented Aug 24, 2022 • edited Loading

qqmyers commented Aug 24, 2022

jggautier commented Aug 24, 2022

xiaya2309 commented Aug 29, 2022 • edited Loading

xiaya2309 commented Sep 9, 2022

qqmyers commented Sep 9, 2022

cheneyfeng3 commented Sep 13, 2022

qqmyers commented Sep 13, 2022

cheneyfeng3 commented Sep 14, 2022

qqmyers commented Sep 14, 2022

coveralls commented Sep 21, 2022 • edited Loading

qqmyers commented Sep 21, 2022

cheneyfeng3 commented Sep 22, 2022 • edited Loading

pdurbin commented Sep 22, 2022

cheneyfeng3 commented Sep 23, 2022

cheneyfeng3 commented Sep 28, 2022

pdurbin commented Sep 28, 2022

cheneyfeng3 commented Sep 29, 2022

cheneyfeng3 commented Sep 30, 2022

pdurbin commented Sep 30, 2022

xiaya2309 commented Oct 8, 2022

jggautier commented Oct 12, 2022

pdurbin commented Oct 14, 2022

qqmyers commented Oct 14, 2022

pdurbin commented Oct 14, 2022

cheneyfeng3 commented Oct 15, 2022

pdurbin commented Oct 21, 2022

jggautier commented Aug 24, 2022 •

edited

Loading

xiaya2309 commented Aug 29, 2022 •

edited

Loading

coveralls commented Sep 21, 2022 •

edited

Loading

cheneyfeng3 commented Sep 22, 2022 •

edited

Loading