Tsenrae / pe

0 stars 0 forks source link

Bug with editing a name at specified index #4

Open Tsenrae opened 5 months ago

Tsenrae commented 5 months ago

Steps to reproduce:

  1. edit +1 n/Alex Yeoh p/87438807 e/alexyeoh@example.com a/Blk 30 Geylang Street 29

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

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

Screenshot 2024-04-19 at 4.27.31 PM.png

nus-se-bot 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 editing a person, and the other is about unarchiving a person. For this bug, you would have to change the execute method in the EditCommand 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

EditCommand Class Screenshot 2024-04-24 at 1.46.07 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 editing a person, you would have to change the MESSAGE_USAGE in the EditCommand 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:

EditCommand Class Screenshot 2024-04-24 at 1.39.09 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.