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.

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

Fix import entry by id does not generate citation key #8361

Merged
merged 16 commits into from
Apr 4, 2022

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Dec 26, 2021

Fixes koppor#553
Fixes #8406

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

@Siedlerchr Siedlerchr marked this pull request as draft December 26, 2021 22:36
@ThiloteE
Copy link
Member

Would also fix #8354

@tobiasdiez
Copy link
Member

Can we reuse the import handler here so that other features (like automatically generating timestamps etc) are also working as expected?

@Siedlerchr
Copy link
Member Author

Good idea, but the import handler does not check for duplicates. That only happens in the New Entry dialog

@tobiasdiez
Copy link
Member

I guess it make sense to extract the duplication check also to the import handler. Then one would get also a duplication warning if one say uses copy&paste of an id to create an entry. That's a nice enhancement in my opinion. (The only situation where you don't need this check is after the import dialog, which already handles this.)

@ThiloteE
Copy link
Member

ThiloteE commented Jan 9, 2022

Related: #8406

* upstream/main: (50 commits)
  New Crowdin updates (#8451)
  Fix library tab exception when saving prefs (#8450)
  Rename Groups interface into Groups (#8449)
  New Crowdin updates (#8445)
  update snap url
  Update bug_report.yml for 5.5
  Show development information\n\n+semver: minor
  Release v5.5
  Update journal abbrev list
  New Crowdin updates (#8439)
  Refresh example styles
  Squashed 'buildres/csl/csl-styles/' changes from 5563ccc..0237ae7
  Fixed add group button changing color after adding 10 groups (#8392)
  Bump slf4j-api from 2.0.0-alpha5 to 2.0.0-alpha6 in /buildSrc (#8438)
  Bump libreoffice from 7.2.3 to 7.2.5 (#8436)
  Bump org.openjfx.javafxplugin from 0.0.10 to 0.0.11 (#8437)
  Fix file directory preferences not respected (#8429)
  Refresh example styles
  Squashed 'buildres/csl/csl-locales/' changes from c38205618f..4a551a87c3
  Refresh example styles
  ...
@ThiloteE ThiloteE mentioned this pull request Mar 14, 2022
* upstream/main: (104 commits)
  update test getPart (#8610)
  Add ControlHelper truncateString tests comments (#8612)
  Allow using custom SSL certificates (#8583)
  Fix protectedTerms not stored due to weaklistener (#8609)
  Fix changelog and version parsing (#8578)
  Creating more unit tests for NumericFieldComparatorTest (#8604)
  Fix merge entries dialog exceeding screen size (#8599)
  StringUtilTest new test for method GetPart (#8594)
  Use unkown entry type
  Add semantic scholar (#8575)
  Add research gate (#8580)
  fix import of unlinked files (#8444) (#8582)
  Missed l10n for fetcher fix (#8597)
  Fix some fetcher test (#8595)
  Bump slf4j-api from 2.0.0-alpha6 to 2.0.0-alpha7 in /buildSrc (#8589)
  Bump ikonli-materialdesign2-pack from 12.3.0 to 12.3.1 (#8591)
  Bump gittools/actions from 0.9.11 to 0.9.13 (#8587)
  Bump mockito-core from 4.3.1 to 4.4.0 (#8588)
  Bump flowless from 0.6.8 to 0.6.9 (#8590)
  Bump ikonli-javafx from 12.3.0 to 12.3.1 (#8592)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/EntryTypeViewModel.java
@Siedlerchr Siedlerchr marked this pull request as ready for review March 27, 2022 18:17
@Siedlerchr Siedlerchr changed the title Fix import entry by id does not genrate citation key Fix import entry by id does not generate citation key Mar 28, 2022
* upstream/main:
  fix unit test
  Add check for developer's documentation
  Merge GitBook view
  Fix zbMath fetcher (#8623)
  GitBook: [#56] No subject
  Add an extra dialog to ask the user whether they want to open the saved file folder when the export the entries (#8567)
  Bump checkstyle from 10.0 to 10.1 (#8620)
  Bump peter-evans/create-pull-request from 3 to 4 (#8619)
  Bump pascalgn/automerge-action from 0.14.3 to 0.15.2 (#8618)
  Bump flexmark from 0.62.2 to 0.64.0 (#8621)
  Bump classgraph from 4.8.141 to 4.8.143 (#8622)
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Mar 29, 2022

When an entry is pasted or imported by id and already exists, the duplicate resolver dialogs opens. And also the keys are generated
Works also for pasting DOI.
@ThiloteE you can test here (when the deployment green)
https://builds.jabref.org/pull/8361/merge/

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 29, 2022
@ThiloteE
Copy link
Member

Found a bug:

  1. have entry1 in libraryA
  2. have entry2 to libraryB
  3. Both have similar entry data, except citationkey.
  4. Copy and paste entry2 to libraryA.
  5. The entry merge dialogue will emerge
  6. Click on "Do not import entry" OR click on the X in the top right corner of the entry merge dialogue (to close the dialogue).
  • The entry2 WILL get imported.
  • (This entry2 will get assigned a new citation key that follows my citation key patterns I declared in my preferences, It will be two different citationkeys, because duplicate prevention adds a letter at the end.)

Expected outcome:

This entry2 SHOULD NOT get imported.

Expected outcome: The entry should NOT get imported.

@ThiloteE
Copy link
Member

ThiloteE commented Mar 29, 2022

Bug 2:

EDIT: SORRY, I noticed ONE CAN DISABLE overwriting the citation key by disabling the preference in options>preferenes>import and export>generate a new key for imported entries (overwriting their default). The other part of this bug here still applies (what you can see in the picture), but should not be too concerning. I might open a new issue for this. I think it should not be relevant enough to prevent merging this pull request, I guess?

0.1 Have "overwrite existing keys" in Keygen preferences DISABLED.

  1. Have a key pattern in keygenerator
  2. Have entry3 with DOI in LibraryC
  3. Copy DOI.
  4. Paste DOI in LibraryC
  5. Entry Editor emerges
    image
    5. Choose "import and keep old entry"

What happens:

- Entry3 is kept (good!)
- A new entry4 is imported, but old citationkey (of entry4) is overwritten by citationkey pattern that was specified under preferences.

  • Old and new entry are flipped.

Expected Outcome:

- Entry4 is imported with original citationkey from internet.
- Citationkey of entry4 should NOT be overwritten.

  • Old entry should be on the left, but is on the right. New entry should be on the right, but is on the left.

@ThiloteE
Copy link
Member

Sorry. 😅

@ThiloteE
Copy link
Member

ThiloteE commented Mar 29, 2022

Solving the first bug might also solve #5858? Just maybe?

@Siedlerchr
Copy link
Member Author

Thanks for testing, will check this again wth the right/left thing

@Siedlerchr
Copy link
Member Author

@ThiloteE I fixed the cancellation and the order in the dialog. Can you please test again?
https://builds.jabref.org/pull/8361/merge/

@ThiloteE
Copy link
Member

ThiloteE commented Mar 30, 2022

Looking quite good! Solved 2 issues. 1 new issue pops up 😂 Hopefully the last one.

Bug3:

  1. Have options>preferenes>import and export>generate a new key for imported entries (overwriting their default) enabled.
  2. Have a customized key-pattern for the citation-key generator set in preferences.
  3. Have this preference set under citation-key generator preferences:
    image
  4. Import entry by ID into library5 OR paste entry5 by ID into library5. Let's call this one entry5.
  5. Import entry by ID into library5 OR paste entry5 by ID into library5 AGAIN. Let's call this one entry5².
    • Duplicate Merge Editor will emerge. So far so good.
  6. Click keep merged entry only

Result:

  • entry5² will get assigned citationkey-according-to-my-pattern+a

Expected result:

  • entry5² will get assigned citationkey-according-to-my-pattern

Additional info:

This becomes really weird when entry5 and entry5² are not exactly identical. E.g.

  • entry5: date = {2016}
  • entry5²: date = {2016-10-02}

My key pattern includes the date into the citation-key. Therefore,

key before merge: 2016
key after merge: 2016-10-02a, EVEN THOUGH 2016-10-02 would be unique in my library.

All the other buttons work and I have not noticed any other bugs so far.

@Siedlerchr
Copy link
Member Author

I reverted the changes to the citation key method as this would break more tests and is an unusual edge case.

@koppor
Copy link
Member

koppor commented Apr 4, 2022

I can confirm it fixes koppor#553.

It does NOT fix #8406. See https://www.loom.com/share/0c29cb50e91c431488a54c3c5266379d, in case of huge diffs

@Article{Kopp2015,
  author    = {Kopp, Oliver and Martin, Daniel and Wutke, Daniel and Leyman, Frank},
  journal   = {Enterprise Modelling and Information Systems Architectures},
  title     = {The Difference Between Graph-Based and Block-Structured Business Process Modelling Languages},
  year      = {2015},
  pages     = {No 1 (2009)},
  volume    = {Vol 4},
  doi       = {10.18417/EMISA.4.1.1},
  language  = {en},
  publisher = {Gesellschaft für Informatik e.V. (The German Informatics Society)},
}
@Article{Kopp2015,
  author    = {Kopp},
  year = {2015},
}

--> Future work

@calixtus calixtus merged commit 4d8e41c into main Apr 4, 2022
@calixtus calixtus deleted the checkKeygenOverwrite branch April 4, 2022 20:57
@koppor koppor mentioned this pull request Apr 4, 2022
6 tasks
Siedlerchr added a commit that referenced this pull request Apr 5, 2022
* upstream/main:
  Remove obsolete comments
  Improve key generation (#8641)
  Refine search code (#8636)
  Fix import entry by id does not generate citation key (#8361)
  Update Gradle Wrapper from 7.4.1 to 7.4.2. (#8637)
  Bump hmarr/auto-approve-action from 2.1.0 to 2.2.0 (#8638)
  GitBook: [#57] test
  Citation keygen: Return vonPart if lastName is empty (#8634)
  Ensure SSL truststore is present at startup (#8631)
  Squashed 'buildres/csl/csl-styles/' changes from 6a7b708..21e2177 (#8632)
  Add more tests for FieldChange Class (#8614)
Siedlerchr added a commit that referenced this pull request Apr 5, 2022
…om.github.tomtung-latex2unicode_2.12-0.3.0

* upstream/main:
  Remove obsolete comments
  Improve key generation (#8641)
  Refine search code (#8636)
  Fix import entry by id does not generate citation key (#8361)
  Update Gradle Wrapper from 7.4.1 to 7.4.2. (#8637)
  Bump hmarr/auto-approve-action from 2.1.0 to 2.2.0 (#8638)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
5 participants