Implement mass addition of bib information (Closes #372) #12025
Conversation
…rences, attach file, send entries, special fields) and improved layout
…D_ENTRIES - Reintroduce previously removed MERGE_WITH_FETCHED_ENTRY action - Add new BATCH_MERGE_WITH_FETCHED_ENTRIES action after it - Fixes unintended removal of single-entry merge functionality
- Improve code structure and reduce duplication - Enhance error handling and user feedback - Streamline fetch and merge process for better performance - Implement more robust field handling and merging logic - Optimize batch processing capabilities - Refactor to use Java 8+ features for cleaner code - Improve modularity and separation of concerns - Reduce unnecessary notifications
|
I dont see any new test case. It should be possible to add at least one test case! |
|
@koppor thank you for the comment. I'm also part of the team responsible for working on this issue. From what I know, no tests have been written for the |
There was a problem hiding this comment.
The functionaliy needs to be in the "Lookup" menu. Also add "Get bibliographic data" to there.
The title "Mass getting" sounds too technical.
Rename to "Get bibliographic data from DOI/ISBN/... (fully automated)" to make clear that no GUI interaction is performed.
Can you use
new OrFields(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT).getDisplayName()
instead of DOI/ISSN/...
This would make it consistent to org.jabref.gui.mergeentries.MergeWithFetchedEntryAction#execute
Replace
new OrFields(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT).getDisplayName()
by
new OrFields(FetchAndMergeEntry.SUPPORTED_FIELDS)
CHANGELOG.md
Outdated
| @@ -38,6 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv | |||
| - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) | |||
| - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) | |||
| - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) | |||
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |||
There was a problem hiding this comment.
Please add where you added it.
It really needs to be in the "Lookup" menu.
We want to keep the right click menu small.
There was a problem hiding this comment.
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |
| - Added functionality for mass addition of bibliographic information for multiple entries to the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) |
| () -> { | ||
| if (hasAnySupportedField(entry)) { | ||
| dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); | ||
| } |
There was a problem hiding this comment.
It is unclear why this function is there.
I think, this can just be removed. No need to inform the user.
Moreover, the logic is wrong. -- It is OK if the entry has an identifier field
There was a problem hiding this comment.
We also can use .map and .flatMap on optionals. I believe we can make the logic clearer by using those.
| @@ -92,7 +93,8 @@ public static ContextMenu create(BibEntryTableViewModel entry, | |||
| new SeparatorMenuItem(), | |||
|
|
|||
| new ChangeEntryTypeMenu(libraryTab.getSelectedEntries(), libraryTab.getBibDatabaseContext(), undoManager, entryTypesManager).asSubMenu(), | |||
| factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)) | |||
| factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)), | |||
| factory.createMenuItem(StandardActions.BATCH_MERGE_WITH_FETCHED_ENTRIES, new MultiEntryMergeWithFetchedDataAction(libraryTab, preferences, taskExecutor, libraryTab.getBibDatabaseContext(), dialogService, undoManager)) | |||
There was a problem hiding this comment.
libraryTab is already passed, thus you can call libraryTab.getBibDatabaseContext() inside the method.
| } | ||
|
|
||
| public void fetchAndMergeBatch(List<BibEntry> entries) { | ||
| entries.forEach(entry -> SUPPORTED_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field))); |
There was a problem hiding this comment.
Rename variable SUPPORTED_FIELDS to SUPPORTED_IDENTIFIER_FIELDS.
| } | ||
|
|
||
| private void handleFetchException(Exception exception, IdBasedFetcher fetcher, BibEntry entry) { | ||
| if (hasAnySupportedField(entry)) { |
There was a problem hiding this comment.
I think, this is not required. The id fetcher should only be called if the id is present. If id present, the exception is shown.
--> remove this if (and remove hasAnySupportedField alltogether)
| // Set of fields present in both the original and fetched entries | ||
| Set<Field> jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| jointFields.addAll(fetchedEntry.getFields()); | ||
| Set<Field> originalFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| originalFields.addAll(originalEntry.getFields()); |
There was a problem hiding this comment.
This is duplicated coude. Please refactor to a method.
| if (edited) { | ||
| ce.end(); | ||
| undoManager.addEdit(ce); | ||
| dialogService.notify(Localization.lang("Updated entry with fetched information")); |
There was a problem hiding this comment.
Add the citation key to this information - otherwise it is unclear which entry is used.
Use getCitationKey().orElse(getAuthorTitleYear()
Without any example, I cannot say anything. I think, it could be hard. You first need to factor out Second step is to make use of With that refactoring, the whole code can be moved to |
| private void executeFetchTask(IdBasedFetcher fetcher, Field field, String fieldContent, BibEntry entry) { | ||
| BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) |
There was a problem hiding this comment.
We don't need the fieldContent parameter here as it can be taken from the entry. Ideally we want to keep the number of parameters passed to a method to a minimum to achieve cleaner code.
| () -> { | ||
| if (hasAnySupportedField(entry)) { | ||
| dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); | ||
| } |
There was a problem hiding this comment.
We also can use .map and .flatMap on optionals. I believe we can make the logic clearer by using those.
| if (!oldType.equals(newType)) { | ||
| originalEntry.setType(newType); | ||
| ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); | ||
| edited = true; |
There was a problem hiding this comment.
I don't think we need the edited boolean flag anymore, we can use NamedCompound#hasEdits.
| Optional<String> originalString = originalEntry.getField(field); | ||
| Optional<String> fetchedString = fetchedEntry.getField(field); | ||
|
|
||
| if (fetchedString.isPresent()) { |
There was a problem hiding this comment.
Let's reduce nesting and return early here.
| } | ||
|
|
||
| private boolean isEdited(BibEntry originalEntry, NamedCompound ce, Set<Field> jointFields, Set<Field> originalFields, boolean edited) { | ||
| for (Field field : originalFields) { |
There was a problem hiding this comment.
Do we need to pass the edited boolean here? We want to minimize the number of parameters.
There was a problem hiding this comment.
The method is named isEdited suggesting a read-only behavior but it can clear some fields, this is not ideal...
| public void execute() { | ||
| List<BibEntry> selectedEntries = libraryTab.getSelectedEntries(); | ||
|
|
||
| // Create an instance of FetchAndMergeEntry |
There was a problem hiding this comment.
Not a very useful comment 😅
HoussemNasri
added
the
status: changes required
- Relocate action from Right-click menu to "Lookup" menu for consistency - Rename action from "Mass Getting" to "Get bibliographic data from DOI/ISBN/... (fully automated)" - Other modifications not finished yet
…ability - Rename SUPPORTED_FIELDS to SUPPORTED_IDENTIFIER_FIELDS for clarity - Remove unnecessary hasAnySupportedField method and associated checks - Refactor duplicate code into utility methods (getFields, getJointFields) - Improve error handling with more specific exception messages - Remove edited boolean flag in favor of NamedCompound#hasEdits() - Enhance use of Optional, Stream, and method references - Reduce method parameters for better maintainability - Improve user notifications with entry citation keys - Restructure methods to separate concerns more effectively Minor modifications to MergeWithFetchedEntryAction.java to align with the new version of FetchAndMergeEntry.java
KUMOAWAI
force-pushed
the
fix-for-issue-#372
branch
from
93a9a8d to
74f2473
Compare
CHANGELOG.md
Outdated
| @@ -38,6 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv | |||
| - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) | |||
| - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) | |||
| - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) | |||
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |||
There was a problem hiding this comment.
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |
| - Added functionality for mass addition of bibliographic information for multiple entries to the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) |
|
|
||
| new SeparatorMenuItem(), | ||
|
|
||
| // Adding new menu item for mass bibliographic data fetching |
| dialogService.notify(message); | ||
| } | ||
|
|
||
| private Set<Field> getFields(BibEntry entry) { |
There was a problem hiding this comment.
New Java21: SortedSet. Otherwise, it is not clear that the returned setis sorted.
There was a problem hiding this comment.
However, the sorting is not necessary at all if I investigate the calling methods.
| } | ||
|
|
||
| private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) { | ||
| NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); |
There was a problem hiding this comment.
Two entries are morged, not a single one.
| NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); | |
| NamedCompound ce = new NamedCompound(Localization.lang("Merge entries without user interaction")); |
| } | ||
| } | ||
|
|
||
| private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) { |
There was a problem hiding this comment.
This whole method can be go into logic, because there is no gui interaction - and it should be testable with a JUnit test.
Annote the test using JUnit
@AllowedToUseSwing("Requires undo/redo functionality")
There was a problem hiding this comment.
I apologize for the oversight; I should have provided my comments after pushing the changes. Over the past few hours, I have been working on optimizing the code further and transitioning the class into the logic directory.
My current progress is that '''
Refactor FetchAndMergeEntry:
The single-entry fetching and merging logic remains in FetchAndMergeEntry.
Extracted logic for batch processing and handling multiple entries from FetchAndMergeEntry.
Introduce MergingIdBasedFetcher:
Created the MergingIdBasedFetcher class in the org.jabref.logic.importer.fetcher package.
MergingIdBasedFetcher handles mass operations such as fetching and merging multiple BibEntry objects.
Uses NotificationService to provide error and status notifications, removing reliance on user confirmation dialogs for better automation.
Create MergeEntriesHelper:
Introduced the MergeEntriesHelper class to centralize and handle the actual merging logic between BibEntry instances.
The class includes methods to update entry types, fields, and remove obsolete fields during merging. '''
| ? Localization.lang("Updated entry with fetched information [%0]", citationKey) | ||
| : Localization.lang("No new information was added [%0]", citationKey); |
There was a problem hiding this comment.
Is this output for each entry?
Imagine a user updates 100 entries - are there 100 messages? How should the user understand this data?
Maybe better to have a single notification with: entries X udpates, where X is a list of the citation keys.
Even better it would be to have a magic JabRef group which is called "JR - updated", where all modified entries are added to. Before the action, all entries are removed from this group. (Not sure if you have time for that)
- Introduce `MergingIdBasedFetcher` for batch operations - Create `MergeEntriesHelper` to encapsulate merge logic - Refactor `FetchAndMergeEntry` for improved separation of concerns - Implement `getFetcherForField` method in `MergingIdBasedFetcher` - Add background processing with progress updates - Enhance user notifications and error handling - Optimize logging
| @@ -1679,6 +1679,10 @@ No\ %0\ found=No %0 found | |||
| Entry\ from\ %0=Entry from %0 | |||
| Merge\ entry\ with\ %0\ information=Merge entry with %0 information | |||
| Updated\ entry\ with\ info\ from\ %0=Updated entry with info from %0 | |||
| Mass\ getting\ bibliographic\ data\ from\ %0=Mass getting bibliographic data from %0 | |||
There was a problem hiding this comment.
I think, this string should get obsoelete
|
@KUMOAWAI I think, you do see that tests are failing? https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025 |
Yes, I will make an effort to resolve the issues |
Hello Koppor, I've been investigating the failing tests in MainArchitectureTest and testsAreIndependentOfGuiPreferences. The core issue is that the MergingIdBasedFetcher class is now in the logic layer, while it has dependencies on GUI components like DialogService and GuiPreferences. These dependencies violate our current architectural rules but seem integral to the system's functionality. I'm finding it challenging to refactor without potentially breaking existing features. Could we consider:
Thank you for your time. |
| return BackgroundTask.wrap(() -> List.of()); | ||
| } | ||
|
|
||
| BackgroundTask<List<String>> backgroundTask = new BackgroundTask<>() { |
There was a problem hiding this comment.
The fetcher itself should not have UI dependencies and implement the background task, that is a UI related component. This has to be done on the caller SIde which I presume is somewhere in the UI
There was a problem hiding this comment.
Thank you for the feedback!
Will refactor to move UI dependencies to the caller side and update the PR in these two days.
There was a problem hiding this comment.
Yeah, the wrapping can be done in the caller.
| import org.jabref.model.entry.field.FieldFactory; | ||
| import org.jabref.model.entry.types.EntryType; | ||
|
|
||
| public class MergeEntriesHelper { |
There was a problem hiding this comment.
Isn't @AllowedToUseSwing missing as I wrote before in the reivew comments?
There was a problem hiding this comment.
I need to admit that when I saw your earlier comment about moving "this whole method can go into logic", I didn't truly understand the full implications. I thought simply moving the method to the logic layer was sufficient, and missed the importance of the "@AllowedToUseSwing". I'll remove the Swing dependency and update the code later. Sorry for my oversight.
There was a problem hiding this comment.
I'll remove the Swing dependency
Please think a bit. I think, i posted the following example:
@AllowedToUseSwing("UndoableUnabbreviator and UndoableAbbreviator requires Swing Compound Edit in order test the abbreviation and unabbreviation of journal titles")
You need undo/redo. This is OK then.
Only not OK is the background task.
| private final BibDatabaseContext bibDatabaseContext; | ||
| private final DialogService dialogService; | ||
|
|
||
| public MergingIdBasedFetcher(GuiPreferences preferences, UndoManager undoManager, BibDatabaseContext bibDatabaseContext, DialogService dialogService) { |
There was a problem hiding this comment.
I think, I wrote to use NotificationService here. This is in logic, whereas DialogService is in GUI.
There was a problem hiding this comment.
Sorry for my oversight. I did notice your previous comment about using NotificationService and tried to implement it. However, I had difficulties implementing the feature without implementing the abstract class, so I kept using DialogService as it was already working. I understand now this breaks the architectural rules and will work on properly implementing NotificationService instead. Sorry for wasting your time again.
There was a problem hiding this comment.
I don't know about your concrete difficulties. - If you just change the parameter type here into NotificationService, it should work. You are only using the notify method.
There was a problem hiding this comment.
Thank you for the explanation! I actually fixed it after my previous comment just now. I was confused cause I did not find NotificationService instance in the MainMenu class. And I was so exhausted for refactoring all the codes at that time, so I did not notice that I just need to pass DialogService instance. Sorry for the confusion in my earlier messages.
|
@KUMOAWAI The unit tests fail. Please check. Current link: https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025 |
The tests are passing both in my local environment and in my repository's PR. This seems to be a GitHub Actions issue. Could any maintainer please rerun the tests to verify if this is an intermittent failure? Thanks! |
Modify the CHANGELOG.md as suggested Co-authored-by: Loay Ghreeb <[email protected]>
- Reorder parameters to follow source->target convention in BatchEntryMergeWithFetchedDataAction and MergeEntriesHelper - Consistently use entryFromFetcher and entryFromLibrary as parameter names
| @@ -1,3 +1,4 @@ | |||
|
|
|||
| @@ -0,0 +1,103 @@ | |||
|
|
|||
There was a problem hiding this comment.
Move the existing context menu action to the menu - above your new action:
It should be automatically deactivated if multiple entries are selected (no additional work on your side)
I thought of keeping the other action at the context menu, but then your action also woud need to be added, which clutters the menu.
I think, you want to finish this one instead on working on perfection of the feature.
One could add all entries which need a manual review on a Group "needs review [JabRef]". Example: The following entry needs manual check, because an author is missing. -- However, this feature should be configurable in the preferences - and I think, this is too much work for now. I just wanted to write down the idea.
@Article{Scheer2020,
author = {Scheer, Dietmar and Glodd, Oliver and Günther, Hartmut and Duhr, Yves},
journal = {Sonderprojekte ATZ/MTZ},
title = {STAR3 - Eine neue Generation der E/E-Architektur},
year = {2020},
issn = {2509-4610},
month = dec,
number = {S1},
pages = {72--79},
volume = {25},
doi = {10.1007/s41491-020-0056-5},
publisher = {Springer Science and Business Media LLC},
}Another follow-up can be more "intelligent" automatic merges of fields. In case the number of authors differ, add authors from extenral (but keep the existing authors) <-- this would be the conservative approach to trust the local data more than the external one.
|
|
||
| task.setTitle(Localization.lang("Fetching and merging entries")); | ||
| task.showToUser(true); | ||
|
|
||
| task.onSuccess(updatedEntries -> | ||
| handleSuccess(updatedEntries, context)); | ||
|
|
||
| task.onFailure(ex -> | ||
| handleFailure(ex, context.notificationService())); |
There was a problem hiding this comment.
You can chain these calls.
General comment - do not do now - read on -- You can also use wrap for a shorter syntax. See org.jabref.gui.preferences.preview.PreviewTabViewModel#setValues for an example.
| int totalEntries = context.entries().size(); | ||
|
|
||
| for (int i = 0; i < totalEntries && !task.isCancelled(); i++) { | ||
| BibEntry libraryEntry = context.entries().get(i); | ||
| updateProgress(i, totalEntries, task); | ||
| fetchAndMergeEntry(context, libraryEntry); | ||
| } | ||
|
|
||
| finalizeCompoundEdit(context); | ||
| return context.updatedEntries(); |
There was a problem hiding this comment.
The parameter task is strange. Can you inline this method?
|
|
||
| context.fetcher().fetchEntry(libraryEntry) | ||
| .filter(MergingIdBasedFetcher.FetcherResult::hasChanges) | ||
| .ifPresent(result -> Platform.runLater(() -> { |
There was a problem hiding this comment.
Why is runLater() needed? The whole thing runs in a background task - and there is no interaction with the UI, is it?
|
|
||
| private static void finalizeCompoundEdit(MergeContext context) { | ||
| if (!context.updatedEntries().isEmpty()) { | ||
| Platform.runLater(() -> { |
| } | ||
| } | ||
|
|
||
| private static void updateProgress(int currentIndex, int totalEntries, BackgroundTask<?> task) { |
There was a problem hiding this comment.
static method are a strong indicator for a bad design.
Please, simply create a new class inherting from BackgroundTask. See org.jabref.gui.externalfiles.UnlinkedFilesCrawler for an example.
| * Source entry data is merged into the library entry, with longer field values preferred | ||
| * and obsolete fields removed. | ||
| */ | ||
| public final class MergeEntriesHelper { |
There was a problem hiding this comment.
There is no test for this. Please add a JUNit test (ctrl+shift+t in IntelliJ gets you started). -- Reading a unit test makes it easier to check whether the logic s correct.
| @@ -1,3 +1,4 @@ | |||
|
|
|||
There was a problem hiding this comment.
No empty line at the beginning of a file (not sure why this file was touched by you)
| private Optional<IdBasedFetcher> getFetcherForField(Field field) { | ||
| return switch (field) { | ||
| case StandardField.DOI -> Optional.of(new DoiFetcher(importFormatPreferences)); | ||
| case StandardField.ISBN -> Optional.of(new IsbnFetcher(importFormatPreferences)); | ||
| case StandardField.EPRINT -> Optional.of(new IacrEprintFetcher(importFormatPreferences)); | ||
| default -> Optional.empty(); | ||
| }; | ||
| } |
…imized MergingIdBasedFetcher - Moved the original single entry update feature button to the lookup menu. MergingIdBasedFetcher: - Deleted redundant getFetcherForField() method - Add StringNormalizer to standardize field value comparisons - Update existing fields when their normalized values differ - Track and report updated fields in FetcherResult - Improve debug logging for merge operations BatchEntryMergeTask (New): - Extract background processing logic from BatchEntryMergeWithFetchedDataAction - Introduce MergeContext record for operation parameters - Improve error handling and progress reporting BatchEntryMergeWithFetchedDataAction: - Split into multiple classes for better separation of concerns - Move single entry update feature button to lookup menu - Simplify to focus on operation orchestration MergeEntriesHelper: - Made methods return boolean to avoid redundant merge actions
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
| class StringNormalizer { | ||
| public static String normalize(String value) { | ||
| if (value == null) { | ||
| return ""; | ||
| } | ||
| return value.trim() | ||
| .replaceAll("\\s+", " ") | ||
| .replaceAll("\\r\\n|\\r|\\n", " ") | ||
| .replaceAll("\\s*-+\\s*", "-") | ||
| .replaceAll("\\s*,\\s*", ", ") | ||
| .replaceAll("\\s*;\\s*", "; ") | ||
| .replaceAll("\\s*:\\s*", ": "); | ||
| } | ||
| } |
There was a problem hiding this comment.
JavaDoc missing.
But! Mnove this to org.jabref.model.strings.StringUtil and add tests (also adapt the architecture test to allow longer lines)
Moreover, use Pattern.compile - and also combine the regex expressions into one.
There was a problem hiding this comment.
Do we really need regex for this? What about strip() https://www.baeldung.com/java-string-remove-whitespace ?
There was a problem hiding this comment.
He wants to keep the whitepace after comma, colon and :
Meaning
abc : def -> abc: def
abc . def ->abc. def
Therefore, I asked for tests
| private FetcherResult mergeBibEntries(BibEntry entryFromLibrary, | ||
| BibEntry fetchedEntry) { | ||
| BibEntry mergedEntry = new BibEntry(entryFromLibrary.getType()); | ||
|
|
||
| entryFromLibrary.getFields().forEach(field -> | ||
| entryFromLibrary.getField(field) | ||
| .ifPresent(value -> mergedEntry.setField(field, value))); | ||
|
|
||
| Set<Field> updatedFields = updateFieldsFromSource(fetchedEntry, mergedEntry); | ||
|
|
||
| return new FetcherResult(entryFromLibrary, mergedEntry, | ||
| !updatedFields.isEmpty(), updatedFields); | ||
| } |
There was a problem hiding this comment.
Sure, will add tests in these two days.
Update JabRef_en.properties as koppor suggested Co-authored-by: Oliver Kopp <[email protected]>
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
- Moved normalize() method to StringUtil - use Pattern.compile for NORMALIZE_PATTERN - Update shouldUpdateField() method in MergingIdBasedFetcher to use StringUtil.normalize()
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
|
Hi maintainers, |
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
|
Hi maintainers, when I was writing some tests, I realized that we don't have a cancellation feature yet. Would it be valuable to add proper cancellation support, particularly for handling large batches or long-running fetch operations? |
There was a problem hiding this comment.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
Closes https://github.com/koppor/jabref/issues/372
Description
I am a teammate with @Arvinyuchen, who was assigned to issue #12275.
This pull request addresses issue #12275 by implementing mass addition of bibliographic information for multiple entries, while also improving the existing single-entry functionality.
Changes made:
Added new classes for mass bibliographic updates:
MergingIdBasedFetcherfor handling batch entry processingMergeEntriesHelperfor managing merge operationsMultiEntryMergeWithFetchedDataActionfor coordinating bulk updatesFeatures of mass update system:
Improved existing functionality:
FetchAndMergeEntryclass for better single-entry handlingThis implementation follows the "Better implementation" approach suggested in the original issue, where popups are not shown for each entry, and the longer string is taken when merging fields.
Testing
Manual testing has been performed to verify functionality:
Batch processing of multiple entries
Field merging behavior
Error handling and user notifications
UI responsiveness during background operations
Automated tests will be added in future updates.
CI/CD Issues
Several CI/CD errors were encountered during the automated checks, but these appear to be unrelated to the changes made in this PR. The issues include:
Pages Deployment Issues: These seem to be related to the repository's GitHub Pages configuration and are likely not caused by our code changes.
Link Checking Failures: The
lycheeworkflow reported broken links in documentation files. These are pre-existing issues in the repository's documentation and not introduced by our changes.Gradle Fetcher Tests Failures: These failures appear to be due to deprecated functionality in the Gradle setup action and possibly inconsistent caching. Our changes do not directly interact with the Gradle configuration or the fetcher tests.
Jekyll Build Failures: These are occurring on both the
mainand our feature branch, suggesting a general issue with the Jekyll configuration rather than a problem introduced by our changes.Annotations and Deprecated Functions: Warnings about deprecated functionality in the Gradle setup are present, but these are part of the existing CI configuration and not related to our code changes.
We've thoroughly reviewed our changes and can confirm that they do not directly contribute to these CI/CD issues. These appear to be pre-existing problems in the repository's CI/CD setup or documentation. We would appreciate any guidance on how to proceed, given that these issues are beyond the scope of our feature implementation.
We're open to addressing any of these issues if guidance is provided on how they relate to our changes.
Notes for reviewers
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)Following is the screenshot of the new action in the look up menu:
Following is the screenshot of the progress showed in the backgroud task view:
Following is the screenshot of the result of running the new feature: