fix entry deletion from database #996
Conversation
| return; | ||
| } | ||
|
|
||
| entries.remove(oldValue.getId()); | ||
|
|
There was a problem hiding this comment.
Ahh, I just fixed that (but worse, just removed the getId()-part). Will removed that commit from my PR.
|
Is it really impossible to add a test case? |
| @Test | ||
| public void testRemoveEntry() { | ||
| BibDatabase db = new BibDatabase(); | ||
| assertEquals(Collections.emptyList(), db.getEntries()); |
There was a problem hiding this comment.
Possibly, this "assertion" for the subsequent test should be made as separate test. This test is repeated in testInsertEntryKeyCollision. Therefore, I vote for extracting these lines.
|
Enhanced the tests a little bit and added a null-check while inserting an object into the database. |
| BibDatabase db = new BibDatabase(); | ||
| assertEquals(Collections.emptyList(), db.getEntries()); | ||
|
|
||
| db.insertEntry(null); |
There was a problem hiding this comment.
Why not two different test cases? One for insert null and one for null removal?
There was a problem hiding this comment.
Because the Test is very small and tests similar things, I'll extract it.
|
LGTM 👍 |
| @@ -158,30 +156,33 @@ public synchronized BibEntry getEntryByKey(String key) { | |||
| * use Util.createId(...) to make up a unique ID for an entry. | |||
| */ | |||
| public synchronized boolean insertEntry(BibEntry entry) throws KeyCollisionException { | |||
| if (entry == null){ | |||
There was a problem hiding this comment.
I think Objects.requireNonNull is more appropriate here.
There was a problem hiding this comment.
Done, didn't knew that trick.
|
Thanks for fixing the bug I had introduced. Please try to adhere to the convention for tests outlined in the wiki https://github.com/JabRef/jabref/wiki/Code-Howtos#test-cases, especially the naming of the tests and only one test per test method. Comments test methods shouldn.t be necessary but the test name should contain all the information. |
| fireDatabaseChanged(new DatabaseChangeEvent(this, DatabaseChangeEvent.ChangeType.ADDED_ENTRY, entry)); | ||
|
|
||
| return checkForDuplicateKeyAndAdd(null, entry.getCiteKey()); | ||
| } | ||
|
|
||
| /** | ||
| * Removes the given entry. |
There was a problem hiding this comment.
Please add in the javadoc comment that the entry is removed based on the id.
|
After some minor naming improvements, this can be merged in in my opinion. Good job. |
fix entry deletion from database
Fixes #991, a Bug introduced in b3aee00.
While moving from
MaptoListas the data structure for the storedBibEntriesis has been forgotten to adjust theremoveEntry-Method.