alvinlim277 / pe

0 stars 0 forks source link

Undo command doesn't work when deleting specialisation #2

Open alvinlim277 opened 7 months ago

alvinlim277 commented 7 months ago

To reproduce:

  1. Have a contact with empty specialisation.
  2. Add a specialisation (used add 1 /spec test)
  3. Delete the specialisation (delete 1 /spec)
  4. Undo the deletion

Expected:

Undo message is shown, and the specialisation should reappear under the contact field.

Actual:

Undo message is shown but specialisation remains missing

image.png

image.png

image.png

soc-pe-bot commented 7 months 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]