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

[Daniel Axtens]

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.)

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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork