Detect and remove ?doi= tag before passing to DOI.parse #9848

sean-leichtle · 2023-05-07T13:50:45Z

Fixes #9821

Modify org/jabref/logic/importer/CompositeIdFetcher.java to detect and remove ?doi= tag before passing to DOI.parse

Add unit tests to org/jabref/model/entry/identifier/DOITest.java

Update CHANGELOG.md

Compulsory checks

Give feedback

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.
Options

koppor · 2023-05-07T13:53:04Z

src/main/java/org/jabref/logic/importer/CompositeIdFetcher.java

-        Optional<DOI> doi = DOI.parse(identifier);
+        Optional<DOI> doi = DOI.parse(filterForDOITag(identifier));


Please no new method if not needed.

Please to

Optional<DOI> doi = DOI.findInText(identifier);

and report back if this works.

That certainly works. Sorry, I misunderstood the requirements you originally outlined:

Test-enabling: Heuristics as own method. This method needs to be tested. Method can be "package private".

Nice!

Sorry for having a discussion with you without updating the main text. I thought, it was clear.

4 days ago, we discussed the usage of that method: #9821 (comment)

(And you even acknowledged 4 days ago - #9821 (comment))

Sorry again that I did not update the main text.

Ah, ok. No worries, my misunderstanding. It's really quite a small fix - I would have been quicker except it didn't occur to me for most of yesterday that the reason it wasn't working was the copy-paste issue.

The copy and paste thing is a hard one to work on. This involves understanding JavaFX behavior on macOS, which is different than on Windows. Therefore, we have some "hacks" included, which might not work as expected (which you encountered).

Nevertheless, the issue at hand requires some GUI (!) tests, which could be hard. See https://github.com/JabRef/jabref/blob/main/src/test/java/org/jabref/gui/search/GlobalSearchBarTest.java for an example test.

Side story: It is hard to craft "medium-sized" issues. Mostly, the issues involve much code understanding, but little code changes. -- You can look at https://github.com/orgs/JabRef/projects/3/views/3 for a curated list of issues...

koppor · 2023-05-07T13:56:18Z

src/test/java/org/jabref/logic/importer/fetcher/CompositeIdFetcherTest.java

@@ -121,4 +121,16 @@ void performSearchByIdReturnsEmptyForInvalidId(String groundInvalidArXivId) thro
    void performSearchByIdReturnsCorrectEntryForIdentifier(String name, BibEntry bibEntry, String identifier) throws FetcherException {
        assertEquals(Optional.of(bibEntry), compositeIdFetcher.performSearchById(identifier));
    }
+
+    @ParameterizedTest
+    @ValueSource(strings = "https://www.scitepress.org/Link.aspx?doi=10.5220/0010404301780189")


The "secret" sauce for parameterized tests is to pass both the expected result and the input.

Try

@CsvSource({"10.5220/0010404301780189,"https://www.scitepress.org/Link.aspx?doi=10.5220/0010404301780189", "10.5220/0010404301780189,10.5220/0010404301780189"}) void filterForDOITagReturnsDOI(String expected, String DOIString) throws FetcherException {

Oh, the test seems to be go after https://github.com/koppor/jabref/blob/main/src/test/java/org/jabref/model/entry/identifier/DOITest.java#L203-L203

Ok, I've read up a bit on CsvSource-parameterized tests, but I don't understand why this doesn't work:

@CsvSource({"https://www.scitepress.org/Link.aspx?doi=10.5220/0010404301780189, 10.5220/0010404301780189", "10.5220/0010404301780189, 10.5220/0010404301780189"}) void performSearchByIdRemovesDOITag(String input, String expectedResult) throws FetcherException { assertEquals(expectedResult, compositeIdFetcher.performSearchById(input)); }

Ah, I didn't read closely enough....

Nope. It seems to me this should work, though of course it doesn't:

@ParameterizedTest @CsvSource({"10.5220/0010404301780189, https://www.scitepress.org/Link.aspx?doi=10.5220/0010404301780189", "10.5220/0010404301780189, 10.5220/0010404301780189"}) void performSearchByIdRemovesDOITag(String expectedResult, String input) throws FetcherException { assertEquals(expectedResult, compositeIdFetcher.performSearchById(input)); }

"it doesn't" is too unspecific for me.

I was about to write follwing

If you set a breakpoint at assertEquals and press the debug button. Which values do input and expectedResult take?

Then highlight compositeIdFetcher.performSearchById(input)

Then press Alt+F8 to evaluate

While writing, it is clear: ´compositeIdFetcher.performSearchById(input)` performs a search on the string. You just want to test the parsing of the ID there.

I am so sorry, I was not clear what I meant with "Oh, the test seems to be go"

I meant: Forget my eduacation about CSV source. Just add two lines after https://github.com/koppor/jabref/blob/main/src/test/java/org/jabref/model/entry/identifier/DOITest.java#L203-L203

I write them for you:

Arguments.of("10.5220/0010404301780189", DOI.findInText("https://www.scitepress.org/Link.aspx?doi=10.5220/0010404301780189").get().getDOI()), Arguments.of("10.5220/0010404301780189", DOI.findInText("10.5220/0010404301780189").get().getDOI()),

Education:

I know, these tests are not an example for good tests. They should be rewritten so that only pairs of expected/input-doi-string are passed. For that, however, the tests need to split up. Needs concentration.

If you set a breakpoint at assertEquals and press the debug button. Which values do input and expectedResult take?
Then highlight compositeIdFetcher.performSearchById(input)
Then press Alt+F8 to evaluate

Well, that was certainly idiotic of me since DOI.findInText returns an Optional and therefore the assertEquals comparing strings will fail. In any case, I'm pleased to have learned about CsvSource. Thanks for the patient explanation!

koppor

Small comment. I assume you won't to a GUI test in this PR.

koppor · 2023-05-07T18:07:59Z

CHANGELOG.md

@@ -71,6 +71,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed the citation key generation for (`[authors]`, `[authshort]`, `[authorsAlpha]`, `authIniN`, `authEtAl`, `auth.etal`)[https://docs.jabref.org/setup/citationkeypatterns#special-field-markers] to handle `and others` properly. [koppor#626](https://github.com/koppor/jabref/issues/626)
 - We fixed the Save/save as file type shows BIBTEX_DB instead of "Bibtex library" [#9372](https://github.com/JabRef/jabref/issues/9372)
 - We fixed an issue regarding recording redundant prefixes in search history. [#9685](https://github.com/JabRef/jabref/issues/9685)
+- We fixed an issue where passing a valid URL containing a "?doi=" tag to DOI.parse yielded a "No entry found" notification. [#9821](https://github.com/JabRef/jabref/issues/9821)


It's more than ?doi=. Moreover, end users are not interested in implementation details. Here a suggestion:

Suggested change

- We fixed an issue where passing a valid URL containing a "?doi=" tag to DOI.parse yielded a "No entry found" notification. [#9821](https://github.com/JabRef/jabref/issues/9821)

- We fixed an issue where passing a URL containing a DOI lead to a "No entry found" notification. [#9821](https://github.com/JabRef/jabref/issues/9821)

Small comment. I assume you won't to a GUI test in this PR.

I'd certainly like to try. I assume I should follow this comment and in particular look at src/test/java/org/jabref/gui/search/GlobalSearchBarTest.java.

Edit: Right, you mentioned GlobalSearchBarTest above.

calixtus · 2023-05-08T09:17:04Z

Looks good. Thanks for you interest in JabRef!
Failing test is not related to you addition.

sean-leichtle added 2 commits May 7, 2023 15:17

Add method to detect and remove ?doi= tag before passing to DOI.parse

f7ebd26

Update CHANGELOG.md

90da9d6

koppor requested changes May 7, 2023

View reviewed changes

Address checkstyle issue

4932744

sean-leichtle changed the title ~~Add method to detect and remove ?doi= tag before passing to DOI.parse~~ Detect and remove ?doi= tag before passing to DOI.parse May 7, 2023

Add corrected tests to DOITest

d026110

koppor requested changes May 7, 2023

View reviewed changes

Update CHANGELOG.md

ac493f9

koppor approved these changes May 7, 2023

View reviewed changes

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 7, 2023

calixtus approved these changes May 8, 2023

View reviewed changes

calixtus merged commit 99ab75a into JabRef:main May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect and remove ?doi= tag before passing to DOI.parse #9848

Detect and remove ?doi= tag before passing to DOI.parse #9848

sean-leichtle commented May 7, 2023 •

edited

Loading

Compulsory checks

koppor May 7, 2023

sean-leichtle May 7, 2023

koppor May 7, 2023

sean-leichtle May 7, 2023

koppor May 7, 2023

koppor May 7, 2023

sean-leichtle May 7, 2023

sean-leichtle May 7, 2023

sean-leichtle May 7, 2023 •

edited

Loading

koppor May 7, 2023

sean-leichtle May 7, 2023 •

edited

Loading

koppor left a comment

koppor May 7, 2023

sean-leichtle May 7, 2023 •

edited

Loading

calixtus commented May 8, 2023

		Optional<DOI> doi = DOI.parse(identifier);
		Optional<DOI> doi = DOI.parse(filterForDOITag(identifier));

	- We fixed an issue where passing a valid URL containing a "?doi=" tag to DOI.parse yielded a "No entry found" notification. [#9821](https://github.com/JabRef/jabref/issues/9821)
	- We fixed an issue where passing a URL containing a DOI lead to a "No entry found" notification. [#9821](https://github.com/JabRef/jabref/issues/9821)

Detect and remove ?doi= tag before passing to DOI.parse #9848

Detect and remove ?doi= tag before passing to DOI.parse #9848

Conversation

sean-leichtle commented May 7, 2023 • edited Loading

Compulsory checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-leichtle May 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-leichtle May 7, 2023 • edited Loading

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-leichtle May 7, 2023 • edited Loading

Choose a reason for hiding this comment

calixtus commented May 8, 2023

sean-leichtle commented May 7, 2023 •

edited

Loading

sean-leichtle May 7, 2023 •

edited

Loading

sean-leichtle May 7, 2023 •

edited

Loading

sean-leichtle May 7, 2023 •

edited

Loading