Fix for issue 2810 #3433

tobous · 2017-11-14T23:07:31Z

As the name suggests, this is a fix for the issue #2810.

We added a new listener that automatically updates the timestamp of changed entries if the feature is enabled in the options ("Update timestamp on modification"). The listener was added to the BasePanel class so that it could listen on the whole database to also work with changes created by other sources (like cleanup operations).

The listener is "public static" so that it can be accessed by the added test cases.

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef

Problem with dependency on JabRefPreferences

The test cases added with UpdateTimestampListenerTest cause the test TestArchitectureTests#testsAreIndependent() to fail as the new tests are dependent on JabRefPreferences.

We were initially using Globals.prefs directly. To remove the dependency on Globals, we changed it so that a JabRefPreferences object is passed to the constructor of the listener instead. This means that the tests are only dependent on JabRefPreferences now.

But we don't know how to avoid this dependency. We tried to pass the TimeStampPreferences object directly to the constructor of the listener (which would remove the dependency). This is, however, not feasible as the TimeStampsPreferences object returned by Globals.prefs.getTimeStampPreferences() does not reflect any changes made in the options menu. This would mean that changes to the option "Update timestamp on modification" (or the date format) would only apply after a restart of JabRef.

We had a look online on how to test with preferences but according to the JabRef testing how-tos, creating a mock object of JabRefPreferences and mocking the needed method calls should be a valid approach.

Potentially unwanted behavior

As the timestamp is set on any change to the entry, if the user decides to change the timestamp by hand it will be reset immediately. We are not sure if this behavior is wanted or not.

Merge Master with fork

This is a fix for the issue JabRef#2810. The functionality has been moved from the EntryEditor to the BasePanel class so that it can listen for EntryChangedEvents on the whole database instead of specific EntryEditor objects. This allows us to also update the timestamps of entries that were changed by other sources (like cleanup operations). The inner class UpdateTimestampListener is defined as "public static" so that it can be used in the test cases for the implemented functionality.

tobiasdiez

The code looks good to me. I just have a few minor remarks concerning the tests.
You handled the preferences correctly in the tests, so there shouldn't be any side effects.

tobiasdiez · 2017-11-16T07:43:44Z

src/main/java/org/jabref/gui/BasePanel.java

+    /**
+     * Updates the timestamp of changed entries if the feature is enabled
+     */
+    public static class UpdateTimestampListener {


I would put it in a new file (we are trying to reduce the size of BasePanel and MainFrame). I know, this was one of your questions before you posted the PR; sorry for the delay and the additional work it now means.

When thinking about whether or not to move the listener into a separate file, we decided against it as we could not find a good place to put it. Could you suggest a package/folder?

Although this isn't perfect, the best place for the listener is in parallel to the BasePanel class (being the only class that uses it). So please extract the listener into a top-level class and put it in org.jabref.gui. When you address my other comment regarding the test, you can also reduce the visibility of the listener class from public to default.

tobiasdiez · 2017-11-16T07:45:23Z

src/test/java/org/jabref/model/database/event/UpdateTimestampListenerTest.java

+        final String NEW_DATE = "2000-1-2";
+        final boolean INCLUDE_TIMESTAMP = true;
+
+        JabRefPreferences preferencesMock = mock(JabRefPreferences.class);


You have a few set-up steps that are shared between the tests methods. Could you please move them to the setup method.

In my opinion, it is not necessary (or even useful) to declare these values beforehand.

The additional time spent when initializing three strings twice instead of only once should be negligible when running the tests. Furthermore, when looking at the test cases, defining the constants in the beginning of each test makes it much clearer exactly which values are used.

But that is just my opinion. What do you (or other contributors) think about this? If you still prefer that the constants are only initialized once, I will change it.
But, as they are constants, initializing them in the set-up would not be the best option in my mind. I would rather make them private final instance variables.

I didn't meant the constants. These are clearly related to the specific test and thus can stay there. However, you have some common initialization (preferences, database with entry, ...) which should be extracted to setup. Not so much for performance but to make the tests smaller and thus easier to understand and to maintain.
Have look at for example

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

Lines 35 to 54 in effe334

private File pdfFolder;

private BibDatabaseContext databaseContext;

private MoveFilesCleanup cleanup;

private BibEntry entry;

private FileDirectoryPreferences fileDirPrefs;

@Before

public void setUp() throws IOException {

MetaData metaData = new MetaData();

pdfFolder = bibFolder.newFolder();

metaData.setDefaultFileDirectory(pdfFolder.getAbsolutePath());

databaseContext = new BibDatabaseContext(new BibDatabase(), metaData, new Defaults());

databaseContext.setDatabaseFile(bibFolder.newFile("test.bib"));

entry = new BibEntry();

entry.setCiteKey("Toot");

entry.setField("title", "test title");

fileDirPrefs = mock(FileDirectoryPreferences.class);

when(fileDirPrefs.isBibLocationAsPrimary()).thenReturn(false); //Biblocation as Primary overwrites all other dirs, therefore we set it to false here

}

Thank you for clarifying. I get your point, but I am still not sure what you want me to move into the setup method.

I could move:

the instantiation of the mocks for JabRefPreferences and TimeStampPreferences

the setup to catch calls to JabRefPreferences#getTimestampPreferences()

the instantiation of the BibEntry as well as its insertion into the database

This would also mean that the mocks and the BibEntry would have to be instance variables.

Would this meet your expectations?

If I should also move more of the initialization into the setup method, I don't see a way around making the constants "TIMESTAMP_FIELD", "BASE_DATE", and "NEW_DATE" instance variables.

Yes, I meant exactly those three points! Thanks.

@Siedlerchr This is the discussion about moving parts of the initialization into the setup method.

tobiasdiez · 2017-11-16T07:46:55Z

src/test/java/org/jabref/model/database/event/UpdateTimestampListenerTest.java

+
+        database.insertEntries(bibEntry);
+
+        assertTrue("Initial timestamp not present", bibEntry.getField(TIMESTAMP_FIELD).isPresent());


The preferred way to test optionals is assertEquals(Optional.of(expected), actualOptional)

tobiasdiez · 2017-11-16T07:48:24Z

src/test/java/org/jabref/model/database/event/UpdateTimestampListenerTest.java

+
+    @Test
+    public void updateTimestampEnabled(){
+        final String TIMESTAMP_FIELD = "timestamp";


I'm not sure about our conventions, but I think constant variables are still denoted by small letters. @koppor ?

tobous · 2017-11-17T10:03:51Z

Regarding the usage of JabRefPreferences: Even though we used the preferences correctly (thank you for clearing that up), the test concerning illegal dependencies still fails. This is the reason the travis check fails. Is there something we can/should do about this?

Furthermore, we have a general question: How should we update this pull request (or pull requests in general)? Should we

create additional commits to implement the changes and just add them to this pull request,
create a new pull request with updated versions of our changes,
(And what if changes are requested for the new pull request as well/multiple times? Should each adjustment request of the (new) pull request warrant another pull request?)
force push amended versions of the existing commits,
or proceed in another fashion we did not think of?

Siedlerchr · 2017-11-17T10:17:48Z

You just update your PR with new commits until all issues/conments are resolved On a merge all commits will be squashed into a single one.
Github auto hides outdated comments on previous commits

lenhard

Overall a nice contribution, thank you very much :)

Essentially, the architecture test fails, because the package structure of source code and test do not match. The class you test (UpdateTimestampListener) is in org.jabref.gui, but the test class is placed in org.jabref.model.database.event. So the solution would be to move the test into org.jabref.gui as well and the architecture problem will go away. When you do that you can also categorize the test class as a gui test using @Category(GUITests.class).

lenhard · 2017-11-17T15:38:39Z

src/main/java/org/jabref/gui/BasePanel.java

+    /**
+     * Updates the timestamp of changed entries if the feature is enabled
+     */
+    public static class UpdateTimestampListener {


Although this isn't perfect, the best place for the listener is in parallel to the BasePanel class (being the only class that uses it). So please extract the listener into a top-level class and put it in org.jabref.gui. When you address my other comment regarding the test, you can also reduce the visibility of the listener class from public to default.

tobiasdiez · 2017-11-18T04:31:15Z

@lenhard

When you do that you can also categorize the test class as a gui test using @category(GUITests.class).

A test flagged with this category actually means that the test starts JabRef, clicks a few buttons and checks the resulting behavior. This is not the case here; it only tests a class in gui.

lenhard · 2017-11-18T21:33:14Z

@tobiasdiez Thanks for clarifying!

@tobous In that case, there's no need to use the category.

The blank lines should help differentiate between the constants that the test cases share and the constant that is different in both test cases.

tobous · 2017-11-19T02:48:48Z

Moving the test into the "org.jabref.gui" package did not resolve the issue with the dependency check. I had a look at other tests using JabRefPreferences but did not see any special set-up to avoid the issues. Other tests using it are located in the packages

"org.jabref" (JabRefPreferencesTest)
"org.jabref.migrations" (PreferencesMigrationsTest)
"org.jabref.preferences" (LastFocusedTabPreferencesTest)

I also have a more general question:
Is it possible that using the JabRefPreferences object held in Globals directly (instead of always using "Globals.prefs" to access the preferences object) could cause any problems.
Or more directly: Is it possible that the JabRefPreferences object held by Globals changes, thereby invalidation the previously held object (that would still be used by the UpdateTimestampListener)?

Siedlerchr · 2017-11-20T12:24:12Z

src/main/java/org/jabref/gui/UpdateTimestampListener.java

+class UpdateTimestampListener {
+    private final JabRefPreferences jabRefPreferences;
+
+    UpdateTimestampListener(JabRefPreferences jabRefPreferences) {


When you here directly pass an object of TimestampPreferences, you then don't need to mock the whole JabRef Preferences and you will get rid of the travis architecture failing

Thank you for your suggestion. I already considered the mentioned approach but I could not get it to work.

The problem is described in third paragraph of the section "Problem with dependency on JabRefPreferences" in the initial pull request. To summarize:
When I tested this approach, the passed TimestampPreferences object did not get updated when the settings were changed. This would mean that

either the application would have to be restarted for the settings change to apply to the listener

or I would have to add a method to change the held TimestamPreferences object and add a method that listens for changes in the timestamp settings (which might also just move the problems to the tests for the new listener).

Both approaches are not valid in my opinion and I could not think of a better one.

Noticing this problem also lead to my other question concerning the usage of the JabRefPreferences object directly instead of the object held in "Globals.prefs".
(The question was: Is it possible that the held object changes, thereby invalidating the previously held object which would still be used by the UpdateTimestampListener?)

Or should the TimestampPreferences object get updated and it not reflecting the settings changes is not the wanted behavior?

I investigated your problem and I know the reason for it.
The registerListener method in BasePanel is called once per Database at the initialization with the current Preferences. Any changes to the prefs will not update this.

Siedlerchr · 2017-11-20T12:25:03Z

src/test/java/org/jabref/gui/UpdateTimestampListenerTest.java

+
+        final boolean INCLUDE_TIMESTAMP = true;
+
+        JabRefPreferences preferencesMock = mock(JabRefPreferences.class);


As mentionend above, when you directly pass the TimestampPreferences Object/Mock, you don't need to have a dependency on JabRef Preferences

Furthermore, you can move the timestamp constants to private static final in the class body.
object creation and add the when(xxx) methods to the setup method. Then you can avoid such code duplication

Will do this when moving the initialization not dependent on the defined constants into the setup method.

I would, however, prefer to not make the instance variable "private static final". I would like to just make it "private" and instantiate a new mock object for both tests in the setup method instead.
Even though old "when" calls on methods should be overwritten by newer "when" calls (according to the mockito javadoc), using new objects avoids confusion when reading the tests and removes the possibility of dependencies or interactions between the tests.

Furthermore I would only move the calls not dependent on the defined constants as I would like to keep them local. I already had a discussion about this with @tobiasdiez. (I will tag you in a response to the discussion. I hope this will make it easier to find it for you.)

Would that also be acceptable?

The setup() method is called before every test

Maybe I was not clear enough when explaining my point of view.
I don't want to have just one mock object for TimestampPreferences that is shared between all tests (which would be the case if TimestampPreferences were made "private static final"). I would like to use private instance variables that are instantiated in the setup method instead. This would mean that we would have a different mock object for each test case.

I am about to push a commit which moves some of the initialization to the setup method. Then we have the code to better discuss the issue and see if any other changes are warranted.

tobous · 2017-11-20T14:46:14Z

I also had another look at the failing test. It looks like the other tests using JabRefPreferences were either situated in the package "org.jabref.preferences" (LastFocusedTabPreferencesTest) or was manually added as an exception in the test.

I could just add UpdateTimestampTest to the exceptions, but I don't know if this would be an acceptable solution.

Siedlerchr · 2017-11-20T14:50:27Z

I will have a look at your code later and will look into the updating issue

This only applies to shared calls that are not dependent on the used constants. As these constants define the values used in each test, they are kept in each test case to improve readability. This also applies to constants that have the same value in all tests.

tobous · 2017-11-20T18:59:34Z

For whatever reason, the first merge with the master was not recognized by GitHub and therefore did not trigger a travis build. The second merge commit does absolutely nothing but GitHub now recognizes that the conflict has been resolved.
As the merge of this pull request should just squash all the commits, this should hopefully not really matter.

Siedlerchr · 2017-11-21T11:01:01Z

As I have no access to your repo, I created a patch with some changes.
Regarding the timestamp issue, a restart is required. That's a thing I don't see a solution for.
0001-Use-TimestampPreferences-and-rename-fields.patch.txt

Maybe another one @JabRef/developers has an idea?

tobous · 2017-11-21T11:16:06Z

The restart could be avoided if the JabRefPreferences object held in Globals.prefs is used. This object is updated when the preferences are changeg.
In other words: the restart is not required with the current implementation

And I still don't get why the dependency on JabRefPreferences is a problem. Could somebody explain this to me?

And even if the dependency were a problem, I think it is important how the issue/problem compares to the restrictions we would create by avoiding it. In my opinion, a solution that requires a restart of the application to apply changed settings diminishes the usability of the feature.

Thanks for your suggested changes. I will have a look at them later today. But I would like to further discuss the usage of TimestampPreferences before I integrate it into the pull request.

tobiasdiez · 2017-11-21T12:24:46Z

I like the current solution. There is nothing inherently bad in directly using JabRefPreferences. I would vote for adding an exception to the architecture test.

lenhard · 2017-11-22T09:14:21Z

I have had a closer look at TestArchitectureTests. I think they are wrong/incomplete. If gui classes are allowed to use the preferences in the main source, why shouldn't that be the case for the tests as well?

I think it would be best to add the complete gui package as an exception here. This requires a refactoring of that test class, because exceptions are specified on class names only so far. But a similar structure is present in MainArchitectureTests.

Siedlerchr · 2017-11-22T09:22:25Z

@lenhard We did this, to prevent unexpected side effects when using Preferences in the tests=> They always modified the "real" preferences. They only should be mocked, so I think this is no longer a valid

lenhard · 2017-11-22T11:25:25Z

@Siedlerchr But that is not what the tests are testing, right? Even when you mock the preferences, you will need to reference them, which makes the test fail. That's exactly what happens in this PR.

What is your suggestion to solve this?

tobiasdiez · 2017-11-22T11:33:02Z

What about adding exceptions case by case, so that you really have to think if you want to use the preferences in the tests? Not perfect but maybe the "easiest" solution?

lenhard · 2017-11-22T11:35:47Z

@tobiasdiez Not optimal, but I guess that's the pragmatic way to move forward. It would be ok for this PR from my point of view.

tobous · 2017-11-24T02:38:00Z

Ok, should I add the class "UpdateTimestampListenerTest" to the exceptions in the TestArchitectureTests then?

@Siedlerchr The txt file you posted contains a lot of changes dealing with lots of classes. I didn't have the time to have a more detailed look at all of them, sorry.

The stuff with the TimestampPreferences should be solved/avoided if the Tests are added as an exception to the architecture test.

Concerning the restructuring of the test cases: I would prefer to keep it the way it is (with the constants being a part of each test case to better show which values are used and with each test having its own mock object instead of one shared static object). But as this is not my project, you guys have the last say with this.

The rest of the changes deal with timestamps in other parts of the code. Is this something that is important for this fix, something that should be changed but is not tied to this fix, or something that was a consequence of using TimestampPreferences?

tobiasdiez · 2017-11-24T07:29:09Z

Yes, add an exception to the architecture test! Please also use camelCase for the constants in the tests (instead of pure upper case) and then we can merge!

This allows the added test cases to be dependent on JabRefPreferences without causing TestArchitectureTests#testsAreIndependent() to fail. The exception is warranted as the tested functionality is dependent on JabRefPreferences and the test cases mock all calls to it.

…fix_for_issue_2810

tobiasdiez · 2017-11-27T19:27:42Z

Since @lenhard and @Siedlerchr already gave their ok (kind of) and I'm trigger happy today, I will merge this PR now.

Thank you very much for your contribution and your patience. Sorry that it took us a bit longer to sort out how to handle the issue with the architecture tests. Looking forward to see more PRs from you in the future!

Lukas Hoffmann and others added 3 commits November 12, 2017 18:13

Merge pull request #1 from JabRef/master

0546077

Merge Master with fork

Add tests for UpdateTimestampListener

fbfef31

tobous mentioned this pull request Nov 14, 2017

timestamp is not updated when entry changes #2810

Closed

tobiasdiez requested changes Nov 16, 2017

View reviewed changes

lenhard suggested changes Nov 17, 2017

View reviewed changes

Tobias Bouschen added 4 commits November 19, 2017 02:20

Move UpdateTimestampListenerTest to gui package

254cfc0

Move UpdateTimestampListener into separate class

cc54c4a

Fix assertions for Optional values in UpdateTimestampListenerTest

aced8a0

Add blank lines in UpdateTimestampListenerTest

878abca

The blank lines should help differentiate between the constants that the test cases share and the constant that is different in both test cases.

Siedlerchr reviewed Nov 20, 2017

View reviewed changes

Tobias Bouschen and others added 3 commits November 20, 2017 16:45

Merge branch 'master' into fix_for_issue_2810

d044a95

Merge branch 'master' into fix_for_issue_2810

f4ed8a5

Tobias Bouschen and others added 5 commits November 27, 2017 18:55

Use camel case for constants in UpdateTimestampListenerTest

4804c4d

Merge branch 'fix_for_issue_2810' of github.com:bimini94/jabref into …

02fe05d

…fix_for_issue_2810

Merge branch 'master' into fix_for_issue_2810

0208538

Merge branch 'master' into fix_for_issue_2810

3ecf8ec

tobous changed the title ~~[WIP] Fix for issue 2810~~ Fix for issue 2810 Nov 27, 2017

tobiasdiez merged commit 42a959d into JabRef:master Nov 27, 2017

	private File pdfFolder;
	private BibDatabaseContext databaseContext;
	private MoveFilesCleanup cleanup;
	private BibEntry entry;
	private FileDirectoryPreferences fileDirPrefs;

	@Before
	public void setUp() throws IOException {
	MetaData metaData = new MetaData();
	pdfFolder = bibFolder.newFolder();
	metaData.setDefaultFileDirectory(pdfFolder.getAbsolutePath());
	databaseContext = new BibDatabaseContext(new BibDatabase(), metaData, new Defaults());
	databaseContext.setDatabaseFile(bibFolder.newFile("test.bib"));
	entry = new BibEntry();
	entry.setCiteKey("Toot");
	entry.setField("title", "test title");

	fileDirPrefs = mock(FileDirectoryPreferences.class);
	when(fileDirPrefs.isBibLocationAsPrimary()).thenReturn(false); //Biblocation as Primary overwrites all other dirs, therefore we set it to false here
	}


		database.insertEntries(bibEntry);

		assertTrue("Initial timestamp not present", bibEntry.getField(TIMESTAMP_FIELD).isPresent());


		final boolean INCLUDE_TIMESTAMP = true;

		JabRefPreferences preferencesMock = mock(JabRefPreferences.class);

Fix for issue 2810 #3433

Fix for issue 2810 #3433

Conversation

tobous commented Nov 14, 2017 • edited

Problem with dependency on JabRefPreferences

Potentially unwanted behavior

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobous commented Nov 17, 2017

Siedlerchr commented Nov 17, 2017

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Nov 18, 2017

lenhard commented Nov 18, 2017

tobous commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobous Nov 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobous commented Nov 20, 2017

Siedlerchr commented Nov 20, 2017

tobous commented Nov 20, 2017

Siedlerchr commented Nov 21, 2017

tobous commented Nov 21, 2017

tobiasdiez commented Nov 21, 2017

lenhard commented Nov 22, 2017

Siedlerchr commented Nov 22, 2017

lenhard commented Nov 22, 2017

tobiasdiez commented Nov 22, 2017

lenhard commented Nov 22, 2017

tobous commented Nov 24, 2017

tobiasdiez commented Nov 24, 2017

tobiasdiez commented Nov 27, 2017

tobous commented Nov 14, 2017 •

edited

tobous Nov 20, 2017 •

edited