Tsenrae / pe

0 stars 0 forks source link

Bug with deleting person with the specified index #5

Open Tsenrae opened 5 months ago

Tsenrae commented 5 months ago

Steps to reproduce:

  1. delete +1

Expected: Person is deleted as +1 is a positive integer

Actual: Error message saying that person can't be deleted

Screenshot 2024-04-19 at 4.30.10 PM.png

nus-se-script commented 5 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]

Bug with unarchiving a person at the specified index

Steps to reproduce:

  1. unarchive +1

Expected: Person is unarchived as +1 is a positive integer

Actual: Error message saying that person can't be unarchived

Screenshot 2024-04-19 at 4.33.09 PM.png


[original: nus-cs2103-AY2324S2/pe-interim#1935] [original labels: type.FunctionalityBug severity.Low]

Their Response to the 'Original' Bug

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

image.png

In the UG, we have stated that the input must be a positive integer, and provided correct examples of valid indexes.

+ is not a number, but a symbol, and the input +1 is clearly not a valid input. Thus, the error message displayed is the expected behavior of our app.

Furthermore, preventing the input +1 does not cause any additional inconvenience for the user, as there exists an even more convenient input 1 which allows the user to achieve the same outcome.

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: This is not a duplicate of the bug about unarchiving a person at the specified index, as they are two totally different commands. This command is about deleting a person, and the other is about unarchiving a person. For this bug, you would have to change the execute method in the DeleteCommand class to account for inputs like +1, and you would also have to change the Index constructor to account for inputs like +1. However, for the bug about unarchiving a person at the specified index, you would have to change the execute method in the UnarchiveCommand class to account for inputs like +1, hence the changes cannot be fixed independently. Below are screenshots showing snippets of the classes:

Index Class Screenshot 2024-04-24 at 1.41.18 AM.png

DeleteCommand Class Screenshot 2024-04-24 at 1.51.33 AM.png

UnarchiveCommand Class Screenshot 2024-04-24 at 1.47.49 AM.png

If you are talking about the other way to fix it, which is changing the error message to say must be a unsigned positive integer, in this case, it also cannot be counted as a duplicate as you would have to change different error messages, for deleting a person, you would have to change the MESSAGE_USAGE in the DeleteCommand class, while for unarchiving a person, you would have to change the MESSAGE_USAGE in the UnarchiveCommand class. Below are screenshots showing snippets of the classes:

DeleteCommand Class Screenshot 2024-04-24 at 1.52.06 AM.png

UnarchiveCommand Class Screenshot 2024-04-24 at 1.37.38 AM.png

Thus, since the bugs cannot be fixed independently, they cannot be considered duplicates of each other.


## :question: Issue response Team chose [`response.Rejected`] - [x] I disagree **Reason for disagreement:** Your point about stating that the input must be a positive integer is wrong as +1 is clearly a positive integer. The error message above clearly states that INDEX (must be a positive integer). If intentions were clearer, it should be must be a unsigned positive integer. This would then make +1 an invalid input. Moreover, the multiple examples states (1, 2, 3...), however, it is still up the reader's interpretation of what a positive integer is. It can't be dependent solely on the example numbers, as then the argument can be that since '4' is not listed in the examples, I can't use '4' as a valid input. Hence, it is not about the examples but the definition of the parameter, which in this case is a positive integer and '+1' is a positive integer. As for your point that preventing the input +1 does not cause any addtional inconvenience for the user, this point is also wrong as you are clearly inconveniencing them. The user would think +1 is a valid input due to your User Guide saying you accept positive integers, and would be confused when they are shown an error message that is also incorrect. Just because there is a more convenient input 1 that allows the user to achieve the same outcome, does not mean that the user is not hindered when a valid input like +1 is not allowed, though they mean the same thing. What if the user is more math-oriented, and is more familiar with a more mathematical way of saying 1 like +1? They would obviously favour the input +1 instead, and be hindered when they find that their input is rejected by your app.