Add ability to review add-ons #1757

nvdaes · 2023-10-11T06:49:12Z

Summary of the issue

Add-ons submitted to the store don't require a previous human review, though some checks are performed automatically to ensure conformance with a predefined json schema, and to avoid the acceptance of add-ons which declare to be tested with non existing API versions.
Though security cannot be warranted, a known place to make and consult human reviews can be provided. This maybe useful to search for feedback of users and reviewers before installing an add-on.

Development process

GitHub Actions have been used to create a discussion for each addonId, and a comment for each submission (add-on version),after add-on metadata is merged to the main branch of the addon-datastore repo.

Some info about created discussions, associated with add-on ids, will be stored in a discussions.json file.

Also, the URL corresponding with the comment for each submission will be stored in the add-on metadata.

Testing performed

Add-ons have been submitted to the nvdaes/addon-datastore repo, emulating the process to be performed in the nvaccess/addon-datastore repo, to test the effect of GitHub Actions.

nvdaes · 2023-10-11T14:07:08Z

Though this is ready for review, I'd like to comment that, instead of body, maybe more useful to include the resourcePath of the comment. Different urls maybe saved in discussion.json, one for each version, though imo this would be useless since I don't think that a specific location in a webpage can be selected opening it from NVDA gui (i.e., from the store).
Also, I think that create json shouldn't include this. Instead, the discussionUrl, reviewUrl or the name used for this should be optional so metadata is valid with it, but I think that the discussion should be created once the add-on has been validated. Then metadata can be added and the add-on shouldn't need an additional validation.
This doesn't require a merge, to avoid that discussions need to be created manually in manual merges when submitters need to be approval.
I'll wait for more feedback.

.github/workflows/checkAndSubmitAddonMetadata.yml

seanbudd · 2023-10-12T00:42:05Z

.github/workflows/checkAndSubmitAddonMetadata.yml

+        repository-id: 'MDEwOlJlcG9zaXRvcnkyMzI4MTU5NTQ='
+        category-id: 'DIC_kwDODeB9Us4CaB4B'
+        title: Reviews for ${{ steps.getAddonIdAndVersion.outputs.addonId }} add-on
+        body: |


it's a shame that the API doesn't support the polls feature We will have to add these manually and the API might not even allow posting discussion to the category without poll options.

.github/workflows/checkAndSubmitAddonMetadata.yml

nvdaes · 2023-10-12T04:39:23Z

@seanbudd thanks for your review. If you don't merge this today, I'll test changes on my fork.

seanbudd · 2023-10-12T04:44:43Z

I think there are still some unresolved suggestions. Please mark resolved ones as resolved.
I would encourage waiting for another review before testing.

.github/workflows/checkAndSubmitAddonMetadata.yml

nvdaes · 2023-10-12T05:12:05Z

@seanbudd I think it's resolved now. Please let me know if I should test this.

seanbudd · 2023-10-12T05:14:26Z

.github/workflows/checkAndSubmitAddonMetadata.yml

+      with:
+        script: |
+          const addonFilename = ${{ needs.getAddonFilename.outputs.addonFileName }}
+          const addonName = addonFilename.split('/')[1]


this is still just using the add-on file name / addonId. Please get the add-on name from the meta data

this is still unresolved

.github/workflows/checkAndSubmitAddonMetadata.yml

seanbudd · 2023-10-12T05:21:12Z

.github/workflows/checkAndSubmitAddonMetadata.yml

+          echo ${{ needs.getAddonFilename.outputs.addonFileName }}
+        )
+        discussionUrl=$(
+          echo ${{ steps.createDiscussion.outputs.discussion-url }}


this should probably use the url to the comment with the version, not the discussion url

.github/workflows/checkAndSubmitAddonMetadata.yml

nvdaes · 2023-10-12T09:16:07Z

We cannot use ${{ github.REPOSITORY_ID }}. This gets the value of an id shown in HTML, I think. This is a numeric value and we need 'MDEwOlJlcG9zaXRvcnkyMzI4MTU5NTQ='

nvdaes · 2023-10-12T19:19:41Z

@seanbudd hopely this goes in the good way. But unfortunately the create review comment steps throws a syntax error due to a {
If you want, feel free to review this:

https://github.com/nvdaes/addon-datastore/actions/runs/6499804024/job/17654017249#step:7:28

.github/workflows/checkAndSubmitAddonMetadata.yml

nvdaes · 2023-10-13T00:48:38Z

no, as I mentioned, the repository id needs to be hard-coded since the variable suggested by you gives an id that is different respect that the id we need here.

nvdaes · 2023-10-13T00:57:13Z

i read that always() is not the same that checking the failure and success, since always includes cancel. i"ll try to fix the remaining points.

seanbudd · 2023-10-13T00:58:26Z

@nvdaes - please reply in response to review comments in a threaded manner. email based response makes tracking the review much harder and also includes a lot of extra junk due to your email signature including the body of the text you are responding to.

nvdaes · 2023-10-13T01:25:44Z

@seanbudd wrote:

@nvdaes - please reply in response to review comments in a threaded manner.

Sure, sorry for replying via mobile device. Also, Ididn't understand well a question about repo id, and I have marked my comment as oupdated.

…to review

nvdaes · 2023-10-22T09:09:25Z

@seanbudd , unfortunately, for any reasonthat I don't know,though the created PR is merged to masterand the add-on metadata is added to the master branch, the createComment job fails with the following message:

Unhandled error: Error: ENOENT: no such file or directory, open 'addons/eMule/14.0.0.json'

seanbudd · 2023-10-22T23:11:38Z

@nvdaes - could you point to a failed action so it can be investigated? My assumption is a git pull is required somewhere

seanbudd · 2023-10-22T23:11:56Z

note that there are also merge conflicts here

nvdaes · 2023-10-23T03:51:05Z

could you point to a failed action so it can be investigated? My assumption is a git pull is required somewhere

Thanks @seanbudd . In fact, the checkout action fetches the commitprevious to merge. I'll try to perform a git pull in a next step. You can see a failed action at
https://github.com/nvdaes/addon-datastore/actions/runs/6608423808/job/17947132879#step:3:30

seanbudd · 2023-10-23T04:08:03Z

See also actions/checkout#439

nvdaes · 2023-10-23T05:46:05Z

I think that this is ready for another review. Now worked as expected.
Anyway, the transform workflow, not modified here, raised a decode error. Also,note that the addonId show in parentheses in the discussion title is between quotation marks:

nvdaes#582

Please see the following actions:

https://github.com/nvdaes/addon-datastore/actions/runs/6609167848

https://github.com/nvdaes/addon-datastore/actions/runs/6608903760

seanbudd · 2023-10-25T01:22:40Z

.github/workflows/sendJsonFile.yml

-        author: github-actions <github-actions@github.com>
+        repository: nvaccess/addon-datastore-transform
+        path: transform
+    - name: Validate metadata


why was this moved here?
Shouldn't this happen after the PR is created and before it is merged?

This appears to close this issue on failure, without leaving a review comment explaining the validation errors.

seanbudd · 2023-10-25T01:30:19Z

.github/workflows/checkAndSubmitAddonMetadata.yml

+          body: "Closes #${{ inputs.issueNumber }}"
+          author: github-actions <github-actions@github.com>
+          delete-branch: true
+  mergeToMaster:


I believe this is missing a step to check metadata before merging the PR

seanbudd · 2023-10-25T01:33:52Z

.github/workflows/sendJsonFile.yml

+      run: |
+        cd datastore
+        git add .
+        $addonFileName = git diff --cached --name-only


this seems fragile - what happens on PRs with multiple file names?
Shouldn't this be tied to the validation checks that only a single file is added a nd no files are modified?

nvdaes · 2023-10-25T06:36:41Z

@seanbudd , my initial intention is to perform the maximum number of checks before creating pull requests, assuming that just one file could be generated from the issue form, and assuming that nobody can push to addon-datastore in NV Access GitHub account, and that nobody from NV Access people would do it.
Anyway, I have addressed your changes. Please see the transformation workflow since on my GitHub account is always failing:

https://github.com/nvdaes/addon-datastore/actions/runs/6636211592

The only change not tested in the above action is that I have changed checkout to v4 for some step.

seanbudd · 2023-10-26T02:24:40Z

The failure in on that line seems intentional - as no review comment exists in discussions.json. If you want to avoid the failure, I would suggest setting an output env variable rather than using exit codes. It seems the real issues is that later on it fails to actually update discussion.json later, and fails to transform due to an encoding issue with the data in your repo.

nvdaes · 2023-10-26T19:36:21Z

@seanbudd , I've addressed your review so tht the createReviewComment job doesn¡t fail. Unfortunately the transform workflow is stil failing. I don't know what wouldhappen if you test this in the staging repo:

https://github.com/nvdaes/addon-datastore/actions/runs/6658652475

If/when you merge this, please let me know if we should create a job to add discussions and comments for all add-ons. I'm not sure about the result due to multiple calls to the GitHub API. I think that next step would be to change the jsonSchema in addon-datastore-validation, and create an action in NVDA.

seanbudd · 2023-10-26T22:11:52Z

I think its possible our step to update the discussion URL is changing the encoding of the addon version JSON file, thus causing the decode issue

seanbudd · 2023-10-26T22:13:15Z

I think the API should be fine to handle it, particularly from actions as a source.

nvdaes · 2023-10-27T07:34:56Z

@seanbudd , the issue with the transformation workflow seems to be fixed:

https://github.com/nvdaes/addon-datastore/actions/runs/6664065150

nvdaes · 2023-10-27T09:34:41Z

@seanbudd I think that this is ready for a review, which unless you have more suggestions maybe the last one.

The latest discussion doesn't show the add-on id between quotation marks in the title:

nvdaes#687

When you merge this, I will work in a different PR trying to generate comments for all versions of add-ons.

seanbudd

Thanks @nvdaes for all your hard work on this, changes look good to me

.github/workflows/checkAndSubmitAddonMetadata.yml

.github/workflows/sendJsonFile.yml

seanbudd · 2023-11-01T01:29:11Z

Could you update the PR description to be more descriptive?

e.g. summary of issue, development process, testing

nvdaes · 2023-11-01T06:49:38Z

@seanbudd , I've updatedthe PR description. When you merge this, I'll work trying to create comments for all submissions in their corresponding discussions, or other steps that you consider appropriate like working in the addon-datastore-validation or to create an action in NVDA. We can use this code for further steps, in particular for GitHub Actions, so I prefer to wait until this is merged.

This reverts commit 121b247.

Add ability to review add-ons

9585fc1

nvdaes mentioned this pull request Oct 11, 2023

add-on store: add ability to evaluate add-ons from NVDA nvaccess/nvda#15576

Closed

seanbudd self-requested a review October 11, 2023 22:25

seanbudd reviewed Oct 12, 2023

View reviewed changes

.github/workflows/checkAndSubmitAddonMetadata.yml Show resolved Hide resolved

nvdaes added 2 commits October 12, 2023 06:18

Address first review

4f8509f

Address second review

f00348f

seanbudd reviewed Oct 12, 2023

View reviewed changes

.github/workflows/checkAndSubmitAddonMetadata.yml Outdated Show resolved Hide resolved

Address third review

81afe1e

seanbudd reviewed Oct 12, 2023

View reviewed changes

nvdaes added 2 commits October 12, 2023 08:04

Parcially address review 4

016d9dc

Get addonName from metadata

eae8ece

Update workflow

ad6d3bd

seanbudd reviewed Oct 13, 2023

View reviewed changes

.github/workflows/checkAndSubmitAddonMetadata.yml Outdated Show resolved Hide resolved

seanbudd requested changes Oct 13, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

nvdaes added 4 commits October 13, 2023 03:26

Merge branch 'master' into review

0a18f10

Addressreview, partially

ce10421

Merge branch 'review' of https://github.com/nvdaes/addon-datastore in…

aea4e4b

…to review

Get category id with script action

d93493e

seanbudd self-requested a review October 22, 2023 23:53

Merge remote-tracking branch 'origin/master' into review

9eccbb4

Fixes

6bc40be

Simplify step

5f9c4bb

seanbudd reviewed Oct 25, 2023

View reviewed changes

nvdaes added 2 commits October 25, 2023 05:57

Address review beforetesting

276e33a

Address review

1184693

nvdaes added 2 commits October 26, 2023 19:33

address review

58b847e

Use fromJSON

121526e

Fixes

72ffccf

Don't show quotation marks in the add-on id for discussion title

5557fef

seanbudd approved these changes Nov 1, 2023

View reviewed changes

.github/workflows/checkAndSubmitAddonMetadata.yml Outdated Show resolved Hide resolved

.github/workflows/sendJsonFile.yml Outdated Show resolved Hide resolved

Apply suggestions from code review

93facc0

seanbudd merged commit 121b247 into nvaccess:master Nov 1, 2023

nvdaes added a commit to nvdaes/addon-datastore that referenced this pull request Nov 8, 2023

Revert "Add ability to review add-ons (nvaccess#1757)"

6385931

This reverts commit 121b247.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to review add-ons #1757

Add ability to review add-ons #1757

nvdaes commented Oct 11, 2023 •

edited

Loading

nvdaes commented Oct 11, 2023

seanbudd Oct 12, 2023

nvdaes commented Oct 12, 2023

seanbudd commented Oct 12, 2023

nvdaes commented Oct 12, 2023

seanbudd Oct 12, 2023 •

edited

Loading

seanbudd Oct 13, 2023

seanbudd Oct 12, 2023 •

edited

Loading

nvdaes commented Oct 12, 2023

nvdaes commented Oct 12, 2023

nvdaes commented Oct 13, 2023 via email •

edited by seanbudd

Loading

nvdaes commented Oct 13, 2023 via email •

edited by seanbudd

Loading

seanbudd commented Oct 13, 2023

This comment was marked as outdated.

nvdaes commented Oct 13, 2023

nvdaes commented Oct 22, 2023

seanbudd commented Oct 22, 2023

seanbudd commented Oct 22, 2023

nvdaes commented Oct 23, 2023

seanbudd commented Oct 23, 2023

nvdaes commented Oct 23, 2023

seanbudd Oct 25, 2023

seanbudd Oct 25, 2023

seanbudd Oct 25, 2023

nvdaes commented Oct 25, 2023

seanbudd commented Oct 26, 2023

nvdaes commented Oct 26, 2023

seanbudd commented Oct 26, 2023

seanbudd commented Oct 26, 2023

nvdaes commented Oct 27, 2023

nvdaes commented Oct 27, 2023

seanbudd left a comment

seanbudd commented Nov 1, 2023

nvdaes commented Nov 1, 2023

Add ability to review add-ons #1757

Add ability to review add-ons #1757

Conversation

nvdaes commented Oct 11, 2023 • edited Loading

Summary of the issue

Development process

Testing performed

nvdaes commented Oct 11, 2023

seanbudd Oct 12, 2023

Choose a reason for hiding this comment

nvdaes commented Oct 12, 2023

seanbudd commented Oct 12, 2023

nvdaes commented Oct 12, 2023

seanbudd Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

seanbudd Oct 13, 2023

Choose a reason for hiding this comment

seanbudd Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

nvdaes commented Oct 12, 2023

nvdaes commented Oct 12, 2023

nvdaes commented Oct 13, 2023 via email • edited by seanbudd Loading

nvdaes commented Oct 13, 2023 via email • edited by seanbudd Loading

seanbudd commented Oct 13, 2023

This comment was marked as outdated.

nvdaes commented Oct 13, 2023

nvdaes commented Oct 22, 2023

seanbudd commented Oct 22, 2023

seanbudd commented Oct 22, 2023

nvdaes commented Oct 23, 2023

seanbudd commented Oct 23, 2023

nvdaes commented Oct 23, 2023

seanbudd Oct 25, 2023

Choose a reason for hiding this comment

seanbudd Oct 25, 2023

Choose a reason for hiding this comment

seanbudd Oct 25, 2023

Choose a reason for hiding this comment

nvdaes commented Oct 25, 2023

seanbudd commented Oct 26, 2023

nvdaes commented Oct 26, 2023

seanbudd commented Oct 26, 2023

seanbudd commented Oct 26, 2023

nvdaes commented Oct 27, 2023

nvdaes commented Oct 27, 2023

seanbudd left a comment

Choose a reason for hiding this comment

seanbudd commented Nov 1, 2023

nvdaes commented Nov 1, 2023

nvdaes commented Oct 11, 2023 •

edited

Loading

seanbudd Oct 12, 2023 •

edited

Loading

seanbudd Oct 12, 2023 •

edited

Loading

nvdaes commented Oct 13, 2023 via email •

edited by seanbudd

Loading

nvdaes commented Oct 13, 2023 via email •

edited by seanbudd

Loading