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