[se-edu#737] Issue with undo/redo and modifications to filtered lists (…

…se-edu#792)

EditCommand and DeleteCommand stores the target index of the person to edit
or delete respectively.

As a result, redoing of these index-based commands will be executed on the incorrect
person due to different indexing in differing views.

Let's update EditCommand and DeleteCommand to find and store the personToEdit
or personToDelete respectively before it's first execution. This ensures that future
execution of the same command will be on the correct person.

src/main/java/seedu/address/logic/commands/DeleteCommand.java

@@ -1,6 +1,9 @@
 package seedu.address.logic.commands;
 
+import static java.util.Objects.requireNonNull;
+
 import java.util.List;
+import java.util.Objects;
 
 import seedu.address.commons.core.Messages;
 import seedu.address.commons.core.index.Index;
@@ -24,22 +27,16 @@ public class DeleteCommand extends UndoableCommand {
 
     private final Index targetIndex;
 
+    private Person personToDelete;
+
     public DeleteCommand(Index targetIndex) {
         this.targetIndex = targetIndex;
     }
 
 
     @Override
-    public CommandResult executeUndoableCommand() throws CommandException {
-
-        List<Person> lastShownList = model.getFilteredPersonList();
-
-        if (targetIndex.getZeroBased() >= lastShownList.size()) {
-            throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
-        }
-
-        Person personToDelete = lastShownList.get(targetIndex.getZeroBased());
-
+    public CommandResult executeUndoableCommand() {
+        requireNonNull(personToDelete);
         try {
             model.deletePerson(personToDelete);
         } catch (PersonNotFoundException pnfe) {
@@ -49,10 +46,22 @@ public CommandResult executeUndoableCommand() throws CommandException {
         return new CommandResult(String.format(MESSAGE_DELETE_PERSON_SUCCESS, personToDelete));
     }
 
+    @Override
+    protected void preprocessUndoableCommand() throws CommandException {
+        List<Person> lastShownList = model.getFilteredPersonList();
+
+        if (targetIndex.getZeroBased() >= lastShownList.size()) {
+            throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
+        }
+
+        personToDelete = lastShownList.get(targetIndex.getZeroBased());
+    }
+
     @Override
     public boolean equals(Object other) {
         return other == this // short circuit if same object
                 || (other instanceof DeleteCommand // instanceof handles nulls
-                && this.targetIndex.equals(((DeleteCommand) other).targetIndex)); // state check
+                && this.targetIndex.equals(((DeleteCommand) other).targetIndex) // state check
+                && Objects.equals(this.personToDelete, ((DeleteCommand) other).personToDelete));
     }
 }

src/main/java/seedu/address/logic/commands/EditCommand.java

@@ -11,6 +11,7 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 
@@ -54,6 +55,9 @@ public class EditCommand extends UndoableCommand {
     private final Index index;
     private final EditPersonDescriptor editPersonDescriptor;
 
+    private Person personToEdit;
+    private Person editedPerson;
+
     /**
      * @param index of the person in the filtered person list to edit
      * @param editPersonDescriptor details to edit the person with
@@ -68,15 +72,6 @@ public EditCommand(Index index, EditPersonDescriptor editPersonDescriptor) {
 
     @Override
     public CommandResult executeUndoableCommand() throws CommandException {
-        List<Person> lastShownList = model.getFilteredPersonList();
-
-        if (index.getZeroBased() >= lastShownList.size()) {
-            throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
-        }
-
-        Person personToEdit = lastShownList.get(index.getZeroBased());
-        Person editedPerson = createEditedPerson(personToEdit, editPersonDescriptor);
-
         try {
             model.updatePerson(personToEdit, editedPerson);
         } catch (DuplicatePersonException dpe) {
@@ -88,6 +83,18 @@ public CommandResult executeUndoableCommand() throws CommandException {
         return new CommandResult(String.format(MESSAGE_EDIT_PERSON_SUCCESS, editedPerson));
     }
 
+    @Override
+    protected void preprocessUndoableCommand() throws CommandException {
+        List<Person> lastShownList = model.getFilteredPersonList();
+
+        if (index.getZeroBased() >= lastShownList.size()) {
+            throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
+        }
+
+        personToEdit = lastShownList.get(index.getZeroBased());
+        editedPerson = createEditedPerson(personToEdit, editPersonDescriptor);
+    }
+
     /**
      * Creates and returns a {@code Person} with the details of {@code personToEdit}
      * edited with {@code editPersonDescriptor}.
@@ -119,7 +126,8 @@ public boolean equals(Object other) {
         // state check
         EditCommand e = (EditCommand) other;
         return index.equals(e.index)
-                && editPersonDescriptor.equals(e.editPersonDescriptor);
+                && editPersonDescriptor.equals(e.editPersonDescriptor)
+                && Objects.equals(personToEdit, e.personToEdit);
     }
 
     /**

src/main/java/seedu/address/logic/commands/UndoableCommand.java

@@ -24,6 +24,12 @@ private void saveAddressBookSnapshot() {
         this.previousAddressBook = new AddressBook(model.getAddressBook());
     }
 
+    /**
+     * This method is called before the execution of {@code UndoableCommand}.
+     * {@code UndoableCommand}s that require this preprocessing step should override this method.
+     */
+    protected void preprocessUndoableCommand() throws CommandException {}
+
     /**
      * Reverts the AddressBook to the state before this command
      * was executed and updates the filtered person list to
@@ -53,6 +59,7 @@ protected final void redo() {
     @Override
     public final CommandResult execute() throws CommandException {
         saveAddressBookSnapshot();
+        preprocessUndoableCommand();
         return executeUndoableCommand();
     }
 }

src/test/java/seedu/address/logic/commands/CommandTestUtil.java

@@ -1,6 +1,7 @@
 package seedu.address.logic.commands;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static seedu.address.logic.parser.CliSyntax.PREFIX_ADDRESS;
 import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
@@ -12,6 +13,9 @@
 import java.util.Arrays;
 import java.util.List;
 
+import seedu.address.commons.core.index.Index;
+import seedu.address.logic.CommandHistory;
+import seedu.address.logic.UndoRedoStack;
 import seedu.address.logic.commands.exceptions.CommandException;
 import seedu.address.model.AddressBook;
 import seedu.address.model.Model;
@@ -107,10 +111,13 @@ public static void assertCommandFailure(Command command, Model actualModel, Stri
     }
 
     /**
-     * Updates {@code model}'s filtered list to show only the first person in the {@code model}'s address book.
+     * Updates {@code model}'s filtered list to show only the person at the given {@code targetIndex} in the
+     * {@code model}'s address book.
      */
-    public static void showFirstPersonOnly(Model model) {
-        Person person = model.getAddressBook().getPersonList().get(0);
+    public static void showPersonAtIndex(Model model, Index targetIndex) {
+        assertTrue(targetIndex.getZeroBased() < model.getFilteredPersonList().size());
+
+        Person person = model.getFilteredPersonList().get(targetIndex.getZeroBased());
         final String[] splitName = person.getName().fullName.split("\\s+");
         model.updateFilteredPersonList(new NameContainsKeywordsPredicate(Arrays.asList(splitName[0])));
 
@@ -128,4 +135,22 @@ public static void deleteFirstPerson(Model model) {
             throw new AssertionError("Person in filtered list must exist in model.", pnfe);
         }
     }
+
+    /**
+     * Returns an {@code UndoCommand} with the given {@code model} and {@code undoRedoStack} set.
+     */
+    public static UndoCommand prepareUndoCommand(Model model, UndoRedoStack undoRedoStack) {
+        UndoCommand undoCommand = new UndoCommand();
+        undoCommand.setData(model, new CommandHistory(), undoRedoStack);
+        return undoCommand;
+    }
+
+    /**
+     * Returns a {@code RedoCommand} with the given {@code model} and {@code undoRedoStack} set.
+     */
+    public static RedoCommand prepareRedoCommand(Model model, UndoRedoStack undoRedoStack) {
+        RedoCommand redoCommand = new RedoCommand();
+        redoCommand.setData(model, new CommandHistory(), undoRedoStack);
+        return redoCommand;
+    }
 }

src/test/java/seedu/address/logic/commands/DeleteCommandTest.java

@@ -1,10 +1,13 @@
 package seedu.address.logic.commands;
 
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import static seedu.address.logic.commands.CommandTestUtil.assertCommandFailure;
 import static seedu.address.logic.commands.CommandTestUtil.assertCommandSuccess;
-import static seedu.address.logic.commands.CommandTestUtil.showFirstPersonOnly;
+import static seedu.address.logic.commands.CommandTestUtil.prepareRedoCommand;
+import static seedu.address.logic.commands.CommandTestUtil.prepareUndoCommand;
+import static seedu.address.logic.commands.CommandTestUtil.showPersonAtIndex;
 import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON;
 import static seedu.address.testutil.TypicalIndexes.INDEX_SECOND_PERSON;
 import static seedu.address.testutil.TypicalPersons.getTypicalAddressBook;
@@ -21,7 +24,8 @@
 import seedu.address.model.person.Person;
 
 /**
- * Contains integration tests (interaction with the Model) and unit tests for {@code DeleteCommand}.
+ * Contains integration tests (interaction with the Model, UndoCommand and RedoCommand) and unit tests for
+ * {@code DeleteCommand}.
  */
 public class DeleteCommandTest {
 
@@ -50,7 +54,7 @@ public void execute_invalidIndexUnfilteredList_throwsCommandException() throws E
 
     @Test
     public void execute_validIndexFilteredList_success() throws Exception {
-        showFirstPersonOnly(model);
+        showPersonAtIndex(model, INDEX_FIRST_PERSON);
 
         Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());
         DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON);
@@ -66,7 +70,7 @@ public void execute_validIndexFilteredList_success() throws Exception {
 
     @Test
     public void execute_invalidIndexFilteredList_throwsCommandException() {
-        showFirstPersonOnly(model);
+        showPersonAtIndex(model, INDEX_FIRST_PERSON);
 
         Index outOfBoundIndex = INDEX_SECOND_PERSON;
         // ensures that outOfBoundIndex is still in bounds of address book list
@@ -78,17 +82,88 @@ public void execute_invalidIndexFilteredList_throwsCommandException() {
     }
 
     @Test
-    public void equals() {
-        DeleteCommand deleteFirstCommand = new DeleteCommand(INDEX_FIRST_PERSON);
-        DeleteCommand deleteSecondCommand = new DeleteCommand(INDEX_SECOND_PERSON);
+    public void executeUndoRedo_validIndexUnfilteredList_success() throws Exception {
+        UndoRedoStack undoRedoStack = new UndoRedoStack();
+        UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack);
+        RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack);
+        Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());
+        DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON);
+        Model expectedModel = new ModelManager(model.getAddressBook(), new UserPrefs());
+
+        // delete -> first person deleted
+        deleteCommand.execute();
+        undoRedoStack.push(deleteCommand);
+
+        // undo -> reverts addressbook back to previous state and filtered person list to show all persons
+        assertCommandSuccess(undoCommand, model, UndoCommand.MESSAGE_SUCCESS, expectedModel);
+
+        // redo -> same first person deleted again
+        expectedModel.deletePerson(personToDelete);
+        assertCommandSuccess(redoCommand, model, RedoCommand.MESSAGE_SUCCESS, expectedModel);
+    }
+
+    @Test
+    public void executeUndoRedo_invalidIndexUnfilteredList_failure() {
+        UndoRedoStack undoRedoStack = new UndoRedoStack();
+        UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack);
+        RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack);
+        Index outOfBoundIndex = Index.fromOneBased(model.getFilteredPersonList().size() + 1);
+        DeleteCommand deleteCommand = prepareCommand(outOfBoundIndex);
+
+        // execution failed -> deleteCommand not pushed into undoRedoStack
+        assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
+
+        // no commands in undoRedoStack -> undoCommand and redoCommand fail
+        assertCommandFailure(undoCommand, model, UndoCommand.MESSAGE_FAILURE);
+        assertCommandFailure(redoCommand, model, RedoCommand.MESSAGE_FAILURE);
+    }
+
+    /**
+     * 1. Deletes a {@code Person} from a filtered list.
+     * 2. Undo the deletion.
+     * 3. The unfiltered list should be shown now. Verify that the index of the previously deleted person in the
+     * unfiltered list is different from the index at the filtered list.
+     * 4. Redo the deletion. This ensures {@code RedoCommand} deletes the person object regardless of indexing.
+     */
+    @Test
+    public void executeUndoRedo_validIndexFilteredList_samePersonDeleted() throws Exception {
+        UndoRedoStack undoRedoStack = new UndoRedoStack();
+        UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack);
+        RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack);
+        DeleteCommand deleteCommand = prepareCommand(INDEX_FIRST_PERSON);
+        Model expectedModel = new ModelManager(model.getAddressBook(), new UserPrefs());
+
+        showPersonAtIndex(model, INDEX_SECOND_PERSON);
+        Person personToDelete = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased());
+        // delete -> deletes second person in unfiltered person list / first person in filtered person list
+        deleteCommand.execute();
+        undoRedoStack.push(deleteCommand);
+
+        // undo -> reverts addressbook back to previous state and filtered person list to show all persons
+        assertCommandSuccess(undoCommand, model, UndoCommand.MESSAGE_SUCCESS, expectedModel);
+
+        expectedModel.deletePerson(personToDelete);
+        assertNotEquals(personToDelete, model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()));
+        // redo -> deletes same second person in unfiltered person list
+        assertCommandSuccess(redoCommand, model, RedoCommand.MESSAGE_SUCCESS, expectedModel);
+    }
+
+    @Test
+    public void equals() throws Exception {
+        DeleteCommand deleteFirstCommand = prepareCommand(INDEX_FIRST_PERSON);
+        DeleteCommand deleteSecondCommand = prepareCommand(INDEX_SECOND_PERSON);
 
         // same object -> returns true
         assertTrue(deleteFirstCommand.equals(deleteFirstCommand));
 
         // same values -> returns true
-        DeleteCommand deleteFirstCommandCopy = new DeleteCommand(INDEX_FIRST_PERSON);
+        DeleteCommand deleteFirstCommandCopy = prepareCommand(INDEX_FIRST_PERSON);
         assertTrue(deleteFirstCommand.equals(deleteFirstCommandCopy));
 
+        // one command preprocessed when previously equal -> returns false
+        deleteFirstCommandCopy.preprocessUndoableCommand();
+        assertFalse(deleteFirstCommand.equals(deleteFirstCommandCopy));
+
         // different types -> returns false
         assertFalse(deleteFirstCommand.equals(1));