Daniel Axtens <d...@axtens.net> writes: > Hi Raxel, > > Some general comments: > >> Currently, there is no state or status associated with patch comments. >> In particular, knowing whether a comment on a patch is addressed is >> useful for transparency and accountability in the review and >> contribution process. This series adds labels to comments to show >> whether they are “Addressed” or “Unaddressed”. Also, the addressed >> status of a comment can be manually changed given the required >> permissions to edit a patch. A future feature that would be useful to >> implement with this new feature is the ability to automatically add >> unaddressed comments to a user’s TODO page. > > So: > > - I wonder if a comment that adds a tag like "Reviewed-by: > <>"/Acked-by/Tested-by should automatically be labelled as > "Addressed" as these comments usually don't require a response from > the patch submitter. I haven't got strong feelings on this, I just > explored your series using some test data with a comment that was > just a "Reviewed-by" and it seemed odd to me. The more I think about > it the more I think if you're using pw as a code review platform you > probably don't mind just pressing "addressed" on these sorts of > comments? OTOH maybe if someone ever wants to build API tooling > around it (e.g. auto-merge a patch if it has an appropriate > Reviewed-by and all comments are addressed) then maybe they'd care a > bit more?
On further thinking, if you haven't already done it, don't worry about this. > > - I haven't checked, but what happens if (somehow) a state change > fails? It looks like the buttons flip from addressed <-> unaddressed > without a pending state --- what happens if something goes wrong? > (like a dropped internet connection) Is that reflected in the UI? I'm still keen to see this addressed. > - One thing we've repeatedly screwed up in the past is accidentally > creating massive db load through the API. django-debug-toolbar is > quite helpful for spotting db query pattern changes but it's a bit > fiddly to get set up in Docker. I haven't looked to see what query > changes have been created yet, but I just wanted to flag that as > something you should check when making API changes. > I will have a look at this myself when you send the next version, I know you're not primarily a backend person. > I haven't reviewed all the patches in detail yet - having a split > between refactoring and user-visible change would help - but looking at > the final product I think this is going in a direction I'm happy with. I'm now done reviewing this version of this series. > Kind regards, > Daniel > >> Something important to note is that this patch series relies on the >> JS cookie library [1] and rest.js file [2], both introduced in my >> previous patch series. Also, the patch series was previously a RFC [3] >> that lacked tests and release notes. Also, the first patch now adds the >> OpenAPI definition of the new comment detail endpoint and upgrades >> the REST API to v1.3 accordingly. >> Please include those two patches in the series when you send the next revision. That will make it a bit easier for me when I go to apply the series. If you can also include a link to the JS cookie library in the commit message that'd help because it gets corrupted at some point between you and me and so I have to redownload the file locally. Kind regards, Daniel >> For the first patch, the addressed field is added to the data model and >> a new endpoint for the REST API to work with a specific comment is added >> as well. The endpoint is upgraded to v1.3 and defined for OpenAPI. >> >> For the second patch, tests are added for the new endpoint. Also, the >> addressed field is added to create_patch_comment in the api tests >> utils.py. >> >> For the third patch, the addressed status label and button to change the >> addressed status are added with styling. >> >> For the fourth patch, the REST API call is added when the buttons are >> clicked to change the addressed status of a comment. Also, the UI is set >> to update accordingly. >> >> For the fifth patch, release notes are added for these changes. >> >> [1] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006994.html >> [2] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006997.html >> [3] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006974.html >> >> Raxel Gutierrez (5): >> api: add addressed field and detail endpoint for patch comments >> tests: add tests for patch comments detail endpoint >> patch-detail: add label and button for comment addressed status >> patch-detail: add functionality for comment status updates >> docs: add release note for addressed/unaddressed comments >> >> docs/api/schemas/generate-schemas.py | 4 +- >> docs/api/schemas/latest/patchwork.yaml | 144 +- >> docs/api/schemas/patchwork.j2 | 148 +- >> docs/api/schemas/v1.0/patchwork.yaml | 35 +- >> docs/api/schemas/v1.1/patchwork.yaml | 35 +- >> docs/api/schemas/v1.2/patchwork.yaml | 35 +- >> docs/api/schemas/v1.3/patchwork.yaml | 2749 +++++++++++++++++ >> htdocs/css/style.css | 55 +- >> htdocs/js/submission.js | 52 + >> patchwork/api/base.py | 24 +- >> patchwork/api/check.py | 21 +- >> patchwork/api/comment.py | 76 +- >> patchwork/api/patch.py | 2 +- >> .../migrations/0045_patchcomment_addressed.py | 18 + >> patchwork/models.py | 5 +- >> patchwork/templates/patchwork/submission.html | 165 +- >> patchwork/tests/api/test_comment.py | 201 +- >> patchwork/tests/utils.py | 1 + >> patchwork/urls.py | 17 +- >> patchwork/views/patch.py | 1 + >> ...essed-patch-comments-bfe71689b6f35a22.yaml | 20 + >> templates/base.html | 2 +- >> 22 files changed, 3653 insertions(+), 157 deletions(-) >> create mode 100644 docs/api/schemas/v1.3/patchwork.yaml >> create mode 100644 htdocs/js/submission.js >> create mode 100644 patchwork/migrations/0045_patchcomment_addressed.py >> create mode 100644 >> releasenotes/notes/comment-detail-endpoint-for-addressed-unaddressed-patch-comments-bfe71689b6f35a22.yaml >> >> -- >> 2.32.0.554.ge1b32706d8-goog >> >> _______________________________________________ >> Patchwork mailing list >> Patchwork@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
