Team's Response

We overlooked this part about clearing it from the event list, although it does not display the deleted contact after it being deleted. Same issue as the issue it is linked to, having a problem updating the lists within events after editing the overall contact list.

The 'Original' Bug

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

Unlinking contact does not do its job

Add a tag clients to the contact Bernice Yu. (If not already present)

Link contact Bernice event NUS Career Fair.

Delete tag clients.

Unlink contact Bernice from the event NUS Career Fair.

Expected: Contact Bernice disappears from the list. Actual: Contact Bernice is still linked to the list as shown by the GUI.

Reselecting the same event (supposedly to refresh) still shows that Bernice is attached to the event.

This problem might be because the list of tags in storage for Bernice in list of contact and Bernice in the list of contacts linked to this event are not in sync.

_{[original: nus-cs2103-AY2324S1/pe-interim#4435] [original labels: type.FunctionalityBug severity.Medium]}

Their Response to the 'Original' Bug

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

We've overlooked the part of updating the 2 different contact lists, specifically the UniqueContactList and the contact list within the event.

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)

[x] I disagree

Reason for disagreement: First, pardon my typo in my original bug report, I meant When I delete the contact from the app instead of When I delete the contact from the event. However, based on the team's response, it looks like the team has got what I meant.

The team is correct in identifying that the contact list within each event needs to be updated properly. However, updating the contact list within each event has to be done separately for each of the following 2 commands:

delete tag command, which corresponds to the 'original' bug report
edit and delete contact command, in which delete contact command corresponds to this bug report (note that deleting a contact without editing it first is not sufficient to reproduce this bug)

What is the bug

First, let me formally highlight a sequence of commands that reproduce each of the 2 bugs above, starting from the sample data. These sequences should have been discovered by the dev team, as the team has accepted both of my bug reports. This section is only for the reader to get a better understanding of the issue. Note that in the sample data, Charlotte Oliveiro is linked to the event JobFest 2023, and Charlottle Oliveiro has the tag customers.

Delete tag command
- delete_tag t/customers
- select_event 1 to verify the linkage.
- unlink ev/JobFest 2023 c/Charlotte Oliveiro, the command does not work as expected.
Edit and delete contact command
- edit_contact 3 p/ 12345678
- delete_contact 3 to delete Charlotte Oliveiro.
- add_contact n/Charlotte Oliveiro p/12345678 e/test@example.com a/Char st, from the user perspective, this old contact has previously left the event management team and now comes back.
- select_event 1, here Charlotte Oliveiro should not be linked to JobFest 2023 as it is a new contact not yet linked to any event, but the app shows that the contact is linked to the event.

The issue lies with edit contact command and delete tag command not updating the contact list within each event properly. This bug can only be resolved by correcting the edit contact command and the delete tag command, and therefore fixing one does not automatically fix the other.

Edit and delete contact command

For edit contact command, probing into /logic/commands/contact/EditContactCommand.java, line 99 is responsible for updating the model:

model.setContact(contactToEdit, editedContact);

Probing into model::setContact, it calls JobFestGo::setContact, which only calls UniqueContactList::setContact. There is no attempt to update the contact list within each event at all.

Probing into /logic/commands/contact/DeleteContactCommand.java, line 49 is responsible for updating the model:

model.deleteContact(contactToDelete);

Probing into model::deleteContact, it calls jobFestGo::removeContact:

    public void removeContact(Contact key) {
        contacts.remove(key);
        events.updateContacts(key);
    }

events::updateContacts is responsible for removing the deleted contact from all the events. Probing into the method,

    public void updateContacts(Contact contact) {
        for (int i = 0; i < internalList.size(); i++) {
            Event curr = internalList.get(i);
            Set<Contact> contactsList = curr.getContacts();
            Set<Contact> updatedContactsList = new HashSet<>();
            if (contactsList.contains(contact)) {
                for (Contact p : contactsList) {
                    if (!p.equals(contact)) {
                        updatedContactsList.add(p);
                    }
                }
            } else {
                updatedContactsList = contactsList;
            }
            Event updatedEvent = new Event(curr.getName(), curr.getDate(), curr.getAddress(),
                    updatedContactsList, curr.getTasks());
            internalList.set(i, updatedEvent);
        }
    }

For each contact p in the current contact list, it is added to the new list of contacts associated with this event if it is not equal to the input contact. This spells trouble when the input contact is already edited but the current list of contacts for this event is never updated upon any edit contact commands.

A possible fix for this is to call UniqueEventList::updateContacts in JobFestGo::setContact, and make appropriate changes to the method UniqueEventList::updateContacts (as the current implementation does not accommodate edit contact command), or create a new method in UniqueEventList to accommodate edit contact command and call that method inside JobFestGo::setContact.

Delete tag command

For delete tag command, probing into logic/commands/tag/DeleteTagCommand.java, line 45 is responsible for removing the tag from the model:

model.deleteTag(toDelete);

Upon probing into the method model::deleteTag, it deletes the tag from JobFestGo by calling jobFestGo::deleteTag, and then it calls updateFilteredTagList, which only updates the displayed list. In JobFestGo::deleteTag:

    public void deleteTag(Tag tagToBeDeleted) {
        tags.delete(tagToBeDeleted);
        contacts.updateTag(tagToBeDeleted);
    }

Which deletes the tag from the list of tag, and update the contacts that use it. There is no attempt to update the list of contacts within each event at all.

A possible fix for this is to call events::updateContacts inside JobFestGo::deleteTag method, and update the method events::updateContacts to accommodate for delete tag command, or declare a new method in UniqueEventList to accommodate for delete tag command.

Summary

In the two bug reports, the most straightforward fix is to correct the methods in ModelManager that correspond to each command, edit contact command and delete tag command. These are two independent fixes, although the concepts behind the bugs are the same.

An alternative consideration

When looking into unlink command, /logic/commands/event/UnlinkCommand.java, lines 71-74:

for (Contact contact : contactsToUnlink) {
    eventToUnlink = model.getEvent(eventNameToUnlink);
    model.unlinkContactFromEvent(contact, eventToUnlink);
}

model::unlinkContactFromEvent is responsible for unlinking the contact from the event. Probing into the method, it calls jobFestGo::unlinkContactFromEvent, which then calls UniqueEventList::unlinkContactFromEvent.

    public void unlinkContactFromEvent(Contact contact, Event event) throws EventIsNotLinkedToContactException {
        if (!event.isLinkedToContact(contact)) {
            throw new EventIsNotLinkedToContactException(contact);
        }

        Set<Contact> newContacts = new HashSet<>();
        newContacts.addAll(event.getContacts());
        newContacts.remove(contact);
        Event updatedEvent = new Event(event.getName(), event.getDate(), event.getAddress(),
                newContacts, event.getTasks());
        setEvent(event, updatedEvent);
    }

A new HashSet for contact is instantiated, and it adds all contacts from the old set, and then remove the input contact. This is not successful when the input contact is already updated, but the old list of contact in each event is not updated. One might be tempted into thinking that we can update the hash code and the equality condition to make it work, i.e. in /model/contact/Contact.java,

    @Override
    public boolean equals(Object other) {
        if (other == this) {
            return true;
        }

        // instanceof handles nulls
        if (!(other instanceof Contact)) {
            return false;
        }

        Contact otherContact = (Contact) other;
        return name.equals(otherContact.name)
                && phone.equals(otherContact.phone);
    }

    @Override
    public int hashCode() {
        // use this method for custom fields hashing instead of implementing your own
        return Objects.hash(name, phone);
    }

This obviously breaks the definition of two equal contacts, and can spell troubles for future development that needs to use the stricter definition of equality for contacts. The list of contacts for each event is basically not updated, which can spell troubles for methods that need to specifically access the list of contacts for an event.

## :question: Issue severity Team chose [`severity.Medium`] Originally [`severity.Low`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]

nknguyenhc / pe

Delete contact does not delete its association from the linked events. #8