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

Refactor of DOI import failure dialog, import format reader and clipboard manager #8839

Merged
merged 37 commits into from
Aug 6, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
02481fa
Rename cleanup actions: "Make LaTeX ready" and "Make BibTeX ready"
zkl-ai Apr 30, 2022
07345ea
modify dio exception handler
zkl-ai May 22, 2022
f786412
Merge branch 'JabRef:main' into improve-DOI-import-failure-dialog
zkl-ai May 22, 2022
f06021a
modify dio exception handler change dialog from "No DOI data exits" t…
zkl-ai May 22, 2022
2348a34
Merge remote-tracking branch 'origin/improve-DOI-import-failure-dialo…
zkl-ai May 22, 2022
d32eae2
modify Localization.lang(message)
zkl-ai May 22, 2022
987d5df
Merge branch 'JabRef:main' into improve-DOI-import-failure-dialog
zkl-ai May 22, 2022
a6ec469
modify Localization.lang("Client Error") and
zkl-ai May 22, 2022
b3ac167
pass LocalizationConsistencyTest
zkl-ai May 22, 2022
6bf38c4
pass fetcher test
zkl-ai May 22, 2022
58361cb
Merge branch 'JabRef:main' into improve-DOI-import-failure-dialog
zkl-ai May 24, 2022
8e5b82a
improve the implementation with two add Exception DOIDataNotFoundExce…
zkl-ai May 24, 2022
d406d5a
Merge remote-tracking branch 'origin/improve-DOI-import-failure-dialo…
zkl-ai May 24, 2022
38f3e52
improve the implementation with two add Exception DOIDataNotFoundExce…
zkl-ai May 24, 2022
6b7dfa9
Merge remote-tracking branch 'upstream/main' into improve-DOI-import-…
Siedlerchr Jul 14, 2022
2561d32
Add FetcherClientException and FetcherServerException
Siedlerchr Jul 14, 2022
a02f5d4
rename test
Siedlerchr Jul 14, 2022
5dfdc8e
refactor impor format reader and clipboard manager
Siedlerchr Jul 14, 2022
3c0ee30
Merge remote-tracking branch 'upstream/main' into improve-DOI-import-…
Siedlerchr Jul 15, 2022
9ed8131
Refactor clipboard manager
Siedlerchr Jul 15, 2022
890e623
remove unused prefs
Siedlerchr Jul 15, 2022
c6deb0c
revert style changes
Siedlerchr Jul 15, 2022
4b0aeb2
fix checkstlye
Siedlerchr Jul 15, 2022
a03e25f
do not throw 404 errors from UrlDownload
Siedlerchr Jul 16, 2022
32fd370
better error messages
Siedlerchr Jul 16, 2022
a9c4589
checkstyle
Siedlerchr Jul 16, 2022
0165d14
Merge remote-tracking branch 'upstream/main' into improve-DOI-import-…
Siedlerchr Jul 18, 2022
73a88b7
improve dialog messages
Siedlerchr Jul 18, 2022
5060e34
change l10n
Siedlerchr Jul 19, 2022
b8a83ac
Merge remote-tracking branch 'upstream/main' into improve-DOI-import-…
Siedlerchr Jul 19, 2022
4c0ebd1
fix merge conflicts
Siedlerchr Jul 19, 2022
0cc4826
fix l10n messages
Siedlerchr Jul 19, 2022
369310e
fix checkstyle and fetcher
Siedlerchr Jul 20, 2022
d26e288
Merge remote-tracking branch 'upstream/main' into improve-DOI-import-…
Siedlerchr Aug 5, 2022
c47ea2d
improve error messages
Siedlerchr Aug 5, 2022
be99801
further fixes
Siedlerchr Aug 5, 2022
7c609f3
fix l10n
Siedlerchr Aug 6, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public void fetchAndMerge(BibEntry entry, List<Field> fields) {
})
.onFailure(exception -> {
LOGGER.error("Error while fetching bibliographic information", exception);
String localMessage = exception.getCause().getLocalizedMessage();
String localMessage = exception.getLocalizedMessage();
Copy link
Member

@Siedlerchr Siedlerchr May 22, 2022

Choose a reason for hiding this comment

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

Why do you use localized messge for checking here? wouldn't that faill in different languages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need to change the dialog title and content according to the localized message.

Copy link
Contributor

Choose a reason for hiding this comment

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

But as far as I can see, you only rely on this variable for the check logic. The value is never actually used for the dialog content.

// client error
if (localMessage.startsWith("Client")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the check, I am talking about. Personally, I think this should not be a string comparison, but a different, more robust check.

I assume, that @Siedlerchr had something similarly in mind with his remark.

dialogService.showInformationDialogAndWait(Localization.lang("Lookup DOI"), Localization.lang("No DOI data was found"));
Expand Down
20 changes: 18 additions & 2 deletions src/main/java/org/jabref/logic/importer/fetcher/DoiFetcher.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.logic.importer.fetcher;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.net.URL;
import java.util.Collections;
Expand Down Expand Up @@ -72,7 +73,6 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc
return new Medra().performSearchById(identifier);
}
URL doiURL = new URL(doi.get().getURIAsASCIIString());

// BibTeX data
URLDownload download = getUrlDownload(doiURL);
download.addHeader("Accept", MediaTypes.APPLICATION_BIBTEX);
Expand All @@ -81,7 +81,12 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc
bibtexString = download.asString();
} catch (IOException e) {
// an IOException will be thrown if download is unable to download from the doiURL
throw new FetcherException(Localization.lang("No DOI data exists"), e);
// dispatch the IOException
if (e instanceof FileNotFoundException) {
throw new FetcherException(Localization.lang("Client Error"), e);
} else {
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
throw new FetcherException(Localization.lang("Server Error"), e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to give those exceptions more meaningful types? Client and Server error seem a bit too arbitrary to me. Also, are exceptions usually localized?

Copy link
Member

Choose a reason for hiding this comment

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

I would introduce new exceptions FetcherClientException extends FetcherException and FetcherServerException extends FetcherException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be great! If it is finished, could you inform me?

}
}

// BibTeX entry
Expand Down Expand Up @@ -109,6 +114,17 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc
}
}

/**
* Returns client error or server error according to the message
*
* @param exception the IOException
*/
private String dispatchIOException(IOException exception) {
String classification = "Client Error";

return classification;
}

private void doPostCleanup(BibEntry entry) {
new FieldFormatterCleanup(StandardField.PAGES, new NormalizePagesFormatter()).cleanup(entry);
new FieldFormatterCleanup(StandardField.URL, new ClearFormatter()).cleanup(entry);
Expand Down
9 changes: 0 additions & 9 deletions src/main/java/org/jabref/logic/net/URLDownload.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;

import org.jabref.logic.l10n.Localization;
import org.jabref.logic.util.io.FileUtil;
import org.jabref.model.util.FileHelper;

Expand Down Expand Up @@ -361,14 +360,6 @@ private URLConnection openConnection() throws IOException {
connection = new URLDownload(newUrl).openConnection();
}
}
// status code 4xx
if (status > HttpURLConnection.HTTP_BAD_REQUEST && status < HttpURLConnection.HTTP_INTERNAL_ERROR) {
throw new IOException(Localization.lang("Client Error %0", status));

// status code 5xx
} else if (status >= HttpURLConnection.HTTP_INTERNAL_ERROR) {
throw new IOException(Localization.lang("Server Error %0", status));
}
}

// this does network i/o: GET + read returned headers
Expand Down
6 changes: 2 additions & 4 deletions src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -580,8 +580,6 @@ No\ journal\ names\ could\ be\ abbreviated.=No journal names could be abbreviate

No\ journal\ names\ could\ be\ unabbreviated.=No journal names could be unabbreviated.

No\ DOI\ data\ exists=No DOI data exists
koppor marked this conversation as resolved.
Show resolved Hide resolved

not=not

not\ found=not found
Expand Down Expand Up @@ -2488,8 +2486,8 @@ Success\!\ Finished\ writing\ metadata.=Success! Finished writing metadata.
Custom\ API\ key=Custom API key
Check\ %0\ API\ Key\ Setting=Check %0 API Key Setting

Client\ Error\ %0=Client Error %0
Client\ Error=Client Error
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
DOI\ server\ not\ available=DOI server not available
Lookup\ DOI=Lookup DOI
No\ DOI\ data\ was\ found=No DOI data was found
Server\ Error\ %0=Server Error %0
Server\ Error=Server Error
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;

@FetcherTest
Expand Down Expand Up @@ -117,6 +118,20 @@ public void testPerformSearchInvalidDOI() {
assertThrows(FetcherException.class, () -> fetcher.performSearchById("10.1002/9781118257517F"));
}

// test invalid DOI client error message "Client Error"
@Test
public void testPerformSearchInvalidDOIClientErrorMessage1() {
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
FetcherException exception = assertThrows(FetcherException.class, () -> fetcher.performSearchById("10.1002/9781118257517F"));
assertTrue(exception.getLocalizedMessage().contentEquals("Client Error"));
}

// test invalid DOI client error message "Client Error"
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
@Test
public void testPerformSearchInvalidDOIClientErrorMessage2() {
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
FetcherException exception = assertThrows(FetcherException.class, () -> fetcher.performSearchById("10.1002/9781517F"));
assertTrue(exception.getLocalizedMessage().contentEquals("Client Error"));
}

@Test
public void testPerformSearchNonTrimmedDOI() throws FetcherException {
Optional<BibEntry> fetchedEntry = fetcher.performSearchById("http s://doi.org/ 10.1109 /ICWS .2007.59 ");
Expand Down
16 changes: 0 additions & 16 deletions src/test/java/org/jabref/logic/net/URLDownloadTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,6 @@ public void testStringDownloadWithSetEncoding() throws IOException {
assertTrue(dl.asString().contains("Google"), "google.com should contain google");
}

// test client error 404
@Test
public void testStringDownloadThrowClientError() throws IOException {
URLDownload dl = new URLDownload(new URL("http://httpstat.us/404"));
IOException exception = assertThrows(IOException.class, dl::asString);
assertTrue(exception.getLocalizedMessage().contentEquals("Client Error 404"), "Should be Client Error.");
}

// test server error 503
@Test
public void testStringDownloadThrowServerError() throws IOException {
URLDownload dl = new URLDownload(new URL("http://httpstat.us/503"));
IOException exception = assertThrows(IOException.class, dl::asString);
assertTrue(exception.getLocalizedMessage().contentEquals("Server Error 503"), "Should be Server Error.");
}

@Test
public void testStringDownload() throws IOException {
URLDownload dl = new URLDownload(new URL("http://www.google.com"));
Expand Down