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