jonasgwt / pe

0 stars 0 forks source link

Overzealous Input Validation for phone numbers #8

Open jonasgwt opened 1 year ago

jonasgwt commented 1 year ago

Steps to reproduce:
edit 6 p/92345674 (Phone)

Expected Output:
Accepted with a warning

Actual Output:
Rejected

Screenshots:

image.png

nus-pe-script commented 1 year ago

Team's Response

Thank you for pointing that out. Yes, we do understand about the overzealous input validation. However, we choose to allow only numbers for phone number because it will hinder our implementation of sort command, where we need to sort the phone numbers. This is also stated in the module website:

image.png

Therefore, we feel that this is not in scope, because we can only consider your suggestion by improving our sort command in the future iterations and we clearly specify in the error message that we do not support non-number characters.

image.png

Items for the Tester to Verify

:question: Issue response

Team chose [response.NotInScope]

Reason for disagreement: I feel that fixing the sort command to account for a wider range of user input for phone numbers is straightforward and does not require a lot of effort.

Taking a look at the team's implementation of the sort command, the team made use of a compareTo method that compares integers of phone numbers to determine the order (screenshot provided below). A quick change to compare the String instead of the Integer would solve the problem and bring much convenience to the user just by changing one line of code.

image.png

As rectifying this issue brings great convenience to the user at little cost to the developer, there is no reason to delay this work and should have been implemented in this version.