search

aexolate / pe

0 stars 0 forks source link

Undo not working for email #6

Open aexolate opened 1 year ago

aexolate commented 1 year ago

State 1 Create person with multiple emails, then use delete

image.png

execute delete 7 /email

State 2

image.png

execute undo

State 3

image.png

Problem: the undo command did not undo the deletion of email

soc-pe-bot commented 1 year ago

Team's Response

We acknowledge that this is a bug in our programme. The issue lies with the delete command used for deleting fields. In our DeleteFieldCommand.java, under the method execute:

    @Override
    public CommandResult execute(Model model) throws CommandException {
        requireNonNull(model);
        List<Person> lastShownList = model.getDisplayedPersonList();

        if (this.indexOfPerson.getZeroBased() >= lastShownList.size()) {
            throw new CommandException(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
        }

        Person personToDeleteField = lastShownList.get(this.indexOfPerson.getZeroBased());
        DeletePersonDescriptor descriptor = new DeletePersonDescriptor(personToDeleteField);
        action.delete(descriptor, indexOfPerson);
        Person personWithFieldDeleted = descriptor.toPerson();

        model.setItem(personToDeleteField, personWithFieldDeleted);
        model.updateDisplayedPersonList(PREDICATE_SHOW_ALL_PERSONS, null);
        return new CommandResult(String.format(MESSAGE_DELETE_PERSON_FIELD_SUCCESS,
                Messages.format(personWithFieldDeleted)));
    }

Notice that after the model replaces the old person with the edited person, the method calls model::updateDisplayedPersonList, which then refreshes the filter on the contact list. However, to accommodate for undo and redo, this call creates a new version of VersionedNetworkBook, hence in this one command execution, two new versions of VersionedNetworkBook are created. Hence it requires two undo command to undo one delete command. Besides, this contact list filter resetting is unnecessary and can cause confusion for users who might want to see the direct different right after they delete a field. Removing this line of code shows the difference made by the delete command better, and resolves this bug.

The 'Original' Bug

[The team marked this bug as a duplicate of the following bug]

Undo/redo command pair is erronenous for a multitude of functions

image.png after deleting phone number from 3 and undoing, nothing happens even though success message is given. redoing said undo does not do anything.

image.png after deleting email from 3 and undoing, nothing happens even though success message is given. redoing said undo does not do anything. same for /link, /grad, /course ( basically all fields for person).redoing said undo does not do anything.

[original: nus-cs2103-AY2324S1/pe-interim#2611] [original labels: type.FunctionalityBug severity.Medium]

Their Response to the 'Original' Bug

[This is the team's response to the above 'original' bug]

We acknowledge that this is a bug in our programme. There are actually two separate bugs here:

  • Undo does not work if last command is edit.
  • Undo does not work if last command is delete.

We would group this bug report together with reports on undo command not working with delete command.

Items for the Tester to Verify

:question: Issue duplicate status

Team chose to mark this issue as a duplicate of another issue (as explained in the Team's response above)

Reason for disagreement: [replace this with your explanation]