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

[Emily Shaffer]

On Wed, Jul 28, 2021 at 06:17:13PM +0000, Raxel Gutierrez wrote:

(Full disclosure: I'm helping to host Raxel's internship and I yelled a
lot about requesting this feature.)

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

Completely underinformed Patchwork-sometimes-user here. It would make
the *most* sense, to me, to be able to mark these comments as addressed
or not only on comments where I am either the patch author or the
comment author. I'm not sure whether that matches the "required
permissions to edit a patch", but it'd be nice.

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

I would really, really like to have this. ;)

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

Hm, didn't you also send a UI mockup of this one? Or no?

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

Typically I'd prefer to see tests added in the same patch where the
thing they are testing was added; but I haven't read the series yet, so
maybe patch 1 was bare bones enough that it makes sense to separate the
tests. I'll comment when I get there, hopefully, if I remember.

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

Note: it's pretty common to include an explicit description of what
changed since last version, whether or not you choose to include a
range-diff. For example:

"""
Since v1:
 - added tests
 - added release notes
 - no other changes (please review me!)
"""


Thanks for sending, will leave my underinformed opinion on the rest of
these too. ;)

 - Emily