ITLimJiaWei / pe

0 stars 0 forks source link

Incorrect name parsing when including names which have "s/o" etc. #5

Open ITLimJiaWei opened 1 week ago

ITLimJiaWei commented 1 week ago

Input: edit 3 n/Maria s/o Hughman Patrick

As seen in the before and after screenshots, Maria s/o Hughman Patrick is shortened to Maria and the rest of the end after s/ is saved as a skill. This invalid parsing could be due to "s/o". The parsing/validation of name should allow for these characters among others e.g. "d/o" as there is a high likelihood these names will be added at some point in time given how this is an app that manages jobs.

Overall, it would be good to allow these names to be added in full by allowing the parser/validation to be more flexible. One could modify the parsing character for skill to be something other than "s/" such as "sk/" or attempt to validate this name while keeping the "s/" identifier.

Before (the command is input)

image.png

After (the command is input)

image.png

After (the command is input) - rest of name is saved as skill

image.png

soc-se-bot commented 1 week ago

Team's Response

No details provided by team.

The 'Original' Bug

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

Invalid error message when using "d/o" in name

Input: edit 3 n/Maria d/o Hughman Patrick

I expected this command to be accepted as "d/o" is a valid name and is highly likely to be come across since this app is for managing job candidates. However, I am met with the following error message that it is an invalid name.

It would be good to allow for "d/o" to be accepted as it is relatively common for people to shorten "Daughter of" to "d/o" and this shortened version even be in their legal name as well.

image.png


[original: nus-cs2103-AY2425S1/pe-interim#334] [original labels: severity.Medium type.FeatureFlaw]

Their Response to the 'Original' Bug

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

As mentioned below, the UG clearly specifies that the name can only be alphanumeric and hence words like 's/o' are not in scope of the UG. Furthermore, the user receives a suitable error message when attempting to add a symbol to their name which would lead to them working around the issue e.g. by fully spelling out "son of".

Therefore, we will be assigning this as "Not_In_Scope"

image.png

Constraints of name stated below:

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: I do not believe this is a duplicate of the other issue as the behaviour shown by the app is different. In the bug report with "d/o", the issue was simpler as it was simply not recognized as a valid name. However, in this issue, using "s/o" which again is part of some people's legal names, it results in a parsing issue instead of simply stating the name is invalid. It lets the name through and parses the text after s/ as a skill.

This behaviour is clearly different from that of the other bug report where an appropriate error message was given. It instead results in a clearly unintended outcome where part of the entered name is taken as a skill.


## :question: Issue response Team chose [`response.NotInScope`] - [x] I disagree **Reason for disagreement:** While I agree that the issue type should be `FeatureFlaw`, from the CS2103 textbook(as seen below), this issue where "s/o" is disallowed or results in the behaviour where the name is split into name and skill field can be a major issue. Since this is an application that manages job candidates, it is entirely reasonable that only **legal** names will be allowed due to adherence to labour laws etc. etc... Thus one cannot expect the user to simply work around the problem by typing "son of" because this is then not the candidate's legal name. One also cannot reasonably expect the user to look at both the name and search through the skills field to retrieve the person's full name by joining the 2 parts back into one. I believe a more appropriate response to this issue would be "Accepted" as it is a genuine problem that will occasionally occur. ![image.png](https://raw.githubusercontent.com/ITLimJiaWei/pe/main/files/e512b6b2-0703-47f4-a4df-5d0befa17270.png)
## :question: Issue type Team chose [`type.FeatureFlaw`] Originally [`type.FunctionalityBug`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]
## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.Medium`] - [x] I disagree **Reason for disagreement:** I disagree that this bug should be downgraded to `Low` severity. As already explained in my response to the developer's decision of choosing `NotInScope` and with reference to the below screenshot, not allowing "s/o" in this scenario is not appropriate and will cause occasional inconvenience to the user. This is because one must use their legal name especially for an application that handles job candidate user information to abide to laws in certain countries etc. etc.. It is reasonable to expect one may not simply use an alias or "son of" to get around this feature flaw. ![image.png](https://raw.githubusercontent.com/ITLimJiaWei/pe/main/files/e512b6b2-0703-47f4-a4df-5d0befa17270.png) I believe the severity should be `Medium` as with reference to the 2nd screenshot, this will cause occasional inconvenience to **some** users(those with s/o in their name). I believe `Low` is not appropriate because this flaw does not only appear in very rare situations and will affect normal usage of the product(whenever someone with s/o in their name is a candidate). ![image.png](https://raw.githubusercontent.com/ITLimJiaWei/pe/main/files/e4436c44-6249-41b3-8870-13ffad503d49.png)