Store user feedback in db #798

afifaniks · 2023-04-18T16:15:14Z

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

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>

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

netlify · 2023-04-18T16:15:22Z

✅ Deploy Preview for eclipsefdn-adoptium-trss canceled.

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

llxia · 2023-04-20T01:58:58Z

@afifaniks Could you resolve the merge conflicts? Thanks

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

afifaniks · 2023-04-20T07:03:06Z

@llxia merge conflicts have been resolved.

llxia · 2023-04-20T13:05:10Z

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

karianna

I've not reviewed carefully, but are we santitizing input here?

afifaniks · 2023-04-20T17:31:37Z

@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 · 2023-04-21T01:59:56Z

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 :-)

TestResultSummaryService/routes/postIssueFeedback.js

afifaniks · 2023-04-21T07:25:43Z

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 · 2023-04-21T15:03:29Z

@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.

LongyuZhang

Besides checking if the output file exists or not first, we could also accumulate the user feedback for a particular issue, e.g. positive feedback 3 times, negative 2 times. So in the DB, we may need to save positive and negative feedback separately.

Regarding the docker folder, I think we can replace the file folder into the volume once starting running TRSS in docker.

afifaniks · 2023-04-24T09:13:48Z

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

llxia · 2023-04-24T16:45:32Z

@afifaniks 3 action items from the above comments:

move output filename generate code within writeTestOutputToFile() to avoid false alarm (Store user feedback in db #798 (comment))
save the output on disk only if the file does not exist (Store user feedback in db #798 (comment))
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. (Store user feedback in db #798 (review)). This can be in a separate PR.

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

afifaniks · 2023-04-25T15:48:25Z

@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?

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

afifaniks · 2023-04-25T16:41:15Z

@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.

TestResultSummaryService/routes/postIssueFeedback.js

+        const fileName = `${testName}_${buildName}_${buildNum}_${domain}.txt`;
+        const outputPath = `${__dirname}/../../MachineLearningPrototype/data/JenkinsDataWithFeedback/${fileName}`;
+
+        const fileExists = await fs.stat(outputPath).then(


TestResultSummaryService/routes/postIssueFeedback.js

+        );
+
+        if (!fileExists) {
+            await fs.writeFile(outputPath, testOutput);


llxia · 2023-04-26T21:10:47Z

@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 · 2023-04-27T10:56:05Z

@llxia the issue has been created.

LongyuZhang

LGTM

llxia · 2023-04-28T18:22:53Z

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

LongyuZhang · 2023-04-28T18:25:30Z

The separate issue to remove accuracy has been created here #800

afifaniks · 2023-04-28T18:30:39Z

@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 · 2023-04-28T18:20:57Z

TestResultSummaryService/routes/postIssueFeedback.js

+        const fileName = `${testName}_${buildName}_${buildNum}_${domain}.txt`;
+        const outputPath = `${__dirname}/../../MachineLearningPrototype/data/JenkinsDataWithFeedback/${fileName}`;
+
+        const fileExists = await fs.stat(outputPath).then(


Please use existsSync(): https://www.geeksforgeeks.org/node-js-fs-existssync-method/

While stat() is providing us with an asynchronous way to check if the file exists or not, shall we move to a blocking method?

yes, we should use existsSync(). In asynchronous, the file could be deleted after the check.

Thanks for the clarification

llxia · 2023-04-28T18:31:11Z

TestResultSummaryService/routes/postIssueFeedback.js

+            await insertNewFeedback(db, req.body);
+        } else {
+            await updateFeedback(db, record, accuracy);
+        }


I think we should use upsert

Hi @llxia, if we want to upsert we may have to query for the testOutput regardless of whether it's an insert or update operation. Branching them based on the record's existence may save us one separate database query. Although I have already implemented upsert on my local branch I thought I should ask what you think about this. Thanks.

In upsert, we should be able to know to insert or update based on the return value.
testOutput is needed if the file does not exist on disk. Having the record in db does not mean that the testOutput is stored on disk.

Mmm, I think I am missing something here. Are you talking to replace updateFeedback() with an upsert operation? What I inferred last time is to remove the check for the record's existence [getRecordIfExists()] and directly upsert the input rather than using two methods (insertNewFeedback, updateFeedback).

llxia · 2023-04-28T18:39:46Z

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

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

llxia · 2023-05-19T14:53:09Z

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.

afifaniks added 3 commits April 16, 2023 01:32

Store user feedback

66f9ed8

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>

Optimize imports, remove comments

87f1801

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

Force commit after ECA sign off

1db70be

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

afifaniks mentioned this pull request Apr 18, 2023

Collect user feedback for Deep AQAtik predictions #496

Closed

afifaniks added 2 commits April 20, 2023 12:44

Resolve merge conflicts

ba05e1e

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

Add file extension in the testOutput fileName

8293b7a

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

afifaniks marked this pull request as ready for review April 20, 2023 13:07

karianna reviewed Apr 20, 2023

View reviewed changes

github-advanced-security bot found potential problems Apr 21, 2023

View reviewed changes

TestResultSummaryService/routes/postIssueFeedback.js Fixed Show fixed Hide fixed

afifaniks closed this Apr 21, 2023

afifaniks reopened this Apr 21, 2023

LongyuZhang reviewed Apr 21, 2023

View reviewed changes

Move outFileName generation to writeTestOutputToFile

bc243a0

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

Add positive and negative counter for submitted feedback

62c9b19

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

github-advanced-security bot found potential problems Apr 26, 2023

View reviewed changes

LongyuZhang mentioned this pull request Apr 27, 2023

Store and display ML user feedback counts #800

Open

LongyuZhang approved these changes Apr 27, 2023

View reviewed changes

llxia reviewed Apr 28, 2023

View reviewed changes

Replace fs.stat() with fs.existSync() to check for output file

1cf855b

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

llxia approved these changes May 19, 2023

View reviewed changes

llxia merged commit 6fda215 into adoptium:master May 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store user feedback in db #798

Store user feedback in db #798

afifaniks commented Apr 18, 2023

netlify bot commented Apr 18, 2023 •

edited

Loading

llxia commented Apr 20, 2023

afifaniks commented Apr 20, 2023

llxia commented Apr 20, 2023

karianna left a comment

afifaniks commented Apr 20, 2023

karianna commented Apr 21, 2023

afifaniks commented Apr 21, 2023 •

edited

Loading

llxia commented Apr 21, 2023 •

edited

Loading

LongyuZhang left a comment •

edited

Loading

afifaniks commented Apr 24, 2023

llxia commented Apr 24, 2023

afifaniks commented Apr 25, 2023 •

edited

Loading

afifaniks commented Apr 25, 2023

llxia commented Apr 26, 2023

afifaniks commented Apr 27, 2023

LongyuZhang left a comment

llxia commented Apr 28, 2023

LongyuZhang commented Apr 28, 2023

afifaniks commented Apr 28, 2023

llxia Apr 28, 2023

afifaniks Apr 28, 2023

llxia May 5, 2023

afifaniks May 5, 2023

afifaniks May 6, 2023

llxia Apr 28, 2023

afifaniks Apr 29, 2023

llxia May 5, 2023

afifaniks May 5, 2023

llxia commented Apr 28, 2023

llxia commented May 19, 2023

Store user feedback in db #798

Store user feedback in db #798

Conversation

afifaniks commented Apr 18, 2023

netlify bot commented Apr 18, 2023 • edited Loading

✅ Deploy Preview for eclipsefdn-adoptium-trss canceled.

llxia commented Apr 20, 2023

afifaniks commented Apr 20, 2023

llxia commented Apr 20, 2023

karianna left a comment

Choose a reason for hiding this comment

afifaniks commented Apr 20, 2023

karianna commented Apr 21, 2023

afifaniks commented Apr 21, 2023 • edited Loading

llxia commented Apr 21, 2023 • edited Loading

LongyuZhang left a comment • edited Loading

Choose a reason for hiding this comment

afifaniks commented Apr 24, 2023

llxia commented Apr 24, 2023

afifaniks commented Apr 25, 2023 • edited Loading

afifaniks commented Apr 25, 2023

llxia commented Apr 26, 2023

afifaniks commented Apr 27, 2023

LongyuZhang left a comment

Choose a reason for hiding this comment

llxia commented Apr 28, 2023

LongyuZhang commented Apr 28, 2023

afifaniks commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llxia commented Apr 28, 2023

llxia commented May 19, 2023

netlify bot commented Apr 18, 2023 •

edited

Loading

afifaniks commented Apr 21, 2023 •

edited

Loading

llxia commented Apr 21, 2023 •

edited

Loading

LongyuZhang left a comment •

edited

Loading

afifaniks commented Apr 25, 2023 •

edited

Loading