woojiahao / pe

1 stars 0 forks source link

Delete when in view mode deletes the wrong person #13

Open woojiahao opened 10 months ago

woojiahao commented 10 months ago

Prerequisites: create at least 2 client/leads

  1. Use view 2 to view the second user
  2. Use delete 1 to delete the first user
  3. Delete succeeds but the second user is deleted instead

This can be classified as a high functionality bug as the user should either not be able to delete anything while in view mode or to delete the appropriate user specified by the index, not the current user which may seem counterintuitive

Screenshot 2023-11-17 at 16.52.54.png Screenshot 2023-11-17 at 16.52.58.png

nus-se-bot commented 10 months ago

Team's Response

No details provided by team.

The 'Original' Bug

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

delete at person view page

For a list with at least one person

view 2
delete 1

would delete the person being viewed.

This is unexpected as I expected to delete the first person of the previous list or not delete any one at all since the UG said "filtered list shown in the address book" when no filtered list is shown


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

Their Response to the 'Original' Bug

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

Thanks for bringing this up! We feel that while this is may be severe when the user deletes a wrong client/lead as it deletes all their corresponding information, however it does not warrant a severity of "Medium". We feel that this should be of severity "Low" due to the following reasons: 1) It is very rare for the user to delete a different person after viewing a person (e.g. view 1 and delete 2) 2) Based on the intended sequence of steps the user takes to delete a person, after viewing a person (e.g. view 1 which views Person A), IF the user wishes to delete a person, it makes sense that the person the user wishes to delete is Person A (the one the user is viewing), which is what happens (although we agree that having the INDEX after the delete should be checked for). It is definitely unexpected that the user views person A and proceeds to delete person B.

We could have additionally mentioned this in our UG that "delete" deletes from the filtered list and the "view" restricts that filtered list to be of the person the user is viewing, to prevent causing such an unintended behaviour.

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]


## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.High`] - [x] I disagree **Reason for disagreement:** > It is very rare for the user to delete a different person after viewing a person (e.g. view 1 and delete 2) While this may be the case, it should absolutely not be an operation that is permitted to happen given that deleting people is a **permanent** operation. Without clarification by the team in their documentation on how the `view` command applies a filter on the persons list as well, users who first use the application may misunderstand how the command works and instead, attempt the exact flow pointed out in the issues and cause **permanent** changes to their client list. This could be absolutely disastrous if the user had important information about the person that was not saved anywhere else. > Based on the intended sequence of steps the user takes to delete a person, after viewing a person (e.g. view 1 which views Person A), IF the user wishes to delete a person, it makes sense that the person the user wishes to delete is Person A (the one the user is viewing), which is what happens (although we agree that having the INDEX after the delete should be checked for). It is definitely unexpected that the user views person A and proceeds to delete person B. If it's a "logical path" to delete the same user that they are viewing, then any other attempts to delete should not be permitted, such as providing an warning/error message when `delete` is called after `view` to inform the user that they would be operating on the currently viewed user. Additional safety precautions can also be taken such as having a confirmation system or even as simple as providing clearer documentation on the behaviour of `view`. **Conclusion:** I think this really boils down to the user expectation when using `view` and `delete` and also the overall clarity of what `view` does. Without clear signals/documentation that the `view` command filters the list of people, the user may be under the impression that the full people list is available for them to access even in "view mode", which is not the case at all. As such, the following is an entirely plausible user flow: 1. User enters `view 2`, system displays person at index 2 2. User attempts to delete the person at index 1 (expecting the original list of people to still work) 3. System deletes person at index 2 instead of index 1 (given the hidden filter) 4. User loses information about the currently viewed person even though that is not their intention Although it may not be something that the team anticipates users to do often, permitting such an operation without any clear documentation or warnings in the system only invites accidental operations. The reason why I believe this is **High** is because deleting is a **permanent** operation. It does irreversible changes to the system that the end user cannot recover. By permitting a "destructive" operation to occur, even if it may be out of the team's expectation, it can make the application unusable.