SungMatt / pe

0 stars 0 forks source link

No UI warning when bad inputs in JSON are given - overrides data #1

Open SungMatt opened 9 months ago

SungMatt commented 9 months ago

Great work! Really enjoying the application so far. There is an issue when the JSON file is not in a valid state (when more than 2 emergency contacts are provided.)

The application starts without warning, defaulting to a blank file. No warning is given, and when new users are added, the previous file gets overwritten without a backup.

Screenshot 2023-11-17 at 4.48.39 PM.png]

Screenshot 2023-11-17 at 4.48.51 PM.png]

Steps to reproduce:

  1. Start with an invalid JSON file with 3 people tagged Emergency.
  2. Open the app.
  3. App opens with a blank interface and no people, and no warnings.
  4. Adding an individual will overwrite the JSOn file

Expected Result: Save a backup of the JSON file somewhere else if a warning occurs, and display the warning to the user.

Actual Result: No backup, no warning. Data file gets overwritten

nus-pe-script commented 9 months ago

Team's Response

Hello! Thank you for bug report! However, we regret to inform you that we will be unfortunately rejecting your bug.

We have stated in the UG that Advanced users are welcome to update the data and if the changes to the data file makes it invalid, NUSCoursemates will discard all data and start with an empty file.

Screenshot 2023-11-20 at 2.33.41 AM.png

In addition, as stated by Prof Damith in forum issue #243, it is great to handle "invalid data in JSON file" but it will not be considered a bug.

Screenshot 2023-11-20 at 2.36.45 AM.png

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: Thanks for your reply and references on this matter. I appreciate the lengths you have gone to take a look at the scope of this question, but I would still like to point out my disagreement on this issue.

The primary issue in this case is the deletion of all data, without any warning to the user. This severely damages both the usability, and the reliability of the application to critical users, who have adopted your platform of NUSCoursemates to do all their tracking. This results in the loss of not just non-critical information, like friend/close friend/emergency tagging, but also loss of names, phone numbers, and tele handles, which can be the result of years of use. The addition of the tagging feature, and the imposition of a forced 2 contact limit on emergencies has caused this issue, that would not be present in AB3. It appears that a large liability is looming over after adding the tagging, instead of an added function.

This would be a simple fix -- simply creating a backup for the user when your app is not able to parse syntactically accurate JSON with modifications that NUSCoursemates is unwilling to accept, and warning the user appropriately.

I've taken a look at similar issues being raised by our classmates with regards to editing the JSON, and catastrophic loss of data. It's generally agreed upon that this should be considered NotInScope for v1.4, rather than rejected as not a bug. Please refer to Issue #510 in the forums for the current semester, as well as the following similar issues raised by our classmates.

https://github.com/HugeNoob/pe/issues/3

https://github.com/nus-cs2103-AY2324S1/pe-dev-response/issues/5896

https://github.com/nus-cs2103-AY2324S1/pe-dev-response/issues/542

https://github.com/nathanielcalimag/pe/issues/2

https://github.com/kimshitong/pe/issues/3

https://github.com/peiran18/pe/issues/6

Screenshot 2023-11-23 at 6.50.43 PM.png

I think your product is great and has lots of potential for use! It's useful to say the least, and has a well-sized target audience of NUS SoC students, many of whom would love to use this at an advanced level too, with the JSON capabilities you have added. I'm hoping this will be a future fix, especially with the future plans and possibilities you have envisioned.


## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.Medium`] - [x] I disagree **Reason for disagreement:** Per the PE Readiness Quiz, Q22: `While it might not happen to most users, it can happen to any user, and the outcome is disastrous (i.e., loss of all data).` And such an issue is marked as High. I’ve positioned it as a medium as the disastrous, silent loss is not made known to advanced users, a subset of the entire population of NUSCoursemates.