From 5c9b14d7a76dfd21ed11042e9ed122beef5a386d Mon Sep 17 00:00:00 2001 From: Jun An Date: Tue, 23 Jan 2018 02:16:31 +0800 Subject: [PATCH 1/6] UndoableCommand: add preprocessUndoableCommand() method UndoableCommand does not have a method to preprocess and generate the values required for the execution of the command. This leads to the commands handling the preprocessing step within the UndoableCommand#executeUndoableCommand() method. This violates SRP, as each method is only supposed to do only one thing. Let's add a new method preprocessUndoableCommand() for these commands to run their preprocessing step on. --- .../java/seedu/address/logic/commands/UndoableCommand.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/main/java/seedu/address/logic/commands/UndoableCommand.java b/src/main/java/seedu/address/logic/commands/UndoableCommand.java index 1ba888ead594..c107ffcd9cb3 100644 --- a/src/main/java/seedu/address/logic/commands/UndoableCommand.java +++ b/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(); } } From 3825b9928e5a04ea485ae960f45c3f7391ec3345 Mon Sep 17 00:00:00 2001 From: Jun An Date: Wed, 31 Jan 2018 14:19:57 +0800 Subject: [PATCH 2/6] CommandTestUtil: update showFirstPersonOnly(Model) CommandTestUtil#showFirstPersonOnly(Model) shows only the first person on the filtered list. Some other tests may require showing a person that is not the first in the unfiltered list. However, writing new methods to show a different single person on the filtered list will lead to boilerplate code. Let's update and rename showFirstPersonOnly(Model) to showPersonAtIndex(Model, Index) so that we can input any arbitrary index for the single person we want to have displayed on the filtered list. --- .../seedu/address/logic/commands/CommandTestUtil.java | 11 ++++++++--- .../address/logic/commands/DeleteCommandTest.java | 6 +++--- .../seedu/address/logic/commands/EditCommandTest.java | 8 ++++---- .../seedu/address/logic/commands/ListCommandTest.java | 5 +++-- .../address/logic/commands/SelectCommandTest.java | 6 +++--- .../address/logic/commands/UndoableCommandTest.java | 7 ++++--- 6 files changed, 25 insertions(+), 18 deletions(-) diff --git a/src/test/java/seedu/address/logic/commands/CommandTestUtil.java b/src/test/java/seedu/address/logic/commands/CommandTestUtil.java index 10190e3642c3..a7b8ed7a54c2 100644 --- a/src/test/java/seedu/address/logic/commands/CommandTestUtil.java +++ b/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,7 @@ import java.util.Arrays; import java.util.List; +import seedu.address.commons.core.index.Index; import seedu.address.logic.commands.exceptions.CommandException; import seedu.address.model.AddressBook; import seedu.address.model.Model; @@ -107,10 +109,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]))); diff --git a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java index e35ba18fc548..9ff76f0cb0c3 100644 --- a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java @@ -4,7 +4,7 @@ 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.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; @@ -50,7 +50,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 +66,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 diff --git a/src/test/java/seedu/address/logic/commands/EditCommandTest.java b/src/test/java/seedu/address/logic/commands/EditCommandTest.java index 0b9f6756e19c..fe82e62bef63 100644 --- a/src/test/java/seedu/address/logic/commands/EditCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/EditCommandTest.java @@ -9,7 +9,7 @@ import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_HUSBAND; 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.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; @@ -85,7 +85,7 @@ public void execute_noFieldSpecifiedUnfilteredList_success() { @Test public void execute_filteredList_success() throws Exception { - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); Person personInFilteredList = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); Person editedPerson = new PersonBuilder(personInFilteredList).withName(VALID_NAME_BOB).build(); @@ -111,7 +111,7 @@ public void execute_duplicatePersonUnfilteredList_failure() { @Test public void execute_duplicatePersonFilteredList_failure() { - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); // edit person in filtered list into a duplicate in address book Person personInList = model.getAddressBook().getPersonList().get(INDEX_SECOND_PERSON.getZeroBased()); @@ -136,7 +136,7 @@ public void execute_invalidPersonIndexUnfilteredList_failure() { */ @Test public void execute_invalidPersonIndexFilteredList_failure() { - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); Index outOfBoundIndex = INDEX_SECOND_PERSON; // ensures that outOfBoundIndex is still in bounds of address book list assertTrue(outOfBoundIndex.getZeroBased() < model.getAddressBook().getPersonList().size()); diff --git a/src/test/java/seedu/address/logic/commands/ListCommandTest.java b/src/test/java/seedu/address/logic/commands/ListCommandTest.java index b282a238a0f5..4ee519e3668e 100644 --- a/src/test/java/seedu/address/logic/commands/ListCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/ListCommandTest.java @@ -1,7 +1,8 @@ package seedu.address.logic.commands; import static seedu.address.logic.commands.CommandTestUtil.assertCommandSuccess; -import static seedu.address.logic.commands.CommandTestUtil.showFirstPersonOnly; +import static seedu.address.logic.commands.CommandTestUtil.showPersonAtIndex; +import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON; import static seedu.address.testutil.TypicalPersons.getTypicalAddressBook; import org.junit.Before; @@ -38,7 +39,7 @@ public void execute_listIsNotFiltered_showsSameList() { @Test public void execute_listIsFiltered_showsEverything() { - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); assertCommandSuccess(listCommand, model, ListCommand.MESSAGE_SUCCESS, expectedModel); } } diff --git a/src/test/java/seedu/address/logic/commands/SelectCommandTest.java b/src/test/java/seedu/address/logic/commands/SelectCommandTest.java index 6442c954750b..4840900602ac 100644 --- a/src/test/java/seedu/address/logic/commands/SelectCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/SelectCommandTest.java @@ -4,7 +4,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import static seedu.address.logic.commands.CommandTestUtil.showFirstPersonOnly; +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.TypicalIndexes.INDEX_THIRD_PERSON; @@ -57,14 +57,14 @@ public void execute_invalidIndexUnfilteredList_failure() { @Test public void execute_validIndexFilteredList_success() { - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); assertExecutionSuccess(INDEX_FIRST_PERSON); } @Test public void execute_invalidIndexFilteredList_failure() { - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); Index outOfBoundsIndex = INDEX_SECOND_PERSON; // ensures that outOfBoundIndex is still in bounds of address book list diff --git a/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java b/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java index 7014654a8450..7d00a7471b86 100644 --- a/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/UndoableCommandTest.java @@ -3,7 +3,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import static seedu.address.logic.commands.CommandTestUtil.deleteFirstPerson; -import static seedu.address.logic.commands.CommandTestUtil.showFirstPersonOnly; +import static seedu.address.logic.commands.CommandTestUtil.showPersonAtIndex; +import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON; import static seedu.address.testutil.TypicalPersons.getTypicalAddressBook; import org.junit.Test; @@ -27,7 +28,7 @@ public void executeUndo() throws Exception { deleteFirstPerson(expectedModel); assertEquals(expectedModel, model); - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); // undo() should cause the model's filtered list to show all persons dummyCommand.undo(); @@ -37,7 +38,7 @@ public void executeUndo() throws Exception { @Test public void redo() { - showFirstPersonOnly(model); + showPersonAtIndex(model, INDEX_FIRST_PERSON); // redo() should cause the model's filtered list to show all persons dummyCommand.redo(); From d840c89cb12c18786d9c12ebadbb3e1a52aa0441 Mon Sep 17 00:00:00 2001 From: Jun An Date: Tue, 23 Jan 2018 02:39:03 +0800 Subject: [PATCH 3/6] DeleteCommand#preprocessUndoableCommand(): find store personToDelete DeleteCommand stores the target index of the person to delete. As a result, redoing of DeleteCommand will be executed on the incorrect person due to different indexing in differing views. Let's update DeleteCommand#preprocessUndoableCommand() to find and store the personToDelete before its first execution. This ensures that future execution of the same command will be on the correct person, independent of the current view. Let's also update DeleteCommand#equals(Object) method to include checking of the personToDelete. This allows other classes to know that DeleteCommand transits between 2 states, preprocessed and not-preprocessed. --- .../address/logic/commands/DeleteCommand.java | 31 +++++++----- .../logic/commands/CommandTestUtil.java | 20 ++++++++ .../logic/commands/DeleteCommandTest.java | 48 +++++++++++++++++-- .../logic/commands/RedoCommandTest.java | 7 ++- 4 files changed, 88 insertions(+), 18 deletions(-) diff --git a/src/main/java/seedu/address/logic/commands/DeleteCommand.java b/src/main/java/seedu/address/logic/commands/DeleteCommand.java index 14142e012906..b539d240001a 100644 --- a/src/main/java/seedu/address/logic/commands/DeleteCommand.java +++ b/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 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 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)); } } diff --git a/src/test/java/seedu/address/logic/commands/CommandTestUtil.java b/src/test/java/seedu/address/logic/commands/CommandTestUtil.java index a7b8ed7a54c2..9a5679cc29b6 100644 --- a/src/test/java/seedu/address/logic/commands/CommandTestUtil.java +++ b/src/test/java/seedu/address/logic/commands/CommandTestUtil.java @@ -14,6 +14,8 @@ 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; @@ -133,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; + } } diff --git a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java index 9ff76f0cb0c3..cfa5e0e9ee67 100644 --- a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java @@ -1,9 +1,12 @@ 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.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; @@ -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 { @@ -77,18 +81,52 @@ public void execute_invalidIndexFilteredList_throwsCommandException() { assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); } + /** + * 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 equals() { - DeleteCommand deleteFirstCommand = new DeleteCommand(INDEX_FIRST_PERSON); - DeleteCommand deleteSecondCommand = new DeleteCommand(INDEX_SECOND_PERSON); + 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)); diff --git a/src/test/java/seedu/address/logic/commands/RedoCommandTest.java b/src/test/java/seedu/address/logic/commands/RedoCommandTest.java index 840fea5031d7..e615f089a4f2 100644 --- a/src/test/java/seedu/address/logic/commands/RedoCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/RedoCommandTest.java @@ -5,6 +5,7 @@ import static seedu.address.logic.commands.CommandTestUtil.assertCommandSuccess; import static seedu.address.logic.commands.CommandTestUtil.deleteFirstPerson; 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; import java.util.Arrays; @@ -25,12 +26,14 @@ public class RedoCommandTest { private final Model model = new ModelManager(getTypicalAddressBook(), new UserPrefs()); private final DeleteCommand deleteCommandOne = new DeleteCommand(INDEX_FIRST_PERSON); - private final DeleteCommand deleteCommandTwo = new DeleteCommand(INDEX_FIRST_PERSON); + private final DeleteCommand deleteCommandTwo = new DeleteCommand(INDEX_SECOND_PERSON); @Before - public void setUp() { + public void setUp() throws Exception { deleteCommandOne.setData(model, EMPTY_COMMAND_HISTORY, EMPTY_STACK); deleteCommandTwo.setData(model, EMPTY_COMMAND_HISTORY, EMPTY_STACK); + deleteCommandOne.preprocessUndoableCommand(); + deleteCommandTwo.preprocessUndoableCommand(); } @Test From 3563cea0184935f5542fd554c3cf685164111ac4 Mon Sep 17 00:00:00 2001 From: Jun An Date: Tue, 23 Jan 2018 02:39:51 +0800 Subject: [PATCH 4/6] EditCommand#preprocessUndoableCommand(): find and store personToEdit EditCommand stores the target index of the person to edit. As a result, redoing of EditCommand will be executed on the incorrect person due to different indexing in differing views. Let's update EditCommand#preprocessUndoableCommand() to find and store the personToEdit before its first execution. This ensures that future execution of the same command will be on the correct person, independent of the current view. Let's also update EditCommand#equals(Object) method to include checking of personToEdit. This allows other classes to know that EditCommand transits between 2 states, preprocessed and not-preprocessed. --- .../address/logic/commands/EditCommand.java | 28 +++++++---- .../logic/commands/EditCommandTest.java | 47 +++++++++++++++++-- 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/src/main/java/seedu/address/logic/commands/EditCommand.java b/src/main/java/seedu/address/logic/commands/EditCommand.java index 5b258a7d32b9..e6c3a3e034bc 100644 --- a/src/main/java/seedu/address/logic/commands/EditCommand.java +++ b/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 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 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); } /** diff --git a/src/test/java/seedu/address/logic/commands/EditCommandTest.java b/src/test/java/seedu/address/logic/commands/EditCommandTest.java index fe82e62bef63..a576ae519c7b 100644 --- a/src/test/java/seedu/address/logic/commands/EditCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/EditCommandTest.java @@ -1,6 +1,7 @@ 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.DESC_AMY; import static seedu.address.logic.commands.CommandTestUtil.DESC_BOB; @@ -9,6 +10,8 @@ import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_HUSBAND; import static seedu.address.logic.commands.CommandTestUtil.assertCommandFailure; import static seedu.address.logic.commands.CommandTestUtil.assertCommandSuccess; +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; @@ -30,7 +33,7 @@ import seedu.address.testutil.PersonBuilder; /** - * Contains integration tests (interaction with the Model) and unit tests for EditCommand. + * Contains integration tests (interaction with the Model, UndoCommand and RedoCommand) and unit tests for EditCommand. */ public class EditCommandTest { @@ -147,18 +150,54 @@ public void execute_invalidPersonIndexFilteredList_failure() { assertCommandFailure(editCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); } + /** + * 1. Edits a {@code Person} from a filtered list. + * 2. Undo the edit. + * 3. The unfiltered list should be shown now. Verify that the index of the previously edited person in the + * unfiltered list is different from the index at the filtered list. + * 4. Redo the edit. This ensures {@code RedoCommand} edits the person object regardless of indexing. + */ @Test - public void equals() { - final EditCommand standardCommand = new EditCommand(INDEX_FIRST_PERSON, DESC_AMY); + public void executeUndoRedo_validIndexFilteredList_samePersonEdited() throws Exception { + UndoRedoStack undoRedoStack = new UndoRedoStack(); + UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack); + RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack); + Person editedPerson = new PersonBuilder().build(); + EditPersonDescriptor descriptor = new EditPersonDescriptorBuilder(editedPerson).build(); + EditCommand editCommand = prepareCommand(INDEX_FIRST_PERSON, descriptor); + Model expectedModel = new ModelManager(new AddressBook(model.getAddressBook()), new UserPrefs()); + + showPersonAtIndex(model, INDEX_SECOND_PERSON); + Person personToEdit = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); + // edit -> edits second person in unfiltered person list / first person in filtered person list + editCommand.execute(); + undoRedoStack.push(editCommand); + + // undo -> reverts addressbook back to previous state and filtered person list to show all persons + assertCommandSuccess(undoCommand, model, UndoCommand.MESSAGE_SUCCESS, expectedModel); + + expectedModel.updatePerson(personToEdit, editedPerson); + assertNotEquals(model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()), personToEdit); + // redo -> edits same second person in unfiltered person list + assertCommandSuccess(redoCommand, model, RedoCommand.MESSAGE_SUCCESS, expectedModel); + } + + @Test + public void equals() throws Exception { + final EditCommand standardCommand = prepareCommand(INDEX_FIRST_PERSON, DESC_AMY); // same values -> returns true EditPersonDescriptor copyDescriptor = new EditPersonDescriptor(DESC_AMY); - EditCommand commandWithSameValues = new EditCommand(INDEX_FIRST_PERSON, copyDescriptor); + EditCommand commandWithSameValues = prepareCommand(INDEX_FIRST_PERSON, copyDescriptor); assertTrue(standardCommand.equals(commandWithSameValues)); // same object -> returns true assertTrue(standardCommand.equals(standardCommand)); + // one command preprocessed when previously equal -> returns false + commandWithSameValues.preprocessUndoableCommand(); + assertFalse(standardCommand.equals(commandWithSameValues)); + // null -> returns false assertFalse(standardCommand.equals(null)); From ea2d584d8cbb1a8484aee258c6a69c481f19d264 Mon Sep 17 00:00:00 2001 From: Jun An Date: Sun, 28 Jan 2018 01:23:16 +0800 Subject: [PATCH 5/6] DeleteCommandTest: add integration tests with Undo/RedoCommand DeleteCommandTest does not have any integration tests for the interaction with UndoCommand and RedoCommand on an unfiltered list. Let's add integration tests for the interaction of DeleteCommand with UndoCommand and RedoCommand on an unfiltered list. --- .../logic/commands/DeleteCommandTest.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java index cfa5e0e9ee67..866e6a9be32a 100644 --- a/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/DeleteCommandTest.java @@ -81,6 +81,43 @@ public void execute_invalidIndexFilteredList_throwsCommandException() { assertCommandFailure(deleteCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); } + @Test + 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. From 4be7dd7dec961db460bc0f91364d8d6fe6f4453a Mon Sep 17 00:00:00 2001 From: Jun An Date: Sun, 28 Jan 2018 01:23:16 +0800 Subject: [PATCH 6/6] EditCommandTest: add integration tests with Undo/RedoCommand EditCommandTest does not have any integration tests for the interaction with UndoCommand and RedoCommand on an unfiltered list. Let's add integration tests for the interaction of EditCommand with UndoCommand and RedoCommand on an unfiltered list. --- .../logic/commands/EditCommandTest.java | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/test/java/seedu/address/logic/commands/EditCommandTest.java b/src/test/java/seedu/address/logic/commands/EditCommandTest.java index a576ae519c7b..a8b104d3a81d 100644 --- a/src/test/java/seedu/address/logic/commands/EditCommandTest.java +++ b/src/test/java/seedu/address/logic/commands/EditCommandTest.java @@ -150,6 +150,46 @@ public void execute_invalidPersonIndexFilteredList_failure() { assertCommandFailure(editCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); } + @Test + public void executeUndoRedo_validIndexUnfilteredList_success() throws Exception { + UndoRedoStack undoRedoStack = new UndoRedoStack(); + UndoCommand undoCommand = prepareUndoCommand(model, undoRedoStack); + RedoCommand redoCommand = prepareRedoCommand(model, undoRedoStack); + Person editedPerson = new PersonBuilder().build(); + Person personToEdit = model.getFilteredPersonList().get(INDEX_FIRST_PERSON.getZeroBased()); + EditPersonDescriptor descriptor = new EditPersonDescriptorBuilder(editedPerson).build(); + EditCommand editCommand = prepareCommand(INDEX_FIRST_PERSON, descriptor); + Model expectedModel = new ModelManager(new AddressBook(model.getAddressBook()), new UserPrefs()); + + // edit -> first person edited + editCommand.execute(); + undoRedoStack.push(editCommand); + + // 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 edited again + expectedModel.updatePerson(personToEdit, editedPerson); + 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); + EditPersonDescriptor descriptor = new EditPersonDescriptorBuilder().withName(VALID_NAME_BOB).build(); + EditCommand editCommand = prepareCommand(outOfBoundIndex, descriptor); + + // execution failed -> editCommand not pushed into undoRedoStack + assertCommandFailure(editCommand, 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. Edits a {@code Person} from a filtered list. * 2. Undo the edit.