Model: Add deleteTag(Tag) #790

yamgent · 2018-01-07T02:35:31Z

Part of #784. Fixed the deleteTag(Tag) exception by overhauling the design. Also added test.

Proposed commit message:

The user may want to remove a particular tag from all persons in the
AddressBook. For example, he may want to delete the tag "colleague"
from all his contacts if he is no longer working at his former company.

No such command exist at the moment, as the model API does not expose a
way to delete a particular tag entirely from the AddressBook (i.e. 
removing the Tag from all Persons and purging the Tag in the 
AddressBook's tag list).

As a step towards allowing the user to remove a particular tag with a
single command, let's implement Model#deleteTag(Tag), so that the
developer dealing with the 'logic' component is able to implement such
a command.

Note: For demo purpose, don't merge this to master!

CanIHasReview-bot · 2018-01-07T03:01:24Z

v1

@yamgent submitted v1 for review.

[v1 1/4] AddressBook: Modify updatePerson() to remove unused tags
[v1 2/4] AddressBook: Add removeTagFromPerson()
[v1 3/4] AddressBook: Add removeTag(Tag)
[v1 4/4] Model: Add deleteTag(Tag)

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/790/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos

[1/4] I've modified the 1st 2 paragraphs of your commit message to fit our standard structure more.

AddressBook#updatePerson() does not update the list of tags in the 
address book correctly, as tags that are no longer used by anyone after
the update remains in the list.

This is a buggy behavior as the list of tags is expected to only 
contain tags that are used persons in the address book.

Let's modify AddressBook#updatePerson() to refresh the tags list after
updating the person, so that any unused tags are purged.

Zhiyuan-Amos · 2018-01-08T11:19:33Z

src/main/java/seedu/address/model/AddressBook.java

+    }
+
+    /**
+     * Returns whether any {@code Person} in this {@code AddressBook} is still using {@code tag}.


Returns true ~~whether~~ if any {@code Person} in ~~this {@code AddressBook}~~ {@code persons} is ~~still~~ using {@code tag}.

Zhiyuan-Amos · 2018-01-08T11:20:57Z

src/main/java/seedu/address/model/AddressBook.java

+    private void removeUnusedTags() {
+        Set<Tag> tagsInUse = new HashSet<Tag>();
+
+        tags.forEach(tag -> {


We should use enhanced for-loop here, since you are doing so as well for the method isTagUsed, so we should standardise it (since there's no additional benefit compared to using forEach over enhanced for-loop.

Alternatively, you can update the UniquePersonList & UniqueTagList to return a Stream<Person> and Stream<Tag> respectively. That will shorten these methods here quite substantially.

Zhiyuan-Amos · 2018-01-08T11:24:41Z

src/main/java/seedu/address/model/AddressBook.java

@@ -112,6 +112,35 @@ public void updatePerson(ReadOnlyPerson target, ReadOnlyPerson editedReadOnlyPer
        // This can cause the tags master list to have additional tags that are not tagged to any person
        // in the person list.
        persons.setPerson(target, editedPerson);
+        removeUnusedTags();


you spotted a bug here, this should actually be fixed in master right? Wanna raise an issue for this?

This is already raised in #753 actually.

This commit is a temporary fix, it is not ideal because the complexity will become O(n^2). An overhaul of the tag list is probably required.

Zhiyuan-Amos · 2018-01-08T11:28:23Z

src/test/java/seedu/address/model/AddressBookTest.java

@@ -69,6 +72,16 @@ public void getTagList_modifyList_throwsUnsupportedOperationException() {
        addressBook.getTagList().remove(0);
    }

+    @Test
+    public void updatePerson_detailsChanged_personsAndTagsListUpdated() throws Exception {
+        addressBook.addPerson(BOB);


I'm wondering if we should use AddressBookBuilder() to create an AddressBook with BOB. Then we call addressBook.updatePerson(BOB, AMY); afterwards.

This is so that we don't need to rely on AddressBook#addPerson(Person) to be working correctly, since this method only tests for updatePerson(Person, Person)

Zhiyuan-Amos · 2018-01-08T11:36:05Z

src/main/java/seedu/address/model/AddressBook.java

+     * Removes all {@code Tag}s that are not used by any {@code Person} in this {@code AddressBook}.
+     */
+    private void removeUnusedTags() {
+        Set<Tag> tagsInUse = new HashSet<Tag>();


Oh, the word Tag in new HashSet<Tag> is unnecessary, as reported by my trustworthy IntelliJ :P

Zhiyuan-Amos

[2/4] Should the tests be more vigorous by including the throwing of PersonNotFoundException?

Zhiyuan-Amos · 2018-01-08T11:41:19Z

src/test/java/seedu/address/model/AddressBookTest.java

+    @Test
+    public void removeTagFromPerson_tagUsedByMultiplePersons_personUpdated() throws Exception {
+        addressBook.addPerson(AMY);
+        addressBook.addPerson(BOB);


Same comment as mentioned above.

Zhiyuan-Amos · 2018-01-08T11:45:14Z

src/main/java/seedu/address/model/AddressBook.java

@@ -191,6 +191,22 @@ public void addTag(Tag t) throws UniqueTagList.DuplicateTagException {
        tags.add(t);
    }

+    /**
+     * Removes {@code tag} from {@code person} in this {@code AddressBook}.
+     * @throws DuplicatePersonException if removing the tag causes the {@code person} to be equivalent to another


The current implementation of equals doesn't allow for this exception to happen actually, since it doesn't consider equality of tags. As such, perhaps we shouldn't throw this exception, but do a try-catch followed with throw new AssertionError()?

I'm not too picky about this though, @damithc what do you think?

Zhiyuan-Amos

[3/4]

Zhiyuan-Amos · 2018-01-08T11:51:14Z

src/main/java/seedu/address/model/AddressBook.java

+    /**
+     * Removes {@code tag} in this {@code AddressBook}.
+     */
+    public void removeTag(Tag tag) throws DuplicatePersonException, PersonNotFoundException {


since this method shouldn't be throwing both Exceptions, we should do a try-catch followed by throw new AssertionError().

Zhiyuan-Amos · 2018-01-08T11:52:06Z

src/test/java/seedu/address/model/AddressBookTest.java

+
+        AddressBook expectedAddressBook = new AddressBookBuilder().withPerson(AMY).withPerson(BOB).build();
+
+        assertEquals(addressBook, expectedAddressBook);


wrong order of assertEquals :P

Zhiyuan-Amos

[4/4]

Zhiyuan-Amos · 2018-01-08T11:54:41Z

src/main/java/seedu/address/model/ModelManager.java

+    public void deleteTag(Tag tag) throws PersonNotFoundException, DuplicatePersonException {
+        for (ReadOnlyPerson person : addressBook.getPersonList()) {
+            addressBook.removeTagFromPerson(tag, person);
+        }


you can just call addressBook.removeTag(tag); right? :P That way, you can make removeTagFromPerson private instead.

Zhiyuan-Amos · 2018-01-08T11:56:25Z

src/test/java/seedu/address/model/ModelManagerTest.java

+        ModelManager modelManager = new ModelManager(addressBook, userPrefs);
+        modelManager.deleteTag(new Tag(VALID_TAG_UNUSED));
+
+        assertEquals(modelManager, new ModelManager(addressBook, userPrefs));


wrong ordering of assertEquals :P

Zhiyuan-Amos · 2018-01-08T11:56:49Z

src/test/java/seedu/address/model/ModelManagerTest.java

+        AddressBook expectedAddressBook = new AddressBookBuilder().withPerson(amyWithoutFriendTag)
+                .withPerson(bobWithoutFriendTag).build();
+
+        assertEquals(modelManager, new ModelManager(expectedAddressBook, userPrefs));


wrong ordering of assertEquals :P

Zhiyuan-Amos · 2018-01-08T11:58:08Z

src/test/java/seedu/address/model/ModelManagerTest.java

@@ -26,6 +35,34 @@ public void getFilteredPersonList_modifyList_throwsUnsupportedOperationException
        modelManager.getFilteredPersonList().remove(0);
    }

+    @Test
+    public void deleteTag_nonExistentTag_sameAddressBook() throws Exception {


should the last segment of this test be named as modelUnchanged instead?

CanIHasReview-bot · 2018-01-12T13:22:36Z

v2

@yamgent submitted v2 for review.

[v2 1/4] AddressBook: Modify updatePerson() to remove unused tags
[v2 2/4] AddressBook: Add removeTagFromPerson()
[v2 3/4] AddressBook: Add removeTag(Tag)
[v2 4/4] Model: Add deleteTag(Tag)

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/790/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos

[1/4] commit message for 2nd paragraph, sorry I typed wrongly:

This is a buggy behavior as the list of tags is expected to only
contain tags that are used by persons in the address book.

Zhiyuan-Amos · 2018-01-12T13:50:47Z

src/test/java/seedu/address/model/AddressBookTest.java

+    @Test
+    public void updatePerson_detailsChanged_personsAndTagsListUpdated() throws Exception {
+        AddressBook addressBookWithBob = new AddressBookBuilder().withPerson(BOB).build();
+        addressBookWithBob.updatePerson(BOB, AMY);


after the update, this address book is no longer addressBookWithBob cos it actually stores AMY. Not sure if this may end up being confusing. I can't think of a better naming though. Maybe addressBookUpdatedToAmy?

Zhiyuan-Amos

[2/4]

Zhiyuan-Amos · 2018-01-12T13:54:37Z

src/main/java/seedu/address/model/AddressBook.java

+            try {
+                updatePerson(person, newPerson);
+            } catch (DuplicatePersonException dpe) {
+                throw new AssertionError("Modifying a person's tags only should not result in a duplicate.");


Should we add something like "See Person#equals(Object)" for clarity?

Zhiyuan-Amos

[3/4] as mentioned #790 (comment), we can make removeTagFromPerson(Tag, ReadOnlyPerson) private. As such, perhaps a better way is to squash commits 2 and 3 together. I think we don't test private methods in our code base right? So I think we can remove the tests in commit 2.

Zhiyuan-Amos · 2018-01-12T14:03:41Z

src/test/java/seedu/address/model/AddressBookTest.java

@@ -116,6 +116,28 @@ public void removeTagFromPerson_tagUsedByMultiplePersons_personUpdated() throws
        assertEquals(expectedAddressBook, addressBookWithBobAndAmy);
    }

+    @Test
+    public void removeTag_nonExistentTag_sameAddressBook() throws Exception {


should we change sameAddressBook -> addressBookUnchanged? Sounds a bit more natural.

CanIHasReview-bot · 2018-01-18T08:04:54Z

v3

@yamgent submitted v3 for review.

[v3 1/3] AddressBook: Modify updatePerson() to remove unused tags
[v3 2/3] AddressBook: Add removeTag(Tag)
[v3 3/3] Model: Add deleteTag(Tag)

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/790/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

CanIHasReview-bot · 2018-01-19T04:29:06Z

v4

@yamgent submitted v4 for review.

[v4 1/3] AddressBook: Modify updatePerson() to remove unused tags
[v4 2/3] AddressBook: Add removeTag(Tag)
[v4 3/3] Model: Add deleteTag(Tag)

(📚 Archive) (📈 Interdiff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/790/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

CanIHasReview-bot · 2018-08-09T05:11:53Z

v7

@yamgent submitted v7 for review.

[v7 1/2] AddressBook: Add removeTag(Tag)
[v7 2/2] Model: Add deleteTag(Tag)

(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/790/7/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamgent · 2018-08-09T07:01:40Z

Changelog:

[v6 1/3] was purged because the codebase finally got rid of syncWithMasterTagList(), which was what the commit was fixing (hooray 🎉!)
20502f5: deleteTag_tagUsedByMultiplePersons_tagRemoved() had its test logic rewritten because we now have version address books.

Zhiyuan-Amos · 2018-08-09T08:01:55Z

src/main/java/seedu/address/model/AddressBook.java

+
+        try {
+            updatePerson(person, newPerson);
+        } catch (DuplicatePersonException dpe) {


DuplicatePersonException will be made RuntimeException in #896 tho :(

Eh, I'll say that is a good thing, given the

throw new AssertionError("Modifying a person's tags only should not result in a duplicate. " + "See Person#equals(Object).");

below.

Zhiyuan-Amos · 2018-08-09T08:03:40Z

src/main/java/seedu/address/model/AddressBook.java

+            for (Person person : persons) {
+                removeTagFromPerson(tag, person);
+            }
+        } catch (PersonNotFoundException pnfe) {


Should we catch PNFE and DPE at the same location (in removeTagFromPerson)?

Hmm, it makes sense to handle PNFE here, because callers of removeTagFromPerson(Tag, Person) might actually pass in a person that does not exist in the address book, so it is a valid exception in that sense. However, handling DPE here would be quite strange since callers are not expected to manipulate Persons themselves, so removeTagFromPerson(Tag, Person) should deal with it.

Nvm I read your comments wrongly, will update it :P.

CanIHasReview-bot · 2018-08-16T12:21:12Z

v8

@yamgent submitted v8 for review.

[v8 1/2] AddressBook: Add removeTag(Tag)
[v8 2/2] Model: Add deleteTag(Tag)

(📚 Archive) (📈 Interdiff between v7 and v8) (📈 Range-Diff between v7 and v8)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/790/8/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamgent · 2018-08-16T12:23:07Z

Conflicts Resolution (Round 2):

Thankfully, only import conflicts.

Update:

Requested change made.

pyokagan · 2018-08-16T15:52:01Z

src/main/java/seedu/address/model/AddressBook.java

+
+        try {
+            updatePerson(person, newPerson);
+        } catch (DuplicatePersonException dpe) {


Seems like this can be removed?

pyokagan · 2018-08-16T15:52:16Z

src/main/java/seedu/address/model/AddressBook.java

+            throw new AssertionError("Modifying a person's tags only should not result in a duplicate. "
+                    + "See Person#equals(Object).");
+        } catch (PersonNotFoundException pnfe) {
+            throw new AssertionError("Original person must be obtained from the address book.");


Seems like this can be removed?

Zhiyuan-Amos · 2018-08-17T04:59:50Z

src/main/java/seedu/address/model/AddressBook.java

+     */
+    public void removeTag(Tag tag) {
+        for (Person person : persons) {
+            removeTagFromPerson(tag, person);


Can we do foreach here instead?

We want to be able to delete a particular Tag from our AddressBook entirely. Let's add AddressBook#removeTag(Tag). An alternative would be to move the logic to a new method ModelManager#deleteTag(Tag). However, it violates SRP as the ModelManager should not be concerned with the details of how the tags are removed from the AddressBook.

The Model API does not expose a way to delete a particular Tag entirely from the AddressBook (i.e. removing the Tag from all Persons and purging the Tag in the AddressBook's tag list). Let's add Model#deleteTag(Tag).

CanIHasReview-bot · 2018-08-18T12:10:21Z

v9

@yamgent submitted v9 for review.

[v9 1/2] AddressBook: Add removeTag(Tag)
[v9 2/2] Model: Add deleteTag(Tag)

(📚 Archive) (📈 Interdiff between v8 and v9) (📈 Range-Diff between v8 and v9)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/790/9/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

CanIHasReview-bot added the s.ToReview label Jan 7, 2018

Zhiyuan-Amos reviewed Jan 8, 2018

View reviewed changes

Zhiyuan-Amos added s.Ongoing and removed s.ToReview labels Jan 9, 2018

yamgent force-pushed the getting-started-model branch 2 times, most recently from b313b09 to b2ad284 Compare January 12, 2018 13:13

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Jan 12, 2018

yamgent requested a review from Zhiyuan-Amos January 12, 2018 13:26

Zhiyuan-Amos reviewed Jan 12, 2018

View reviewed changes

Zhiyuan-Amos added s.Ongoing and removed s.ToReview labels Jan 12, 2018

yamgent force-pushed the getting-started-model branch 2 times, most recently from bf83718 to 299e83a Compare January 18, 2018 07:35

se-edu deleted a comment Jan 18, 2018

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Jan 18, 2018

yamgent added s.Ongoing and removed s.ToReview labels Jan 19, 2018

yamgent force-pushed the getting-started-model branch from 299e83a to c398d58 Compare January 19, 2018 04:21

yamgent removed the s.ToReview label Jan 25, 2018

natania-d mentioned this pull request Mar 9, 2018

Model enhancement CS2103JAN2018-W13-B4/main#68

Merged

yamgent mentioned this pull request Aug 4, 2018

Rebase all answer PRs #891

Closed

7 tasks

yamgent force-pushed the getting-started-model branch 2 times, most recently from 1b03d80 to 20502f5 Compare August 9, 2018 04:57

yamgent added the s.Ongoing label Aug 9, 2018

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Aug 9, 2018

yamgent requested a review from a team August 9, 2018 05:20

Zhiyuan-Amos reviewed Aug 9, 2018

View reviewed changes

yamgent force-pushed the getting-started-model branch 3 times, most recently from 2b094b4 to 04fa653 Compare August 16, 2018 12:12

yamgent requested a review from Zhiyuan-Amos August 16, 2018 12:23

pyokagan reviewed Aug 16, 2018

View reviewed changes

Zhiyuan-Amos reviewed Aug 17, 2018

View reviewed changes

yamgent added 2 commits August 18, 2018 20:02

Model: Add deleteTag(Tag)

f798470

The Model API does not expose a way to delete a particular Tag entirely from the AddressBook (i.e. removing the Tag from all Persons and purging the Tag in the AddressBook's tag list). Let's add Model#deleteTag(Tag).

yamgent force-pushed the getting-started-model branch from 04fa653 to f798470 Compare August 18, 2018 12:03

yamgent requested a review from a team August 18, 2018 12:11

pyokagan approved these changes Aug 18, 2018

View reviewed changes

yamgent requested a review from damithc August 18, 2018 12:52

damithc approved these changes Aug 18, 2018

View reviewed changes

yamgent removed the s.ToReview label Aug 18, 2018

damithc closed this Dec 11, 2020


		AddressBook expectedAddressBook = new AddressBookBuilder().withPerson(AMY).withPerson(BOB).build();

		assertEquals(addressBook, expectedAddressBook);

Model: Add deleteTag(Tag) #790

Model: Add deleteTag(Tag) #790

Conversation

yamgent commented Jan 7, 2018

CanIHasReview-bot commented Jan 7, 2018

v1

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Jan 12, 2018

v2

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Jan 18, 2018

v3

CanIHasReview-bot commented Jan 19, 2018

v4

CanIHasReview-bot commented Aug 9, 2018

v7

yamgent commented Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Aug 16, 2018

v8

yamgent commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Aug 18, 2018

v9

yamgent commented Aug 9, 2018 •

edited

Loading

yamgent Aug 16, 2018 •

edited

Loading