Download book cover when preview panel is opened#15250
Conversation
* feat: add a test for requirement RE02 I also had to change the access modifier to `protected` for one download function. Additionally, I refactored the download method a bit, in order to mock at test it more easily. * fix: cleanup imports
|
Hey @melkerolle! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
This comment has been minimized.
This comment has been minimized.
|
The cooldown logic looks fine but the destination.get( ) calls might fail since destination is already a Path. Also worth adding a null check after resolveNameWithType returns empty. |
This comment has been minimized.
This comment has been minimized.
Do you mean that the variable name indicates that it should be a Path and not Optional<Path>?
Are we not already doing this? |
Co-authored-by: Melker Trané <melkert@kth.se>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…gai (#62) Co-authored-by: Melker Trané <melkert@kth.se>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ All tests passed ✅🏷️ Commit: e9cff95 Learn more about TestLens at testlens.app. |
|
You committed your code on the For this pull request, this is OK. For subsequent pull requests, please start with a different branch with a proper branch name. See CONTRIBUTING.md for more details. |
Review Summary by QodoAutomatic book cover download with cooldown mechanism
WalkthroughsDescription• Automatically download book covers when preview panel opens • Implement 24-hour cooldown for failed download attempts • Create .not-available marker files for unavailable covers • Add comprehensive test coverage for cover fetching logic Diagramflowchart LR
A["Preview Panel Opens"] --> B["Check if Cover Downloaded"]
B --> C{Cover Exists?}
C -->|Yes| D["Display Cover"]
C -->|No| E["Check .not-available File"]
E --> F{Within 24 Hours?}
F -->|Yes| G["Skip Download"]
F -->|No| H["Download Cover"]
H --> I{Success?}
I -->|Yes| J["Delete .not-available File"]
I -->|No| K["Create/Update .not-available File"]
J --> L["Refresh Preview"]
K --> L
File Changes1. jabgui/src/main/java/org/jabref/gui/importer/BookCoverFetcher.java
|
Code Review by Qodo
1. Tests miss getMimeType stub
|
| protected URLDownload getURLDownload(String url) throws MalformedURLException { | ||
| return new URLDownload(url); | ||
| } |
There was a problem hiding this comment.
The only purpose of this method is for the URLDownload to be mockable in the tests
There was a problem hiding this comment.
This looks like a code smell to me. Maybe dependency injection by constructor or factory. Without seeing context right now: HttpClient as service?
There was a problem hiding this comment.
Since we want to test with real downloads anyway, mocking the URLDownload is not needed, so I will just remove this method.
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| class BookCoverFetcherTest { |
There was a problem hiding this comment.
Mark this as FetcherTest and test the real download
There was a problem hiding this comment.
I cannot simply use the FetcherTest annotation since it is in jabref/jablib and this is in jabref/jabgui and I do not really know how to solve this.
Another thing I noted was that BookCoverFetcher seems to be the only "fetcher" that is outside of jabref/jablib. Would it make more sense if BookCoverFetcher was moved?
There was a problem hiding this comment.
I would appreciate if someone more knowledgeable about the JabRef architecture would give their opinion about this issue, so it can be solved in an elegant way.
|
Closed due to contributor not responding |
|
This pull requests was closed without merging. You have been unassigned from the respective issue #14848. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
|
Can this be opened again? I did not know that not responding would lead to it being closed. I intend to implement the suggestions in the coming days |
Review Summary by QodoAutomatic book cover download with cooldown mechanism
WalkthroughsDescription• Automatically download book covers when preview panel opens • Implement 24-hour cooldown for failed download attempts • Create .not-available marker files for unavailable covers • Add comprehensive test coverage for cover fetching logic Diagramflowchart LR
A["Preview Panel Opens"] --> B["Check if Cover Downloaded"]
B --> C{Cover Exists?}
C -->|Yes| D["Display Cover"]
C -->|No| E["Check .not-available File"]
E --> F{Within 24 Hours?}
F -->|Yes| G["Skip Download"]
F -->|No| H["Download Cover"]
H --> I{Success?}
I -->|Yes| J["Delete .not-available File"]
I -->|No| K["Create/Update .not-available File"]
J --> L["Refresh Preview"]
K --> L
File Changes1. jabgui/src/main/java/org/jabref/gui/importer/BookCoverFetcher.java
|
Code Review by Qodo
1. Caches transient HTTP errors
|
|
Yes, there are several other contributors in the last weeks who just submitted some initial pr but forget about them, we became more strict with the prs. If someone is not reacting - or at least commenting - in reasonable time (about a week) we close to get the number rof open prs down. |
| download.toFile(destination); | ||
| deleteNotAvailableFileIfExists(name, directory); | ||
| } catch (FetcherClientException | FetcherServerException e) { | ||
| LOGGER.info("Remote book cover does not exist or server returned an error for URL: {}", url); | ||
| LOGGER.info("Flagging book cover as not available"); | ||
| flagAsNotAvailable(name, directory); | ||
| } catch (FetcherException e) { |
There was a problem hiding this comment.
1. Caches transient http errors 🐞 Bug ⛯ Reliability
BookCoverFetcher flags a cover as not available (creating a .not-available file that triggers a 24h cooldown) for any HTTP 5xx and all 4xx responses, which can suppress retries for transient conditions like 503 or 429 for an entire day.
Agent Prompt
### Issue description
`BookCoverFetcher` currently creates/updates the `.not-available` marker for *all* `FetcherClientException` (any 4xx) and `FetcherServerException` (any 5xx). This turns transient outages and rate limiting (e.g., HTTP 503/429) into a 24-hour suppression of download attempts.
### Issue Context
`URLDownload.openConnection()` maps HTTP statuses to these exception types, and `FetcherException` provides access to the `SimpleHttpResponse` (status code) via `getHttpResponse()`.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/importer/BookCoverFetcher.java[119-134]
- jabgui/src/main/java/org/jabref/gui/importer/BookCoverFetcher.java[61-71]
- jablib/src/main/java/org/jabref/logic/net/URLDownload.java[379-387]
- jablib/src/main/java/org/jabref/logic/importer/FetcherException.java[110-124]
### Implementation direction
- Split the catch blocks:
- For `FetcherClientException`, inspect `e.getHttpResponse().map(SimpleHttpResponse::statusCode)`:
- If 404/410 (or other permanent “no cover” statuses you decide), create/update `.not-available`.
- If 429 or other potentially transient 4xx, do **not** create `.not-available` (or create a separate marker with a short cooldown).
- For `FetcherServerException` (5xx), do **not** create `.not-available`; log and allow retry later (or implement a short backoff).
- Keep deleting `.not-available` on successful download.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
No activity for the last two weeks |
|
This pull requests was closed without merging. You have been unassigned from the respective issue #14848. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
|
I have not made changes since I was waiting for you to respond to some of my comments @Siedlerchr |
Related issues and pull requests
Closes #14848
PR Description
This PR makes it so the book cover is automatically downloaded when the preview panel is opened. To make this possible,
BookCoverFetcherwas modified to create.not-availablefiles when it could not find a remote cover. Repeated attempts to download the cover should not be made if a previous attempt failed within 24 hours. A new test file,BookCoverFetcherTest.javawas created to test this functionality. Additionaly,PreviewViewerwas modified to download the book cover when opened and refresh when that is done.We open this PR as a draft. We plan to add more tests in the coming days and complete all items in the checklist.
Steps to test
Test download and refresh
Disable download book covers in preferences. Create a new entry with the ISBN filled in. Enable download book covers in preferences. Open the preview panel for the created entry. The book cover should download and refresh the panel content.
Test download cooldown
Go to the book covers directory and create the file
isbn-9780141036144.not-available. Make a new entry with ISBN 9780141036144. The book cover should not be downloaded (see logs). Change your time to be more than 24 hours in the future. Open the entry again. The book cover should now be downloaded and the panel refreshed. The.not-availablefile should be deleted.Screenshot
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)