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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork