Emberlynn-Loo / pe

0 stars 0 forks source link

valid index for addOrder feature does not work #10

Open Emberlynn-Loo opened 5 months ago

Emberlynn-Loo commented 5 months ago

Steps to reproduce

  1. Input addorder +1 d/2020-01-01 r/100 chicken wings

Expected No error message to be shown, error message says positive integer not unsigned positive integer

Actual Error message shown

Screenshot 2024-04-19 at 4.53.59 PM.png

nus-pe-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]

Editing positive integer does not work

Steps to reproduce

  1. Input edit +1 n/Alex Yeoh

Expected No error message to be shown because it doesn't state in the UG if it is unsigned or signed so both should be accepted

Actual Error message shown

Screenshot 2024-04-19 at 4.34.06 PM.png

Screenshot 2024-04-19 at 4.34.22 PM.png


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

Their Response to the 'Original' Bug

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

This is not a bug. Screenshot 2024-04-22 at 3.07.11 PM.png

Furthermore, the UG had specified positive integers (1,2,3,...)

Reasonable users would not think of signed / unsigned when attempting to input an index.

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: The two bugs marked duplicates are completely separate in both command and features. I explicitly stated that this bug reports an issue for the index parameter in the addOrder command while the other bug marked duplicate has an issue in their index for the edit command. This bug is for the orders feature, while the other is for the contacts/persons feature.

Screenshot 2024-04-23 at 1.36.49 PM.png

Therefore, it is not the exact same bug. Furthermore, what is considered a duplicate as per the picture is if the bug cannot be fixed independently. 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: However, your code has the checks dependently:

photo_2024-04-23 13.40.46.jpeg

Screenshot 2024-04-25 at 10.28.26 AM.png

The above is from your addOrderCommand class.

If you're talking about editing the error message, it also has to be done in different classes so it is not as simple as just changing the one in the Messages class.

Screenshot 2024-04-25 at 1.29.43 PM.png

Therefore, I disagree with your decision to mark these two bugs as duplicates, which by the way, you also did not explain.


## :question: Issue response Team chose [`response.Rejected`] - [x] I disagree **Reason for disagreement:** I think that it is reasonable for users to put a sign when specifying an index considering in the UG, you specified a negative signed integer as a negative example. ![Screenshot 2024-04-23 at 12.44.14 PM.png](https://raw.githubusercontent.com/Emberlynn-Loo/pe/main/files/c15d0d9f-654d-41eb-9be9-5c069526be78.png) I don't see why this could not have been done for other commands or why the word 'unsigned' could not have just been mentioned. To include a signed integer as an example in the UG itself already shows that user might consider doing so. You only did so for negative though, and not for positive signed integers. Few users would add a positive sign in front of the number does not discount the fact that it is a bug. 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.` The multiple examples states (1, 2, 3...) yet it is still up to the reader's interpretation of what a positive integer is. It can't be dependent solely on the example numbers if not the argument can be that since '4' is not listed in the example, means I can't use '4'. 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.