Changeset 18009 in josm

Timestamp:

2021-07-13T00:27:59+02:00 (4 years ago)

Author:

Don-vip

Message:

fix #20860 - make sure to not try to close a note when none is selected

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/osm/NoteData.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/dialogs/NotesDialog.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/data/osm/NoteDataTest.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/data/osm/NoteData.java

@@ -224 +224 @@
             Logging.debug("closing note {0} with comment: {1}", note.getId(), text);
         }
-        NoteComment comment = new NoteComment(Instant.now(), getCurrentUser(), text, NoteComment.Action.CLOSED, true);
-        note.addComment(comment);
+        Instant now = Instant.now();
+        note.addComment(new NoteComment(now, getCurrentUser(), text, NoteComment.Action.CLOSED, true));
         note.setState(State.CLOSED);
-        note.setClosedAt(Instant.now());
+        note.setClosedAt(now);
         dataUpdated();
     }

trunk/src/org/openstreetmap/josm/gui/dialogs/NotesDialog.java

@@ -404 +404 @@
             }
             Note note = displayList.getSelectedValue();
-            int selectedIndex = displayList.getSelectedIndex();
-            noteData.closeNote(note, dialog.getInputText());
-            noteData.setSelectedNote(model.getElementAt(selectedIndex));
+            if (note != null) {
+                int selectedIndex = displayList.getSelectedIndex();
+                noteData.closeNote(note, dialog.getInputText());
+                noteData.setSelectedNote(model.getElementAt(selectedIndex));
+            }
         }
     }

trunk/test/unit/org/openstreetmap/josm/data/osm/NoteDataTest.java

@@ -3 +3 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.Arrays;
+import java.util.List;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.notes.Note;
+import org.openstreetmap.josm.data.notes.Note.State;
+import org.openstreetmap.josm.data.notes.NoteComment;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 /**
@@ -14 +25 @@
  */
 class NoteDataTest {
+
+    /**
+     * Setup test.
+     */
+    @RegisterExtension
+    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
+    public JOSMTestRules test = new JOSMTestRules().preferences();
 
     /**
@@ -25 +43 @@
         assertEquals(1, notEmpty.getNotes().size());
     }
+
+    /**
+     * Unit test for {@link NoteData#closeNote}
+     */
+    @Test
+    void testCloseNote_nominal() {
+        Note note = new Note(LatLon.ZERO);
+        note.setState(State.OPEN);
+        assertNull(note.getClosedAt());
+        assertTrue(note.getComments().isEmpty());
+
+        NoteData data = new NoteData(Arrays.asList(note));
+        data.closeNote(note, "foo");
+
+        assertEquals(State.CLOSED, note.getState());
+        assertNotNull(note.getClosedAt());
+        List<NoteComment> comments = note.getComments();
+        assertEquals(1, comments.size());
+        NoteComment comment = comments.get(0);
+        assertEquals("foo", comment.getText());
+        assertEquals(note.getClosedAt(), comment.getCommentTimestamp());
+    }
+
+    /**
+     * Checks that closeNote does not throw NPE on null arguments
+     */
+    @Test
+    void testCloseNote_nullsafe() {
+        assertEquals("Note to close must be in layer",
+                assertThrows(IllegalArgumentException.class,
+                        () -> new NoteData().closeNote(null, null)).getMessage());
+    }
 }