Adds feature CleanupJob for removing non-existent files #10960

LinusWallin · 2024-03-02T16:30:45Z

We added a functionality to remove non-existent files and added it as an option in the cleanup entries. We also grouped the checkboxes in cleanup entries according to the suggestion made by koppor.

Closes #10929

Mandatory checks

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.

Images

Non-existent file is linked to article

Cleanup Entries

Remove non-existent entries added, and the checkboxes are grouped

After cleanup the non-existent file is no longer linked to the article

Siedlerchr · 2024-03-02T22:30:08Z

CHANGELOG.md

@@ -24,6 +24,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 - We added the possibility to redownload files that had been present but are no longer in the specified location. [#10848](https://github.com/JabRef/jabref/issues/10848)
 - We added the citation key pattern `[camelN]`. Equivalent to the first N words of the `[camel]` pattern.
 - We added ability to export in CFF (Citation File Format) [#10661](https://github.com/JabRef/jabref/issues/10661).
+- We added CleanupJob for removing non-existent files and added a checkbox for it in Cleanup entries, as well as grouped the checkboxes in Cleanup entries. [#10929] (<https://github.com/JabRef/jabref/issues/10929>)


Suggested change

- We added CleanupJob for removing non-existent files and added a checkbox for it in Cleanup entries, as well as grouped the checkboxes in Cleanup entries. [#10929] (<https://github.com/JabRef/jabref/issues/10929>)

- We added a Cleanup for removing non-existent files and grouped the related options [#10929](https://github.com/JabRef/jabref/issues/10929)

Context: Changelog is intended for end users, so no technical details
And no space between the link text in round braces and the link in square brackets

Had misunderstood that there shouldn't be any technicality in changelog. It should be fixed now! Also updated the link so that it matches your suggestion!

koppor

Good first version.

Please refine the tests to also include the "real" final BibEntry. You can use following "pattern"

assertEquals(List.of(expectedBibEntry), result);

koppor · 2024-03-03T11:38:14Z

src/main/java/org/jabref/logic/cleanup/RemoveLinksToNotExistentFiles.java

+        for (LinkedFile file : files) {
+            LinkedFileHandler fileHandler = new LinkedFileHandler(file, entry, databaseContext, filePreferences);
+
+            if (file.isOnlineLink() == false) {


Please simplify the condition:

Suggested change

if (file.isOnlineLink() == false) {

if (!file.isOnlineLink()) {

https://rewriteoss.slack.com/archives/C01A843MWG5/p1709465857842709

koppor · 2024-03-03T11:58:53Z

src/main/java/org/jabref/logic/cleanup/RemoveLinksToNotExistentFiles.java

+    @Override
+    public List<FieldChange> cleanup(BibEntry entry) {
+        List<LinkedFile> files = entry.getFiles();
+        List<LinkedFile> files2 = new ArrayList<LinkedFile>();


files2 is not a "speaking" name. Please rename to something self-descriptive.

No need to repeat <LinkedFile> at the end. Use new ArrayList<>().

src/main/java/org/jabref/logic/cleanup/RemoveLinksToNotExistentFiles.java

koppor · 2024-03-03T12:03:45Z

src/test/java/org/jabref/logic/cleanup/RemoveLinksToNotExistentFilesTest.java

+        entry = new BibEntry();
+        entry.setCitationKey("Toot");
+        entry.setField(StandardField.TITLE, "test title");
+        entry.setField(StandardField.YEAR, "1989");
+        LinkedFile fileField = new LinkedFile("", fileBefore.toAbsolutePath(), "");
+        entry.setField(StandardField.FILE, FileFieldWriter.getStringRepresentation(fileField));


Please use the withField "magic".

See org.jabref.gui.edit.ReplaceStringViewModelTest#setUp for an example

koppor · 2024-03-03T12:04:19Z

src/test/java/org/jabref/logic/cleanup/RemoveLinksToNotExistentFilesTest.java

+    void deleteLinkedFile() {
+        fileBefore.toFile().delete();
+        List<FieldChange> changes = removeLinks.cleanup(entry);
+        assertFalse(changes.isEmpty());


Please test the "real" contents (if possible). If that is not possible, please comment why.

burcukilic · 2024-03-04T10:03:28Z

@koppor We have addressed the changes you requested. Could you please re-review it when you get a chance?

koppor

Thank you for the quick actions taken.

Some more "nitpicks" towards very good readable code.

src/main/java/org/jabref/logic/cleanup/RemoveLinksToNotExistentFiles.java

src/test/java/org/jabref/logic/cleanup/RemoveLinksToNotExistentFilesTest.java

* Adds feature CleanupJob for removing non-existent files (#2) * feat: RemoveLinksToNotExistentFiles.java * feat: added option to cleanup entries * feat: improved cleanup entry dialogue * Test: implements test for linked files which exist Implementation of a test which calls the cleanup functionality of RemoveLinksToNotExistentFiles with a entry that is linked to an existing file. The test makes sure that no changes are made since the linked file exists. * Refactor: refactored test to match the standard of the project Removed the JavaDoc comment from the test, since other tests in the project don't have JavaDoc:s. Also removed test from the name of the test function, as it seems that the standard way of naming tests is to not have test in the testfunctions name. * Refactor: removed system.out.print() Removed an print from the tests. * fix: non english character deleted * Docs: Added information about changes Added information about the changes made for solving issue#10929 to the CHANGES.md file. --------- Co-authored-by: burcukilic <[email protected]> * Fix: Removes unused imports and solves other errors Removes unused imports and solves the error of language keys that were not in the english laguage file. * Fix: Removes empty line and solves other issues Removes an empty line where 2 empty lines appear in after each other. Also adds brackets to URL and removes an key which is not in the english language file and was missed in the last commit. * Docs: Removes technicality from changelog Removes technical details for the change made in changelog and also follows the suggestion for removing the space and brackets between the link and the issue. * test: enhanced RemoveLinksToNotExistentFilesTest (#3) * test: enhanced RemoveLinksToNotExistentFilesTest * test: add real content * test: change structure * test: added expectedChanges * test: added a third test to RemoveLinksToNotExistentFilesTest * refactor: removed unnecessary toString * style: removed a whitespace * refactor: checkstyle formatting. formatted files according to checkstyle plugin. --------- Co-authored-by: karlsb <[email protected]> * Resolve request changes (#4) * test: enhanced RemoveLinksToNotExistentFilesTest * test: add real content * test: change structure * test: added expectedChanges * test: added a third test to RemoveLinksToNotExistentFilesTest * refactor: removed unnecessary toString * style: removed a whitespace * refactor: checkstyle formatting. formatted files according to checkstyle plugin. * refactor: replaced .get(0) with .getFirst() for first accessing element of List. Changed was required from ./gradlew rewriteDryRun --------- Co-authored-by: burcukilic <[email protected]> * Resolve more request changes and warnings (#5) * Reverse the condition and the content in RemoveLinksToNotExistentFiles Resolves comment "Reverse the condition and the content. Reason: You have BOTH (true and false) cases covered. The true case should normally come first." * Fix: Store bibFolder.resolve("test.bib") in a variable * Refactor: Rename variable fileFolder to originalFileFolder and remove comment. * Refactor: Rename defaultFileFolder to newFileFolder * Refactor: Fix indentation in RemoveLinksToNotExistentFilesTest * Refactor: Replaced Arrays.asList() with List.of() * Refactor: Replaced Arrays.asList() with List.of() * Refactor: Change to use java.nio for file deletion. * Refactor: Move comment to line above. * Refactor: Added "PDF" as third argument to LinkedFile for OnlineLink files * Refactor: Removed unused variables in RemoveLinksToNotExistentFiles * Refactor: Throw IOException in RemoveLinksToNotExistentFilesTest functions * Refactor: Tests assert using List.of() in RemoveLinksToNotExistentFilesTest * Refactor: Corrected Arguments for LinkedFile in RemoveLinksToNotExistentFilesTest * Refactor: Class fields into local variables This refactor fixed 3 warnings. * docs: updated docs to remove merge conflict --------- Co-authored-by: Linus Wallin <[email protected]> Co-authored-by: burcukilic <[email protected]> Co-authored-by: Linus Wallin <[email protected]>

burcukilic · 2024-03-06T14:58:49Z

@koppor Hi, we have fixed the parts you have mentioned, could you check it again?

koppor · 2024-03-06T15:35:34Z

@burcukilic Thank you for the follow up. - Looks good. Regrading the process, some small thing: In future, please choose another branch name than main. 😅

Siedlerchr reviewed Mar 2, 2024

View reviewed changes

koppor requested changes Mar 3, 2024

View reviewed changes

koppor requested changes Mar 4, 2024

View reviewed changes

karlsb force-pushed the main branch from 96966a7 to f5efb34 Compare March 6, 2024 14:31

LinusWallin requested a review from koppor March 6, 2024 15:22

koppor approved these changes Mar 6, 2024

View reviewed changes

koppor added this pull request to the merge queue Mar 6, 2024

Merged via the queue into JabRef:main with commit 985b40b Mar 6, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds feature CleanupJob for removing non-existent files #10960

Adds feature CleanupJob for removing non-existent files #10960

LinusWallin commented Mar 2, 2024

Siedlerchr Mar 2, 2024

LinusWallin Mar 3, 2024

koppor left a comment

koppor Mar 3, 2024

koppor Mar 3, 2024

koppor Mar 3, 2024

koppor Mar 3, 2024

burcukilic commented Mar 4, 2024

koppor left a comment

burcukilic commented Mar 6, 2024

koppor commented Mar 6, 2024

	- We added CleanupJob for removing non-existent files and added a checkbox for it in Cleanup entries, as well as grouped the checkboxes in Cleanup entries. [#10929] (<https://github.com/JabRef/jabref/issues/10929>)
	- We added a Cleanup for removing non-existent files and grouped the related options [#10929](https://github.com/JabRef/jabref/issues/10929)

	if (file.isOnlineLink() == false) {
	if (!file.isOnlineLink()) {

Adds feature CleanupJob for removing non-existent files #10960

Adds feature CleanupJob for removing non-existent files #10960

Conversation

LinusWallin commented Mar 2, 2024

Mandatory checks

Images

Siedlerchr Mar 2, 2024

Choose a reason for hiding this comment

LinusWallin Mar 3, 2024

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

koppor Mar 3, 2024

Choose a reason for hiding this comment

koppor Mar 3, 2024

Choose a reason for hiding this comment

koppor Mar 3, 2024

Choose a reason for hiding this comment

koppor Mar 3, 2024

Choose a reason for hiding this comment

burcukilic commented Mar 4, 2024

koppor left a comment

Choose a reason for hiding this comment

burcukilic commented Mar 6, 2024

koppor commented Mar 6, 2024