Elinengu / pe

0 stars 0 forks source link

Two find command are accepted by the app and showing the last command #5

Open Elinengu opened 1 week ago

Elinengu commented 1 week ago

find n/Irfan find n/Alex will be accepted by the app and only showing the result that matches Alex.

Which might potentially lead to the understanding that no student with name of Irfan is in the list if the user accidentally mixing up the commands.

same result for find t/tag find t/physics find ld/thu find ld/tue find lt/0920-0930 lt/1200-1300 find a/John find a/blk 45 Only the result for second find command is shown. The best way to work around with this perhaps is to display an error message to user saying that two find command is not allowed.

In which, the lecture timing and the lecture day might be the biggest issue here and might lead to the confusion for the user. For example, if user mistakenly thinks that find ld/thu find ld/tue works normally, he/she might miss out the schedule with the student on Thurs.

soc-pe-bot commented 4 days ago

Team's Response

It is unlikely for users to encounter confusion regarding the find command because the User Guide clearly specifies the command format: find [n/NAME_KEYWORDS] [a/ADDRESS_KEYWORDS] [t/TAG_KEYWORDS] [ld/LESSON_DAY_KEYWORDS] [lt/LESSON_TIME_KEYWORDS]

The UG does not suggest that multiple find commands can be chained together. This means users are expected to issue a single find command at a time, as per the documented behavior.

Furthermore, other commands do not allow chaining or combining multiple commands either, so it is unlikely that users would expect to be able to do this for find.

For the above reasons, this is highly unlikely to happen (only happens in very rare situations), I have adjusted it to severity.Low.

Additionally, while the described behavior could potentially cause confusion in very rare cases, such cases are mitigated by clear documentation and the application’s graceful handling of the input (processing only the second find command without crashing). Given that detecting two find keywords is not critical to the application's core operations and addressing it would detract from higher-priority improvements, this issue is considered NotInScope for the current iteration.

Items for the Tester to Verify

:question: Issue response

Team chose [response.NotInScope]

Reason for disagreement: Here is the section How to prove that something is response.NotInScope from the course website:

image.png

In which, I believe it should not be considered for response.NotInScope because fixing this issue only requires minimal effort. This opposes the guidelines for "NotInScope" because it will not take more effort than the current implementation, reducing the effort available to spend on other more important tasks.

For your information, the methods to solve this issue are provided within the original AB3. It can be fixed by copying line 46-47 from AddCommandParser to the method parse(String args) in FindCommandParser, and changing the input fields for that line with input fields from argMultimap in FindCommandParser from line 47-48.

image.png

image.png

After the fixing: image.png

Besides, since the other commands such as add, addRemark, edit, delete that do not allow multiple commands or parameters do provide a valid error message for the user. Hence, it is unlikely to say that provide the same error message for the user will reduce the effort available to spend on other more important tasks.

Furthermore, the implementation for this error message should have the same priority for all commands, i.e.: add, addRemark, edit, delete commands. If providing error messages for similar commands has been deemed necessary, it implies that these checks have a sufficiently high priority. Especially because error messages are provided for the other commands, the user might assume that the find command accepts multiple parameters for the same field if they accidentally provide multiple parameters for the same field.

image.png

Other than that, just a side note, the unusual behavior actually stems from multiple parameters for the same field being provided by the user as seen in the third example I provided before: find lt/0920-0930 lt/1200-1300.

This issue occurs because the app currently accepts multiple parameters for the same field (such as lt for lesson time), but only processes the last one provided.

I apologize for the original issue title, the original title Two find commands are accepted by the app and showing the last command can be somewhat misleading and ambiguous. It suggests that the app is accepting multiple independent find commands, which isn't exactly the case. The app is actually receiving multiple parameters for the same command, but only processes the last parameter for the same field, if multiple parameters for the same field are provided, leading to the incorrect behavior.

From the developer team's perpective:

It is unlikely for users to encounter confusion regarding the find command because the User Guide clearly specifies the command format.

While the User Guide does specify that only one parameter should be used for each field, it might not be that unlikely for users to mistakenly provide multiple parameters for the same field, especially for the new user. This confusion can stem from the way other command-line interface (CLI) applications handle similar functions.

For example, in GitHub's Filters function, users are required to provide a prefix for every new keyword, even if it belongs to the same field. An example of this would be: is:issue is:open.

Here, each new keyword is explicitly prefixed, indicating it is part of the same field. Users familiar with this pattern might expect our application to handle multiple keywords in a similar way and could inadvertently use: find lt/0920-0930 lt/1200-1300 This results in only the last parameter being processed, which is contrary to their expectations. (This was the reason that I discovered this issue)


## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.Medium`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]