Skip to content

1625 subfield reference value#6521

Closed
mderuijter wants to merge 19 commits intoIQSS:developfrom
mderuijter:1625-subfield-reference-value
Closed

1625 subfield reference value#6521
mderuijter wants to merge 19 commits intoIQSS:developfrom
mderuijter:1625-subfield-reference-value

Conversation

@mderuijter
Copy link
Contributor

@mderuijter mderuijter commented Jan 15, 2020

What this PR does / why we need it:
Makes it possible to reference the value of a metadata subfield in another subfield. In this particular case as i explain a use case in the test suggestions below the referenced value is a configurable and optional alternative value.

Although it covers the use case i explain below, it might not work for all use cases that this issue intends to address.
Which issue(s) this PR closes:

Closes #1625 Dataset Metadata - allow one subfield to reference the value of another

Special notes for your reviewer:
After receiving some feedback and more ideas on my suggested approach (#1625), I continued with the ##authorIdentifierScheme tag implementation that can be added to the citation.tsv file.

Suggestions on how to test this:
Our particular use case at DANS would be to turn the authorIdentifierScheme + authorIdentifier into a clickable link like so:
image

The first step to do this is add ##authorIdentifierScheme inside a hyperlink tag like this:
<a href="#authorIdentifierScheme#VALUE" target="_blank">#VALUE</a> in the authorIdentifier row with fieldType set to text. You can find an example in they citation.tsv file i commited.

The second step is to add the details(in this case urlprefixes) to the details column:
image

Lastly an existing or new dataset should be tested to see if the links work and display correctly.

Does this PR introduce a user interface change?:
No

Is there a release notes update needed for this change?:
I'm not sure

Additional documentation:
Yes additional documentation would be required, it would look like the explanation above.

@coveralls
Copy link

coveralls commented Jan 15, 2020

Coverage Status

Coverage decreased (-0.01%) to 19.643% when pulling 600b242 on mderuijter:1625-subfield-reference-value into e7f5448 on IQSS:develop.

@djbrooke
Copy link
Contributor

Thanks @mderuijter. Can you provide more information in the fields in the template before we review this? Having this information will help us with the review, QA, and release notes.

@mderuijter
Copy link
Contributor Author

Thanks @mderuijter. Can you provide more information in the fields in the template before we review this? Having this information will help us with the review, QA, and release notes.

@djbrooke yes sorry about that, the information should be more helpful now

@mderuijter
Copy link
Contributor Author

@scolapasta Hi! I managed to commit my code changes yesterday. I'm not sure if you get notified automatically, hence this message :)

@djbrooke
Copy link
Contributor

djbrooke commented Feb 5, 2020

Thanks @mderuijter, we'll review this and either move it to QA or we will discuss further with you.

@scolapasta
Copy link
Contributor

Hi @mderuijter, I reviewed and I think it's closer. I still don't think it handles well the case where some values will have the details and others will not (I could be missing something, of course). This is why I suggested the actual display format could be associated with the actual CVV. In the case of the URL, it may be ok, but I'm hoping for a scalable solution that could work for any dependent value on CVV.

I hope you'll understand that I'm being especially careful with this, because how we store the metadata is fundamental to dataverse and we do need a scalable approach so as to not have to make many changes in the future.

If it helps to have a conversation over video, let me know. (I'll be away next week, but can find time the following week)

@mderuijter
Copy link
Contributor Author

Hi @scolapasta, the code differentiates between the 'normal' value and the 'detailed' value by checking for a value inside the controlledvocabularydetail table. If there's no detail, the 'normal' value is used. And specifying a detail is done in the .tsv file. If left blank the normal value will be used.

Perhaps you can help me visualise some cases where this approach would not suffice, I'm probably just seeing the use case DANS has in mind.

I actually prefer carefully writing my thoughts down, even though a video call might be a better way to discuss things.

@scolapasta
Copy link
Contributor

Hi @mderuijter would you mind merging the latest from develop into this branch?

@mderuijter
Copy link
Contributor Author

Hi @scolapasta I merged from the IQSS repo (upstream) to my local 'develop' branch, it fast-forwarded and i pushed it to my fork's 'develop' branch (origin). Would that do the trick?

@scolapasta
Copy link
Contributor

Hi @mderuijter actually from what we can tell, it looks as if you still need to merge 1625 with your develop branch. Once that is done we can put in on a server and see it in practice, which should help me understand some of the answers to my questions.

@mderuijter
Copy link
Contributor Author

Hi @scolapasta, must have misunderstood you the first time, should be done now.

@scolapasta
Copy link
Contributor

@mderuijter I still don't see a commit showing a merge here (like the one from Jan 14: "Merge branch 'develop' of https://github.com/IQSS/dataverse into 1625"

@scolapasta
Copy link
Contributor

@mderuijter ok, I'm getting back around to this - we've uploaded this on an S3 server and I want to understand if I've done something wrong OR if it's not working as expected.

See this dataset:
http://ec2-3-88-56-12.compute-1.amazonaws.com/dataset.xhtml?persistentId=doi%3A10.5072%2FFK2%2F5DD1MM

I've added two authors, one with Orcid ID, one with Scopus. Both show as links however (the orcid one working as expected). I would've expected that the Scopus one would not be a link.

Let me know; and we can discuss what the best way to solve this is (as again, ideally we can figure out how to have this could be useful for more than just author identifiers).

@mderuijter
Copy link
Contributor Author

@scolapasta I think I'm starting to understand more your idea to store full display formats in a table...

I feel a bit silly not noticing sooner. The configuration in the .tsv file will make all author identifiers in links because of the href tag. Although I assumed all author identifiers would get url prefixes. I assumed wrong perhaps?

Then there's the fact that if there's no 'detailed' value configured the 'normal' value will be shown, in this case you get ScopusID123-456-789 you'd rather want just the value 123-456-678.

Now I think I that if you would store the display format in a table you might not have to solve the above with more code. But what would that mean for the displayFormat column in the .tsv file?

@scolapasta
Copy link
Contributor

@mderuijter this is why I was suggesting we tie the displayformat for these to the specific controlled vocabulary values by putting them there. (this is on the issue (2 comments from Dec 19)). Please review that, let me know what you think, and if you agree something like that could work, we can further design the details so that it can be coded.

@mderuijter
Copy link
Contributor Author

@scolapasta Hi :) I updated the PR with the solution I think you intended in the first place.

@qqmyers
Copy link
Member

qqmyers commented Nov 11, 2020

@mderuijter in trying to start the controlled vocabular value topic in the Dataverse Metadata working group, I'm looking for people who might want to present some requirements/work relevant to the topic. Would you be interested? If so, the best way to connect would probably be to follow the process to join the WG and get into the slack space and contact me there (see https://groups.google.com/g/dataverse-community/c/dyNNdNXNe-I/m/29Z5bgdCAQAJ).

@scolapasta
Copy link
Contributor

Discussed with @mderuijter. As #7979 was merged and handles links to ORCID accounts, we both agreed this could be closed. We can always reopen this effort if anyone wants, of course.

@scolapasta scolapasta closed this Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dataset Metadata - allow one subfield to reference the value of another

6 participants