| Message ID | 20210820045030.3364156-5-raxel@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | patch-detail: add unaddressed/addressed status to patch comments | expand |
On Fri, 2021-08-20 at 04:50 +0000, Raxel Gutierrez wrote: > Currently, there is no state or status associated with comments. In > particular, knowing whether a comment on a patch or cover letter is > addressed or not is useful for transparency and accountability in the > patch review and contribution process. This patch is backend setup for > tracking the state of patch and cover comments. > > Add `addressed` boolean field to patch and cover comments to be able to > distinguish between unaddressed and addressed comments in the > patch-detail page. > > Signed-off-by: Raxel Gutierrez <raxel@google.com> > Reviewed-by: Daniel Axtens <dja@axtens.net> I'm still not entirely happy with this design. It's functional, but it seems overly verbose for what I think we're trying to do here. In particular, the idea that every single comment is actionable and is unaddressed by default feels wrong to me. Who is this field intended for? Is it for maintainers to track whether a question they asked has been answered, or is it for submitter to track whether they've answered all of a reviewers questions? I suspect it's the former and, if so, what are your thoughts on defaulting to addressed being unset by default (i.e. NULL or no action item). This would mean maintainers would be required to set 'addressed=False' when needed? We could event allow them to do this via an email header which would be easily set in mail clients like mutt. If it's the latter, the same approach would probably still work, I think. WDYT? Aside from this design question, the rest of the changes, particularly those to the REST API look okay, but I'll need to spend more time on this over the weekend. Cheers, Stephen
Stephen Finucane <stephen@that.guru> writes: > On Fri, 2021-08-20 at 04:50 +0000, Raxel Gutierrez wrote: >> Currently, there is no state or status associated with comments. In >> particular, knowing whether a comment on a patch or cover letter is >> addressed or not is useful for transparency and accountability in the >> patch review and contribution process. This patch is backend setup for >> tracking the state of patch and cover comments. >> >> Add `addressed` boolean field to patch and cover comments to be able to >> distinguish between unaddressed and addressed comments in the >> patch-detail page. >> >> Signed-off-by: Raxel Gutierrez <raxel@google.com> >> Reviewed-by: Daniel Axtens <dja@axtens.net> > > I'm still not entirely happy with this design. It's functional, but it seems > overly verbose for what I think we're trying to do here. In particular, the idea > that every single comment is actionable and is unaddressed by default feels > wrong to me. > > Who is this field intended for? Is it for maintainers to track whether a > question they asked has been answered, or is it for submitter to track whether > they've answered all of a reviewers questions? I suspect it's the former and, if > so, what are your thoughts on defaulting to addressed being unset by default > (i.e. NULL or no action item). This would mean maintainers would be required to > set 'addressed=False' when needed? We could event allow them to do this via an > email header which would be easily set in mail clients like mutt. If it's the > latter, the same approach would probably still work, I think. WDYT? From my point of view, it's mostly aimed at submitters who want a method to keep track of their own tasks. (I know Emily S was hoping to one day see unaddressed comments in a user's to-do list, which I think gives you more insight into the broader use case.) I think maintainers could chose to make it part of their workflow, but given that the submitter gets to mark a comment as addressed, a maintainer couldn't really trust that 'addressed' meant 'addressed satisfactorily'. It's not really a tool to tell maintainers that a patch is ready. IMO the impact is fairly small on people who don't want to use it - they can just ignore the changes in the comment header bar. The git folks do seem to have a use case in mind for this, and so unless it's going to really mess up someone else's use case I'm inclined to accept it and clean up any fallout later. Kind regards, Daniel > > Aside from this design question, the rest of the changes, particularly those to > the REST API look okay, but I'll need to spend more time on this over the > weekend. > > Cheers, > Stephen > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On Mon, 2021-08-23 at 15:12 +1000, Daniel Axtens wrote: > Stephen Finucane <stephen@that.guru> writes: > > > On Fri, 2021-08-20 at 04:50 +0000, Raxel Gutierrez wrote: > > > Currently, there is no state or status associated with comments. In > > > particular, knowing whether a comment on a patch or cover letter is > > > addressed or not is useful for transparency and accountability in the > > > patch review and contribution process. This patch is backend setup for > > > tracking the state of patch and cover comments. > > > > > > Add `addressed` boolean field to patch and cover comments to be able to > > > distinguish between unaddressed and addressed comments in the > > > patch-detail page. > > > > > > Signed-off-by: Raxel Gutierrez <raxel@google.com> > > > Reviewed-by: Daniel Axtens <dja@axtens.net> > > > > I'm still not entirely happy with this design. It's functional, but it seems > > overly verbose for what I think we're trying to do here. In particular, the idea > > that every single comment is actionable and is unaddressed by default feels > > wrong to me. > > > > Who is this field intended for? Is it for maintainers to track whether a > > question they asked has been answered, or is it for submitter to track whether > > they've answered all of a reviewers questions? I suspect it's the former and, if > > so, what are your thoughts on defaulting to addressed being unset by default > > (i.e. NULL or no action item). This would mean maintainers would be required to > > set 'addressed=False' when needed? We could event allow them to do this via an > > email header which would be easily set in mail clients like mutt. If it's the > > latter, the same approach would probably still work, I think. WDYT? > > From my point of view, it's mostly aimed at submitters who want a method > to keep track of their own tasks. (I know Emily S was hoping to one day > see unaddressed comments in a user's to-do list, which I think gives you > more insight into the broader use case.) Right, but this should absolutely be opt-in. We shouldn't default to assuming that all users or maintainers want to track work items like this. We *definitely* shouldn't default to all comments, past and present, being classed as actionable things... > > I think maintainers could chose to make it part of their workflow, but > given that the submitter gets to mark a comment as addressed, a > maintainer couldn't really trust that 'addressed' meant 'addressed > satisfactorily'. It's not really a tool to tell maintainers that a patch > is ready. > > IMO the impact is fairly small on people who don't want to use it - they > can just ignore the changes in the comment header bar. Perhaps, but for users that don't plan on using this, this bit of "noise" will suddenly appear on every single patch and cover letter comment, past and present. That doesn't seem reasonable. Even uses that do plan to use this will suffer since every patch the submitter has ever submitted to any list managed by the Patchwork instance would immediately be classed as a needing action (i.e. unresolved) unless and until a maintainer or the submitter went through the tedious process of setting 'addressed' to True for every. single. comment. ever submitter. This would make Emily's idea of adding this to the TODO list a no-go since that TODO list would be already be populated with a huge and unmanageable list of most useless "action items". > The git folks do seem to have a use case in mind for this, and so unless > it's going to really mess up someone else's use case I'm inclined to > accept it and clean up any fallout later. Hopefully it's clear by now what me issues with this series are. I'd rather we'd figured this out on the list before merging, but I appreciate there are time constraints here. In the vein of cleaning up the fallout, I've just submitted a series to make this whole thing opt-in as it should have been from day 0. I'd appreciate eye-on this and would obviously welcome suggestions for other approaches. Cheers, Stephen > > Kind regards, > Daniel > > > > > Aside from this design question, the rest of the changes, particularly those to > > the REST API look okay, but I'll need to spend more time on this over the > > weekend. > > > > Cheers, > > Stephen > > > > _______________________________________________ > > Patchwork mailing list > > Patchwork@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/patchwork
diff --git a/patchwork/migrations/0045_auto_20210817_0136.py b/patchwork/migrations/0045_auto_20210817_0136.py new file mode 100644 index 00000000..ed3527bc --- /dev/null +++ b/patchwork/migrations/0045_auto_20210817_0136.py @@ -0,0 +1,23 @@ +# Generated by Django 3.1.12 on 2021-08-17 01:36 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('patchwork', '0044_add_project_linkname_validation'), + ] + + operations = [ + migrations.AddField( + model_name='covercomment', + name='addressed', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='patchcomment', + name='addressed', + field=models.BooleanField(default=False), + ), + ] diff --git a/patchwork/models.py b/patchwork/models.py index 00273da9..90e34815 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -657,6 +657,7 @@ class CoverComment(EmailMixin, models.Model): related_query_name='comment', on_delete=models.CASCADE, ) + addressed = models.BooleanField(default=False) @property def list_archive_url(self): @@ -693,6 +694,7 @@ class PatchComment(EmailMixin, models.Model): related_query_name='comment', on_delete=models.CASCADE, ) + addressed = models.BooleanField(default=False) @property def list_archive_url(self):