awhb / pe

0 stars 0 forks source link

bsort accepts implicitly invalid prefix #3

Open awhb opened 10 months ago

awhb commented 10 months ago

Screenshot 2023-11-17 at 4.33.13 PM.png

UG flags prefixes outside the 4 listed ones as invalid.

Command run: bsort t/a

output: Got it. I've sorted the buyer list!

Screenshot 2023-11-17 at 4.35.08 PM.png

Misleading success message of bsort will inconvenience users who misstype the sort command

soc-pe-bot commented 10 months ago

Team's Response

This feature flaw was identified in the PE dry run, but because of the feature freeze it was unable to be changed. The feature flaw is mentioned in the UG, which states how an invalid prefix is allowed but ignored and the sort command is still executed. In this case, bsort t/a does the default sort bsort, ignoring the invalid prefix. The 'planned enhancements' section of the DG also mentions about better error handling of invalid prefix inputs like these in future iterations of the product, to amend this feature flaw.

The 'Original' Bug

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

bsort, ssort commands accept invalid arguments

There is no general argument validation for the commands bsort and ssort, only if a prefix is matched.

Steps to reproduce

  • Enter the command bsort n/d and press Enter
  • The list is sorted using default order with no errors reported

Expected behaviour

  • Error of "invalid argument" is reported

Why is this a bug?

  • This could lead to behaviour where I typo n/d to be m/d
  • I don't see my command reflected, so I can't tell if I typed n/d or m/d
  • Command reports that it successfully sorted, but the order isn't what I expect

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

Their Response to the 'Original' Bug

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

This feature flaw was identified in the PE dry run, but because of the feature freeze it was unable to be changed. The feature flaw is mentioned in the UG, which states how an invalid prefix is allowed but ignored and the sort command is still executed. In this case, bsort m/d does the default sort bsort, ignoring the invalid prefix. The 'planned enhancements' section of the DG also mentions about better error handling of invalid prefix inputs like these in future iterations of the product, to amend this feature flaw.

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.Rejected`] - [x] I disagree **Reason for disagreement:** The feature flaw mentioned in both the UG and DG which the developer team refers to (see below) mentions that invalid prefixes "after the bsort keyword **and before the next valid prefix**" will be ignored by the sort command. Neither document mentions the case where there is **only a single invalid prefix** after the bsort keyword as either a flaw or a planned enhancement, invalidating the developer team's claim that the issue flagged is included as a specific case in their planned enhancements. In fact, it is precisely the sort command still displaying a successfully sorted message (without displaying the exact sort command executed, no less) when given a single invalid prefix input that is concerning enough to make it be flagged as a functionality bug. Users who have made such typos in their sort command will view their contacts in a different sort order than they expected without being able to verify exactly on either document why that is the case. ![Screenshot 2023-11-21 at 10.11.19 PM.png](https://raw.githubusercontent.com/awhb/pe/main/files/d4fe0ec1-c524-40fa-b17d-0f37e642564b.png) ![Screenshot 2023-11-21 at 10.01.08 PM.png](https://raw.githubusercontent.com/awhb/pe/main/files/1dae47e4-f5d3-4610-b820-2f1a02c7332a.png)
## :question: Issue severity Team chose [`severity.Medium`] Originally [`severity.Low`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]