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.

Sign up for GitHub

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

Added error msg when isbn entry was not found #7036

Merged
merged 3 commits into from
Oct 22, 2020
Merged

Added error msg when isbn entry was not found #7036

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f31a2d3
add error msg when isbn entry not found
tmrd993 Oct 22, 2020
06ff696
move error dialog to EntryTypeViewModel, add changelog entry
tmrd993 Oct 22, 2020
a951fa0
Update CHANGELOG.md
calixtus Oct 22, 2020
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
10 changes: 10 additions & 0 deletions src/main/java/org/jabref/logic/importer/fetcher/IsbnFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
import java.util.Optional;
import java.util.regex.Pattern;

import javafx.scene.control.Alert.AlertType;

import org.jabref.gui.FXDialog;
import org.jabref.logic.help.HelpFile;
import org.jabref.logic.importer.EntryBasedFetcher;
import org.jabref.logic.importer.FetcherException;
Expand Down Expand Up @@ -59,6 +62,13 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc
bibEntry = isbnViaOttoBibFetcher.performSearchById(identifier);
}

// nothing found at ebook.de and ottobib
if (!bibEntry.isPresent()) {
FXDialog noEntryFoundDialog = new FXDialog(AlertType.ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

This is not the right place. You cannot put GUI code into logic/model classes.
See https://devdocs.jabref.org/getting-into-the-code/high-level-documentation

The right place to check if the results are empty would be here: Than you have it generic as well for all kind of fetchers

jabref/src/main/java/org/jabref/gui/EntryTypeViewModel.java

Line 147 in 7f4c36c

Optional<BibEntry> result = fetcherWorker.getValue();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, my mistake. I'll fix it later today. I have another question about the ottobib fetcher though, should getting no results throw an exception? Ottobib seems like a special case since supplying an ISBN that leads to no entry doesn't give a 404 but instead leads to a special page with an error message.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about throwing a fetcher exception as well. But on the other hand we have optional for those cases.
So I would not throw an exception.
@tobiasdiez

Copy link
Member

@calixtus calixtus Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your work so far.

Exceptions are only for exceptional program states. An unkown ISBN is a possible case in Ottobib. Thats probably why Ottobib does not show 404, but a page saying: unkown ISBN.
See: https://github.com/HugoMatilla/Effective-JAVA-Summary#57-use-exceptions-only-for-exceptional-conditions

noEntryFoundDialog.setContentText("No entry found for ISBN: " + identifier + ".");
noEntryFoundDialog.show();
}

return bibEntry;
}

Expand Down
7 changes: 7 additions & 0 deletions src/main/java/org/jabref/logic/importer/fetcher/IsbnViaOttoBibFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc
throw new FetcherException("Could not ", e);
}
Element textArea = html.select("textarea").first();

// inspect the "no results" error message (if there is one)
Element potentialErrorMessageDiv = html.select("div#flash-notice.notice.add-bottom").first();
if (potentialErrorMessageDiv.hasText() && potentialErrorMessageDiv.text().contains("No Results")) {
LOGGER.error("ISBN " + identifier + " not found at ottobib");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LOGGER.error("ISBN " + identifier + " not found at ottobib");
LOGGER.error("ISBN { } not found at ottobib", identifier);

This is the recommended way

}

Optional<BibEntry> entry = Optional.empty();
try {
entry = BibtexParser.singleFromString(textArea.text(), importFormatPreferences, new DummyFileUpdateMonitor());
Expand Down