yejiadong / pe

0 stars 0 forks source link

Handling of Invalid Dates automatically converts invalid dates to valid ones without error messages (e.g. 2021-02-31 to 2021-02-28) #4

Open yejiadong opened 3 years ago

yejiadong commented 3 years ago

Issue description

The handling of invalid dates (e.g. 2021-02-31 since Feb has 28 days on non-leap years or attempting to input a 31st date for months which only have 30 days) is not done correctly. LocalDateTime automatically converts these to say 2021-02-28 or the 30th instead of prompting with an error message. This will lead to guests creating bookings on the wrong days.

Steps to reproduce the issue

  1. attempt to add a booking with "addBooking pid/5 rid/2120 sd/2021-02-12 ed/2021-02-31"

What's the expected result?

What's the actual result?

Additional details / screenshot

Screenshot 2020-11-13 at 12.49.37 PM.png

nus-se-bot commented 3 years ago

Team's Response

We are just using an existing, popular Java library LocalDate.parse() method. This LocalDate library is also recommended to all students to use in iP by Prof Damith. This way of error handling is decided by the Java libary developers and not up to us to decide. Therefore, this is not in the scope.

Items for the Tester to Verify

:question: Issue response

Team chose [response.NotInScope]

Reason for disagreement: I respectfully disagree with the team's decision to reject this as a feature flaw. First, I would like to point out that "recommended to all students to use in iP by Prof Damith" does not imply that one would be able to use the libraries and tools well, and in an appropriate manner, depending on the nature of the product. As we know, libraries carry external behaviour that might be variable depending on the different requirements and constraints of products that use them.

In this case, I first want to point out that the team's response of "This way of error handling is decided by the Java libary developers and not up to us to decide" is in fact, not accurate. In fact, the team is able to adjust the way the LocalDateTime library deals with invalid dates (i.e. 31st for months that do not have 31 days or 29-31 for February) We dealt with this similar issue in our own project, and simply toggled the .withResolverStyle parameter passed to the LocalDateTime object to ensure that it would parse dates in a strict manner. For more details, feel free to take a look here: https://howtodoinjava.com/java/date-time/resolverstyle-strict-date-parsing/

At the same time, it is also not impossible to spot such a behaviour w.r.t invalid dates, since the equivalence test methods taught in the later half of the module by Prof Damith specifically focused on testing invalid dates, and even posed the question of which dates should be part of equivalence partitions with respect to testing valid dates in months. In fact, 29, 30, 31 and 32 of the month were recommended as suitable testing points to ensure invalid dates would be processed correctly and properly.

Hence, existing popular Java libraries provide teams with variable behaviour, so as to allow flexibility in project utilisation and employment. However, I argue that it is important to specify the right behaviour for the library to adopt, so that the library does not become an impediment to proper functionality of the product.

Looking at the team's product in particular, it is a ConciergeBook app, that has hotel receptionists as the target profile. It has to handle bookings of guests, and more importantly, this means that booking dates have to be especially accurate and strict to ensure no errors. It is stated as one of the high priority user stories in the team's DG that "I want to answer guest queries about which rooms are available for a block of dates so that I know which rooms I can book for them". In this case, if invalid booking dates were specified in the command as 31st of the month (and 31st did not exist), and the app would automatically convert them to 30th, without throwing any error, and hence, permitting such an operation, it can lead to unimaginable consequences for the receptionist, who would have unwanted/invalid bookings successfully accepted by the booking system, and also rooms that were blocked out for no apparent reason. In this case, majority of functions that deal with checking availability of rooms and also booking accuracy will be compromised. In fact, if a guest specified an invalid date, and the booking was then successfully created, then such a situation would even lead to cost accumulation/double charging for the particular guest.

This explains why I had tagged severity high as well for this behaviour, as it is a flaw that affects most users and causes major problems for users. The same explanation is replicated below w.r.t to severity. In any case, response.notInScope applies to only "missing features", hence, I do not understand why the team feels that this is a missing feature, when it clearly affects the functionality of the current product in meeting its current specified needs.


:question: Issue severity

Team chose [severity.VeryLow] Originally [severity.High]

Reason for disagreement: I respectfully disagree with the team's decision to indicate this as a very low severity bug. This bug is clearly not cosmetic, nor does it have entirely no implications on usage. In fact, I argue that this can cause major problems for the target profile (receptionists), especially in context of the team's project of building a booking cum scheduling system for hotel receptionists. At the least, I believe a Medium will be reasonable.

Looking at the team's product in particular, it is a ConciergeBook app, that has hotel receptionists as the target profile. It has to handle bookings of guests, and more importantly, this means that booking dates have to be especially accurate and strict to ensure no errors. It is stated as one of the high priority user stories in the team's DG that "I want to answer guest queries about which rooms are available for a block of dates so that I know which rooms I can book for them". In this case, if invalid booking dates were specified in the command as 31st of the month (and 31st did not exist), and the app would automatically convert them to 30th, without throwing any error, and hence, permitting such an operation, it can lead to unimaginable consequences for the receptionist, who would have unwanted/invalid bookings successfully accepted by the booking system, and also rooms that were blocked out for no apparent reason. In this case, majority of functions that deal with checking availability of rooms and also booking accuracy will be compromised. In fact, if a guest specified an invalid date, and the booking was then successfully created, then such a situation would even lead to cost accumulation/double charging for the particular guest.

This explains why I had tagged severity high for this behaviour, as it is a flaw that does not only occassionally affect rare users (receptionists), but in fact most users and causes major problems for users on a routine basis. Hence, I stick to my original decision of recognising this as a medium/high severity and priority functionality bug.