search

Hikoya / pe

0 stars 0 forks source link

Invalid/long past dates are accepted for loan #12

Open Hikoya opened 2 years ago

Hikoya commented 2 years ago

Description: Long past dates such as 0001-01-01 or invalid date strings are accepted.
eg. loan 1 1 0001-01-01
eg. loan 1 1 2021-0-1
eg. loan 1 1 2021-0-0

image.png

image.png

nus-pe-script commented 2 years ago

Team's Response

Linked to #3998

The 'Original' Bug

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

Loan due date gives unexpected results

In UG its mentioned that:

Screenshot 2022-11-11 at 4.49.52 PM.png

Users may expect that inputing 25-12-2022 will return 25 Dec 2022 or at least throw an error. This is not ambiguous as 25 is more than the 12 and will clearly be the day not the month.

Input:

Screenshot 2022-11-11 at 4.49.20 PM.png

Expected:

Deadline is 25 Dec 2022

Actual Result:

Deadline is 20 Dec 2022

Screenshot 2022-11-11 at 4.51.23 PM.png

Severity is medium as in places such as SG where users are used to DD MM YYYY format, the unexpected result may be frequent. Furthermore there is no warning. Oddly 25/12/2022 (using "/" instead of "-") will give the expected result which may add to the confusion for users.

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

Their Response to the 'Original' Bug

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

As stated in the UG, only the due date formats dd/MM/yyyy and yyyy-MM-dd are currently supported. Other due date formats such as dd-MM-yyyy is not currently supported. The reason why it does not throw an error message while accepting and parsing it as a wrong date is because the parsing of date formats is performed by an external library, PrettyTimeParser.

The external library uses NLP (Natural Language Processing) and is able to parse all date formats. It attempts to parse all phrases and input that resemble dates to fit its NLP structure. For example, in the example above, it may be possible that it parses 25-12-2022 to 00:25 12-20-22 as it supports the input of other date formats such as hours, minutes, and seconds too. It is not reasonable for us to test or hardcode all available possible date formats to ensure it supports all date formats properly.

However, the library lacks documentation and all date formats had to be tested individually by us, and we have managed to locate and code to support the two date formats which works properly. In addition, it is unfeasible for us to fix it directly as it is an open source library, and even if we were to fix it locally, it is too difficult to fix the code as it uses NLP which is out of the scope of this module.

The reason why we have decided to go through with importing the library is as below:

  1. It is one of the most widely used date parsing library used, and is quite reputable, and we initially did not expect it to fail at a variety of date formats.
  2. It allows for flexibility since it uses NLP and can support a variety of phrases such as following friday or other commonly used phrases used to indicate dates which is quite a useful feature for users and is difficult to code up on our own, and we decided that the benefits outweigh the costs.
  3. It is too late to remove the feature and replace it with a new loan implementation, and it was simpler to state the date formats it support.

Furthermore, we have stated in UG as shown below, that other date formats may not work properly, and a fix is coming in the future.

We have also changed the severity to low as it should not be a big issue as long as the user follows the correct date formats. Our intention is that the user will stick to the recommend format, which is why we included the statement about compound sentences.

image.png

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 response

Team chose [response.NotInScope]

Reason for disagreement: [replace this with your explanation]

:question: Issue severity

Team chose [severity.Low] Originally [severity.Medium]

Reason for disagreement: [replace this with your explanation]