search

nus-cs2103-AY1920S1 / pe-dev-response

0 stars 0 forks source link

Command result not shown for edit, duplicate names allowed #1074

Open nus-pe-bot opened 4 years ago

nus-pe-bot commented 4 years ago

I try to edit the name of the tag to another name which is the same as another bookmark. The application seems to allow this as the name is updated. Also, the command result panel does not show anything after I press enter.

Screenshot 2019-11-15 at 4.28.07 PM.png Screenshot 2019-11-15 at 4.28.16 PM.png

[original: nus-cs2103-AY1920S1/pe-interim#1083]

dorcastan commented 4 years ago

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). photo_2019-11-17 15.44.18.jpeg

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: image.png

  2. Do edit 1 n/David Li: image.png

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

  4. But when doing input validation inside EditCommand#execute: image.png 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: image.png

  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: image.png

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

  3. Now, when setting the bookmarks inside UniqueBookmarkList#setBookmarks, image.png 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:

image.png

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

Duplicate status (if any):

--