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

[Daniel Axtens]

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