-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add add/delete function to TagCommand #52
Conversation
Instead of replacing all tags with user inputed tags, now user can add and delete individual tags from the student. However, users can still mass delete or replace using the old function.
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
============================================
+ Coverage 76.55% 77.01% +0.45%
- Complexity 468 478 +10
============================================
Files 74 76 +2
Lines 1497 1540 +43
Branches 151 152 +1
============================================
+ Hits 1146 1186 +40
- Misses 306 308 +2
- Partials 45 46 +1
|
Added test cases for adding and deleting tags to ensure that it is working as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, solid effort for add/delete function to TagCommand. Great job!
Would it be possible for you to make the code pass the Java CI?
package seedu.address.logic.commands; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertFalse; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
import static seedu.address.logic.commands.CommandTestUtil.VALID_TAG_FRIENDS; | ||
import static seedu.address.logic.commands.CommandTestUtil.assertCommandFailure; | ||
import static seedu.address.logic.commands.CommandTestUtil.assertCommandSuccess; | ||
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; | ||
|
||
import java.util.Set; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import seedu.address.commons.core.index.Index; | ||
import seedu.address.logic.Messages; | ||
import seedu.address.model.AddressBook; | ||
import seedu.address.model.Model; | ||
import seedu.address.model.ModelManager; | ||
import seedu.address.model.UserPrefs; | ||
import seedu.address.model.person.Person; | ||
import seedu.address.model.tag.Tag; | ||
import seedu.address.model.util.SampleDataUtil; | ||
import seedu.address.testutil.PersonBuilder; | ||
import seedu.address.testutil.TypicalPersons; | ||
|
||
public class DeleteTagCommandTest { | ||
|
||
private Model model = new ModelManager(getTypicalAddressBook(), new UserPrefs()); | ||
|
||
@Test | ||
public void execute_deleteTag_success() { | ||
Person editedStudent = new PersonBuilder(model.getFilteredPersonList().get(1)) | ||
.withTags(VALID_TAG_FRIENDS).build(); | ||
Set<Tag> tagToDelete = SampleDataUtil.getTagSet("owesMoney"); | ||
DeleteTagCommand deleteTagCommand = new DeleteTagCommand(INDEX_SECOND_PERSON, | ||
tagToDelete); | ||
|
||
String expectedMessage = String.format(TagCommand.MESSAGE_DELETE_TAG_SUCCESS, | ||
editedStudent.getName()) + tagToDelete; | ||
|
||
Model expectedModel = new ModelManager(new AddressBook(model.getAddressBook()), new UserPrefs()); | ||
expectedModel.setPerson(model.getFilteredPersonList().get(1), editedStudent); | ||
|
||
assertCommandSuccess(deleteTagCommand, model, expectedMessage, expectedModel); | ||
assertEquals(editedStudent.getTags(), model.getFilteredPersonList().get(1).getTags()); | ||
} | ||
|
||
@Test | ||
public void execute_indexOutOfBound_failure() { | ||
showPersonAtIndex(model, INDEX_FIRST_PERSON); | ||
Index outOfBoundIndex = INDEX_SECOND_PERSON; | ||
|
||
assertTrue(outOfBoundIndex.getZeroBased() < model.getAddressBook().getPersonList().size()); | ||
|
||
DeleteTagCommand deleteTagCommand = new DeleteTagCommand(outOfBoundIndex, TypicalPersons.ALICE.getTags()); | ||
|
||
assertCommandFailure(deleteTagCommand, model, Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); | ||
} | ||
|
||
@Test | ||
public void equals() { | ||
final DeleteTagCommand standardCommand = new DeleteTagCommand(INDEX_FIRST_PERSON, | ||
TypicalPersons.ALICE.getTags()); | ||
|
||
DeleteTagCommand commandWithSameValue = new DeleteTagCommand(INDEX_FIRST_PERSON, | ||
SampleDataUtil.getTagSet(VALID_TAG_FRIENDS)); | ||
|
||
assertTrue(standardCommand.equals(commandWithSameValue)); | ||
|
||
assertTrue(standardCommand.equals(standardCommand)); | ||
|
||
assertFalse(standardCommand.equals(null)); | ||
|
||
assertFalse(standardCommand.equals(new ClearCommand())); | ||
} | ||
|
||
@Test | ||
public void toStringMethod() { | ||
Index targetIndex = Index.fromOneBased(1); | ||
DeleteTagCommand deleteTagCommand = new DeleteTagCommand(targetIndex, TypicalPersons.ALICE.getTags()); | ||
String expected = DeleteTagCommand.class.getCanonicalName() + "{tags=" + TypicalPersons.ALICE.getTags() + "}"; | ||
assertEquals(expected, deleteTagCommand.toString()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your effort on sufficient testing on deleting tests.
package seedu.address.logic.commands; | ||
|
||
import static seedu.address.logic.Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX; | ||
|
||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Set; | ||
|
||
import seedu.address.commons.core.index.Index; | ||
import seedu.address.logic.commands.exceptions.CommandException; | ||
import seedu.address.model.Model; | ||
import seedu.address.model.person.Person; | ||
import seedu.address.model.tag.Tag; | ||
|
||
/** | ||
* Deletes the tags of an existing student in the address book. | ||
*/ | ||
public class DeleteTagCommand extends TagCommand { | ||
|
||
public DeleteTagCommand(Index index, Set<Tag> tags) { | ||
super(index, tags); | ||
} | ||
|
||
@Override | ||
public CommandResult execute(Model model) throws CommandException { | ||
List<Person> lastShownList = model.getFilteredPersonList(); | ||
|
||
if (index.getZeroBased() >= lastShownList.size()) { | ||
throw new CommandException(MESSAGE_INVALID_PERSON_DISPLAYED_INDEX); | ||
} | ||
|
||
Person personToEdit = lastShownList.get(index.getZeroBased()); | ||
Set<Tag> newTags = deleteTags(personToEdit.getTags(), super.tags); | ||
Person editedPerson = new Person( | ||
personToEdit.getName(), personToEdit.getPhone(), personToEdit.getEmail(), | ||
personToEdit.getAddress(), newTags); | ||
|
||
model.setPerson(personToEdit, editedPerson); | ||
model.updateFilteredPersonList(Model.PREDICATE_SHOW_ALL_PERSONS); | ||
|
||
return new CommandResult(generateSuccessMessage(editedPerson)); | ||
} | ||
|
||
private Set<Tag> deleteTags(Set<Tag> studentTags, Set<Tag> tagsToDelete) { | ||
Set<Tag> newTags = new HashSet<>(studentTags); | ||
newTags.removeAll(tagsToDelete); | ||
return newTags; | ||
} | ||
|
||
private String generateSuccessMessage(Person personToEdit) { | ||
return String.format(MESSAGE_DELETE_TAG_SUCCESS, personToEdit.getName()) + tags; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear and neat implementation of the deleteTagCommand Class.
Updated TagCommand to tag students by specifying their student number instead of index. This allows TAs to efficiently tag students instead of searching through the list the find the index of the student.
Update tagCommand in UserGuide to follow the updated tagcommand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Users can now add/delete specific tags from students, instead of always replacing tags with a new set of tags