Undo command undoes more actions than specified

[ShirleyWangxt]

After I changed the name of mark 25 to google, I tried to undo this action with undo 1. But the app undoes 2 actions I've done instead of one. (changed name back to github and added back tag)

[nus-pe-bot]

Team's Response

Thanks for reporting this issue. It is indeed one that we had not noticed.

There are two parts to this bug:

1. Mark allows duplicate bookmarks to be added.
2. The command result panel does not reflect the change in name.

The first bug is an inherited bug from AB3 (which also allows duplicate persons to be created via editing, as seen below).

The second bug is not visible in AB3. It occurs because the bookmark list is reset whenever an add, edit, or autotag command is executed. However, the method that throws DuplicateBookmarkException is UniqueBookmarkList#setBookmarks(List<Bookmark>), which is an existing AB3 method. No exception would have been thrown if the first bug was not present.

Hence, we are rejecting both parts of this bug as inherited from AB3.

---

Here is where the bug occurs in AB3:

1. Load AB3 with the default data:
   

2. Do edit 1 n/David Li:
   

3. Do edit 1 p/91031282 (same phone as index 4):
   
   Note that index 1 and index 4 are the same person according to:
   
   Both 1 and 4 have the same name and phone number.

4. But when doing input validation inside EditCommand#execute:
   
   We find the bug! AB3 checks that you are not editing to the same person with !personToEdit.isSamePerson(editedPerson) and then checks to see if the model contains that person.
   However, when we are only editing the phone number of a person, personToEdit will be the same person as editedPerson, since only the phone number is different.
   This means that inside here the validation fails. Also, inside UniquePersonList#setPerson, we can see the same incorrect check done:
   

5. Therefore, the duplicate person gets added successfully.

---

1. And so in Mark's case, since it inherited the duplicate person bug from AB3, it also adds a duplicate bookmark successfully. After that, the whole list is processed again inside applyAllTaggers:
   

2. Which in turn applies tags to all bookmarks, and sets the whole bookmark list to the new tagged bookmark list in Mark#applyAllTaggers:
   

3. Now, when setting the bookmarks inside UniqueBookmarkList#setBookmarks,
   
   We finally see that it checks to see if all bookmarks are unique, which they are not and thus throws the DuplicateBookmarkException.

Because UniqueBookmarkList should guarantee that all bookmarks inside are unique, when retrieving data from it, then adding tags (which will not affect the sameness of bookmarks as only URL and name are compared), and then resetting this updated list, the DuplicateBookmarkException should never be thrown, and therefore is never caught.

The only flaw was that it was possible for UniqueBookmarkList to complain duplicate data due to an inherited bug from AB3 as described above, violating the constraints of UniquePersonList plainly stated here:

And therefore we reject this bug due to it being inherited from AB3.

Items for the Tester to Verify

❓ Issue response

Team chose [response.Rejected]

• I disagree

Reason for disagreement: [replace this with your reason]

---