Implement mass addition of bib information (Closes #372)

[BojiZhang]

Closes https://github.com/koppor/jabref/issues/372

Description

I am a teammate with @Arvinyuchen, who was assigned to issue #372.

This pull request addresses issue #372 by implementing mass addition of bibliographic information for multiple entries. We've refactored the FetchAndMergeEntry class and added a new MultiEntryMergeWithFetchedDataAction class to support batch merging of multiple entries with fetched data.

---

Changes made:

1. Refactored FetchAndMergeEntry class:

   • Improved code structure and reduced duplication
   • Enhanced error handling and user feedback
   • Streamlined fetch and merge process for better performance
   • Implemented more robust field handling and merging logic
   • Optimized batch processing capabilities
   • Improved modularity and separation of concerns
   • Reduced unnecessary notifications

2. Added MultiEntryMergeWithFetchedDataAction class:

   • Implements functionality for merging multiple entries with fetched data
   • Integrates with the existing FetchAndMergeEntry class

3. Updated related components:

   • Restored MERGE_WITH_FETCHED_ENTRY action
   • Added new BATCH_MERGE_WITH_FETCHED_ENTRIES action
   • Enhanced right-click context menu with new actions and improved layout

This 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

We attempted to write test cases for the modifications but encountered difficulties due to the complexity of the application framework. As a result, we were unable to correctly set up the tests. Manual testing has been performed to verify the functionality, but additional testing by reviewers would be appreciated.

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:

1. Pages Deployment Issues: These seem to be related to the repository's GitHub Pages configuration and are likely not caused by our code changes.

2. Link Checking Failures: The lychee workflow reported broken links in documentation files. These are pre-existing issues in the repository's documentation and not introduced by our changes.

3. 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.

4. Jekyll Build Failures: These are occurring on both the main and our feature branch, suggesting a general issue with the Jekyll configuration rather than a problem introduced by our changes.

5. 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

• Please pay special attention to the refactored FetchAndMergeEntry class and the new MultiEntryMergeWithFetchedDataAction class.
• The changes aim to improve code quality, performance, and user experience.
• Any suggestions for writing effective tests for these changes would be greatly appreciated, as we struggled with setting up the test environment correctly.

Mandatory checks

• [x] I own the copyright of the code submitted and I licence it under the MIT license
• [x] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
• [ ] Tests created for changes (if applicable)
• [x] Manually tested changed features in running JabRef (always required)
• [x] Screenshots added in PR description (for UI changes)
• [x] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
• [x] 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.

---

Following is the screenshot of the new action in the right click menu:

Following is the screenshot of the result of running the new feature:

[koppor]

I dont see any new test case. It should be possible to add at least one test case!

[SaltedTan]

@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 FetchAndMergeEntry class, which is also used in the implementation of the MultiEntryMergeWithFetchedDataAction aimed at fixing issue JabRef/jabref#12275. This impeded our attempt in writing test cases for the newly added class. Furthermore, we also met some difficulties in writing the setUp() part of the tests, not sure what is the cause of this. Could you please provide some more information that could be helpful in writing the tests?

[koppor]

This impeded our attempt in writing test cases for the newly added class. Furthermore, we also met some difficulties in writing the setUp() part of the tests, not sure what is the cause of this.

Without any example, I cannot say anything.

I think, it could be hard. You first need to factor out WebFetchers.getIdBasedFetcherForField( of ´org.jabref.gui.mergeentries.FetchAndMergeEntry#fetchAndMerge(org.jabref.model.entry.BibEntry, java.util.List<org.jabref.model.entry.field.Field>). Maybe, for an initial refactoring, a simple Functioncan be passed which mimics the behavior ofWebFetchers.getIdBasedFetcherForField`. Then, a mock can be passed.

Second step is to make use of org.jabref.logic.util.NotificationService only. There should be no confirmation dialog if something goes wrong. The user can look into the log. (The current dialog does not allow to jump to an entry; thus it is not useful)

With that refactoring, the whole code can be moved to logic somewhere. Maybe org.jabref.logic.importer.fetcher and as name MergingIdBasedFetcher.

[koppor]

@KUMOAWAI I think, you do see that tests are failing? https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

[BojiZhang]

@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

[BojiZhang]

@KUMOAWAI I think, you do see that tests are failing? https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

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:

1. Adjusting the architectural rules to allow these specific dependencies, or
2. Implementing a refactoring strategy that maintains separation between layers?

Thank you for your time.

[koppor]

@KUMOAWAI The unit tests fail. Please check. Current link: https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

[BojiZhang]

@KUMOAWAI The unit tests fail. Please check. Current link: https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

Hi Koppor, I would like to inform you that I have already fixed all the failed tests that are related to my modifications. However, I noticed that the fetcherTest is still failing with several fetcher tests (APS, ADS, Biodiversity Library, IEEE, Springer, and Grobid). These appear to be authentication/connectivity issues unrelated to my changes.

[Siedlerchr]

You can ignore the unrelated fetcher tests

[BojiZhang]

Small comments, please refactor.

Then, we will invest time again for a review.

Thank you for the review. We'll get on working on the refactoring now.

[BojiZhang]

Basic comment - I found duplicated code.

It is very hard to review your PR, because you refactored src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java. Thus, I need to check: Which functionality is new and which one is "just" a rewrite of existing functionality. It is even more difficult, because there is no test for FetchAndMergeEntry.

Therefore, I am going babysteps and stop as soon as I cannot imagine how the code looks like after my comments.

If this back and forth is too much for you, please raise your voice. Then we maybe will finish the PR for ourselves.

Thank you for the detailed review. I have to admit that I did use AI to help with the refactoring, which led to some oversights. I am sincerely sorry for that. And about the name, cause you said ''' replace "mass" by "batch". You used "batch" in the localization - and I like it. ''' And I misunderstood it as use it for everywhere, so I changed the name to BATCH_MERGE_WITH_FETCHED_ENTRY. I am happy to roll back FetchAndMergeEntry class and MergeWithFetchedEntryAction class to the original version.(And I have tested this action would not break anything since the new feature is independent.)

[koppor]

Thank you for the detailed review. I have to admit that I did use AI to help with the refactoring, which led to some oversights.

It's all about training - in this case: The training of us developers how to use AI for more productivy. 😅 - The blog article for that is nice https://roe.dev/blog/using-ai-in-open-source.

And about the name, cause you said ''' replace "mass" by "batch". You used "batch" in the localization - and I like it. ''' And I misunderstood it as use it for everywhere, so I changed the name to BATCH_MERGE_WITH_FETCHED_ENTRY. I am happy to roll back FetchAndMergeEntry class and MergeWithFetchedEntryAction class to the original version.(And I have tested this action would not break anything since the new feature is independent.)

Sometimes, I forgot what I posted - currently so many different PRs are running.

i think, "BATCH" is good, because the other functionality is also handles multiple entries, but opens the merge entries dialog?

[BojiZhang]

Thank you for the detailed review. I have to admit that I did use AI to help with the refactoring, which led to some oversights.

It's all about training - in this case: The training of us developers how to use AI for more productivy. 😅 - The blog article for that is nice https://roe.dev/blog/using-ai-in-open-source.

And about the name, cause you said ''' replace "mass" by "batch". You used "batch" in the localization - and I like it. ''' And I misunderstood it as use it for everywhere, so I changed the name to BATCH_MERGE_WITH_FETCHED_ENTRY. I am happy to roll back FetchAndMergeEntry class and MergeWithFetchedEntryAction class to the original version.(And I have tested this action would not break anything since the new feature is independent.)

Sometimes, I forgot what I posted - currently so many different PRs are running.

i think, "BATCH" is good, because the other functionality is also handles multiple entries, but opens the merge entries dialog?

Thank you for understanding regarding using AI assistance. While we read the blog post early in the project and tried to carefully review AI-generated code, we still made some oversights. I sincerely apologize for this.

[BojiZhang]

Your code currently does not meet JabRef's code guidelines. We use OpenRewrite to ensure "modern" Java coding practices. The issues found can be automatically fixed. Please execute the gradle task rewriteRun, check the results, commit, and push.

You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

There was something wrong with GitHub Actions...

[BojiZhang]

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.

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!

[BojiZhang]

Hi maintainers, The StringUtilClassIsSmall() test is failing, though I added the code to StringUtil as previously instructed. Could you help adjust the size limit or suggest how to proceed?

[BojiZhang]

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?

[BojiZhang]

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?

Hi @koppor, I think there might be a misunderstanding. I haven't added the tests yet because I was first asking whether adding cancellation support would be a feature the project wants. I was waiting for your advice before proceeding with any implementation and tests.

[koppor]

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?

Hi @koppor, I think there might be a misunderstanding. I haven't added the tests yet because I was first asking whether adding cancellation support would be a feature the project wants. I was waiting for your advice before proceeding with any implementation and tests.

Cancellation is orthogonal to the merging features, isn't it? I consider working logic more important than some cancellation support.

Isn't cancellation automatically given by wrapping into a BackgroundTask.with "showToUser" true? See other places in the code.

[BojiZhang]

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?

Hi @koppor, I think there might be a misunderstanding. I haven't added the tests yet because I was first asking whether adding cancellation support would be a feature the project wants. I was waiting for your advice before proceeding with any implementation and tests.

Cancellation is orthogonal to the merging features, isn't it? I consider working logic more important than some cancellation support.

Isn't cancellation automatically given by wrapping into a BackgroundTask.with "showToUser" true? See other places in the code.

Thank you for the clarification. I will add tests for the merge functionality after late February, as I'll be busy until then.

[koppor]

@KUMOAWAI Please fix tests - this saves time of the main developers.

[BojiZhang]

@KUMOAWAI Please fix tests - this saves time of the main developers.

Will start the work from today

[koppor]

Need proper check by a @JabRef/developers

[calixtus]

Normalize method seems not to work as expected - or am i wrong?

[subhramit]

Normalize method seems not to work as expected - or am i wrong?

something with the spaces...I just converted the regex as-is, will take a look at the logic tomorrow

[trag-bot[bot]]

@trag-bot didn't find any issues in the code! ✅✨

[jabref-machine]

Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of [x] (done), [ ] (not done yet) or [/] (not applicable).