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

Invalid DOI should not show "Connection error" #8127

Closed
2 tasks done
koppor opened this issue Oct 7, 2021 · 13 comments · Fixed by #8228
Closed
2 tasks done

Invalid DOI should not show "Connection error" #8127

koppor opened this issue Oct 7, 2021 · 13 comments · Fixed by #8228

Comments

@koppor
Copy link
Member

koppor commented Oct 7, 2021

JabRef version

Latest development branch build (please note build date below)

Operating system

Windows

Details on version and operating system

Windows 10

Checked with the latest development build

  • I made a backup of my libraries before testing the latest development version.
  • I have tested the latest development version and the problem persists

Steps to reproduce the behaviour

  1. Open https://link.springer.com/book/10.1007/978-3-319-47590-5
  2. Download PDF ("Download book PDF")
  3. Go to page 207
  4. Select and copy the DOI:
    image
  5. Open "New entry dialog" and paste DOI:
    image
  6. See the question mark at the end
  7. Click "Generate"
  8. Result:
    image

Expected behavior:

  • The invalid character at the end should be removed internally
  • The DOI data should be fetched

Appendix

No response

@koppor koppor added good first issue An issue intended for project-newcomers. Varies in difficulty. type: enhancement ui labels Oct 7, 2021
@TheDeveloperDino
Copy link

Hello, is this issue free? I can work on this issue.

@koppor
Copy link
Member Author

koppor commented Oct 7, 2021

Sure! Go ahead!

@koppor
Copy link
Member Author

koppor commented Oct 9, 2021

@TheDeveloperDino You can to test-driven development. A start can be #8124. I think, one should add test cases to org.jabref.model.entry.identifier.DOITest and then work on org.jabref.model.entry.identifier.DOI.

@TheDeveloperDino
Copy link

Okey.

@mrcstan
Copy link
Contributor

mrcstan commented Nov 6, 2021

@TheDeveloperDino, are you still working on this? @koppor , I tried to copy and paste the DOI as described above in Windows 10 but there is no such special character at the end, and no error result. However, I could produce the error if I use invalid characters like question mark at the end of the DOI. I would like to re-define this issue as one caused by any trailing invalid characters at the end of the DOI, and I would like to volunteer to fix it if @TheDeveloperDino is no longer working on it

@jiezheng5
Copy link
Contributor

jiezheng5 commented Nov 7, 2021

Hi,

I am a cs student, I repeated the issue in both Windows 10 and Linux Ubuntn OS 20.04. I fixed this issue by adding a method to automatically remove non-valid characters at the end of DOI string and let the DOI constructor call this method. I already verified the solution using both Windows 10 and Linux. I also wrote a ParameterizedTest for the method I created. I just submitted a PR (#8215) for review.

@mrcstan
Copy link
Contributor

mrcstan commented Nov 7, 2021

@jiezheng5, this is not really in the spirit of open source contribution. You requested to work on this issue after me and we have not heard from @TheDeveloperDino. I have also started working on the issue.

@jiezheng5
Copy link
Contributor

@jiezheng5, this is not really in the spirit of open source contribution. You requested to work on this issue after me and we have not heard from @TheDeveloperDino. I have also started working on the issue.

I actually started working on this issue 2 weeks ago. Then I saw your request, I thought maybe I should also post my progress. Do we have to first get permission before we can work on the open issues? This is my first time contributing to open source, I am sorry that if I have done something wrong.

@koppor
Copy link
Member Author

koppor commented Nov 8, 2021

Example DOI for this behavior: 10.3218/3846-0. It is a valid DOI string - but not not found on doi org: https://doi.org/10.3218/3846-0 returns 404.

When a user fetches the DOI data, "Connection error" is returned. The improved implementation should check for 404 and handle that as "No DOI data existing".

Side note: The DOI is listed at https://vdf.ch/studienarbeiten-e-book.html - and thus users may think, it is a valid one. However, the publisher seems not to have registered it at the official DOI registry.

@koppor koppor removed the good first issue An issue intended for project-newcomers. Varies in difficulty. label Nov 8, 2021
@koppor
Copy link
Member Author

koppor commented Nov 8, 2021

I fixed this issue by adding a method to automatically remove non-valid characters at the end of DOI string

@mrcstan Could you add a test case to org.jabref.model.entry.identifier.DOITest? I think, the solution is found by working on org.jabref.model.entry.identifier.DOI#DOI_EXP, which seems to include more characters than needed in the resulting DOI?

@koppor
Copy link
Member Author

koppor commented Nov 8, 2021

Then I saw your request, I thought maybe I should also post my progress.

This is actually a good idea. We also outlined that at https://devdocs.jabref.org/contributing#create-a-pull-request:

If you want to indicate that a pull request is not yet complete before creating the pull request, you may consider creating a draft pull request.

Do we have to first get permission before we can work on the open issues?

No, just start working. The general reason is that the probability at 500+ opened issues that two volunteers start working on the same issue at the same time is nearly 0. Maybe in the concrete setting, this issue was a) tagged as "good first issue" and b) at the very beginning of the list.

I am sorry that we have this situation now. I reviewed #8215 and think, it would be good to see an alternative solution.

Proposal:

  1. @mrcstan Submits his solution as WIP PR
  2. The JabRef developers compare the two solutions and decide which one is closer to a solution based on our 15+ years experience in Java development

Would that be OK?

This is my first time contributing to open source,

Welcome to the open source development world!

I am sorry that if I have done something wrong.

You "just" hit an edge case in the open source world. We are now working to resolve this case. Meanwhile, you could work on another issue, maybe koppor#534?

@koppor
Copy link
Member Author

koppor commented Nov 8, 2021

This PR has two parts:

  1. Invalid characters at a DOI --> fixed by DOI determining now uses "DOI.findInText" (and not "DOI.parse") #8227
  2. DOI is a valid one, but not returned by the server --> still open

@mrcstan
Copy link
Contributor

mrcstan commented Nov 8, 2021

@koppor , i just submitted a PR #8228 addressing 1 and 2. I found a case where #8227 did not work and mentioned that in that PR.

mrcstan added a commit to mrcstan/jabref that referenced this issue Nov 9, 2021
… into fix-for-issue-JabRef#8127

Rebased with main after pushing
koppor pushed a commit that referenced this issue Nov 13, 2021
Siedlerchr added a commit that referenced this issue Nov 19, 2021
* upstream/main: (181 commits)
  Add of ADRs 22 and 23 (#8256)
  [Bot] Update CSL styles (#8245)
  Replace styfle/cancel-workflow-action@0.9.1 by GitHub's "concurrency" feature (#8243)
  Bump gittools/actions from 0.9.10 to 0.9.11 (#8248)
  Bump commons-cli from 1.4 to 1.5.0 (#8250)
  Bump byte-buddy-parent from 1.12.0 to 1.12.1 (#8249)
  Bump antlr4 from 4.9.2 to 4.9.3 (#8251)
  Bump archunit-junit5-api from 0.21.0 to 0.22.0 (#8252)
  Fix search: NOT binds more than AND (#8241)
  New Crowdin updates (#8240)
  Make PdfGrobiImporterTest as FetcherTest
  Oobranch g : add actions (#7792)
  Fix mixed CRLF / CR (#8238)
  Fix "Library has changed externally" with CRLF markers (#8239)
  Fix for issue 8198, 8133 (#8229)
  Added more unit tests in AuthorTest (#8214)
  Add confirmation dialog for empty entries in JabRef (#8218)
  Fix entry editor column visibility (#8232)
  Use regexp to remove non-ASCII characters from DOI and inform user when data for valid DOI does not exist #8127 (#8228)
  Fix exception for search flags (#8237)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants