[PATCH v4 4/9] models: add addressed field

[Stephen Finucane]

On Mon, 2021-08-23 at 15:12 +1000, Daniel Axtens wrote:
> Stephen Finucane <stephen at 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 at google.com>
> > > Reviewed-by: Daniel Axtens <dja at 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 at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork