[PATCH v2 0/5] patch-detail: add unaddressed/addressed status to patch comments

Fri Aug 6 13:20:44 AEST 2021

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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork