nus-pe-script
commented
2 years ago Team's Response
This is the same issue!
The 'Original' Bug
[The team marked this bug as a duplicate of the following bug]
Manually editing studentbook.json and lessonbook.json with unalloyed inputs wipes both files
How to replicate the issue:
Manually edit any field within either file studentbook.json or lessonbook.json, with an invalid input. For example, I manually edited durationHours here to an unaccepted input "&&". Next, start the application again, and then exit the application.
After performing the above, the lessonbook is completely wiped.
Users may accidentally perform editing of the json files and as such there should be some safeguard to prevent all user data from being lost here.
[original: nus-cs2103-AY2122S2/pe-interim#1256] [original labels: type.FunctionalityBug severity.High]
Their Response to the 'Original' Bug
[This is the team's response to the above 'original' bug]
Good catch! But at this stage, wiping both files is an expected behaviour for the program, as mentioned in the UG's FAQ section (reproduced below):
Reason for current implementation
Wiping both files makes sense as these two lists are related to one another; ie the student assigned to a lesson in the LessonList is stored in StudentList.
Thus, if some student in the StudentList is corrupted (ie some fields were modified so that they are now invalid), the entry should be removed. Also, all the lessons that the student was assigned to should un-assign this student too (as he/she would no longer exist).
Other scenarios are also possible (eg the same case as above, but some lesson was corrupted instead).
To implement this logic would take a considerable amount of time, and in view of time constraints, our team decided to err on the safe side by wiping both lists to ensure that users would not be misled by inconsistencies caused by corrupted data lingering in the system.
Reason for downgrade
Additionally, we feel that this is a flaw that is unlikely to affect normal operations of the product (only happens when a user explicitly attempts to modify the JSON files). Hence, we would downgrade the severity to
low.Comments
Although you do raise a good point: ie wiping both files seems to be pretty harsh and some smarter way of detecting and fixing corrupted data would definitely make the user experience better, for the reasons mentioned above we would be tagging this report as
NotInScope!
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)
- [ ] I disagree
Reason for disagreement: [replace this with your explanation]
:question: Issue response
Team chose [response.NotInScope]
- [x] I disagree
Reason for disagreement: ### Benefits of having a backup file In the current implementation, the user's data is completely wiped from the JSON file, making it irretrievable. However, a better workaround (as mentioned in the bug report) exists and that would be to create a backup for the data whenever a corrupted JSON file is read by the app. This ensures that even after wiping away all the data entries from the JSON file, the user's data is never completely lost and users are able to retrieve them.
Effort Required
Implementing this workaround is as simple as adding a backup file logic in the JsonStudentBookStorage#readStudentBook method and this would require < 60 LoC (Including documentation). An example of the implementation of the backup feature can be seen here (my product's repo).
Although I understand that the implementation details would vary between teams and so is the "effort required" to implement the feature, the example I have provided is a rough gauge of the effort required to implement the "backup" feature. Based on the example link I have provided, it can be seen that the backup feature requires lesser effort and LoC than some of the features the dev team has implemented.
Furthermore, the backup feature would be an extension of the existing feature as such, the dev team would not need to change much of the existing code base to implement the backup feature.
Conclusion
As I have shown that there exists a better alternative to the existing feature and it can be implemented without much additional effort, this feature flaw cannot be claimed as NotInScope as described by point (2) of the image.
:question: Issue type
Team chose [type.FunctionalityBug]
Originally [type.FeatureFlaw]
- [ ] I disagree
Reason for disagreement: [replace this with your explanation]
:question: Issue severity
Team chose [severity.Low]
Originally [severity.High]
- [x] I disagree
Reason for disagreement: ### Expected Frequency of the bug Since the UG had specified that this data file exists (in the Q&A section) and the dev team did not put any effort into restricting user access to the data files, it is reasonable to assume that accessing the JSON files and trying to add students and lessons through those files is a legitimate feature intended by the dev team.
However, the team did not put the effort into showing what a valid JSON file should look like or providing explicit instructions on how to modify the contents in the JSON files safely in their UG. Therefore, the moment users attempt to modify the JSON files, it is very likely that they would be creating an invalid JSON file. As the app reads the invalid JSON file, all the user's existing data would be wiped clean from the app and it becomes irretrievable.
Admittedly, not all the users would try to modify the JSON files but it is reasonable to assume that a significant portion of the users might modify it since the existence of the data files is mentioned in the UG and the folder containing the JSON files is explicitly created (as opposed to hiding it) in the same directory as the app's jar file during the first start-up of the program. It is very easy for users to access that data folder and once the bug is encountered, it would lead to irreversible situations where the user's data would be completely deleted.
Severity of the bug
Also, taking a look at the UG, there is no warning specified with regards to parsing corrupted data into the app. As we cannot expect users to be perfect, there would be situations where they would parse a corrupted JSON file unknowingly into the app (be it manually modifying the JSON file or system/app failure that causes corruption to the JSON file), and with the current implementation, this would wipe away all the user's existing data without their acknowledgment.
As a target user, if those situations were to happen, would you be satisfied knowing all the data you originally had is lost and becomes irretrievable? From the end-user perspective, this feature would provide a major inconvenience as users would have to recall the data they had and try to type it into the app again. If the user is not able to recall the data they had previously input into the app, that data is truly lost forever. Important schedules and lessons might be missed as a result and this would be a major feature flaw of the application.
Better Alternatives
Also, from a developer's perspective, completely deleting the user's data and making it irretrievable to ensure that only valid data is "presented" in the app is a poor design choice. There are better alternatives out there which include only showing valid entries in the JSON files and having a backup feature (the argument for it can be seen in the response for NotInScope issue).
Conclusion
In conclusion, the current implementation the dev team has is a flaw that would affect most users as the UG talked about data files that exist in the application but did not mention or caution users in using/interacting with them. As such, if a corrupted file is read by the application (and it most likely will), the consequence would be disastrous, causing major problems for the users and making the app unusable for them. Hence, this issue should not be classified as severity.Low but severity.High instead.
Step to reproduce:
Save the file and close it.
Open the app.
This is shown:
Type
listlessons.Close the app.
All original data in the json file is gone and irretrievable as shown in the image:
Expected: Some form of backup when reading corrupted json files.
Actual: No backup was created. The original data file is lost forever.