| Message ID | 20210728181718.3613124-1-raxel@google.com |
|---|---|
| Headers | show |
| Series | patch-detail: add unaddressed/addressed status to patch comments | expand |
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
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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
Daniel Axtens <dja@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@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/patchwork
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