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

[Stephen Finucane]

On Wed, 2021-07-28 at 18:17 +0000, Raxel Gutierrez wrote:
> 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. 

o/

So I haven't applied this series locally yet, but the fact that I think I need
to suggests we're still missing a little context about what you're trying to
achieve here :)

Could you go into a little more detail about how you expect this to work? From
what I can tell from the above, it sounds like this series proposes adding a
binary attribute - "Addressed" or "Unaddressed" - to every patch comment. What
about cover letter comments? Also, how will this be added? Will a maintainer (or
the contributor) need to add it manually via the web UI or will it be added
automatically to every comment? If the former, is this a reasonable request for
maintainers on high-volume mailing lists? Will they be able to add it as part of
the email (via a custom header) or will they need to use the REST API or web UI?
If the latter, how will we distinguish between an email with an actionable
request and, say, the response to that request? Remember, comments in Patchwork
are flat with no support for threading. We could add threading, but the
unstructured nature of emails means it's a complicated task [1] that even the
best clients often get wrong IME. Adding this functionality would be a
significant amount of work by its own right, and is (I suspect?) work that may
fall outside your area of expertise.

As a general point, I can think of two existing patterns for implementing this
feature. The first is the GitLab (+ recent versions of Gerrit and possibly
GitHub) model of individual comment "threads" which are marked unresolved by
default but can be marked as "Resolved" by either a maintainer or the submitter.
I suspect this won't really work here due to the lack of threading support for
comments. The other approach is the old school "needinfo" approach used by
Bugzilla, whereby a user can set a "needinfo <assignee>" flag on a comment that
the assignee can later clear with a response. If I were to guess, you're likely
taking a approach closer to the latter only without the "<assignee>" attribute
of this flag and with the maintainer being the only person that can clear the
flag. This is only a guess at this point though...

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

It would be good to have a documentation patch in the 'user' section of the docs
that outlines this feature. We already provide docs for e.g. the auto-delegation
feature so this would fit in around the same area. The Sphinx docs have a useful
guide [1] on reStructuredText in case you haven't worked with it before (it's
similar to Markdown but more powerful and unfortunately more complex/finicky).
Perhaps hold off writing said doc until we've addressed the questions above
since they'll feed into the doc.

[1] https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html

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

Cheers,
Stephen

PS: Apologies for only dropping into this now. Let me know if I missed something
obvious higher up the chain.

PPS: If possible, could you turn off "smart quotes" in your email client, i.e.
instead of unicode "’", use ASCII "'"? The former is janky in terminals and
tooling that doesn't offer proper unicode support, which is still more common
that you'd like (try exporting 'LC_ALL=C' and watch the world crumble).

[1] https://www.jwz.org/doc/threading.html