-
Notifications
You must be signed in to change notification settings - Fork 491
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
Display overview fix (external controlled vocabulary) #11034
Conversation
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.
Thanks for the PR! Looks good. I added some testing notes - feel free to add/change. I also ask that you remove the entry in the API change log (since it is API only). I'm looking into the build failure - it's a known issue not related to the PR contents.
@@ -7,6 +7,10 @@ This API changelog is experimental and we would love feedback on its usefulness. | |||
:local: | |||
:depth: 1 | |||
|
|||
v6.5 |
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.
This is only for API changes so I don't think we want an entry here.
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, the release note is enough. Thanks!
Hi there, I have just committed a removal from changelog.rst. |
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.
@ffritze - in testing at QDR, I think I found a bug - details in the other comment.
@qqmyers Thanks for the bugfix. Could you please upload here your cvocConf.json file so that I can get a better understanding of your environment? |
This should work. Local copies of the scripts. I think I saw the issue of the missing author name when both the author name and affiliation were text strings. ```
|
@qqmyers In our environment your fix is working correctly. Do I understand it right that in yours it is also running flawlessly? If so, I will accept your changes. |
Yes - the fix I proposed works in all the cases I've tried. |
I open it again. I don't know how to approve and proceed. |
@qqmyers Should I push your changes into my pull request branch? |
Yeah - just make the change in your branch. Once it's in I can move things forward (approve the build, move it to ready for QA, etc.) |
@qqmyers It is pushed to my pull request branch. Please do an additional review because I accidentally removed your comprehensive comment regarding your proposed changes. |
Looks good. The comment is still at #11034 (comment) - just hidden. |
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 good. Tested at QDR and by @ffritze.
Merging - no issues found during regression testing |
What this PR does / why we need it:
On the metadata page there can be duplicated / redundant metadata entries. This pull-request fixes an IF-clause in metadataFragment.xhtml to eliminate the redundant display of metadata fields
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this: (from Jim) - I thought I could see the issue if I configured the ORCID script but not the ROR script via the external vocabulary mechanism but trying this on demo/v6.4 isn't showing the problem. I'd suggest testing with no scripts (can add author name/affiliation/id metadata) and with the ORCID/ROR set up on the author field (able to add a person via ORCID lookup and free text, affiliation by ROR and free text) and making sure that those work.
(authorIDandAffilationUsingORCIDandROR.md describes adding both using the authorsOrcidAndRor.json file. ).
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?: included
Additional documentation: