Ticket #17288: 17288.patch

File 17288.patch, 12.2 KB (added by taylor.smock, 4 years ago)

Improve UI feedback for uploading notes, add basic test to ensure that notes are uploaded when expected. Original patch by gaben, modified. Tests by taylor.smock.

  • src/org/openstreetmap/josm/actions/upload/UploadNotesTask.java 

    diff --git a/src/org/openstreetmap/josm/actions/upload/UploadNotesTask.java b/src/org/openstreetmap/josm/actions/upload/UploadNotesTask.java
index 60fb2c91e8..3a704a07bd 100644
    a b package org.openstreetmap.josm.actions.upload;  
    44import static org.openstreetmap.josm.tools.I18n.tr;
    55
    66import java.io.IOException;
     7import java.util.Collection;
    78import java.util.HashMap;
    89import java.util.Map;
     10import java.util.Optional;
    911import java.util.stream.Collectors;
    1012
    1113import javax.swing.JOptionPane;
    … … public class UploadNotesTask {  
    6365
    6466        @Override
    6567        protected void realRun() throws SAXException, IOException, OsmTransferException {
    66             ProgressMonitor monitor = progressMonitor.createSubTaskMonitor(ProgressMonitor.ALL_TICKS, false);
    6768            OsmApi api = OsmApi.getOsmApi();
    68             for (Note note : noteData.getNotes()) {
     69            final Collection<Note> modifiedNotes = noteData.getNotes().stream()
     70                    .filter(note -> Optional.ofNullable(note.getLastComment()).map(NoteComment::isNew).orElse(false))
     71                    .collect(Collectors.toList());
     72            getProgressMonitor().setTicksCount(modifiedNotes.size());
     73            for (Note note : modifiedNotes) {
     74                getProgressMonitor().setCustomText(tr("Uploading notes {0}/{1}", getProgressMonitor().getTicks(),
     75                        getProgressMonitor().getTicksCount()));
    6976                if (isCanceled) {
    7077                    Logging.info("Note upload interrupted by user");
    7178                    break;
    … … public class UploadNotesTask {  
    7380                for (NoteComment comment : note.getComments()) {
    7481                    if (comment.isNew()) {
    7582                        Logging.debug("found note change to upload");
    76                         processNoteComment(monitor, api, note, comment);
     83                        processNoteComment(getProgressMonitor(), api, note, comment);
    7784                    }
    7885                }
     86                getProgressMonitor().worked(1);
    7987            }
    8088        }
    8189

  • new file test/unit/org/openstreetmap/josm/actions/upload/UploadNotesTaskTest.java 

    diff --git a/test/unit/org/openstreetmap/josm/actions/upload/UploadNotesTaskTest.java b/test/unit/org/openstreetmap/josm/actions/upload/UploadNotesTaskTest.java
new file mode 100644
index 0000000000..1a879a9ad4
    - +  
     1// License: GPL. For details, see LICENSE file.
     2package org.openstreetmap.josm.actions.upload;
     3
     4import static org.junit.jupiter.api.Assertions.assertAll;
     5import static org.junit.jupiter.api.Assertions.assertFalse;
     6import static org.junit.jupiter.api.Assertions.assertTrue;
     7
     8import java.time.Instant;
     9import java.util.ArrayList;
     10import java.util.Collection;
     11import java.util.Collections;
     12import java.util.concurrent.ExecutionException;
     13import java.util.concurrent.TimeUnit;
     14import java.util.stream.Collectors;
     15import java.util.stream.Stream;
     16
     17import org.junit.jupiter.api.extension.RegisterExtension;
     18import org.junit.jupiter.params.ParameterizedTest;
     19import org.junit.jupiter.params.provider.Arguments;
     20import org.junit.jupiter.params.provider.MethodSource;
     21import org.openstreetmap.josm.TestUtils;
     22import org.openstreetmap.josm.data.coor.LatLon;
     23import org.openstreetmap.josm.data.notes.Note;
     24import org.openstreetmap.josm.data.notes.NoteComment;
     25import org.openstreetmap.josm.data.osm.NoteData;
     26import org.openstreetmap.josm.data.osm.User;
     27import org.openstreetmap.josm.gui.MainApplication;
     28import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
     29import org.openstreetmap.josm.gui.progress.ProgressMonitor;
     30import org.openstreetmap.josm.gui.util.GuiHelper;
     31import org.openstreetmap.josm.io.OsmTransferException;
     32import org.openstreetmap.josm.testutils.FakeOsmApi;
     33import org.openstreetmap.josm.testutils.JOSMTestRules;
     34import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
     35import org.openstreetmap.josm.tools.Logging;
     36
     37import mockit.Mock;
     38import mockit.MockUp;
     39
     40/**
     41 * Test class for {@link UploadNotesTask}
     42 * @author Taylor Smock
     43 */
     44@BasicPreferences
     45class UploadNotesTaskTest {
     46    @RegisterExtension
     47    static JOSMTestRules josmTestRules = new JOSMTestRules().fakeAPI();
     48
     49    static Stream<Arguments> testUpload() {
     50        final NoteData commonData = new NoteData();
     51        for (int i = 0; i < 12; i++) {
     52            for (Note.State state : Note.State.values()) {
     53                final Note note1 = new Note(LatLon.ZERO);
     54                note1.setId((state.ordinal() + 1) * (i + 1));
     55                note1.setCreatedAt(Instant.ofEpochSecond(TimeUnit.DAYS.toSeconds(365) * i));
     56                note1.setState(state);
     57                if (i > 2) {
     58                    note1.addComment(new NoteComment(note1.getCreatedAt().plusSeconds(60),
     59                            User.getAnonymous(), state.toString() + i, NoteComment.Action.OPENED, false));
     60                }
     61                if (i > 4) {
     62                    note1.addComment(new NoteComment(note1.getCreatedAt().plusSeconds(120),
     63                            User.getAnonymous(), state.toString() + i, NoteComment.Action.COMMENTED, false));
     64                }
     65                if (i > 6) {
     66                    Instant closedAt = note1.getCreatedAt().plusSeconds(180);
     67                    note1.addComment(new NoteComment(closedAt,
     68                            User.getAnonymous(), state.toString() + i, NoteComment.Action.CLOSED, false));
     69                    note1.setClosedAt(closedAt);
     70                    note1.setState(Note.State.CLOSED);
     71                }
     72                if (i > 8) {
     73                    note1.addComment(new NoteComment(note1.getCreatedAt().plusSeconds(240),
     74                            User.getAnonymous(), state.toString() + i, NoteComment.Action.REOPENED, false));
     75                    note1.setClosedAt(null);
     76                    note1.setState(Note.State.OPEN);
     77                }
     78                if (i > 10) {
     79                    note1.addComment(new NoteComment(note1.getCreatedAt().plusSeconds(300),
     80                            User.getAnonymous(), state.toString() + i, NoteComment.Action.HIDDEN, false));
     81                }
     82                commonData.addNotes(Collections.singleton(note1));
     83            }
     84        }
     85        return Stream.of(
     86                Arguments.of(new NoteData(commonData.getNotes()), Collections.singleton(generateNote(1, null, null,
     87                        new NoteComment.Action[] {NoteComment.Action.OPENED}, new boolean[] {true}))),
     88                Arguments.of(new NoteData(commonData.getNotes()), Collections.singleton(generateNote(2, Instant.now(), null,
     89                        new NoteComment.Action[] {NoteComment.Action.OPENED, NoteComment.Action.COMMENTED}, new boolean[] {false, true}))),
     90                Arguments.of(new NoteData(commonData.getNotes()), Collections.singleton(generateNote(3, Instant.now(),
     91                        Instant.now().plusSeconds(60), new NoteComment.Action[] {NoteComment.Action.OPENED,
     92                                NoteComment.Action.COMMENTED, NoteComment.Action.CLOSED}, new boolean[] {false, false, true}))),
     93                Arguments.of(new NoteData(commonData.getNotes()), Collections.singleton(generateNote(4, Instant.now(),
     94                        Instant.now().plusSeconds(60), new NoteComment.Action[] {NoteComment.Action.OPENED,
     95                                NoteComment.Action.COMMENTED, NoteComment.Action.CLOSED, NoteComment.Action.REOPENED},
     96                        new boolean[] {false, false, false, true})))
     97        );
     98    }
     99
     100    private static Note generateNote(int id, Instant openedAt, Instant closedAt, NoteComment.Action[] actions, boolean[] isNew) {
     101        final Note newNote = new Note(LatLon.ZERO);
     102        newNote.setId(id);
     103        if (openedAt != null) {
     104            newNote.setState(Note.State.OPEN);
     105            newNote.setCreatedAt(openedAt);
     106        } else {
     107            openedAt = Instant.now();
     108        }
     109        if (closedAt != null) {
     110            newNote.setState(Note.State.CLOSED);
     111            newNote.setClosedAt(closedAt);
     112        }
     113
     114        for (int i = 0; i < actions.length; i++) {
     115            NoteComment.Action action = actions[i];
     116            newNote.addComment(new NoteComment(openedAt.plusSeconds(30L * i), User.getAnonymous(),
     117                    action.toString() + i, action, isNew[i]));
     118        }
     119
     120        return newNote;
     121    }
     122
     123    @ParameterizedTest
     124    @MethodSource
     125    void testUpload(final NoteData noteData, final Collection<Note> shouldBeUploaded)
     126            throws ExecutionException, InterruptedException {
     127        TestUtils.assumeWorkingJMockit();
     128        Logging.clearLastErrorAndWarnings();
     129        FakeOsmApiMocker fakeOsmApiMocker = new FakeOsmApiMocker();
     130        noteData.addNotes(shouldBeUploaded);
     131        new UploadNotesTask().uploadNotes(noteData, NullProgressMonitor.INSTANCE);
     132        // Sync both threads.
     133        MainApplication.worker.submit(() -> { /* Sync worker thread */ }).get();
     134        GuiHelper.runInEDTAndWait(() -> { /* Sync UI thread */ });
     135        assertTrue(noteData.getNotes().containsAll(shouldBeUploaded));
     136        for (Note note : noteData.getNotes()) {
     137            for (NoteComment comment : note.getComments().stream().filter(NoteComment::isNew).collect(Collectors.toList())) {
     138                assertTrue(shouldBeUploaded.contains(note));
     139                NoteComment.Action action = comment.getNoteAction();
     140                if (action == NoteComment.Action.CLOSED) {
     141                    assertTrue(fakeOsmApiMocker.closed.contains(note));
     142                } else if (action == NoteComment.Action.COMMENTED) {
     143                    assertTrue(fakeOsmApiMocker.commented.contains(note));
     144                } else if (action == NoteComment.Action.REOPENED) {
     145                    assertTrue(fakeOsmApiMocker.reopened.contains(note));
     146                } else if (action == NoteComment.Action.OPENED) {
     147                    assertTrue(fakeOsmApiMocker.created.stream().anyMatch(n -> n.getFirstComment().getText().equals(comment.getText())));
     148                }
     149            }
     150            if (!shouldBeUploaded.contains(note)) {
     151                assertAll("All comments should not be new", note.getComments().stream().map(comment -> () -> assertFalse(comment.isNew())));
     152                assertAll("All comments should not be uploaded",
     153                        () -> assertFalse(fakeOsmApiMocker.closed.contains(note)),
     154                        () -> assertFalse(fakeOsmApiMocker.commented.contains(note)),
     155                        () -> assertFalse(fakeOsmApiMocker.created.contains(note)),
     156                        () -> assertFalse(fakeOsmApiMocker.reopened.contains(note)));
     157            }
     158        }
     159        assertTrue(Logging.getLastErrorAndWarnings().isEmpty());
     160    }
     161
     162    private static class FakeOsmApiMocker extends MockUp<FakeOsmApi> {
     163        Collection<Note> closed = new ArrayList<>();
     164        Collection<Note> commented = new ArrayList<>();
     165        Collection<Note> created = new ArrayList<>();
     166        Collection<Note> reopened = new ArrayList<>();
     167        @Mock
     168        public Note createNote(LatLon latlon, String text, ProgressMonitor monitor) throws OsmTransferException {
     169            final Note newNote = new Note(latlon);
     170            this.created.add(newNote);
     171            newNote.setId(Instant.now().toEpochMilli());
     172            newNote.setClosedAt(Instant.now());
     173            newNote.addComment(new NoteComment(Instant.now(), User.getAnonymous(), text, NoteComment.Action.OPENED, false));
     174            return newNote;
     175        }
     176
     177        @Mock
     178        public Note addCommentToNote(Note note, String comment, ProgressMonitor monitor) throws OsmTransferException {
     179            this.commented.add(note);
     180            return note;
     181        }
     182
     183        @Mock
     184        public Note closeNote(Note note, String closeMessage, ProgressMonitor monitor) throws OsmTransferException {
     185            this.closed.add(note);
     186            return note;
     187        }
     188
     189        @Mock
     190        public Note reopenNote(Note note, String reactivateMessage, ProgressMonitor monitor) throws OsmTransferException {
     191            this.reopened.add(note);
     192            return note;
     193        }
     194    }
     195}