Store user feedback in db

[afifaniks]

This patch collects and stores user feedback on a collection upon feedback button clicks

Fixes: #496

Signed-off-by: Afif Al Mamun afifanik@gmail.com

[netlify[bot]]

✅ Deploy Preview for eclipsefdn-adoptium-trss canceled.

Name	Link
🔨 Latest commit	1cf855bc5cfc0e0d9e36187daa8a43ed8278bfa7
🔍 Latest deploy log	https://app.netlify.com/sites/eclipsefdn-adoptium-trss/deploys/64565ba06348a5000771e49a

[llxia]

@afifaniks Could you resolve the merge conflicts? Thanks

[afifaniks]

@llxia merge conflicts have been resolved.

[llxia]

Is this PR ready for review? If so, please move it out of the draft. Thanks

[afifaniks]

@karianna Hi. There is no "dynamic" user input that we are storing for the time being. Do we still need sanitization for the data we are storing? Thanks.

[karianna]

I think we're OK as long as someone can't hit that end point using CURL or other HTTP libs to send in whatever they like :-)

[afifaniks]

Hello @llxia @LongyuZhang, looks like we have an issue with the file name. Any recommendation on file name generation? We may also try implementing the recommendations given here: https://codeql.github.com/codeql-query-help/go/go-path-injection/

[llxia]

@afifaniks This is a false alarm. We do not allow users to define any file path/name. The file path/name is pre-defined/hardcoded. To avoid the false alarm in the future, please put the output filename generate code within writeTestOutputToFile(). Hopefully, the code scan can understand it better.

Also, we should not store the output whenever feedback is provided. We need to check if the output file exists or not first. Only store the output on disk if the file does not exist.

@LongyuZhang On a related note, we are planning to run TRSS via docker. If we do, the files will be lost if we turn off the container unless the volume is used.

[afifaniks]

@llxia @LongyuZhang have we decided on the new additions we are talking about? If yes, then I can start working on them. Thanks.

[llxia]

@afifaniks 3 action items from the above comments:

• move output filename generate code within writeTestOutputToFile() to avoid false alarm (https://github.com/adoptium/aqa-test-tools/pull/798#issuecomment-1517969248)
• save the output on disk only if the file does not exist (https://github.com/adoptium/aqa-test-tools/pull/798#issuecomment-1517969248)
• add two counters - positive feedback counter and negative feedback counter. These two counters should be displayed in the UI and the values should be stored in DB. (https://github.com/adoptium/aqa-test-tools/pull/798#pullrequestreview-1396482391). This can be in a separate PR.

[afifaniks]

@llxia As we are now considering keeping counters of positive and negative feedback, do we need the "accuracy" field in the database anymore? We can simply update the existing counter of a record each time feedback is provided, right? Additionally, do you have any design in mind that I may work on to show the count on UI?

[afifaniks]

@llxia I have made some changes to store positive and negative counters with the aforementioned approach. Please have a look if it would justify the requirement. Thanks.

[llxia]

@afifaniks Thanks for the update.

do we need the "accuracy" field in the database anymore? We can simply update the existing counter of a record each time feedback is provided, right? Additionally, do you have any design in mind that I may work on to show the count on UI?

Yes, that is correct. We do not need the "accuracy" field anymore. It can be removed. We should display the counter in UI. For example, 5 positive feedback and 2 negative feedback:

👍 5 👎 2

The UI update can be in a separate PR.

We still have Uncontrolled data used in path expression error. I think we can merge this PR and fix it later by using an off-the-shelf library like the sanitize-filename npm package. Please open an issue to track this.

[afifaniks]

@llxia the issue has been created.

[llxia]

@afifaniks if you plan to remove accuracy in another PR, please open an issue to track it.

[LongyuZhang]

The separate issue to remove accuracy has been created here https://github.com/adoptium/aqa-test-tools/issues/800

[afifaniks]

@llxia accuracy field is already removed in this PR. We are not writing this field in the database rather the value of the request parameter "accuracy" determines whether it is positive feedback or negative; i.e. 0 = negative, 1 = positive. That being said, do we want to remove the accuracy field from the request body?

[llxia]

With the positive feedback counter and negative feedback counter, we do not need accuracy at all.

[llxia]

Sorry @afifaniks , I was busy with the release. I think we can merge this PR for now. We can make the enhancements in the future PR.