Skip to content
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

handle doi url #74

Merged
merged 4 commits into from Jun 28, 2018
Merged

handle doi url #74

merged 4 commits into from Jun 28, 2018

Conversation

Stefan-Wolff
Copy link
Contributor

@Stefan-Wolff Stefan-Wolff commented Jun 25, 2018

don't complete the URL if the full URL is given;

supported doi variants:

https://jira.duraspace.org/browse/VIVO-1527

@gneissone gneissone self-requested a review June 25, 2018 18:30
@gneissone
Copy link
Member

gneissone commented Jun 25, 2018

For historical continuity, see #73 for previous comments.

Copy link
Member

@gneissone gneissone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it works as described. Added an additional request to revert a change I just noticed, apologies for missing it the first time around.

As a note, you can push another commit to your existing fork at Stefan-Wolff:develop and the pull request will automatically include the changes. No need to make a new pull request.

replace("http://dx.doi.org/", "").
replace("https://dx.doi.org/", "").
replace("doi:", "").trim()>
<a href="https://doi.org/${doi}" title="${i18n().doi_link}" target="_blank">${doi}</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the modified DOI should only be used for the link. I would say revert to 'statement.value' for the text and the citation meta tag so the DOI is displayed in the format the user entered it in.

CrossRef guidelines actually say to display the DOI as a link with https://doi.org/, but I say lets leave it up to the data owner to follow the rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i have thought on this. The problem is, that we are changing the link exactly to "https://doi.org/", but we are handling 5 different formats of the DOI URL. So we could get a link to "https://doi.org/" having a link text of "http://dx.doi.org/". This breaks with the user's expectation, when he clicks the link.
I think there are two possible solutions:

  1. link href und link text are the same value
  2. link text contains only the part of all handled DOI URLs -> the DOI

I prefer the second one.

Copy link
Member

@gneissone gneissone Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you're coming from, but I think we should consider the data manager's expectations as well. If they enter the DOI in a certain format, I think they would expect it to be displayed the same way. The broken links are the big issue. I'm not sure if the average user is going to notice a discrepancy between what is displayed and the URL being followed if the end result is the same no matter the URL format.

I am in favor of displaying the DOI as entered (meanwhile fixing the link)... but I could also get behind reformatting the DOI to adhere to the Crossref guidelines, i.e. remove all the unapproved prefixes, then add https://doi.org to the display as you're doing with the link, i.e. solution #1. But I don't agree with solution #2, stripping all prefixes and displaying the DOI in a deprecated format.

Perhaps we can discuss during the developer call tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the data manager expect to see exactly the same link as he entered before, then he also expect to open exactly this link when he clicks it.
Its a problem to show an URL and link to another URL. If only the DOI part is shown, then there would be no misinformation.
#1 would also be a correct way to do it, but i think it looks not very nice to show such a long URL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are appear to be two issues:

  1. fixing variants provided by users. I think this is a good idea.
  2. distinguishing between "DOI the URL" and "DOI the data value to be stored in the triple store". I'm not sure there is such a difference. The DOI as specified by CrossRef comes with https://doi.org/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the only change to this PR is in https://github.com/vivo-project/VIVO/pull/74/files#diff-19fb9fd9986b7f1f9d103022cdd1005dR19 , making the display the full https://doi.org/xxx URL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is bad UX to change what a user enters in the background and without their knowledge. If VIVO expects the url to be in a specific format, and the user enters a different format, tell the user to fix it. That's the whole point of validation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, @tworrall . The point that @gneissone made on today's call is that not all pathways to entering a DOI have validation (e.g. direct SPARQL-Update).

There is currently a bug. Would you be ok with the resolution proposed here to fix the bug, followed by another ticket that provides UI validation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at CU we don't use the editorial interface, we ingest via the harvester, and we only ingest the DOI number without the http[s]:// prefix, we count on VIVO for consistency. I would prefer the change made in VIVO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awoods Yes.

Copy link
Member

@awoods awoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while DOI URL is used as link target, now its also used as link text
Copy link
Member

@awoods awoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gneissone gneissone merged commit 1343966 into vivo-project:develop Jun 28, 2018
dofeldsc referenced this pull request in FISvirtUOS/VIVO Jul 19, 2018
* handle doi url (#74)

don't add "https://doi.org/" if the full URL is given

* add dx.doi.org to DOI URL handling

* Update propStatement-doi.ftl

* use DOI URL as link text

while DOI URL is used as link target, now its also used as link text
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.

None yet

7 participants